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 = {
