udev rule works only sometimes

[n3vu0r]

Sometimes after rebooting the udev rule doesn't change ownership/permission of charge_thresholds/fn_lock_state. I haven't found log files, but I assume it is a race condition because mostly it works. Maybe when the huawei-wmi folder is created in sysfs, it triggers the rule before the files charge_thresholds/fn_lock_state have been created.

[nekr0z]

Looks like an issue with the driver itself. @aymanbagabas, ideas?

[aymanbagabas]

It's not the driver, it might be a race condition as @n3vu0r stated. I would try adding another udev rule with ACTION=='change' and see if this solves it.

[nekr0z]

@n3vu0r Can you test? If the solution works, we may add a second udev rule to this repo.

[n3vu0r]

Ah okay, i'm testing it.

[n3vu0r]

It seems to work with both, add and change event rules. A change event rule only does not work.

But doesn't this mean that the driver adds the sysfs device before its attributes are set? Is it possible to do this atomically, so that an add event is enough @aymanbagabas?

[aymanbagabas]

It seems to work with both, add and change event rules. A change event rule only does not work.

But doesn't this mean that the driver adds the sysfs device before its attributes are set? Is it possible to do this atomically, so that an add event is enough @aymanbagabas?

Indeed, it seems to be a race condition at least according to [1], [2], [3], and [4].
This is now should be fixed in master branch. Now using one "add" rule should be sufficient.

Side note: a default value charge threshold value can be set using something like

SUBSYSTEM=="platform", DRIVER=="huawei-wmi", ATTR{charge_thresholds}="40 70"

[n3vu0r]

Wow, great! I will test it with the original udev rule which went into the race condition.

(Initially, I used this ATTR assignment, but wanted it to be configured by the applet by delegating the event to systemd with TAG+="systemd")

[aymanbagabas]

Wow, great! I will test it with the original udev rule which went into the race condition.

(Initially, I used this ATTR assignment, but wanted it to be configured by the applet by delegating the event to systemd with TAG+="systemd")

Reverted back to sysfs. It turns out that this actually won't fix! Actually, using device_add_group during probing is the same as using sysfs_create_group. The provided solution is for static variables/attributes, while the driver creates these attributes only if the corresponding WMI GUID is found which makes it rather dynamic.

An alternative solution could be to use devtmpfs or systemd instead of udev rules. For now, two udev rules needed.

[nekr0z]

@n3vu0r What's the status on this? I'm not shipping udev rule since 1.3.1 and actually rely on your work instead. Should this issue be closed or transfered?

[n3vu0r]

Sorry, I'm late. Thanks for the effort @aymanbagabas, pity it wouldn't fix it.

I haven't observed any race condition with following workarounds:

• Two udev rules: add event fails sometimes but gets recovered by change event.
• One udev rule plus systemd unit: add event always worked so far but could fail in theory and then there won't be a change event because systemd WantedBy only cares about an add event. Im not sure why this always works: either this mechanism is just slow enough or there is some extra magic. The change event triggers a reload on the systemd device unit and could be caught by ReloadPropagatedFrom= and ExecReload= but this gets ugly. Instead I would like to add Restart=on-failure to be on the safe side. With default settings it will restart the unit up to 5 times with 100 ms between the restarts, this should easily cover the race condition.

To summarize, both solutions solve this issue. I prefer the systemd solution for simple logging and enabling/disabling the two services separately. I will add Restart=on-failure to the huawei-wmi-privilege unit and release the package version 1.0.0. For this I want to figure out if suspend-then-hibernate.target should be added to WantedBy or if it is already covered by a more general hibernate.target.

If @aymanbagabas has plans to eliminate the race condition at the driver level in future, we might transfer the issue there instead of closing it?

[nekr0z]

or if it is already covered by a more general hibernate.target

Even if it is (which I doubt) adding it explicitly shouldn't hurt as far a I understand.

we might transfer the issue there instead of closing it?

I was rather thinking of transferring it to you, actually, since you're the one maintaining udev rules now ;)

[n3vu0r]

Ahh, fine with that :D

For both user space repos the issue is solved, so we can close it (or transfer and close).

Restart=on-failure is not supported yet for Type=oneshot units. This will do the same in the end:

ExecStart=sh -c  '[ -e ${SYS}/charge_thresholds -a -e ${SYS}/fn_lock_state ] || sleep 1'

[nekr0z]

Allright, closing it now.