Multiple EC_CMD_LIGHTBAR_CMD command errors on kernel boot |
|
Issue descriptionOn most platforms (at least 4.14 kernel, maybe others), we see errors in the EC log on boot: [37.155200 HC 0x28.0:0c00000000...] EC_CMD_LIGHTBAR_CMD [37.156059 HC 0x28 err 1] [37.157645 HC 0x28.0:0c00000000...] [37.158484 HC 0x28 err 1] [37.160304 HC 0x28.0:0c00000000...] [37.161142 HC 0x28 err 1] [37.162917 HC 0x28.0:0c00000000...] [37.163756 HC 0x28 err 1] [37.165376 HC 0x28.0:0c00000000...] [37.166214 HC 0x28 err 1] [37.167832 HC 0x28.0:0c00000000...] [37.168670 HC 0x28 err 1] [37.170369 HC 0x28.0:0c00000000...] [37.171207 HC 0x28 err 1] 0x28 is EC_CMD_LIGHTBAR_CMD This is due to cros_ec_lightbar_attrs_are_visible being called multiple times (https://elixir.bootlin.com/linux/latest/source/drivers/platform/chrome/cros_ec_lightbar.c#L592), and probing the lightbar once per call (ec_has_lightbar function). cros_ec_lightbar_attr_group is defined as follows: struct attribute_group cros_ec_lightbar_attr_group = { .name = "lightbar", .attrs = __lb_cmds_attrs, (7 attributes) .is_visible = cros_ec_lightbar_attrs_are_visible, }; Due to the way attribute groups visibility work, the function cros_ec_lightbar_attrs_are_visible is called 7 times, once per attribute (https://elixir.bootlin.com/linux/latest/source/fs/sysfs/group.c#L53). And each of these calls makes an EC transaction. This is probably not a big deal, but may be slowing down boot by ~15ms. We should probably cache the answer the EC_CMD_LIGHTBAR_CMD command, and not try again if it fails on the first attempt.
,
Nov 2
I agree with Guenter. This not only affects the lightbar driver, IMHO we should get rid of all these attributes from the mfd/cros_ec driver to the specific sub-driver. This will imply, probably, that you should fix the path in your userspace but that's a trivial fix I guess. In the beginning, the mfd maintainer accepted this, but now he will be more than happy if we move and document the sysfs attributes into the respective sub-drivers. I volunteer to do it and take care to upstream if you want.
,
Nov 2
It should be possible to retain the path at least for the lightbar group by creating a "lightbar" driver attached to cros_class. The same should be possible for the vbc and for the pd attributes, as they all reside in their own subdirectories. Other drivers in other classes do this all the time, so it should be possible here as well. I have not explored if the same is possible for cros_ec_attr_group, where the attributes reside in the class directory itself. eballetbo@: Sure, it would be great if you can take this upstream. Consider it approved.
,
Nov 2
> eballetbo@: Sure, it would be great if you can take this upstream. Consider it approved. As cleanups like this land upstream, please backport them to the Chrome OS 4.19 tree too. This will help keep us from getting quite as forked in the future.
,
Nov 5
#1 caching can be done in cros_ec_lightbar_attrs_are_visible(), as lightbar is only supported for the first ec (name cros_ec). If ec_has_lightbar() returns false once, there won't be any lightbar. #2 But a lightbar driver is definitively cleaner. The intent was to be loaded only if EC_FEATURE_LIGHTBAR is defined, but that flag is not in the very first Pixel EC (link), only samus. |
|
►
Sign in to add a comment |
|
Comment 1 by groeck@chromium.org
, Nov 2