| Message ID | 20220907101402.4685-2-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: Fix driver removal | expand |
On 9/7/22 12:13, Richard Fitzgerald wrote: > Do not call pm_runtime_disable() of a child driver in > sdw_delete_slave(). We really should never be trying to disable > another driver's pm_runtime - it is up to the child driver to > disable it or the core driver framework cleanup. The driver core > will runtime-resume a driver before calling its remove() so we > shouldn't break that. > > The patch that introduced this is > commit dff70572e9a3 ("soundwire: bus: disable pm_runtime in sdw_slave_delete") > which says: > > "prevent any race condition with the resume being executed after the > bus and slave devices are removed" > > The actual problem is that the bus driver is shutting itself down before > the child drivers have been removed, which is the wrong way around (see > for example I2C and SPI drivers). If this is fixed, the bus driver will > still be operational when the driver framework runtime_resumes the child > drivers to remove them. Then the bus driver will remove() and can shut > down safely. The description of the fix looks good, but "if this is fixed" is very confusing to me. Don't you have a dependency issue here? There should be first a patch to fix the bus issue and then remove this pm_runtime_disable second. > > Also note that the child drivers are not necessarily idle when the bus > driver is removed, so disabling their pm_runtime and stopping the bus > might break more than only their remove(). > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > --- > drivers/soundwire/bus.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index 0bcc2d161eb9..99429892221b 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -151,8 +151,6 @@ static int sdw_delete_slave(struct device *dev, void *data) > struct sdw_slave *slave = dev_to_sdw_dev(dev); > struct sdw_bus *bus = slave->bus; > > - pm_runtime_disable(dev); > - > sdw_slave_debugfs_exit(slave); > > mutex_lock(&bus->bus_lock);
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 0bcc2d161eb9..99429892221b 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -151,8 +151,6 @@ static int sdw_delete_slave(struct device *dev, void *data) struct sdw_slave *slave = dev_to_sdw_dev(dev); struct sdw_bus *bus = slave->bus; - pm_runtime_disable(dev); - sdw_slave_debugfs_exit(slave); mutex_lock(&bus->bus_lock);
Do not call pm_runtime_disable() of a child driver in sdw_delete_slave(). We really should never be trying to disable another driver's pm_runtime - it is up to the child driver to disable it or the core driver framework cleanup. The driver core will runtime-resume a driver before calling its remove() so we shouldn't break that. The patch that introduced this is commit dff70572e9a3 ("soundwire: bus: disable pm_runtime in sdw_slave_delete") which says: "prevent any race condition with the resume being executed after the bus and slave devices are removed" The actual problem is that the bus driver is shutting itself down before the child drivers have been removed, which is the wrong way around (see for example I2C and SPI drivers). If this is fixed, the bus driver will still be operational when the driver framework runtime_resumes the child drivers to remove them. Then the bus driver will remove() and can shut down safely. Also note that the child drivers are not necessarily idle when the bus driver is removed, so disabling their pm_runtime and stopping the bus might break more than only their remove(). Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- drivers/soundwire/bus.c | 2 -- 1 file changed, 2 deletions(-)