linux-kernel - Re: [PATCH 1/7] soundwire: bus: Do not forcibly disable child pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f67f29bc-64af-52dc-a63f-3b74523c06b0@linux.intel.com>
Date:   Mon, 12 Sep 2022 12:43:55 +0200
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>, vkoul@...nel.org,
        yung-chuan.liao@...ux.intel.com, lgirdwood@...il.com,
        peter.ujfalusi@...ux.intel.com, ranjani.sridharan@...ux.intel.com,
        kai.vehmanen@...ux.intel.com, daniel.baluta@....com,
        sanyog.r.kale@...el.com, broonie@...nel.org
Cc:     alsa-devel@...a-project.org, sound-open-firmware@...a-project.org,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH 1/7] soundwire: bus: Do not forcibly disable child
 pm_runtime



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@...nsource.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);