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
