linux-kernel - Re: [PATCH WIP] parport: add device model

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150411052651.GC3496@sudip-PC>
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(&registration_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(&registration_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/