Message-ID: <CACVXFVO5x9pePZekJPnywr7czXG_3_xCRj-rXEgf2KSQXnmPbA@mail.gmail.com>
Date:	Tue, 26 Jun 2012 15:23:19 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Oliver Neukum <oliver@...kum.org>
Cc:	Bjørn Mork <bjorn@...k.no>,
	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Marius Bjørnstad Kotsbak 
	<marius.kotsbak@...il.com>
Subject: Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum <oliver@...kum.org> wrote:
> Am Montag, 25. Juni 2012, 09:15:21 schrieb Ming Lei:
>> Any locking isn't needed if the set to NULL is put after
>> driver_info->unbind,  since ->unbind will call subdriver->disconnect,
>> which will hold the open/disconnect lock of wdm_mutex.
>
> True for cdc_wdm. But usbnet needs to work well for everything.

Suppose there are other usbnet drivers which may have this kind of
subdriver, and they have to take one lock to avoid open/disconnect
race, there are only two ways to do it:

          - the lock is held before calling usbnet_disconnect
          - the lock is held inside driver_info->unbind

So putting the set to NULL after driver_info->unbind should work
for the both two ways above.

Also we can document the usage in comments.

>
>> > We can move to after unregister_netdev()
>> > I am unhappy with it going after unbind.
>> >
>>
>> Could you let us know the reason? I think it may let the
>> patch not necessary.
>
> Very well. This is the code:
>
>  void usbnet_disconnect (struct usb_interface *intf)
> {
>        struct usbnet           *dev;
>        struct usb_device       *xdev;
>        struct net_device       *net;
>
>        dev = usb_get_intfdata(intf);
>        usb_set_intfdata(intf, NULL);
>        if (!dev)
>                return;
>
> This code needs to check for NULL (cdc_ether and similar drivers)
> It is cleaner that if we need to check for NULL we also set to NULL.
> But that is no good reason to keep it if there's real trouble
>
>        xdev = interface_to_usbdev (intf);
>
>        netif_info(dev, probe, dev->net, "unregister '%s' usb-%s-%s, %s\n",
>                   intf->dev.driver->name,
>                   xdev->bus->bus_name, xdev->devpath,
>                   dev->driver_info->description);
>
>        net = dev->net;
>        unregister_netdev (net);
>
> Here intfdata is NULL.
>
>        cancel_work_sync(&dev->kevent);
>
>        if (dev->driver_info->unbind)
>                dev->driver_info->unbind (dev, intf);
>
> At this point a minidriver must not follow the intfdata pointer,
> because the interface may again be probed. So if here a minidriver

IMO, probe is serialized strictly with driver unbind since both the parent
lock and its own device lock have been held, so the probe may only be
started after driver unbinding is completed.

> still uses intfdata, locking will be needed. We want to catch those
> casees.

Suppose infdata is used here somewhere, it is surely a bug because
the usbnet instance pointed by intfdata will be freed soon.

So looks putting the set to NULL after driver_info->unbind is good,
doesn't it?

>
>        usb_kill_urb(dev->interrupt);
>        usb_free_urb(dev->interrupt);
>
>        free_netdev(net);
>        usb_put_dev (xdev);
> }
>
>> > Sure, it is a debugging aid. It has the drawback that minidrivers have
>> > to be able to deal with intfdata being NULL. That is not hard to do.
>>
>> The check isn't needed if the set to NULL is put after  driver_info->unbind
>> in usbnet_disconnect.
>
> True, but we don't catch bugs.

If the check is added, the bugs may be hided, and no stack will be
dumped, :-)


Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists