Handling devices that don't have a bus

Handling devices that don't have a bus

[Alan Stern]

Greg et al.:

I recently tried running the dummy_hcd driver for the first time in a 
while, and it crashed when the gadget driver was unloaded.  It turns out 
this was because the gadget's embedded struct device is registered without 
a bus, which triggers an oops when the device's driver is unbound.  The 
oops could be fixed by doing this:

Index: usb-2.6/drivers/base/dd.c
===================================================================
--- usb-2.6.orig/drivers/base/dd.c
+++ usb-2.6/drivers/base/dd.c
@@ -209,7 +209,7 @@ static void __device_release_driver(stru
 		sysfs_remove_link(&dev->kobj, "driver");
 		klist_remove(&dev->knode_driver);
 
-		if (dev->bus->remove)
+		if (dev->bus && dev->bus->remove)
 			dev->bus->remove(dev);
 		else if (drv->remove)
 			drv->remove(dev);

but I'm not so sure this is the right approach.  (Russell wrote the line 
that this would change; that's why I have CC'ed him.)  Is the current 
policy that every device is supposed to belong to a bus?

If gadgets were registered on a bus, you would expect it to be the bus of
their parent USB device controllers.  As it happens, most of the UDC
drivers don't register their gadgets in sysfs at all.  dummy_hcd and
net2280 are exceptions.  Presumably this same oops would affect net2280
but I haven't tried it.

Part of the problem here is that most of the USB controllers are platform
devices and so belong on the platform bus.  That's true of dummy_hcd.  
But struct usb_gadget contains an embedded struct device, not an embedded
struct platform_device... so the gadget _can't_ be registered on its 
parent's bus.

I suppose David could change things so that usb_gadget does contain a
platform_device.  But then what about the net2280, which is a PCI device
rather than a platform device?  Would it want to register its child on the
platform bus?

What's the right thing to do here?

Alan Stern

Re: Handling devices that don't have a bus

[David Brownell]

On Thursday 30 March 2006 12:45 pm, Alan Stern wrote:

> What's the right thing to do here?

I suppose one solution would be to use a class device, but that
gets into the utter pointlessness of having classes that may
never have more than a single member ... :)

Re: Handling devices that don't have a bus

[Greg KH]

On Thu, Mar 30, 2006 at 03:45:50PM -0500, Alan Stern wrote:
> Greg et al.:
> 
> I recently tried running the dummy_hcd driver for the first time in a 
> while, and it crashed when the gadget driver was unloaded.  It turns out 
> this was because the gadget's embedded struct device is registered without 
> a bus, which triggers an oops when the device's driver is unbound.  The 
> oops could be fixed by doing this:

Why not make the dummy gadget a platform device?  That should keep this
from happening, right?

> Index: usb-2.6/drivers/base/dd.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/dd.c
> +++ usb-2.6/drivers/base/dd.c
> @@ -209,7 +209,7 @@ static void __device_release_driver(stru
>  		sysfs_remove_link(&dev->kobj, "driver");
>  		klist_remove(&dev->knode_driver);
>  
> -		if (dev->bus->remove)
> +		if (dev->bus && dev->bus->remove)
>  			dev->bus->remove(dev);
>  		else if (drv->remove)
>  			drv->remove(dev);
> 
> but I'm not so sure this is the right approach.  (Russell wrote the line 
> that this would change; that's why I have CC'ed him.)  Is the current 
> policy that every device is supposed to belong to a bus?
> 
> If gadgets were registered on a bus, you would expect it to be the bus of
> their parent USB device controllers.  As it happens, most of the UDC
> drivers don't register their gadgets in sysfs at all.  dummy_hcd and
> net2280 are exceptions.  Presumably this same oops would affect net2280
> but I haven't tried it.
> 
> Part of the problem here is that most of the USB controllers are platform
> devices and so belong on the platform bus.  That's true of dummy_hcd.  
> But struct usb_gadget contains an embedded struct device, not an embedded
> struct platform_device... so the gadget _can't_ be registered on its 
> parent's bus.

ah, ick :(

> I suppose David could change things so that usb_gadget does contain a
> platform_device.  But then what about the net2280, which is a PCI device
> rather than a platform device?  Would it want to register its child on the
> platform bus?
> 
> What's the right thing to do here?

I think your patch is the right thing.  Care to resend it with a proper
Signed-off-by: line so I can apply it?

thanks,

greg k-h

Re: Handling devices that don't have a bus

[Russell King]

On Thu, Mar 30, 2006 at 02:26:26PM -0800, Greg KH wrote:
> On Thu, Mar 30, 2006 at 03:45:50PM -0500, Alan Stern wrote:
> > Greg et al.:
> > 
> > I recently tried running the dummy_hcd driver for the first time in a 
> > while, and it crashed when the gadget driver was unloaded.  It turns out 
> > this was because the gadget's embedded struct device is registered without 
> > a bus, which triggers an oops when the device's driver is unbound.  The 
> > oops could be fixed by doing this:
> 
> Why not make the dummy gadget a platform device?  That should keep this
> from happening, right?
> 
> > Index: usb-2.6/drivers/base/dd.c
> > ===================================================================
> > --- usb-2.6.orig/drivers/base/dd.c
> > +++ usb-2.6/drivers/base/dd.c
> > @@ -209,7 +209,7 @@ static void __device_release_driver(stru
> >  		sysfs_remove_link(&dev->kobj, "driver");
> >  		klist_remove(&dev->knode_driver);
> >  
> > -		if (dev->bus->remove)
> > +		if (dev->bus && dev->bus->remove)
> >  			dev->bus->remove(dev);
> >  		else if (drv->remove)
> >  			drv->remove(dev);
> > 
> > but I'm not so sure this is the right approach.  (Russell wrote the line 
> > that this would change; that's why I have CC'ed him.)  Is the current 
> > policy that every device is supposed to belong to a bus?

If a device belongs to a bus, dev->bus will be non-NULL.  I don't see
what "every device is supposed to belong to a bus" fits with the problem.

> > Part of the problem here is that most of the USB controllers are platform
> > devices and so belong on the platform bus.  That's true of dummy_hcd.  
> > But struct usb_gadget contains an embedded struct device, not an embedded
> > struct platform_device... so the gadget _can't_ be registered on its 
> > parent's bus.
> 
> ah, ick :(

If the device's dev->bus is NULL, we don't register it on the parents
bus, but we do register it in the device tree as a child of the parent
device.

I think there's confusion here.

> I think your patch is the right thing.  Care to resend it with a proper
> Signed-off-by: line so I can apply it?

First lets sort out the confusion before applying any patches.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Handling devices that don't have a bus

[Russell King]

On Thu, Mar 30, 2006 at 03:45:50PM -0500, Alan Stern wrote:
> I recently tried running the dummy_hcd driver for the first time in a 
> while, and it crashed when the gadget driver was unloaded.  It turns out 
> this was because the gadget's embedded struct device is registered without 
> a bus, which triggers an oops when the device's driver is unbound.  The 
> oops could be fixed by doing this:

Can you provide the oops itself please?

> Part of the problem here is that most of the USB controllers are platform
> devices and so belong on the platform bus.

You're making a connection where no such connection exists.  Devices
are only part of the platform bus if they explicitly want to be (in
much the same way that devices are only part of the PCI bus if they
explicitly set dev->bus to be the PCI bus.)

> But struct usb_gadget contains an embedded struct device, not an embedded
> struct platform_device... so the gadget _can't_ be registered on its 
> parent's bus.

>From what I can see, the embedded device does not belong to any bus at
all since dev->bus is NULL.  Hence, I don't think it's the embedded
struct device which is causing the problem here.

It would be good to see the entire oops to see what's going on.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Handling devices that don't have a bus

[Alan Stern]

On Sat, 1 Apr 2006, Russell King wrote:

> On Thu, Mar 30, 2006 at 03:45:50PM -0500, Alan Stern wrote:
> > I recently tried running the dummy_hcd driver for the first time in a 
> > while, and it crashed when the gadget driver was unloaded.  It turns out 
> > this was because the gadget's embedded struct device is registered without 
> > a bus, which triggers an oops when the device's driver is unbound.  The 
> > oops could be fixed by doing this:
> 
> Can you provide the oops itself please?

No, I don't have it any more.  But I can tell you exactly where the oops 
occurred.  In __device_release_driver() (in drivers/base/dd.c), this line 
you added:

		if (dev->bus->remove)

crashed because dev->bus was NULL.  My patch changes the line to:

		if (dev->bus && dev->bus->remove)

Any objection to that?

> > Part of the problem here is that most of the USB controllers are platform
> > devices and so belong on the platform bus.
> 
> You're making a connection where no such connection exists.  Devices
> are only part of the platform bus if they explicitly want to be (in
> much the same way that devices are only part of the PCI bus if they
> explicitly set dev->bus to be the PCI bus.)

I think you have misunderstood my point.  Yes, devices are part of the
platform bus only if they explicitly want to be.  My point was that even
though they _do_ want to be on the platform bus, in this situation they
_can't_ because they are forced to register a struct device, not a struct
platform_device.  The choice is not up to the driver; it is determined by
the USB Gadget framework.  (See the definition of struct usb_gadget in 
include/linux/usb_gadget.h -- there's an embedded struct device, not an 
embedded struct platform_device.)

> > But struct usb_gadget contains an embedded struct device, not an embedded
> > struct platform_device... so the gadget _can't_ be registered on its 
> > parent's bus.
> 
> From what I can see, the embedded device does not belong to any bus at
> all since dev->bus is NULL.  Hence, I don't think it's the embedded
> struct device which is causing the problem here.
> 
> It would be good to see the entire oops to see what's going on.

Yes, the device did not belong to any bus.  Hence the unguarded reference
to dev->bus->remove caused the oops.  If it's legal for devices not to
belong to a bus, then my patch should be applied to guard the reference.  
If it's not legal then the Gadget framework needs to be changed.

Alan Stern

Re: Handling devices that don't have a bus

[Russell King]

On Sat, Apr 01, 2006 at 11:46:26AM -0500, Alan Stern wrote:
> On Sat, 1 Apr 2006, Russell King wrote:
> 
> > On Thu, Mar 30, 2006 at 03:45:50PM -0500, Alan Stern wrote:
> > > I recently tried running the dummy_hcd driver for the first time in a 
> > > while, and it crashed when the gadget driver was unloaded.  It turns out 
> > > this was because the gadget's embedded struct device is registered without 
> > > a bus, which triggers an oops when the device's driver is unbound.  The 
> > > oops could be fixed by doing this:
> > 
> > Can you provide the oops itself please?
> 
> No, I don't have it any more.  But I can tell you exactly where the oops 
> occurred.  In __device_release_driver() (in drivers/base/dd.c), this line 
> you added:
> 
> 		if (dev->bus->remove)
> 
> crashed because dev->bus was NULL.  My patch changes the line to:
> 
> 		if (dev->bus && dev->bus->remove)
> 
> Any objection to that?

Nope.

> I think you have misunderstood my point.

Yes I did.  Oops.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Handling devices that don't have a bus

[David Brownell]

On Saturday 01 April 2006 8:46 am, Alan Stern wrote:

> I think you have misunderstood my point.  Yes, devices are part of the
> platform bus only if they explicitly want to be.  My point was that even
> though they _do_ want to be on the platform bus, 

I'm not clear on why it would want to be on the platform bus;
what would its inner platform-ness consist of?

There really isn't a "bus" that makes much sense for such a
singleton device to sit on.  


> in this situation they 
> _can't_ because they are forced to register a struct device, not a struct
> platform_device.  The choice is not up to the driver; it is determined by
> the USB Gadget framework. 

Re: Handling devices that don't have a bus

[Alan Stern]

On Sat, 1 Apr 2006, David Brownell wrote:

> On Saturday 01 April 2006 8:46 am, Alan Stern wrote:
> 
> > I think you have misunderstood my point.  Yes, devices are part of the
> > platform bus only if they explicitly want to be.  My point was that even
> > though they _do_ want to be on the platform bus, 
> 
> I'm not clear on why it would want to be on the platform bus;
> what would its inner platform-ness consist of?
> 
> There really isn't a "bus" that makes much sense for such a
> singleton device to sit on.  

I suppose you could argue that the "platform-ness" is related to the fact
that a gadget is a sort-of synthetic device, made up entirely of software.  
Not very convincing, I admit.

The real reason for wanting to use the platform bus is because USB gadgets
don't really belong anywhere else, and the platform bus is supposed to be
(among other things) the bus for devices that don't have a home of their
own.  It took over that role from the old "misc" bus, or whatever it used
to be called.

Of course, there's always the possibility of creating a "usb-gadget" bus
-- but that really would be going over the top!

Alan Stern