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

[Dan Carpenter]

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 <su...@vectorindia.org>
> ---
>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/