Re: [PATCH WIP] parport: add device model

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> 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.

Probably, it's better to leave it as-is if you aren't sure.  I was just
asking because I didn't know myself.

> > 
> > > -	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.

Yes, they work the same, you're right.  But the other style is better
and more robust.

I have been trying to explain this to people but "return 0;" is
beautiful code.  Functions normally are a list of statements in a row
with exceptions for error handling.  The last statement in the success
path should be "return 0;".

Don't mix error and success paths.  I see a quite a few bugs like this
where the error handling doesn't have a return then later we add some
code at the end of the function and forget to add the return.

	ret = parport_bus_init();
	if (ret)
		unregister_sysctl_table();

	ret = something_else();

	return ret;

> > 
> > 
> > > +	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.

My concern is that we dereference port to get the next port.  If it's
freed now it causes a use after free.  It's easier to detect if you have
free poisoning turned on.

> > 
> > > +		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.

Heh.  No, I really know less than you do about the driver model at this
point.  Sorry.  It's going to take me a bit to get up to speed.

regards,
dan carpenter