Re: [PATCH 4/6] soundwire: sysfs: have the driver core handle the creation of the device groups

[Dan Williams <dan.j.williams@intel.com>]

Greg Kroah-Hartman wrote:
> The driver core supports the ability to handle the creation and removal
> of device-specific sysfs files in a race-free manner.  Take advantage of
> that by converting this driver to use this by moving the sysfs
> attributes into a group and assigning the dev_groups pointer to it.
> 
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Sanyog Kale <sanyog.r.kale@intel.com>
> Cc: alsa-devel@alsa-project.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/soundwire/bus_type.c    | 1 +
>  drivers/soundwire/sysfs_local.h | 3 +++
>  drivers/soundwire/sysfs_slave.c | 6 +-----
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 9fa93bb923d7..5abe5593395a 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -221,6 +221,7 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner)
>  	drv->driver.probe = sdw_drv_probe;
>  	drv->driver.remove = sdw_drv_remove;
>  	drv->driver.shutdown = sdw_drv_shutdown;
> +	drv->driver.dev_groups = sdw_attr_groups;
>  
>  	return driver_register(&drv->driver);
>  }
> diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h
> index 7268bc24c538..3ab8658a7782 100644
> --- a/drivers/soundwire/sysfs_local.h
> +++ b/drivers/soundwire/sysfs_local.h
> @@ -11,6 +11,9 @@
>  /* basic attributes to report status of Slave (attachment, dev_num) */
>  extern const struct attribute_group *sdw_slave_status_attr_groups[];
>  
> +/* attributes for all soundwire devices */
> +extern const struct attribute_group *sdw_attr_groups[];
> +
>  /* additional device-managed properties reported after driver probe */
>  int sdw_slave_sysfs_init(struct sdw_slave *slave);
>  int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave);
> diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
> index 8876c7807048..3afc0dc06c98 100644
> --- a/drivers/soundwire/sysfs_slave.c
> +++ b/drivers/soundwire/sysfs_slave.c
> @@ -214,7 +214,7 @@ static const struct attribute_group dp0_group = {
>  	.name = "dp0",
>  };
>  
> -static const struct attribute_group *slave_groups[] = {
> +const struct attribute_group *sdw_attr_groups[] = {
>  	&slave_attr_group,
>  	&sdw_slave_dev_attr_group,
>  	&dp0_group,
> @@ -225,10 +225,6 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave)
>  {
>  	int ret;
>  
> -	ret = devm_device_add_groups(&slave->dev, slave_groups);
> -	if (ret < 0)
> -		return ret;
> -

The subtle scary thing about this usage in general is that this makes
the sysfs attributes live before it is known that the driver probe
succeeded. So beyond the cleanup of using devm to do something that the
driver-core already handles it removes a hard to reason about race
compared to the well known lifetime of driver->dev_groups.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>