Re: [PATCH] Remove process freezer from suspend to RAM pathway

Alan Stern
2007-07-09T16:03:29Z

On Mon, 9 Jul 2007, Benjamin Herrenschmidt wrote:

> > This will be up to the people responsible for the subsystems. I can
> > take care of USB.
>
> USB is not that much of a problem in the sense that for most "leaf"
> drivers, USB is a provider (ie, the bus they sit on), not the client
> (like the network stack is to network drivers).

That's why I thought I would be able to handle it! :-)

> There is still the race of:
>
> drivers_sysfs_write()
> try_to_icebox()
> <---- <sleep request gets here>
> hit hardware
>
> Those are akin, in some ways, to the freezer races. Some kind of RCU
> might take care of them if we enable the icebox, then wait for all tasks
> to hit an explicit schedule point once (or return to userland). That
> would mean that drivers need to try_to_icebox() again if they do
> something that may schedule (such as __get_user). So it's not a magic
> solution, it has issues, but it can handle a lot of the simplest cases.

It depends on what kind of race you're worried about. In general a
sysfs access that causes I/O isn't much different from a char device's
read or write. For such cases the driver would have to do something
like this:

drivers_sysfs_write()
{
restart:
mutex_lock(private_io_mutex);
if (device is suspended) {
mutex_unlock(private_io_mutex);
icebox();
goto restart;
}
... hit hardware ...
mutex_unlock(private_io_mutex);
}

And of course the suspend method would have to acquire the
private_io_mutex. Some drivers might be able to use the device
semaphore instead of adding a new mutex.

I'm more concerned about races involving device registration. The best
solution I can come up with is to use an rwsem:

drivers_sysfs_write()
{
down_read(private_rwsem);
... register/unregister devices ...
up_read(private_rwsem);
}

where the suspend notifer callout would do down_write() and the resume
callout would do up_write(). Can you suggest anything better?

> > Drivers are already prepared for device registration to fail (or they
> > ought to be), so this change shouldn't knock the bottom out of things.
> > device_add() isn't on a hot path, so adding an extra check and
> > srcu_read_lock() won't hurt.
>
> True. Also, bus drivers could just flag the port with something saying
> "try registering again later". Don't underestimate the power of "try
> later" constructs :-)

-ESUSPENDING would naturally imply that the caller should retry after
the resume.

Alan Stern