On 11/21/22 10:24, Richard Fitzgerald wrote:
> Don't hold sdw_dev_lock while calling the peripheral driver
> probe() and remove() callbacks.
>
> Holding sdw_dev_lock around the probe() and remove() calls
> causes a theoretical mutex inversion which lockdep will
> assert on. The peripheral driver probe will probably register
> a soundcard, which will take ALSA and ASoC locks. During
It's extremely unlikely that a peripheral driver would register a sound
card, this is what machine drivers do.
Which leads me to the question: is this a real problem?
Or did you mean 'register components', and if yes what would the problem
with lockdep be?
> normal operation a runtime resume suspend can be triggered
> while these locks are held and will then take sdw_dev_lock.
>
> It's not necessary to hold sdw_dev_lock when calling the
> probe() and remove(), it is only used to prevent the bus core
> calling the driver callbacks if there isn't a driver or the
> driver is removing.
> If sdw_dev_lock is held while setting and clearing the
> 'probed' flag this is sufficient to guarantee the safety of
> callback functions.
not really, the 'probed' flag was kept for convenience. what this lock
really protects is the dereferencing of ops after the driver .remove
happens.
> The potential race of a bus event happening while probe() is
> executing is the same as the existing race of the bus event
> handler taking the mutex first and processing the event
> before probe() can run. In both cases the event has already
> happened before the driver is probed and ready to accept
> callbacks.
Sorry, I wasn't able to parse the first sentence in this paragraph. what
'existing race' are you referring to?
>
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/soundwire/bus_type.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 04b3529f8929..963498db0fd2 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -105,20 +105,19 @@ static int sdw_drv_probe(struct device *dev)
> if (ret)
> return ret;
>
> - mutex_lock(&slave->sdw_dev_lock);
> -
> ret = drv->probe(slave, id);
> if (ret) {
> name = drv->name;
> if (!name)
> name = drv->driver.name;
> - mutex_unlock(&slave->sdw_dev_lock);
>
> dev_err(dev, "Probe of %s failed: %d\n", name, ret);
> dev_pm_domain_detach(dev, false);
> return ret;
> }
>
> + mutex_lock(&slave->sdw_dev_lock);
> +
> /* device is probed so let's read the properties now */
> if (drv->ops && drv->ops->read_prop)
> drv->ops->read_prop(slave);
> @@ -167,14 +166,12 @@ static int sdw_drv_remove(struct device *dev)
> int ret = 0;
>
> mutex_lock(&slave->sdw_dev_lock);
> -
> slave->probed = false;
> + mutex_unlock(&slave->sdw_dev_lock);
>
> if (drv->remove)
> ret = drv->remove(slave);
>
> - mutex_unlock(&slave->sdw_dev_lock);
> -
> dev_pm_domain_detach(dev, false);
>
> return ret;
