Date:	Mon, 4 May 2015 08:58:58 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new
 parport device model

On Mon, 4 May 2015 11:10:12 +0530, Sudip Mukherjee wrote:
> On Sun, May 03, 2015 at 03:33:40PM +0200, Jean Delvare wrote:
> > On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
> > > +	adapter->pdev = parport_register_dev_model(port, name,
> > > +						   &i2c_parport_cb);
> > > +	kfree(name);
> > 
> > If you can free "name" at this point, this suggests that
> > parport_register_dev_model made a copy somehow. In that case, please
> > don't use dynamic memory allocation in the first place. Either use a
> > static buffer and sprintf to it, or (probably better) pass the instance
> > number to parport_register_dev_model() as a separate parameter.
> well, first thought - there will be some drivers who will not have multiple
> instances.

Please check how the platform bus handles this. You can see the device
names under /sys/bus/platform/devices, some end with .%d and some
don't. The code handling this is in
drivers/base/platform.c:platform_device_add(). That being said, I can't
think of a reason why any specific parallel port device could not have
several instances (contrary to some platform devices which can only
have a single instance by nature.) So even if a driver does not support
multiple instances today, it would make sense to stick to %s.%d as the
bus id pattern for all devices.

> but second thought - if we have separately device name and
> instance number, we can just combine them when registering with the device
> model, but it will become easier in the probe for the name comparison which
> you have pointed out later in your reply.

This was indeed one of the reasons for my suggestion. The other is that
identifiers need to be unique for a given bus type, and leaving the
responsibility of this to individual device drivers is risky. They
could get it wrong, or accidentally step on each other's toes. And even
if they do it right, they are likely to do it inconsistently, which is
never good.

> > > (...)
> > > -static void i2c_parport_detach(struct parport *port)
> > > +static int i2c_parport_probe(struct device *dev)
> > >  {
> > > -	struct i2c_par *adapter, *_n;
> > > +	char *name = dev_name(dev);
> > 
> > This adds the following warning at build time:
> > 
> >   CC [M]  drivers/i2c/busses/i2c-parport.o
> > drivers/i2c/busses/i2c-parport.c: In function ‘i2c_parport_probe’:
> > drivers/i2c/busses/i2c-parport.c:274:15: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]
> >   char *name = dev_name(dev);
> > 
> > Very easy to fix, just declare "name" as const char *.
> I didnot get this warning, maybe I need to upgrade my gcc or will
> W=1 show it?

I see it without W=1, as the message says this type of warning is
enabled by default (gcc 4.8.1.) The check on const vs. non-const
pointers is very old so I am very surprised that you don't see it, even
if your gcc isn't recent.

> > 
> > >  
> > > -	/* Walk the list */
> <snip>
> > >  
> > > -	return parport_register_driver(&i2c_parport_driver);
> > > +	return parport_register_drv(&i2c_parport_driver);
> > 
> > Can't you call parport_register_driver() for both models and decide
> > what to do based on which members of struct parport_driver are set?
> > This would be less confusing IMHO.
> I guess it can be done. let me try it out.
> > 
> > >  }
> > >  
> <snip>
> > > +	}
> > > +	mutex_unlock(&adapter_list_lock);
> > 
> > Moving all this code here seems inappropriate. What if a parallel port
> > is removed from the system while the i2c-parport driver is loaded? I
> > think this can happen with laptop docking stations or parallel ports on
> > PCI cards for example. Your new model should be able to deal with that,
> > so each driver still needs a detach or remove function which the core
> > can call on parallel port removal.
> ok, will be done.
> To be frank, I am actually confused with this part, not only for parport
> subsystem but for all the other codes I have seen. We have a remove
> function for all subsystems, lets assume PCI, so a pci driver is having
> a remove callback. So if the particular pci device is removed then the
> remove is called which does all the clearing part. But the driver still
> remains registered, then what happens to the driver?

This was a key design decision of the (then) new device driver model in
kernel v2.5 that the lifetime of drivers should be independent from
the lifetime of device instances. Ideally, devices are even created
and deleted outside their driver. That works well for enumerated
devices such as PCI or USB devices. That won't work in your case
because parallel port devices have no unique ID so they can't be
enumerated.

Still, the lifetime of devices should be independent from the lifetime
of the driver. The driver should be registered as long as the module is
loaded. The devices, however, must be created and deleted dynamically
whenever the relevant parallel ports appear or disappear from the
system.

So basically the module's __init function should only register the
driver, and let the core call back its probe function for every parallel
port available on the system at that time. And likewise, the __exit
function should only unregister the driver, and let the core call back
its remove function for every device that still exists at that point in
time. This is what the platform bus subsystem does (see for example
drivers/i2c/busses/i2c-viperboard.c but there are many many others.)

> my next WIP will have some changes in the core level also, so I shouldnot
> add your Tested-by: to it. And I will again request you to check that.

I will do, no problem.

-- 
Jean Delvare
SUSE L3 Support
--
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