The remaining three patches contain miscellaneous fixes to usbfs.
This one fixes up the disconnect callback to only shoot down urbs
on the disconnected interface, and not on all interfaces. It also adds
a sanity check (this check is pointless because the interface could
never have been claimed in the first place if it failed, but I feel better
having it there).
devio.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c Wed Apr 14 12:18:20 2004
+++ b/drivers/usb/core/devio.c Wed Apr 14 12:18:20 2004
@@ -341,6 +341,7 @@
static void driver_disconnect(struct usb_interface *intf)
{
struct dev_state *ps = usb_get_intfdata (intf);
+ unsigned int ifnum = intf->altsetting->desc.bInterfaceNumber;
if (!ps)
return;
@@ -349,11 +350,12 @@
* all pending I/O requests; 2.6 does that.
*/
- clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
+ if (ifnum < 8*sizeof(ps->ifclaimed))
+ clear_bit(ifnum, &ps->ifclaimed);
usb_set_intfdata (intf, NULL);
/* force async requests to complete */
- destroy_all_async (ps);
+ destroy_async_on_interface(ps, ifnum);
}
struct usb_driver usbdevfs_driver = {
Am Mittwoch, 14. April 2004 12:45 schrieb Duncan Sands:
> The remaining three patches contain miscellaneous fixes to usbfs.
> This one fixes up the disconnect callback to only shoot down urbs
> on the disconnected interface, and not on all interfaces. It also adds
> a sanity check (this check is pointless because the interface could
> never have been claimed in the first place if it failed, but I feel better
> having it there).
Well, I don't. If you care about it, add a WARN_ON().
Checking without consequences is bad.
Regards
Oliver
On Wednesday 14 April 2004 15:30, Oliver Neukum wrote:
> Am Mittwoch, 14. April 2004 12:45 schrieb Duncan Sands:
> > The remaining three patches contain miscellaneous fixes to usbfs.
> > This one fixes up the disconnect callback to only shoot down urbs
> > on the disconnected interface, and not on all interfaces. It also adds
> > a sanity check (this check is pointless because the interface could
> > never have been claimed in the first place if it failed, but I feel
> > better having it there).
>
> Well, I don't. If you care about it, add a WARN_ON().
> Checking without consequences is bad.
If the check fails then you are scribbling over kernel memory. So the
consequences of the check failing are bad. Also, it is in a slow path.
Thus I prefer to have the check even if it is supposed to never fail. I
agree that a message should also be output.
Duncan.
On Wednesday 14 April 2004 15:30, Oliver Neukum wrote:
> Am Mittwoch, 14. April 2004 12:45 schrieb Duncan Sands:
> > The remaining three patches contain miscellaneous fixes to usbfs.
> > This one fixes up the disconnect callback to only shoot down urbs
> > on the disconnected interface, and not on all interfaces. It also adds
> > a sanity check (this check is pointless because the interface could
> > never have been claimed in the first place if it failed, but I feel
> > better having it there).
>
> Well, I don't. If you care about it, add a WARN_ON().
> Checking without consequences is bad.
Hi Oliver, how about this instead?
--- gregkh-2.6/drivers/usb/core/devio.c.orig 2004-04-14 16:02:44.000000000 +0200
+++ gregkh-2.6/drivers/usb/core/devio.c 2004-04-14 16:57:15.000000000 +0200
@@ -335,6 +341,7 @@
static void driver_disconnect(struct usb_interface *intf)
{
struct dev_state *ps = usb_get_intfdata (intf);
+ unsigned int ifnum = intf->altsetting->desc.bInterfaceNumber;
if (!ps)
return;
@@ -343,13 +350,15 @@
* all pending I/O requests; 2.6 does that.
*/
- /* prevent new I/O requests */
- ps->dev = 0;
- clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
+ if (ifnum < 8*sizeof(ps->ifclaimed))
+ clear_bit(ifnum, &ps->ifclaimed);
+ else
+ warn("interface number %u out of range", ifnum);
+
usb_set_intfdata (intf, NULL);
/* force async requests to complete */
- destroy_all_async (ps);
+ destroy_async_on_interface(ps, ifnum);
}
struct usb_driver usbdevfs_driver = {
> > Well, I don't. If you care about it, add a WARN_ON().
> > Checking without consequences is bad.
>
> Hi Oliver, how about this instead?
>
[..]
> - clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
> + if (ifnum < 8*sizeof(ps->ifclaimed))
> + clear_bit(ifnum, &ps->ifclaimed);
> + else
> + warn("interface number %u out of range", ifnum);
> +
I would prefer a real WARN_ON() so that the imbedded people compiling
for size are not affected.
Regards
Oliver
On Wednesday 14 April 2004 17:33, Oliver Neukum wrote:
> > > Well, I don't. If you care about it, add a WARN_ON().
> > > Checking without consequences is bad.
> >
> > Hi Oliver, how about this instead?
>
> [..]
>
> > - clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
> > + if (ifnum < 8*sizeof(ps->ifclaimed))
> > + clear_bit(ifnum, &ps->ifclaimed);
> > + else
> > + warn("interface number %u out of range", ifnum);
> > +
>
> I would prefer a real WARN_ON() so that the imbedded people compiling
> for size are not affected.
What do you mean? How is a real WARN_ON() better?
Duncan.
On Wed, 14 Apr 2004, Duncan Sands wrote:
> diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> --- a/drivers/usb/core/devio.c Wed Apr 14 12:18:20 2004
> +++ b/drivers/usb/core/devio.c Wed Apr 14 12:18:20 2004
> @@ -341,6 +341,7 @@
> static void driver_disconnect(struct usb_interface *intf)
> {
> struct dev_state *ps = usb_get_intfdata (intf);
> + unsigned int ifnum = intf->altsetting->desc.bInterfaceNumber;
>
> if (!ps)
> return;
> @@ -349,11 +350,12 @@
> * all pending I/O requests; 2.6 does that.
> */
>
> - clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
> + if (ifnum < 8*sizeof(ps->ifclaimed))
> + clear_bit(ifnum, &ps->ifclaimed);
> usb_set_intfdata (intf, NULL);
>
> /* force async requests to complete */
> - destroy_all_async (ps);
> + destroy_async_on_interface(ps, ifnum);
> }
>
> struct usb_driver usbdevfs_driver = {
Quite apart from the stylistic questions about sanity tests and so on,
this code contains a bug. It wasn't introduced by your patch; it was
there from before and I should have caught it earlier, along with a few
others.
The real problem is that the code in devio.c doesn't make a clear visual
distinction between interface number (i.e., desc.bInterfaceNumber) and
interface index (i.e., dev->actconfig->interface[index]). The two values
do not have to agree.
The claimintf(), releaseintf(), and checkintf() routines take an index as
argument, and the ifclaimed bitvector uses the same index. findintfif()
takes a number and returns the corresponding index, duplicating much of
the functionality of usb_ifnum_to_if(). Likewise, findintfep() returns an
index.
The code here in driver_disconnect() uses a number where it needs to use
an index.
Similarly, there's a typo in proc_releaseinterface(); the second argument
it passes to releaseintf() should be ret, not intf.
And in proc_submiturb(), the value stored in as->intf is an index when it
should be an interface number. Or possibly it could remain an index, but
then the value passed to destroy_async_on_interface() by
proc_releaseinterface() should be the index and not the number.
Alan Stern
> On Wed, 14 Apr 2004, Duncan Sands wrote:
> > diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > --- a/drivers/usb/core/devio.c Wed Apr 14 12:18:20 2004
> > +++ b/drivers/usb/core/devio.c Wed Apr 14 12:18:20 2004
> > @@ -341,6 +341,7 @@
> > static void driver_disconnect(struct usb_interface *intf)
> > {
> > struct dev_state *ps = usb_get_intfdata (intf);
> > + unsigned int ifnum = intf->altsetting->desc.bInterfaceNumber;
> >
> > if (!ps)
> > return;
> > @@ -349,11 +350,12 @@
> > * all pending I/O requests; 2.6 does that.
> > */
> >
> > - clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
> > + if (ifnum < 8*sizeof(ps->ifclaimed))
> > + clear_bit(ifnum, &ps->ifclaimed);
> > usb_set_intfdata (intf, NULL);
> >
> > /* force async requests to complete */
> > - destroy_all_async (ps);
> > + destroy_async_on_interface(ps, ifnum);
> > }
> >
> > struct usb_driver usbdevfs_driver = {
>
> Quite apart from the stylistic questions about sanity tests and so on,
> this code contains a bug. It wasn't introduced by your patch; it was
> there from before and I should have caught it earlier, along with a few
> others.
Hi Alan, it was introduced after your last devio.c fixes by the patch
"fix xsane breakage, hangs on device scan at launch" by someone
who will remain nameless :)
> The real problem is that the code in devio.c doesn't make a clear visual
> distinction between interface number (i.e., desc.bInterfaceNumber) and
> interface index (i.e., dev->actconfig->interface[index]). The two values
> do not have to agree.
>
> The claimintf(), releaseintf(), and checkintf() routines take an index as
> argument, and the ifclaimed bitvector uses the same index. findintfif()
> takes a number and returns the corresponding index, duplicating much of
> the functionality of usb_ifnum_to_if(). Likewise, findintfep() returns an
> index.
>
> The code here in driver_disconnect() uses a number where it needs to use
> an index.
>
> Similarly, there's a typo in proc_releaseinterface(); the second argument
> it passes to releaseintf() should be ret, not intf.
>
> And in proc_submiturb(), the value stored in as->intf is an index when it
> should be an interface number. Or possibly it could remain an index, but
> then the value passed to destroy_async_on_interface() by
> proc_releaseinterface() should be the index and not the number.
Good catch! I guess the index and the interface differ because interfaces are
not always consecutively numbered. Is that right? When can it happen?
Thanks,
Duncan.
On Wed, 14 Apr 2004, Duncan Sands wrote:
> > Quite apart from the stylistic questions about sanity tests and so on,
> > this code contains a bug. It wasn't introduced by your patch; it was
> > there from before and I should have caught it earlier, along with a few
> > others.
>
> Hi Alan, it was introduced after your last devio.c fixes by the patch
> "fix xsane breakage, hangs on device scan at launch" by someone
> who will remain nameless :)
Okay, that's a relief. Of course there's still the other two places. I
did check for such things a while back, but apparently I forgot to look at
all occurrences of "ifclaimed".
> > Similarly, there's a typo in proc_releaseinterface(); the second argument
> > it passes to releaseintf() should be ret, not intf.
> >
> > And in proc_submiturb(), the value stored in as->intf is an index when it
> > should be an interface number. Or possibly it could remain an index, but
> > then the value passed to destroy_async_on_interface() by
> > proc_releaseinterface() should be the index and not the number.
>
> Good catch! I guess the index and the interface differ because interfaces are
> not always consecutively numbered. Is that right? When can it happen?
Yes. Actually I spoke too strongly before; these aren't bugs, just things
that need to be changed.
Right now the configuration parsing code doesn't allow devices to have
interfaces that aren't numbered consecutively starting from 0, so there's
no problem. But I'm trying to update all the USB drivers to eliminate
such assumptions about device sanity. When that's done we will accept
funny interface numbers. There's a surprisingly large number of devices
that number their interfaces starting from 1, and we should be able to
handle them correctly.
Anyway, if you would like to fix these issues, my suggestion is to adopt a
variable-name scheme that makes it clear which things are interface
numbers and which are interface indices. (I don't want to go so far as to
advocate Hungarian notation, but the concept of using part of a variable's
name to indicate its type goes back at least to early Fortran with its
"names starting with letters from I-N are integers" convention.)
Alan Stern
> > I would prefer a real WARN_ON() so that the imbedded people compiling
> > for size are not affected.
>
> What do you mean? How is a real WARN_ON() better?
WARN_ON can be defined away to make a smaller kernel. Code that does
not use it takes away that option.
Regards
Oliver
On Wednesday 14 April 2004 22:39, Oliver Neukum wrote:
> > > I would prefer a real WARN_ON() so that the imbedded people compiling
> > > for size are not affected.
> >
> > What do you mean? How is a real WARN_ON() better?
>
> WARN_ON can be defined away to make a smaller kernel. Code that does
> not use it takes away that option.
Hi Oliver, I thought you meant that CONFIG_EMBEDDED made WARN_ON go away
(or something like that). If you just mean that it is easy to redefine WARN_ON by
hand, then all I can say is: it is also easy to redefine warn by hand! Anyway, I made
you the following patch:
--- gregkh-2.6/include/linux/usb.h.orig 2004-04-15 09:52:36.000000000 +0200
+++ gregkh-2.6/include/linux/usb.h 2004-04-15 09:56:30.000000000 +0200
@@ -1073,9 +1073,15 @@
#define dbg(format, arg...) do {} while (0)
#endif
-#define err(format, arg...) printk(KERN_ERR "%s: " format "\n" , __FILE__ , ## arg)
+#if !defined(CONFIG_EMBEDDED) || defined(DEBUG)
#define info(format, arg...) printk(KERN_INFO "%s: " format "\n" , __FILE__ , ## arg)
#define warn(format, arg...) printk(KERN_WARNING "%s: " format "\n" , __FILE__ , ## arg)
+#else
+#define info(format, arg...) do {} while (0)
+#define warn(format, arg...) do {} while (0)
+#endif
+
+#define err(format, arg...) printk(KERN_ERR "%s: " format "\n" , __FILE__ , ## arg)
#endif /* __KERNEL__ */
Am Donnerstag, 15. April 2004 10:05 schrieb Duncan Sands:
> On Wednesday 14 April 2004 22:39, Oliver Neukum wrote:
> > > > I would prefer a real WARN_ON() so that the imbedded people compiling
> > > > for size are not affected.
> > >
> > > What do you mean? How is a real WARN_ON() better?
> >
> > WARN_ON can be defined away to make a smaller kernel. Code that does
> > not use it takes away that option.
>
> Hi Oliver, I thought you meant that CONFIG_EMBEDDED made WARN_ON go away
> (or something like that). If you just mean that it is easy to redefine
> WARN_ON by hand, then all I can say is: it is also easy to redefine warn by
> hand! Anyway, I made you the following patch:
Yes, but I don't trust gcc to optimise away the 'if' if you redefine warn().
But there is another point. The embedded people deserve a single switch
to remove assertion checks. The purpose of macros like WARN_ON() is
easy and _central_ choice of debugging output vs. kernel size.
Regards
Oliver
> > Hi Oliver, I thought you meant that CONFIG_EMBEDDED made WARN_ON go away
> > (or something like that). If you just mean that it is easy to redefine
> > WARN_ON by hand, then all I can say is: it is also easy to redefine warn
> > by hand! Anyway, I made you the following patch:
>
> Yes, but I don't trust gcc to optimise away the 'if' if you redefine
> warn().
The "if" cannot be optimized away for the case in point, because it
does something (clears the bit) if it passes the test. If I used WARN_ON
then it would have to be WARN_ON(1) in the else branch of the if.
> But there is another point. The embedded people deserve a single switch
> to remove assertion checks. The purpose of macros like WARN_ON() is
> easy and _central_ choice of debugging output vs. kernel size.
This is not an argument against using USB's warn, it is an argument for
building warn on top of a centralized macro like WARN_ON or a friend.
All the best,
Duncan.
Am Donnerstag, 15. April 2004 10:47 schrieb Duncan Sands:
> > > Hi Oliver, I thought you meant that CONFIG_EMBEDDED made WARN_ON go
> > > away (or something like that). If you just mean that it is easy to
> > > redefine WARN_ON by hand, then all I can say is: it is also easy to
> > > redefine warn by hand! Anyway, I made you the following patch:
> >
> > Yes, but I don't trust gcc to optimise away the 'if' if you redefine
> > warn().
>
> The "if" cannot be optimized away for the case in point, because it
> does something (clears the bit) if it passes the test. If I used WARN_ON
> then it would have to be WARN_ON(1) in the else branch of the if.
True. You should use BUG_ON().
If this ever happens the device tree is screwed. There's no use going on.
> > But there is another point. The embedded people deserve a single switch
> > to remove assertion checks. The purpose of macros like WARN_ON() is
> > easy and _central_ choice of debugging output vs. kernel size.
>
> This is not an argument against using USB's warn, it is an argument for
> building warn on top of a centralized macro like WARN_ON or a friend.
It is an argument against USB making its own constructs. There's nothing
terribly specific about USB that would justify it. If the usual debug statements
are inadequate, improve them.
Regards
Oliver
> > The "if" cannot be optimized away for the case in point, because it
> > does something (clears the bit) if it passes the test. If I used WARN_ON
> > then it would have to be WARN_ON(1) in the else branch of the if.
>
> True. You should use BUG_ON().
>
> If this ever happens the device tree is screwed. There's no use going on.
I'm not sure - isn't it more likely that someone stuffed up in usbfs? BUG_ON
seems kind of harsh, since it will kill the hub thread. If it wasn't for that I
wouldn't hesitate to use BUG_ON here.
> > > But there is another point. The embedded people deserve a single switch
> > > to remove assertion checks. The purpose of macros like WARN_ON() is
> > > easy and _central_ choice of debugging output vs. kernel size.
> >
> > This is not an argument against using USB's warn, it is an argument for
> > building warn on top of a centralized macro like WARN_ON or a friend.
>
> It is an argument against USB making its own constructs. There's nothing
> terribly specific about USB that would justify it. If the usual debug
> statements are inadequate, improve them.
I don't see that there is anything wrong with USB using it's own constructs even
if they were just defined to be equal to some centralized macro (as they probably
should be). In fact there is an advantage: they can be modified for debugging
purposes (to add a backtrace for example) without disturbing the rest of the
kernel.
All the best,
Duncan.
Alan, do you have a suggestion for how best to go from
a struct usb_interface to an index?
Thanks,
Duncan.
On Saturday 17 April 2004 20:31, Duncan Sands wrote:
> Alan, do you have a suggestion for how best to go from
> a struct usb_interface to an index?
(though probably usbfs shouldn't use indices at all,
just interface numbers and struct usb_interface).
On Sat, 17 Apr 2004, Duncan Sands wrote:
> On Saturday 17 April 2004 20:31, Duncan Sands wrote:
> > Alan, do you have a suggestion for how best to go from
> > a struct usb_interface to an index?
The only way is by searching the interface list. That was part of the
reason I left findintfif() alone rather than replacing it with a call to
usb_ifnum_to_if().
> (though probably usbfs shouldn't use indices at all,
> just interface numbers and struct usb_interface).
It's true that code is generally cleaner using numbers rather than
indices, and that's how I've been modifying the USB drivers when they need
it. In general I would expect an interface number to be not much larger
than the index (i.e., normally not many numbers will be skipped), so the
bitmask size should be adequate in either case. We probably don't have to
worry about trying to support devices with a single interface numbered 77
-- but if you wanted to then you'd have to use the index.
Alan Stern