Driver bind/unbind and __devinit

Driver bind/unbind and __devinit

[Dmitry Torokhov]

Hi,

Many drivers have their probe routines declared as __devinit which is
a no-op unless CONFIG_HOTPLUG is set. However driver's bind/unbind
attributes are created unconditionally, as fas as I can see. Would not
it cause an oops if someone tries to use these attributes with
CONFIG_HOTPLUG=N? Am I missing something?

Also, unbind implementation does not seem safe - we check the driver
before taking device's semaphore so we risk unbinding wrong driver (in
the unlikely event that we manage to unbind and bind another driver in
another thread).

--
Dmitry

Re: Driver bind/unbind and __devinit

[Greg KH]

On Thu, Dec 08, 2005 at 04:14:58PM -0500, Dmitry Torokhov wrote:
> Hi,
> 
> Many drivers have their probe routines declared as __devinit which is
> a no-op unless CONFIG_HOTPLUG is set. However driver's bind/unbind
> attributes are created unconditionally, as fas as I can see. Would not
> it cause an oops if someone tries to use these attributes with
> CONFIG_HOTPLUG=N? Am I missing something?

You are missing the CONFIG_HOTPLUG checks around the functions that add
and check the device ids from these sysfs files.  If CONFIG_HOTPLUG is
not enabled, those files do not do anything.

In 2.6.16, CONFIG_HOTPLUG is moving under CONFIG_EMBEDDED, so the odds
of people disabling it are going to be pretty small now.

> Also, unbind implementation does not seem safe - we check the driver
> before taking device's semaphore so we risk unbinding wrong driver (in
> the unlikely event that we manage to unbind and bind another driver in
> another thread).

Do you have a suggestion as to how to fix this?

thanks,

greg k-h

Re: Driver bind/unbind and __devinit

[Dmitry Torokhov]

On 12/8/05, Greg KH <gregkh@suse.de> wrote:
> On Thu, Dec 08, 2005 at 04:14:58PM -0500, Dmitry Torokhov wrote:
> > Hi,
> >
> > Many drivers have their probe routines declared as __devinit which is
> > a no-op unless CONFIG_HOTPLUG is set. However driver's bind/unbind
> > attributes are created unconditionally, as fas as I can see. Would not
> > it cause an oops if someone tries to use these attributes with
> > CONFIG_HOTPLUG=N? Am I missing something?
>
> You are missing the CONFIG_HOTPLUG checks around the functions that add
> and check the device ids from these sysfs files.  If CONFIG_HOTPLUG is
> not enabled, those files do not do anything.
>

I am slow today... I don't see any dependencies on CONFIG_HOTPLUG in
drivers/base... Or you talking about one particular subsystem that
handles this correctly?

> In 2.6.16, CONFIG_HOTPLUG is moving under CONFIG_EMBEDDED, so the odds
> of people disabling it are going to be pretty small now.
>

I dont think this is a valid argument.

> > Also, unbind implementation does not seem safe - we check the driver
> > before taking device's semaphore so we risk unbinding wrong driver (in
> > the unlikely event that we manage to unbind and bind another driver in
> > another thread).
>
> Do you have a suggestion as to how to fix this?
>

I think we could take the semaphore before checking driver and then
use __device_release_driver(). But we'd need to make it global or move
bind/unbind code into drivers/base/dd.c

--
Dmitry

Re: Driver bind/unbind and __devinit

[Greg KH]

On Thu, Dec 08, 2005 at 05:22:12PM -0500, Dmitry Torokhov wrote:
> On 12/8/05, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Dec 08, 2005 at 04:14:58PM -0500, Dmitry Torokhov wrote:
> > > Hi,
> > >
> > > Many drivers have their probe routines declared as __devinit which is
> > > a no-op unless CONFIG_HOTPLUG is set. However driver's bind/unbind
> > > attributes are created unconditionally, as fas as I can see. Would not
> > > it cause an oops if someone tries to use these attributes with
> > > CONFIG_HOTPLUG=N? Am I missing something?
> >
> > You are missing the CONFIG_HOTPLUG checks around the functions that add
> > and check the device ids from these sysfs files.  If CONFIG_HOTPLUG is
> > not enabled, those files do not do anything.
> >
> 
> I am slow today... I don't see any dependencies on CONFIG_HOTPLUG in
> drivers/base... Or you talking about one particular subsystem that
> handles this correctly?

Ugh, very sorry about that, I was thinking of the USB and PCI new_id
stuff.  You are right.

Yes, bind happening after the __init data section is thrown away, if
CONFIG_HOTPLUG is not enabled would be a bad thing.  But unbind can
stay.  I'll go make up a patch for that.

> > > Also, unbind implementation does not seem safe - we check the driver
> > > before taking device's semaphore so we risk unbinding wrong driver (in
> > > the unlikely event that we manage to unbind and bind another driver in
> > > another thread).
> >
> > Do you have a suggestion as to how to fix this?
> >
> 
> I think we could take the semaphore before checking driver and then
> use __device_release_driver(). But we'd need to make it global or move
> bind/unbind code into drivers/base/dd.c

I don't have a problem moving the code if it makes it easier.  Have a
patch?  :)

thanks,

greg k-h

Re: Driver bind/unbind and __devinit

[Dmitry Torokhov]

On 12/8/05, Greg KH <gregkh@suse.de> wrote:
> On Thu, Dec 08, 2005 at 05:22:12PM -0500, Dmitry Torokhov wrote:
> > On 12/8/05, Greg KH <gregkh@suse.de> wrote:
> > > On Thu, Dec 08, 2005 at 04:14:58PM -0500, Dmitry Torokhov wrote:
> > > > Hi,
> > > >
> > > > Many drivers have their probe routines declared as __devinit which is
> > > > a no-op unless CONFIG_HOTPLUG is set. However driver's bind/unbind
> > > > attributes are created unconditionally, as fas as I can see. Would not
> > > > it cause an oops if someone tries to use these attributes with
> > > > CONFIG_HOTPLUG=N? Am I missing something?
> > >
> > > You are missing the CONFIG_HOTPLUG checks around the functions that add
> > > and check the device ids from these sysfs files.  If CONFIG_HOTPLUG is
> > > not enabled, those files do not do anything.
> > >
> >
> > I am slow today... I don't see any dependencies on CONFIG_HOTPLUG in
> > drivers/base... Or you talking about one particular subsystem that
> > handles this correctly?
>
> Ugh, very sorry about that, I was thinking of the USB and PCI new_id
> stuff.  You are right.
>
> Yes, bind happening after the __init data section is thrown away, if
> CONFIG_HOTPLUG is not enabled would be a bad thing.  But unbind can
> stay.  I'll go make up a patch for that.
>

Unbind may invoke ->remove code which is __devexit[_p] and may be also
discarded if !MODULE && !HOTPLUG.

> > > > Also, unbind implementation does not seem safe - we check the driver
> > > > before taking device's semaphore so we risk unbinding wrong driver (in
> > > > the unlikely event that we manage to unbind and bind another driver in
> > > > another thread).
> > >
> > > Do you have a suggestion as to how to fix this?
> > >
> >
> > I think we could take the semaphore before checking driver and then
> > use __device_release_driver(). But we'd need to make it global or move
> > bind/unbind code into drivers/base/dd.c
>
> I don't have a problem moving the code if it makes it easier.  Have a
> patch?  :)
>

Just handwaving for now ;)

--
Dmitry