Re: [PATCH 1/3 RFC] Driver core: Add offline/online device operations

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Apr 30, 2013 at 01:59:55PM +0200, Rafael J. Wysocki wrote:
> On Monday, April 29, 2013 04:10:19 PM Greg Kroah-Hartman wrote:
> > On Mon, Apr 29, 2013 at 02:26:56PM +0200, Rafael J. Wysocki wrote:
> > > +static inline bool device_supports_offline(struct device *dev)
> > > +{
> > > +	return dev->bus && dev->bus->offline && dev->bus->online;
> > 
> > Wouldn't it be easier for us to also check offline_disabled here as
> > well?  That would save the extra check when we go to create the sysfs
> > file.
> 
> Yes, it would, but I want device_offline() to return an error in case
> when offline_disabled is set while the above returns 'true'.  If that check
> were folded into device_supports_offline(), device_offline() would return 0
> in that case.

Ok, that makes sense.

> > > +static ssize_t store_online(struct device *dev, struct device_attribute *attr,
> > > +			    const char *buf, size_t count)
> > > +{
> > > +	int ret;
> > > +
> > > +	lock_device_offline();
> > > +	switch (buf[0]) {
> > > +	case '0':
> > > +		ret = device_offline(dev);
> > > +		break;
> > > +	case '1':
> > > +		ret = device_online(dev);
> > > +		break;
> > 
> > Should we also accept 'y', 'Y', 'n', and 'N', like most boolean sysfs
> > files do?  I think we even have a kernel helper function for it
> > somewhere...
> 
> Yes, we do, but it doesn't accept '0' as false. :-)

It doesn't?  That's crazy, and should be fixed.

> Well, I suppose I can modify that function and use it here.  What do
> you think?

Yes please.

> > > +static DEFINE_MUTEX(device_offline_lock);
> > > +
> > > +void lock_device_offline(void)
> > > +{
> > > +	mutex_lock(&device_offline_lock);
> > > +}
> > > +
> > > +void unlock_device_offline(void)
> > > +{
> > > +	mutex_unlock(&device_offline_lock);
> > > +}
> > 
> > Why have functions?  Why not just do the mutex_lock/unlock instead
> > everywhere?
> 
> Ah, that's something I forgot to write about in the changelog.
> 
> Patch [3/3] depends on that, because it has to take device_offline_lock around
> a larger piece of code.  Specifically, it needs to put acpi_bus_trim() under
> that lock too to avoid situations in which a previously offlined device would
> be onlined from user space right before (or worse yet during) acpi_bus_trim()
> (which would then remove it without offlining).
> 
> It is not necessary in [1/3], so I can move it to [3/3] if that's better.

No, that makes sense, but doesn't that mean you need to export the
symbols as well?  Oh, nevermind, acpi can't be a module, that's fine.

thanks,

greg k-h