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@opensource.cirrus.com
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;