| Message ID | 1428492040-5581-2-git-send-email-sudipm.mukherjee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
1) We can't apply this patch on its own so this way of breaking up the patches doesn't work. 2) I was thinking that all the ->attach() calls would have to succeed or we would bail. Having some of them succeed and some fail doesn't seem like it will simplify the driver code very much. But I can also see your point. Hm... Minor comment: No need to preserve the error code if there are lots which we miss. We may as well hard code an error code. But that's a minor thing. Does this actually simplify the driver code? That's the more important thing. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: > 1) We can't apply this patch on its own so this way of breaking up the > patches doesn't work. > The right thing is to do add an attach_ret(). static int do_attach(drv) { if (drv->attach_ret) return drv->attach_ret(); drv->attach(); return 0; } Then we convert one driver to use the new function pointer and see if it simplifies the code. If so we can transition the others as well. If not then we give up. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: > 1) We can't apply this patch on its own so this way of breaking up the > patches doesn't work. yes, if the first patch is reverted for any reason all the others need to be reverted also. so then everything in one single patch? > > 2) I was thinking that all the ->attach() calls would have to succeed or > we would bail. Having some of them succeed and some fail doesn't seem > like it will simplify the driver code very much. But I can also see > your point. Hm... to clarify my point more here: any system might have more than one parallel port but the module might decide to use just one. so in that case attach will return 0 for the port that it wishes to use, for others it will be a error code. So in parport_register_driver if we get error codes in all the attach calls then we know that attach has definitely failed, but atleast one 0 means one attach call has succeeded, which will happen in case of staging/panel, net/plip... > > Minor comment: No need to preserve the error code if there are lots > which we miss. We may as well hard code an error code. But that's a > minor thing. Does this actually simplify the driver code? That's the > more important thing. i don't think this will simplify the driver code, but atleast now parport_register_driver() will not report success when we have actually failed. And as a result module_init will also fail which is supposed to be the actual behviour. regards sudip > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 08, 2015 at 02:44:37PM +0300, Dan Carpenter wrote: > On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: > > Then we convert one driver to use the new function pointer and see if > it simplifies the code. If so we can transition the others as well. If > not then we give up. i am sending a v2 and also a patch to change one driver. regards sudip > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote: > On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: > > 1) We can't apply this patch on its own so this way of breaking up the > > patches doesn't work. > yes, if the first patch is reverted for any reason all the others need > to be reverted also. so then everything in one single patch? The problem is that patch 1/1 breaks the build. The rule is that we should be able to apply part of a patch series and nothing breaks. If we apply the patch series out of order than things break that's our problem, yes. But if we apply only 1/1 and it breaks, that's a problem with the series. > > > > 2) I was thinking that all the ->attach() calls would have to succeed or > > we would bail. Having some of them succeed and some fail doesn't seem > > like it will simplify the driver code very much. But I can also see > > your point. Hm... My other issue with this patch series which is related to #2 is that it's not clear that anyone is checking the return value and doing correct things with it. Hopefully, when we use the attach_ret() approach then it will be more clear if/how the return value is useful. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 08, 2015 at 03:27:37PM +0300, Dan Carpenter wrote: > On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote: > > On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: > > > 1) We can't apply this patch on its own so this way of breaking up the > > > patches doesn't work. > > yes, if the first patch is reverted for any reason all the others need > > to be reverted also. so then everything in one single patch? > > The problem is that patch 1/1 breaks the build. The rule is that we > should be able to apply part of a patch series and nothing breaks. If > we apply the patch series out of order than things break that's our > problem, yes. But if we apply only 1/1 and it breaks, that's a problem > with the series. Yep, keep in mind that "git bisect" will randomly land in the middle of any set of patches during a debugging session and it could very well land on this one. If it breaks, that's a real pain for the people trying to bisect their bug because suddenly they have to deal with a second bug totally different from theirs. Regards, Willy -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/parport/share.c b/drivers/parport/share.c index 3fa6624..640ce41 100644 --- a/drivers/parport/share.c +++ b/drivers/parport/share.c @@ -148,23 +148,33 @@ static void get_lowlevel_driver (void) * 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; + int ret, err; + bool attached = false; 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) { + err = drv->attach(port); + if (err == 0) + attached = true; + else + ret = err; + } + if (attached) { + list_add(&drv->list, &drivers); + ret = 0; + } mutex_unlock(®istration_lock); - return 0; + return ret; } /** diff --git a/include/linux/parport.h b/include/linux/parport.h index c22f125..9411065 100644 --- a/include/linux/parport.h +++ b/include/linux/parport.h @@ -249,7 +249,7 @@ struct parport { struct parport_driver { const char *name; - void (*attach) (struct parport *); + int (*attach)(struct parport *); void (*detach) (struct parport *); struct list_head list; };
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 check the return of attach, and if attach fails then parport_register_driver should also fail. We can have multiple parallel port so we only mark attach as failed only if it has never returned a 0. Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> --- drivers/parport/share.c | 20 +++++++++++++++----- include/linux/parport.h | 2 +- 2 files changed, 16 insertions(+), 6 deletions(-)