| Message ID | 20220907101402.4685-8-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: Fix driver removal | expand |
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@opensource.cirrus.com> > --- > 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 = {
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 = {
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 Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- drivers/soundwire/bus.c | 30 ++++++++++++++++++++++++++---- drivers/soundwire/slave.c | 21 +++++++++++++++++---- 2 files changed, 43 insertions(+), 8 deletions(-)