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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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);
