From patchwork Thu Sep 27 14:03:00 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Russell King - ARM Linux X-Patchwork-Id: 1514251 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id 95CB83FE1C for ; Thu, 27 Sep 2012 14:05:12 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1THEgP-0002HU-Uw; Thu, 27 Sep 2012 14:03:18 +0000 Received: from caramon.arm.linux.org.uk ([78.32.30.218]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1THEgL-0002Gf-2q for linux-arm-kernel@lists.infradead.org; Thu, 27 Sep 2012 14:03:14 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=arm.linux.org.uk; s=caramon; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=as43jXDylY0gJBfwKGc1GU0+fLX0oXloFa8qD00jLqc=; b=mUwvJxpQslEDiphA9aQSbsLVeXOLGk1/zvSuabuUVVN17rQb+/2Fk3dEmdNG2kcgGvvoH38Agz6mu4xes5oNL95/cbDVfpugiru+IITPMjWO//5dT1lHU1YaR+1bktKPFhQexQ8626dshTWE0cv7YSjCDwuaOPE77Hb3ozFcGLo=; Received: from n2100.arm.linux.org.uk ([2002:4e20:1eda:1:214:fdff:fe10:4f86]:40222) by caramon.arm.linux.org.uk with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.76) (envelope-from ) id 1THEgA-0000FB-66; Thu, 27 Sep 2012 15:03:02 +0100 Received: from linux by n2100.arm.linux.org.uk with local (Exim 4.76) (envelope-from ) id 1THEg9-0000PI-6B; Thu, 27 Sep 2012 15:03:01 +0100 Date: Thu, 27 Sep 2012 15:03:00 +0100 From: Russell King - ARM Linux To: Ming Lei Subject: Re: [BUG] Deferred probing in driver model is racy, resulting in lost probes Message-ID: <20120927140300.GF14358@n2100.arm.linux.org.uk> References: <20120818145856.GP18957@n2100.arm.linux.org.uk> <20120916082510.GN12245@n2100.arm.linux.org.uk> <20120926200833.GA14340@kroah.com> <20120926202321.GD30938@n2100.arm.linux.org.uk> <20120927135809.GE14358@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20120927135809.GE14358@n2100.arm.linux.org.uk> User-Agent: Mutt/1.5.19 (2009-01-05) X-Spam-Note: CRM114 invocation failed X-Spam-Score: -5.1 (-----) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-5.1 points) pts rule name description ---- ---------------------- -------------------------------------------------- -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, medium trust [78.32.30.218 listed in list.dnswl.org] -0.8 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Arnd Bergmann , Greg Kroah-Hartman , Mark Brown , linux-kernel@vger.kernel.org, Grant Likely , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Thu, Sep 27, 2012 at 02:58:09PM +0100, Russell King - ARM Linux wrote: > On Thu, Sep 27, 2012 at 07:47:46AM +0800, Ming Lei wrote: > > On Thu, Sep 27, 2012 at 4:23 AM, Russell King - ARM Linux > > wrote: > > > To be honest, I've not bothered to test the above patch, and now when I > > > look at it, I notice it's broken - in that on error it will corrupt the > > > driver list. Take a look at the error path. > > > > > > priv is drv->p. We add priv->knode_bus to the driver list. If > > > driver_attach() returns an error, then we go to out_unregister, which > > > > In fact, driver_attach() always returns zero, so it does __not__ affect > > your test. > > It may not affect my test, but the fact is, that patch lays down the > foundations for bugs to be introduced. Now, while I can test it (and > have done) I don't think it's suitable for mainline because of _that_. > > If driver_attach() always returns zero, there's no point in it returning > a value - it might as well return void and stop leading people to > believe that it might return an error. So I suggest this alternative > patch instead, which gets rid of what is effectively dead code, and > probably a few warnings about not checking the return value from a > __must_check function (see usb-serial.c). > > With this patch, no one is lead into a false sense of security that, > after your patch, if driver_attach() ever returns an error, proper > cleanup will happen - it's blatently obvious to anyone modifying it > that if they do want it to return an error, they have to audit all the > users of it, which IMHO is a good thing. Sorry, old version of that patch. Here's the right one. diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 2bcef65..39b3ef4 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -714,12 +714,10 @@ int bus_add_driver(struct device_driver *drv) if (error) goto out_unregister; - if (drv->bus->p->drivers_autoprobe) { - error = driver_attach(drv); - if (error) - goto out_unregister; - } klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers); + if (drv->bus->p->drivers_autoprobe) + driver_attach(drv); + module_add_driver(drv->owner, drv); error = driver_create_file(drv, &driver_attr_uevent); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 4b01ab3..77e2412 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -456,9 +456,9 @@ static int __driver_attach(struct device *dev, void *data) * returns 0 and the @dev->driver is set, we've found a * compatible pair. */ -int driver_attach(struct device_driver *drv) +void driver_attach(struct device_driver *drv) { - return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach); + bus_for_each_dev(drv->bus, NULL, drv, __driver_attach); } EXPORT_SYMBOL_GPL(driver_attach); diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c index 444f8b6..4c16681 100644 --- a/drivers/char/agp/amd64-agp.c +++ b/drivers/char/agp/amd64-agp.c @@ -785,10 +785,12 @@ int __init agp_amd64_init(void) /* Look for any AGP bridge */ agp_amd64_pci_driver.id_table = agp_amd64_pci_promisc_table; - err = driver_attach(&agp_amd64_pci_driver.driver); - if (err == 0 && agp_bridges_found == 0) { + driver_attach(&agp_amd64_pci_driver.driver); + if (agp_bridges_found == 0) { pci_unregister_driver(&agp_amd64_pci_driver); err = -ENODEV; + } else { + err = 0; } } return err; diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index bb0608c..d2a8de5 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1722,9 +1722,9 @@ static ssize_t store_new_id(struct device_driver *drv, const char *buf, list_add_tail(&dynid->list, &hdrv->dyn_list); spin_unlock(&hdrv->dyn_lock); - ret = driver_attach(&hdrv->driver); + driver_attach(&hdrv->driver); - return ret ? : count; + return count; } static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id); diff --git a/drivers/input/gameport/gameport.c b/drivers/input/gameport/gameport.c index da739d9..84f9878 100644 --- a/drivers/input/gameport/gameport.c +++ b/drivers/input/gameport/gameport.c @@ -670,12 +670,7 @@ static int gameport_driver_remove(struct device *dev) static void gameport_attach_driver(struct gameport_driver *drv) { - int error; - - error = driver_attach(&drv->driver); - if (error) - pr_err("driver_attach() failed for %s, error: %d\n", - drv->driver.name, error); + driver_attach(&drv->driver); } int __gameport_register_driver(struct gameport_driver *drv, struct module *owner, diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c index d0f7533..83f4eeb 100644 --- a/drivers/input/serio/serio.c +++ b/drivers/input/serio/serio.c @@ -802,12 +802,7 @@ static void serio_shutdown(struct device *dev) static void serio_attach_driver(struct serio_driver *drv) { - int error; - - error = driver_attach(&drv->driver); - if (error) - pr_warning("driver_attach() failed for %s with error %d\n", - drv->driver.name, error); + driver_attach(&drv->driver); } int __serio_register_driver(struct serio_driver *drv, struct module *owner, const char *mod_name) diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 099f46c..07b7ece 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -72,9 +72,9 @@ int pci_add_dynid(struct pci_driver *drv, list_add_tail(&dynid->node, &drv->dynids.list); spin_unlock(&drv->dynids.lock); - retval = driver_attach(&drv->driver); + driver_attach(&drv->driver); - return retval; + return 0; } static void pci_free_dynids(struct pci_driver *drv) diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c index 079629b..ee631c9 100644 --- a/drivers/pcmcia/ds.c +++ b/drivers/pcmcia/ds.c @@ -103,7 +103,6 @@ pcmcia_store_new_id(struct device_driver *driver, const char *buf, size_t count) __u8 func_id, function, device_no; __u32 prod_id_hash[4] = {0, 0, 0, 0}; int fields = 0; - int retval = 0; fields = sscanf(buf, "%hx %hx %hx %hhx %hhx %hhx %x %x %x %x", &match_flags, &manf_id, &card_id, &func_id, &function, &device_no, @@ -127,10 +126,8 @@ pcmcia_store_new_id(struct device_driver *driver, const char *buf, size_t count) list_add_tail(&dynid->node, &pdrv->dynids.list); mutex_unlock(&pdrv->dynids.lock); - retval = driver_attach(&pdrv->drv); + driver_attach(&pdrv->drv); - if (retval) - return retval; return count; } static DRIVER_ATTR(new_id, S_IWUSR, NULL, pcmcia_store_new_id); diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index f536aeb..473c4fd 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -71,10 +71,8 @@ ssize_t usb_store_new_id(struct usb_dynids *dynids, list_add_tail(&dynid->node, &dynids->list); spin_unlock(&dynids->lock); - retval = driver_attach(driver); + driver_attach(driver); - if (retval) - return retval; return count; } EXPORT_SYMBOL_GPL(usb_store_new_id); diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c index 27483f9..c48ba9a 100644 --- a/drivers/usb/serial/usb-serial.c +++ b/drivers/usb/serial/usb-serial.c @@ -1444,7 +1444,7 @@ int usb_serial_register_drivers(struct usb_serial_driver *const serial_drivers[] /* Now set udriver's id_table and look for matches */ udriver->id_table = id_table; - rc = driver_attach(&udriver->drvwrap.driver); + driver_attach(&udriver->drvwrap.driver); return 0; failed: diff --git a/include/linux/device.h b/include/linux/device.h index 6de9415..ab2440d 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -829,7 +829,7 @@ static inline void *dev_get_platdata(const struct device *dev) extern int __must_check device_bind_driver(struct device *dev); extern void device_release_driver(struct device *dev); extern int __must_check device_attach(struct device *dev); -extern int __must_check driver_attach(struct device_driver *drv); +extern void driver_attach(struct device_driver *drv); extern int __must_check device_reprobe(struct device *dev); /*