Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Alan Stern]

On Thu, 8 Oct 2015, Grygorii Strashko wrote:

> It is unsafe [1] if probing of devices will happen during suspend or
> hibernation and system behavior will be unpredictable in this case.
> So, lets prohibit device's probing in dpm_prepare() and defer their

s/lets/let's/, and same for the comment in the code.

> probing instead. The normal behavior will be restored in
> dpm_complete().


> @@ -172,6 +179,26 @@ static void driver_deferred_probe_trigger(void)
>  }
>  
>  /**
> + * device_defer_all_probes() - Enable/disable probing of devices
> + * @enable:  Enable/disable probing of devices
> + *
> + * if @enable = true
> + *	It will disable probing of devices and defer their probes.
> + * otherwise
> + *	It will restore normal behavior and trigger re-probing of deferred
> + *	devices.
> + */
> +void device_defer_all_probes(bool enable)
> +{
> +	defer_all_probes = enable;
> +	if (enable)
> +		/* sync with probes to avoid any races. */
> +		wait_for_device_probe();
> +	else
> +		driver_deferred_probe_trigger();
> +}

Some people might prefer to see two separate functions, an enable
routine and a disable routine.  I don't much care.

> @@ -277,9 +304,15 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>  
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
> -	int ret = 0;
> +	int ret = -EPROBE_DEFER;
>  	int local_trigger_count = atomic_read(&deferred_trigger_count);
>  
> +	if (defer_all_probes) {
> +		dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
> +		driver_deferred_probe_add(dev);
> +		return ret;
> +	}

In theory there's a race here.  If one CPU sets defer_all_probes, the 
new value might not be perceived by another CPU until a little while 
later.  Is there an easy way to insure that this race won't cause any 
problems?

Or do we already know that when this mechanism gets used, the system is 
already running on a single CPU?  I forget when that happens.

> @@ -1624,6 +1627,16 @@ int dpm_prepare(pm_message_t state)
>  	trace_suspend_resume(TPS("dpm_prepare"), state.event, true);
>  	might_sleep();
>  
> +	/* Give a chance for the known devices to complete their probing. */
> +	wait_for_device_probe();
> +	/*
> +	 * It is unsafe if probing of devices will happen during suspend or
> +	 * hibernation and system behavior will be unpredictable in this case.
> +	 * So, lets prohibit device's probing here and defer their probes
> +	 * instead. The normal behavior will be restored in dpm_complete().
> +	 */
> +	device_defer_all_probes(true);

Don't you want to call these two functions in the opposite order?  
First prevent new probes from occurring, then wait for any probes that 
are already in progress?  The way you have it here, a new probe could 
start between these two lines.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/