Re: [PATCH 7/7] soundwire: bus: Fix premature removal of sdw_slave objects

[Pierre-Louis Bossart]

On 9/7/22 12:14, Richard Fitzgerald wrote:
> When the bus manager is removed sdw_bus_master_delete() should not
> be deleting the struct sdw_slave objects until the bus manager has
> been stopped. The first step of removing child drivers should only
> be calling device_unregister() on the child. The counterpart to
> sdw_drv_probe() is sdw_drv_remove(), not sdw_delete_slave().
> 
> The sdw_slave objects are created by the bus manager probe() from
> ACPI/DT information. They are not created when a child driver probes
> so should not be deleted by a child driver remove.
> 
> Change-Id: I25cc145df12fdc7c126f8f594a5f76eedce25488

spurious Change-Id

> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/soundwire/bus.c   | 30 ++++++++++++++++++++++++++----
>  drivers/soundwire/slave.c | 21 +++++++++++++++++----
>  2 files changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 1327a312be86..5533eb589286 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -146,9 +146,8 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>  }
>  EXPORT_SYMBOL(sdw_bus_master_add);
>  
> -static int sdw_delete_slave(struct device *dev, void *data)
> +static int sdw_delete_slave(struct sdw_slave *slave)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>  	struct sdw_bus *bus = slave->bus;
>  
>  	sdw_slave_debugfs_exit(slave);
> @@ -163,7 +162,24 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  	list_del_init(&slave->node);
>  	mutex_unlock(&bus->bus_lock);
>  
> +	mutex_destroy(&slave->sdw_dev_lock);
> +	kfree(slave);
> +
> +	return 0;
> +}
> +
> +static int sdw_remove_child(struct device *dev, void *data)
> +{
> +	/*
> +	 * Do not remove the struct sdw_slave yet. This is created by
> +	 * the bus manager probe() from ACPI information and used by the
> +	 * bus manager to hold status of each peripheral. Its lifetime
> +	 * is that of the bus manager.
> +	 */
> +
> +	/* This will call sdw_drv_remove() */
>  	device_unregister(dev);
> +
>  	return 0;
>  }
>  
> @@ -171,16 +187,22 @@ static int sdw_delete_slave(struct device *dev, void *data)
>   * sdw_bus_master_delete() - delete the bus master instance
>   * @bus: bus to be deleted
>   *
> - * Remove the instance, delete the child devices.
> + * Remove the child devices, remove the master instance.
>   */
>  void sdw_bus_master_delete(struct sdw_bus *bus)
>  {
> -	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
> +	struct sdw_slave *slave, *tmp;
> +
> +	device_for_each_child(bus->dev, NULL, sdw_remove_child);
>  
>  	/* Children have been removed so it is now safe for the bus to stop */
>  	if (bus->ops->remove)
>  		bus->ops->remove(bus);
>  
> +	/* Now the bus is stopped it is safe to free things */
> +	list_for_each_entry_safe(slave, tmp, &bus->slaves, node)
> +		sdw_delete_slave(slave);
> +
>  	sdw_master_device_del(bus);
>  
>  	sdw_bus_debugfs_exit(bus);
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index c1c1a2ac293a..b6161d002b97 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -10,10 +10,23 @@
>  
>  static void sdw_slave_release(struct device *dev)
>  {
> -	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> -
> -	mutex_destroy(&slave->sdw_dev_lock);
> -	kfree(slave);
> +	/*
> +	 * The release() callback should not be empty
> +	 * (see Documentation/core-api/kobject.rst) but the ownership
> +	 * of struct sdw_slave is muddled. It is used for two separate
> +	 * purposes:
> +	 * 1) by the bus driver to track its own state information for
> +	 *    physical devices on the bus and found in ACPI/DT, whether
> +	 *    or not there is a child driver for it;
> +	 * 2) to hold the child driver object.
> +	 *
> +	 * The struct sdw_slave cannot be freed when the child driver
> +	 * is released because it is holding info used by the bus
> +	 * driver. It is freed when the bus driver is removed.
> +	 *
> +	 * Until the ownership issue is untangled this cannot free
> +	 * the struct sdw_slave object containing the child dev.
> +	 */
>  }
>  
>  struct device_type sdw_slave_type = {