diff mbox series

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

Message ID 2024013030-worsening-rocket-a3cb@gregkh
State New
Headers
Series Soundwire: clean up sysfs group creation |

Commit Message

Greg KH Jan. 30, 2024, 6:46 p.m. UTC 
  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(-)

Comments

Dan Williams Jan. 31, 2024, 5:37 a.m. UTC | #1 
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>
diff mbox series

Patch

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;
-
 	if (slave->prop.source_ports || slave->prop.sink_ports) {
 		ret = sdw_slave_sysfs_dpn_init(slave);
 		if (ret < 0)