Dan Williams <dcbw@xxxxxxxxxx> writes:
> On Wed, 2013-04-10 at 15:25 +0200, Bjørn Mork wrote:
>> Dan Williams <dcbw@xxxxxxxxxx> writes:
>>
>> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
>> > +{
>> > + /* Only drivers that implement a status hook should call this */
>> > + BUG_ON(dev->interrupt == NULL);
>> >
>> I still don't think there is any reason to BUG out. See for example
>> http://article.gmane.org/gmane.linux.kernel/52102
>>
>> > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
>> > +{
>> > + if (dev->interrupt) {
>> > + mutex_lock(&dev->interrupt_mutex);
>> > + if (!force)
>> > + BUG_ON(dev->interrupt_count == 0);
>>
>> Same here. You can deal with this just fine. Warn once, and go on
>> ignoring the problem. Why kill the machine because of some minor driver
>> issue?
>
> Actually in the stop case, no, we can't deal with it, because then (due
> to the buggy sub-driver) we'd go on to decrement 0 into UINT_MAX. It
> really is a bug if, *not* at suspend time, we're told to stop the
> interrupt URB when it's not yet submitted.
Sure it is a bug. All I'm saying is that you can deal with it. Warn
about the bug and give up. Or continue. But don't roll over and die.
Let the user unload the buggy driver and email the author instead.
> I'm happy to add another
> if() here though, which would end up looking like this:
>
> if (dev->interrupt_count && --dev->interrupt_count == 0)
> usb_kill_urb(dev->interrupt);
>
> which seems odd, but fine.
Yes, there are too many decision factors, so it does look odd. You
could also decide early that the bogus dev->interrupt_count means that
nothing needs to be done, because no interrupt URB was subitted. Like
static void __usbnet_status_stop(struct usbnet *dev, bool force)
{
if (dev->interrupt) {
mutex_lock(&dev->interrupt_mutex);
if (!force && dev->interrupt_count == 0) {
print loud warning blaming the minidriver author;
goto out;
}
if (force || --dev->interrupt_count == 0)
usb_kill_urb(dev->interrupt);
dev_dbg(&dev->udev->dev,
"decremented interrupt URB count to %d\n",
dev->interrupt_count);
out:
mutex_unlock(&dev->interrupt_mutex);
}
}
But it is pretty much the same. "force" makes a mess of it.
In any case, I believe any of those solutions are a lot nicer to any
unsuspecting user, who may not agree with you that a failing 3G modem is
important enough to kill the machine.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
