[PATCH v3 0/1] platform/chrome: Update cros_ec sysfs attribute after sensors are found

[PATCH v3 0/1] platform/chrome: Update cros_ec sysfs attribute after sensors are found

[Gwendal Grignou]

Attribute "kb_wake_angle" in /sys/<cros_ec hw path>/chromeos/cros_ec,
used to set at which angle the embedded controller must wake up the
host, is only set when there is at least 2 accelerometers in the
chromebook.

The detection of these sensors is done in cros_ec_sensorhub, driver that
can be probed after the cros_ec_sysfs driver that sets the attribute on
behalf of the chromeos EC class driver.
Therefore, we need to upgrade the sysfs group in the sensorhub probe
routine.

Gwendal Grignou (1):
  platform/chrome: Poke kb_wake_angle attribute visibility when needed

 drivers/platform/chrome/cros_ec_sensorhub.c | 6 +++++-
 drivers/platform/chrome/cros_ec_sysfs.c     | 5 +++--
 include/linux/platform_data/cros_ec_proto.h | 2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.32.0.554.ge1b32706d8-goog

[PATCH v3 1/1] platform/chrome: Poke kb_wake_angle attribute visibility when needed

[Gwendal Grignou]

When the sensorhub detects 2 accelerometers, kb_wake_angle attribute
in the chromeos EC class device needs to be visible.
When detected, the sensorhub requests the class device to
revisit the visibility of the kb_wake_angle attribute.
Expose the attribute group to alter to close a potiential race between
cros-ec-sensorhub and cros-ec-sysfs (that creates the attribute group
on behalf of the class driver).

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/platform/chrome/cros_ec_sensorhub.c | 6 +++++-
 drivers/platform/chrome/cros_ec_sysfs.c     | 5 +++--
 include/linux/platform_data/cros_ec_proto.h | 2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
index 9c4af76a9956e..8f24ed90b229c 100644
--- a/drivers/platform/chrome/cros_ec_sensorhub.c
+++ b/drivers/platform/chrome/cros_ec_sensorhub.c
@@ -106,8 +106,12 @@ static int cros_ec_sensorhub_register(struct device *dev,
 		sensor_type[sensorhub->resp->info.type]++;
 	}
 
-	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
+	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2) {
 		ec->has_kb_wake_angle = true;
+		if (ec->group && sysfs_update_group(&ec->class_dev.kobj,
+						    ec->group))
+			dev_warn(dev, "Unable to update cros-ec-sysfs");
+	}
 
 	if (cros_ec_check_features(ec,
 				   EC_FEATURE_REFINED_TABLET_MODE_HYSTERESIS)) {
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f07eabcf9494c..17fe657f3ffd5 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -341,7 +341,8 @@ static int cros_ec_sysfs_probe(struct platform_device *pd)
 	struct device *dev = &pd->dev;
 	int ret;
 
-	ret = sysfs_create_group(&ec_dev->class_dev.kobj, &cros_ec_attr_group);
+	ec_dev->group = &cros_ec_attr_group;
+	ret = sysfs_create_group(&ec_dev->class_dev.kobj, ec_dev->group);
 	if (ret < 0)
 		dev_err(dev, "failed to create attributes. err=%d\n", ret);
 
@@ -352,7 +353,7 @@ static int cros_ec_sysfs_remove(struct platform_device *pd)
 {
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
 
-	sysfs_remove_group(&ec_dev->class_dev.kobj, &cros_ec_attr_group);
+	sysfs_remove_group(&ec_dev->class_dev.kobj, ec_dev->group);
 
 	return 0;
 }
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 02599687770c5..c6dca260bbd5d 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -191,6 +191,7 @@ struct cros_ec_platform {
 /**
  * struct cros_ec_dev - ChromeOS EC device entry point.
  * @class_dev: Device structure used in sysfs.
+ * @groups: sysfs attributes groups for this EC.
  * @ec_dev: cros_ec_device structure to talk to the physical device.
  * @dev: Pointer to the platform device.
  * @debug_info: cros_ec_debugfs structure for debugging information.
@@ -200,6 +201,7 @@ struct cros_ec_platform {
  */
 struct cros_ec_dev {
 	struct device class_dev;
+	const struct attribute_group *group;
 	struct cros_ec_device *ec_dev;
 	struct device *dev;
 	struct cros_ec_debugfs *debug_info;
-- 
2.32.0.554.ge1b32706d8-goog

Re: [PATCH v3 1/1] platform/chrome: Poke kb_wake_angle attribute visibility when needed

[Gwendal Grignou]

This patch is still needed to get the attribute to be present on most machines.
Putting the attribute in the sensor hub device is more complicated, as
that device is not as easily findable as `cros_ec`, present in
`/sys/class`.

Gwendal.

On Wed, Aug 4, 2021 at 2:31 PM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> When the sensorhub detects 2 accelerometers, kb_wake_angle attribute
> in the chromeos EC class device needs to be visible.
> When detected, the sensorhub requests the class device to
> revisit the visibility of the kb_wake_angle attribute.
> Expose the attribute group to alter to close a potiential race between
> cros-ec-sensorhub and cros-ec-sysfs (that creates the attribute group
> on behalf of the class driver).
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_sensorhub.c | 6 +++++-
>  drivers/platform/chrome/cros_ec_sysfs.c     | 5 +++--
>  include/linux/platform_data/cros_ec_proto.h | 2 ++
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
> index 9c4af76a9956e..8f24ed90b229c 100644
> --- a/drivers/platform/chrome/cros_ec_sensorhub.c
> +++ b/drivers/platform/chrome/cros_ec_sensorhub.c
> @@ -106,8 +106,12 @@ static int cros_ec_sensorhub_register(struct device *dev,
>                 sensor_type[sensorhub->resp->info.type]++;
>         }
>
> -       if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
> +       if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2) {
>                 ec->has_kb_wake_angle = true;
> +               if (ec->group && sysfs_update_group(&ec->class_dev.kobj,
> +                                                   ec->group))
> +                       dev_warn(dev, "Unable to update cros-ec-sysfs");
> +       }
>
>         if (cros_ec_check_features(ec,
>                                    EC_FEATURE_REFINED_TABLET_MODE_HYSTERESIS)) {
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index f07eabcf9494c..17fe657f3ffd5 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -341,7 +341,8 @@ static int cros_ec_sysfs_probe(struct platform_device *pd)
>         struct device *dev = &pd->dev;
>         int ret;
>
> -       ret = sysfs_create_group(&ec_dev->class_dev.kobj, &cros_ec_attr_group);
> +       ec_dev->group = &cros_ec_attr_group;
> +       ret = sysfs_create_group(&ec_dev->class_dev.kobj, ec_dev->group);
>         if (ret < 0)
>                 dev_err(dev, "failed to create attributes. err=%d\n", ret);
>
> @@ -352,7 +353,7 @@ static int cros_ec_sysfs_remove(struct platform_device *pd)
>  {
>         struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
>
> -       sysfs_remove_group(&ec_dev->class_dev.kobj, &cros_ec_attr_group);
> +       sysfs_remove_group(&ec_dev->class_dev.kobj, ec_dev->group);
>
>         return 0;
>  }
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index 02599687770c5..c6dca260bbd5d 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -191,6 +191,7 @@ struct cros_ec_platform {
>  /**
>   * struct cros_ec_dev - ChromeOS EC device entry point.
>   * @class_dev: Device structure used in sysfs.
> + * @groups: sysfs attributes groups for this EC.
>   * @ec_dev: cros_ec_device structure to talk to the physical device.
>   * @dev: Pointer to the platform device.
>   * @debug_info: cros_ec_debugfs structure for debugging information.
> @@ -200,6 +201,7 @@ struct cros_ec_platform {
>   */
>  struct cros_ec_dev {
>         struct device class_dev;
> +       const struct attribute_group *group;
>         struct cros_ec_device *ec_dev;
>         struct device *dev;
>         struct cros_ec_debugfs *debug_info;
> --
> 2.32.0.554.ge1b32706d8-goog
>

Re: [PATCH v3 1/1] platform/chrome: Poke kb_wake_angle attribute visibility when needed

[Gwendal Grignou]

[+chrome-platform@lists.linux.dev]

On Mon, Nov 14, 2022 at 8:10 PM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> This patch is still needed to get the attribute to be present on most machines.
> Putting the attribute in the sensor hub device is more complicated, as
> that device is not as easily findable as `cros_ec`, present in
> `/sys/class`.
>
> Gwendal.
>
> On Wed, Aug 4, 2021 at 2:31 PM Gwendal Grignou <gwendal@chromium.org> wrote:
> >
> > When the sensorhub detects 2 accelerometers, kb_wake_angle attribute
> > in the chromeos EC class device needs to be visible.
> > When detected, the sensorhub requests the class device to
> > revisit the visibility of the kb_wake_angle attribute.
> > Expose the attribute group to alter to close a potiential race between
> > cros-ec-sensorhub and cros-ec-sysfs (that creates the attribute group
> > on behalf of the class driver).
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_sensorhub.c | 6 +++++-
> >  drivers/platform/chrome/cros_ec_sysfs.c     | 5 +++--
> >  include/linux/platform_data/cros_ec_proto.h | 2 ++
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
> > index 9c4af76a9956e..8f24ed90b229c 100644
> > --- a/drivers/platform/chrome/cros_ec_sensorhub.c
> > +++ b/drivers/platform/chrome/cros_ec_sensorhub.c
> > @@ -106,8 +106,12 @@ static int cros_ec_sensorhub_register(struct device *dev,
> >                 sensor_type[sensorhub->resp->info.type]++;
> >         }
> >
> > -       if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
> > +       if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2) {
> >                 ec->has_kb_wake_angle = true;
> > +               if (ec->group && sysfs_update_group(&ec->class_dev.kobj,
> > +                                                   ec->group))
> > +                       dev_warn(dev, "Unable to update cros-ec-sysfs");
> > +       }
> >
> >         if (cros_ec_check_features(ec,
> >                                    EC_FEATURE_REFINED_TABLET_MODE_HYSTERESIS)) {
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> > index f07eabcf9494c..17fe657f3ffd5 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -341,7 +341,8 @@ static int cros_ec_sysfs_probe(struct platform_device *pd)
> >         struct device *dev = &pd->dev;
> >         int ret;
> >
> > -       ret = sysfs_create_group(&ec_dev->class_dev.kobj, &cros_ec_attr_group);
> > +       ec_dev->group = &cros_ec_attr_group;
> > +       ret = sysfs_create_group(&ec_dev->class_dev.kobj, ec_dev->group);
> >         if (ret < 0)
> >                 dev_err(dev, "failed to create attributes. err=%d\n", ret);
> >
> > @@ -352,7 +353,7 @@ static int cros_ec_sysfs_remove(struct platform_device *pd)
> >  {
> >         struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> >
> > -       sysfs_remove_group(&ec_dev->class_dev.kobj, &cros_ec_attr_group);
> > +       sysfs_remove_group(&ec_dev->class_dev.kobj, ec_dev->group);
> >
> >         return 0;
> >  }
> > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > index 02599687770c5..c6dca260bbd5d 100644
> > --- a/include/linux/platform_data/cros_ec_proto.h
> > +++ b/include/linux/platform_data/cros_ec_proto.h
> > @@ -191,6 +191,7 @@ struct cros_ec_platform {
> >  /**
> >   * struct cros_ec_dev - ChromeOS EC device entry point.
> >   * @class_dev: Device structure used in sysfs.
> > + * @groups: sysfs attributes groups for this EC.
> >   * @ec_dev: cros_ec_device structure to talk to the physical device.
> >   * @dev: Pointer to the platform device.
> >   * @debug_info: cros_ec_debugfs structure for debugging information.
> > @@ -200,6 +201,7 @@ struct cros_ec_platform {
> >   */
> >  struct cros_ec_dev {
> >         struct device class_dev;
> > +       const struct attribute_group *group;
> >         struct cros_ec_device *ec_dev;
> >         struct device *dev;
> >         struct cros_ec_debugfs *debug_info;
> > --
> > 2.32.0.554.ge1b32706d8-goog
> >

Re: [PATCH v3 1/1] platform/chrome: Poke kb_wake_angle attribute visibility when needed

[Tzung-Bi Shih]

On Wed, Nov 16, 2022 at 10:23:38AM -0800, Gwendal Grignou wrote:
> [+chrome-platform@lists.linux.dev]

Please also Cc to the mailing list if the patch gets chance to have
next version.

> On Mon, Nov 14, 2022 at 8:10 PM Gwendal Grignou <gwendal@chromium.org> wrote:
[...]
> > > Expose the attribute group to alter to close a potiential race between
> > > cros-ec-sensorhub and cros-ec-sysfs (that creates the attribute group
> > > on behalf of the class driver).

I failed to realize the potential race.  Could you explain it a bit?

> > > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > > index 02599687770c5..c6dca260bbd5d 100644
> > > --- a/include/linux/platform_data/cros_ec_proto.h
> > > +++ b/include/linux/platform_data/cros_ec_proto.h
> > > @@ -191,6 +191,7 @@ struct cros_ec_platform {
> > >  /**
> > >   * struct cros_ec_dev - ChromeOS EC device entry point.
> > >   * @class_dev: Device structure used in sysfs.
> > > + * @groups: sysfs attributes groups for this EC.

The field name has extra "s".

Re: [PATCH v3 1/1] platform/chrome: Poke kb_wake_angle attribute visibility when needed

[Gwendal Grignou]

On Thu, Nov 17, 2022 at 6:01 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Wed, Nov 16, 2022 at 10:23:38AM -0800, Gwendal Grignou wrote:
> > [+chrome-platform@lists.linux.dev]
>
> Please also Cc to the mailing list if the patch gets chance to have
> next version.
>
> > On Mon, Nov 14, 2022 at 8:10 PM Gwendal Grignou <gwendal@chromium.org> wrote:
> [...]
> > > > Expose the attribute group to alter to close a potiential race between
> > > > cros-ec-sensorhub and cros-ec-sysfs (that creates the attribute group
> > > > on behalf of the class driver).
>
> I failed to realize the potential race.  Could you explain it a bit?
The decision to show or not an attribute is done at the attribute file
creation time, once. If the module cros_ec_sysfs is loaded before
cros_ec_sensorhub, the attribute kb_wake_angle will never be shown. If
it is loaded after, and there are 2 accelerometers, the is_visible()
function will return true and the attribute is shown.

This patch ensures the attribute is_visible() is run again after the
sensorhub driver is loaded.

Gwendal.
>
> > > > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > > > index 02599687770c5..c6dca260bbd5d 100644
> > > > --- a/include/linux/platform_data/cros_ec_proto.h
> > > > +++ b/include/linux/platform_data/cros_ec_proto.h
> > > > @@ -191,6 +191,7 @@ struct cros_ec_platform {
> > > >  /**
> > > >   * struct cros_ec_dev - ChromeOS EC device entry point.
> > > >   * @class_dev: Device structure used in sysfs.
> > > > + * @groups: sysfs attributes groups for this EC.
>
> The field name has extra "s".

Re: [PATCH v3 1/1] platform/chrome: Poke kb_wake_angle attribute visibility when needed

[Tzung-Bi Shih]

On Thu, Nov 17, 2022 at 10:41:06PM -0800, Gwendal Grignou wrote:
> On Thu, Nov 17, 2022 at 6:01 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > On Wed, Nov 16, 2022 at 10:23:38AM -0800, Gwendal Grignou wrote:
> > > [+chrome-platform@lists.linux.dev]
> >
> > Please also Cc to the mailing list if the patch gets chance to have
> > next version.
> >
> > > On Mon, Nov 14, 2022 at 8:10 PM Gwendal Grignou <gwendal@chromium.org> wrote:
> > [...]
> > > > > Expose the attribute group to alter to close a potiential race between
> > > > > cros-ec-sensorhub and cros-ec-sysfs (that creates the attribute group
> > > > > on behalf of the class driver).
> >
> > I failed to realize the potential race.  Could you explain it a bit?
> The decision to show or not an attribute is done at the attribute file
> creation time, once. If the module cros_ec_sysfs is loaded before
> cros_ec_sensorhub, the attribute kb_wake_angle will never be shown. If
> it is loaded after, and there are 2 accelerometers, the is_visible()
> function will return true and the attribute is shown.
> 
> This patch ensures the attribute is_visible() is run again after the
> sensorhub driver is loaded.

I see.  Thanks.