From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754202AbbDHOSJ (ORCPT ); Wed, 8 Apr 2015 10:18:09 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:49872 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753267AbbDHOSG (ORCPT ); Wed, 8 Apr 2015 10:18:06 -0400 Date: Wed, 8 Apr 2015 17:17:54 +0300 From: Dan Carpenter To: Sudip Mukherjee Cc: Arnd Bergmann , Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver Message-ID: <20150408141754.GT16501@mwanda> References: <1428499817-12065-1-git-send-email-sudipm.mukherjee@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428499817-12065-1-git-send-email-sudipm.mukherjee@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 08, 2015 at 07:00:16PM +0530, Sudip Mukherjee wrote: > as of now, we are not checking if attach or parport_register_driver > has succeeded or failed. But attach can fail in the places where they > have been used. Lets create an attach_ret where we will check the > return of attach, and if attach fails then parport_register_driver > should also fail. We can have multiple parallel port, like parport0, > parport1 and one driver may decide to only use parport0. so we mark > attach as failed only if it has never returned a 0. > > Signed-off-by: Sudip Mukherjee > --- > drivers/parport/share.c | 27 ++++++++++++++++++++++----- > include/linux/parport.h | 1 + > 2 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/parport/share.c b/drivers/parport/share.c > index 3fa6624..4aab733 100644 > --- a/drivers/parport/share.c > +++ b/drivers/parport/share.c > @@ -143,28 +143,45 @@ static void get_lowlevel_driver (void) > * pointer it must call parport_get_port() to do so. Calling > * parport_register_device() on that port will do this for you. > * > + * The attach_ret() function will check for the return value. > + * > * The driver's detach() function may block. The port that > * detach() is given will be valid for the duration of the > * callback, but if the driver wants to take a copy of the > * pointer it must call parport_get_port() to do so. > * > - * Returns 0 on success. Currently it always succeeds. > + * Returns 0 on success. > **/ > > int parport_register_driver (struct parport_driver *drv) > { > struct parport *port; > + bool attached = false; > + int err, ret = 0; > > if (list_empty(&portlist)) > get_lowlevel_driver (); > > mutex_lock(®istration_lock); > - list_for_each_entry(port, &portlist, list) > - drv->attach(port); > - list_add(&drv->list, &drivers); > + list_for_each_entry(port, &portlist, list) { > + if (drv->attach_ret) { > + err = drv->attach_ret(port); > + } else { > + drv->attach(port); > + err = 0; > + } > + if (err == 0) > + attached = true; > + else > + ret = err; > + } > + if (attached) { > + list_add(&drv->list, &drivers); > + ret = 0; > + } I still think it would be nicer to create a do_attach() wrapper like I said earlier. list_for_each_entry(port, &portlist, list) { ret = do_attach(); if (ret) continue; attached = true; } if (attached) list_add(&drv->list, &drivers); mutex_unlock(®istration_lock); if (!attached) return -ENODEV; return 0; The attach_driver_chain() function needs to be updated as well or it will break. regards, dan carpenter