On Mon, Jun 14, 2021 at 05:08:40PM +0200, Christoph Hellwig wrote:
@@ -679,8 +666,6 @@ static int really_probe(struct device *dev, struct device_driver *drv) dev->pm_domain->dismiss(dev); pm_runtime_reinit(dev); dev_pm_set_driver_flags(dev, 0);
- if (probe_ret == -EPROBE_DEFER)
driver_deferred_probe_add_trigger(dev, local_trigger_count);done:
I like the new arrangement - however I'm looking at the ordering relative to this:
atomic_dec(&probe_count); wake_up_all(&probe_waitqueue);
And wondering if the idea is that driver_deferred_probe_add_trigger() is supposed to be enclosed by the atomic, so that the device_block_probing() / wait_for_device_probe() sequence is actually a fence against queuing new work?
Which is suggesting that the other driver_deferred_probe_add_trigger() at the top of really_probe is already ordered wrong?
Although, if that is the idea the wait_for_device_probe() doesn't look entirely sequenced right..
It looks easy enough to fix by moving the probe_count up:
+static int driver_probe_device(struct device_driver *drv, struct device *dev) +{
- int trigger_count = atomic_read(&deferred_trigger_count);
- int ret;
- ret = __driver_probe_device(drv, dev);
- if (ret == -EPROBE_DEFER || ret == EPROBE_DEFER) {
driver_deferred_probe_add(dev);/** Did a trigger occur while probing? Need to re-trigger if yes*/if (trigger_count != atomic_read(&deferred_trigger_count) &&!defer_all_probes)driver_deferred_probe_trigger();- }
into here?
I didn't see a reason why it couldn't enclose the pm stuff too..
Jason