Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver

From: Dan Carpenter
Date: Wed Apr 08 2015 - 10:18:12 EST


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 <sudip@xxxxxxxxxxxxxxx>
> ---
> 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(&registration_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(&registration_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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/