lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 11 Apr 2015 10:56:51 +0530 From: Sudip Mukherjee <sudipm.mukherjee@...il.com> To: Dan Carpenter <dan.carpenter@...cle.com> Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH WIP] parport: add device model On Fri, Apr 10, 2015 at 05:49:55PM +0300, Dan Carpenter wrote: > On Fri, Apr 10, 2015 at 08:00:38PM +0530, Sudip Mukherjee wrote: <snip> > > + > > parport_default_sysctl_table.sysctl_header = > > register_sysctl_table(parport_default_sysctl_table.dev_dir); > > Should we return an error if this fails? not sure. but even if it fails it will not affect the normal functioning of the parallel port. but I will add that in the next WIP patch. > > > - return 0; > > + ret = parport_bus_init(); > > + if (ret) > > + unregister_sysctl_table(parport_default_sysctl_table. > > + sysctl_header); > > > ret = parport_bus_init(); > if (ret) { > unregister_sysctl_table( > parport_default_sysctl_table.sysctl_header); > return ret; > } > > return 0; do we need two returns here? parport_bus_init() will return 0 if it succeeds, so return ret will return either 0 or the error code whatever the case maybe. > > > > + return ret; > > } > > > > static void __exit parport_default_proc_unregister(void) > > @@ -570,6 +576,7 @@ static void __exit parport_default_proc_unregister(void) > > sysctl_header); > > parport_default_sysctl_table.sysctl_header = NULL; > > } > > + parport_bus_exit(); > > Do we need this function? Can't we call bus_unregister() directly? no, we dont need. on similar reasoning we also donot need parport_bus_init(). I will remove both. :) > <snip> > > > +struct bus_type parport_bus_type = { > > + .name = "parport", > > + .match = parport_match, > > There is no need for a match function. If it's NULL that's the same a > "return 1" fuction. This is called from driver_match_device(). ok. > <snip> > > + ret = driver_register(&drv->driver); > > + if (ret < 0) { > > if (ret) { > > > + mutex_lock(®istration_lock); > > + list_del_init(&drv->list); > > + list_for_each_entry(port, &portlist, list) > > + drv->detach(port); > > Does this free port? Should this be list_for_each_entry_safe? I am not sure what you meant by "free port". attach will claim the port, and the port will be marked. detach will just remove that connection and the driver will release the port. > > > + mutex_unlock(®istration_lock); > > return ret; > > + } > > + > > + return ret; > > return 0; do we need two returns? as ret will be either 0 or error code. > > > } > > <snip> > > Please use "if (ret) " everywhere unless it returns positive on success. sure. > > I know that I have done a rubbish review. I'm going to have to review > this properly later. main thing i wanted to know is if my approach is correct. since nothing on that so I hope I am on the correct track. Thanks. I will send in the next version in a day or two. regards sudip > > regards, > dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists