Re: [PATCH 1/4] parport: modify parport subsystem to use devicemodel

[Sudip Mukherjee <sudipm.mukherjee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed, Apr 15, 2015 at 03:31:15PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 15, 2015 at 01:18:41PM +0530, Sudip Mukherjee wrote:
> > @@ -29,6 +31,7 @@
<snip>
> > +struct bus_type parport_bus_type = {
> > +	.name           = "parport",
> > +};
> > +EXPORT_SYMBOL(parport_bus_type);
> 
> They bus type shouldn't need to be exported, unless something is really
> wrong.  Why did you do this?
I was thinking why it was needed but i saw the same for pci_bus_type,
i2c_bus_type and spi_bus_type and i blindly followed. :(
> 
> > +
<snip>
> > +int __parport_register_drv(struct parport_driver *drv,
> > +			   struct module *owner, const char *mod_name)
> > +{
> > +	struct parport *port;
> > +	int ret, err = 0;
> > +	bool attached = false;
> 
> You shouldn't need to track attached/not attached with the driver model,
> that's what it is there for to handle for you.
this attach is different from the driver model. The driver when it calls
prport_register_drv(), it will again call back a function which was called
attach, so i named the variable as attached. but i guess the name is
misleading, i will rename it in v2.
> 
> > +
> > +	if (list_empty(&portlist))
> > +		get_lowlevel_driver();
> 
> what does this call do?
if parport_pc is not loaded it does a request_module, and the
documentation says to add "alias parport_lowlevel parport_pc"
in /etc/modprobe.d directory. parport_pc will actually register the
ports, so if that module is not loaded then the system is not yet
having any parallel port registered.
> 
<snip>
> > +			err = ret;
> > +	}
> > +	if (attached)
> > +		list_add(&drv->list, &drivers);
> 
> Why all of this?  You shouldn't need your own matching anymore, the
> driver core does it for you when you register the driver, against the
> devices you have already registered, if any.
ok. I will remove. actully, I have copied the old function and just
added the device-model specific parts to it, and I thought after we have
a working model then we can remove these parts which will not be needed.
> 
> 
<snip>
> > +		list_del(&tmp->full_list);
> > +		kfree(tmp);
> > +		return NULL;
> 
> Please read the kerneldoc comments for device_register(), you can't
> clean things up this way if device_register() fails :(
oops.. sorry, i read that and did that while unregistering the device,
but here the old habit of kfree came ... :(
> 
<snip>
> >  	tmp->prev = NULL;
> > @@ -630,6 +703,133 @@ parport_register_device(struct parport *port, const char *name,
> >  	return NULL;
> >  }
> >  
> > +void free_pardevice(struct device *dev)
> > +{
> > +}
> 
> empty free functions are a huge red flag.  So much so the kobject
> documentation in the kernel says I get to make fun of anyone who tries
> to do this.  So please don't do this :)
yeahh, i remember reading it, but when i got the stackdump, that went
out of my head. working with the device-mode for the first time, so i
guess it can be excused. :)
> 
> 
> > +struct pardevice *
> > +parport_register_dev(struct parport *port, const char *name,
> > +		     int (*pf)(void *), void (*kf)(void *),
> > +		     void (*irq_func)(void *), int flags,
> > +		     void *handle, struct parport_driver *drv)
> 
> I think you need a structure, what's with all of the function pointers?
ok. that will keep the code tidy.
> 
> > +	tmp->waiting = 0;
> > +	tmp->timeout = 5 * HZ;
> > +
> > +	tmp->dev.parent = &port->ddev;
> > +	devname = kstrdup(name, GFP_KERNEL);
> > +	dev_set_name(&tmp->dev, "%s", name);
> 
> Why create devname and name here in different ways?
I was experimenting and finally forgot to remove devname. sorry.
> 
> > +	tmp->dev.driver = &drv->driver;
> > +	tmp->dev.release = free_pardevice;
> > +	tmp->devmodel = true;
> > +	ret = device_register(&tmp->dev);
> > +	if (ret)
> > +		goto out_free_all;
> 
> Again, wrong error handling.
will fix it.
> 
> 
> > +	/* Chain this onto the list */
<snip>
> > +	if (port->physport->devices)
> > +		port->physport->devices->prev = tmp;
> > +	port->physport->devices = tmp;
> > +	spin_unlock(&port->physport->pardevice_lock);
> 
> This whole list of device management seems odd, is it really still
> needed with the conversion to the new model?
like i said before, I have not removed anything from the function.
> 
> 
> >  		return;
> >  	}
> > +	if (dev->devmodel)
> > +		device_release_driver(&dev->dev);
> >  
> >  #ifdef CONFIG_PARPORT_1284
> >  	/* If this is on a mux port, deselect it. */
> > @@ -1022,6 +1235,7 @@ EXPORT_SYMBOL(parport_remove_port);
> >  EXPORT_SYMBOL(parport_register_driver);
> >  EXPORT_SYMBOL(parport_unregister_driver);
> >  EXPORT_SYMBOL(parport_register_device);
> > +EXPORT_SYMBOL(parport_register_dev);
> 
> checkpatch doesn't like you putting this here :)
oops... missed that.
I will send in a v2, better I will send to u and Dan and ofcourse lkml
as RFC. After the final version of RFC is approved then I will send the
patch for applying.

regards
sudip

> thanks,
> 
> greg k-h