Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Tue, Feb 09, 2016 at 12:32:55PM +0100, Takashi Iwai wrote:
> On Sun, 07 Feb 2016 15:49:34 +0100,
> Sudip Mukherjee wrote:
> > 
> > On Saturday 06 February 2016 12:41 AM, Takashi Iwai wrote:
> > > On Fri, 05 Feb 2016 18:21:46 +0100,
> > > Sudip Mukherjee wrote:
> > >>
> > >> On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
> > >>> On Fri, 05 Feb 2016 18:01:16 +0100,
> > >>> Takashi Iwai wrote:
> > >>>>
> > >>>> On Fri, 05 Feb 2016 17:50:51 +0100,
> > >>>> Sudip Mukherjee wrote:
> > >>>>>
> > >>>>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> > >>>>>> On Fri, 05 Feb 2016 07:17:06 +0100,
> > >>>>>> Sudip Mukherjee wrote:
> > >>>>>>>
> > >>>>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> > >>>>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
> > >>>>>>>> Sudip Mukherjee wrote:
> > >>>>>>>>>
> > >>>>>>>>> Modify portman driver to use the new parallel port device model.
> > >>>>>>>>> The advantage of using the device model is that the device gets binded
> > >>>>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
> > >>>>>>>>> the driver at runtime.
> > >>>>>>>>> The only change is in the way the driver gets registered with the
> > >>>>>>>>> parallel port subsystem and so as a result there is no user visible
> > >>>>>>>>> change or any chance of regression.
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
> > >>>>>>>>> ---
> > >>>>>>>>>
> > >>>>>>>>> v3: changed commit message
> > >>>>>>>>> v2:
> > >>>>>>>>>     1. pardev_cb is initialized while declaring, thus removing the use of
> > >>>>>>>>> memset.
> > >>>>>>>>>     2. used pdev->id.
> > >>>>>>>>>     3. v1 did not have the parport probe callback, but
> > >>>>>>>>> we will need the probe callback for binding as the name of the driver
> > >>>>>>>>> and the name of the device is different.
> > >>>>>>>>>     4. in v1 I missed modifying snd_portman_probe_port().
> > >>>>>>>>>
> > >>>>>>>>>     sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> > >>>>>>>>>     1 file changed, 39 insertions(+), 14 deletions(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> > >>>>>>>>> index 172685d..a22f56c 100644
> > >>>>>>>>> --- a/sound/drivers/portman2x4.c
> > >>>>>>>>> +++ b/sound/drivers/portman2x4.c
> > >>>>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> > >>>>>>>>>     {
> > >>>>>>>>>     	struct pardevice *pardev;
> > >>>>>>>>>     	int res;
> > >>>>>>>>> -
> > >>>>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
> > >>>>>>>>> -					 NULL, NULL, NULL,
> > >>>>>>>>> -					 0, NULL);
> > >>>>>>>>> +	struct pardev_cb pdev_cb = {
> > >>>>>>>>> +		.preempt = NULL,
> > >>>>>>>>> +		.wakeup = NULL,
> > >>>>>>>>> +		.private = NULL,
> > >>>>>>>>> +		.irq_func = NULL,
> > >>>>>>>>> +		.flags = 0,
> > >>>>>>>>> +	};
> > >>>>>>>>> +
> > >>>>>>>>> +	/*
> > >>>>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> > >>>>>>>>> +	 * device id alloted to this temporary device will never clash
> > >>>>>>>>> +	 * with an actual device already registered.
> > >>>>>>>>> +	 */
> > >>>>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> > >>>>>>>>> +					    SNDRV_CARDS + 1);
> > >>>>>>>>
> > >>>>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
> > >>>>>>>
> > >>>>>>> yes, it will. But this is a temoporary device just to check if the
> > >>>>>>> sound card is connected to that particular parallel port or not. After
> > >>>>>>> checking this device is immediately unregistered. My idea here was to
> > >>>>>>> have a device number which will never clash with another device number.
> > >>>>>>> And we can never have a device like "xxx.33", so no conflict. :)
> > >>>>>>
> > >>>>>> Ah, this is the temporary one.  If so, does it make sense to convert
> > >>>>>> this to dev_model one?  This means that the device will be notified to
> > >>>>>> udev even though this is a temporary one to be removed immediately.
> > >>>>>
> > >>>>> But since we are registering a device it should ideally follow the
> > >>>>> dev_model.
> > >>>>
> > >>>> We shouldn't advertise the device that shouldn't be handled by the
> > >>>> user-space.  The device you're trying to register there is the one
> > >>>> that lives only shortly just for probing the address.
> > >>>>
> > >>>>
> > >>>>>> It's what we'd want to avoid.  The function serves just as probing the
> > >>>>>> availability of the given port, not really registering anything
> > >>>>>> there.
> > >>>>>
> > >>>>> To my understanding, it is probing for the availability of the port and
> > >>>>> it is also calling portman_probe() which is initializing hardware
> > >>>>> handshake lines to midi box and checking if the portman card is
> > >>>>> connected to that parallel port or not.
> > >>>>>
> > >>>>>>
> > >>>>>> That is, we need to change the registration flow itself if we really
> > >>>>>> want to move dev_model for the whole.
> > >>>>>
> > >>>>> Any hint, how to register then?
> > >>>>> Without probing (reading and writing to that port) I can not know if
> > >>>>> that port is having the card and to use the port I need to register a
> > >>>>> device with that port.
> > >>>>
> > >>>> Just returning the error at probe of the parport device itself instead
> > >>>> of doing the probe twice?  The current way is racy in anyway.
> > >>>
> > >>> ... and the problem with that is, there is no way to check whether
> > >>> your upcoming change works correctly without the hardware.  It would
> > >>> be no longer a "cleanup", and it's risky to do that blindly.
> > >>
> > >> Yes. That is why I try to change the driver with the minimum possible
> > >> change.
> > >
> > > But it's no 100% compatible transition.  That's the first problem.
> > 
> > Well, the first problem that i can see is using the same fixed number as 
> > the temporary device, so we can have a race there.
> 
> The original code uses parport_register_port() and this assigns a new
> temporary number by itself.  Well, the issue is cosmetic, but...

I think you mean parport_register_device()...
> 
> > Another problem might 
> > be that the same device number can be tried for platform device.
> > 
> > BTW, why do we need the platform device here? we can directly probe for 
> > the device and register the sound card if the device is available from 
> > the attach function (now match_port). And the device number can be 
> > automatically generated. I think that will solve many of the problems. 
> > But the changes without checking on hardware will be risky again.
> 
> ... that's the only and biggest problem.  The whole rewrite needs the
> check with the actual hardware, ideally.

:(
If I get the chance to rewrite I will remove the platform device.		

> 
> 
> > >>> I appreciate your work, but it doesn't look worthy enough.  If we're
> > >>> trying to eliminate the all old-style parport code from the kernel
> > >>> code, OK, it's an ambitious project and we may consider taking a risk
> > >>> of breakage.  Is that the case?
> > >>
> > >> Yes, the old api is supposed to be removed and we should only have the
> > >> device model api. I was expecting to remove the old API by 4.7.
> > >> Is there any way to get the hardware?
> > >
> > > No, unfortunately.  It's an old hardware, after all.  It's difficult
> > > to find even a decent machine with a parallel port...
> > 
> > I have an i5 with an onboard parallel port, additionally one more pci 
> > card parallel port.
> > So what do you suggest? how should we approach?
> 
> This really depends on the demand.  As already mentioned, if your
> change is about getting rid of the whole legacy
> parport_register_port() and its old API, it'd be worth to take a
> risk.  But then you should really concentrate only on that.  Just
> convert it without playing too much with white space changes, etc, and
> make it in a series of the whole patchset (or at least show a "big
> picture").

But since all of it depends on old hardware so I am approaching very
slowly. Only 2 - 3 drivers converterd in one release and I wait for any
news about some regression. Fortunately till now no regression, and some
of the other drivers (panel, joystick, paride, i2c-parport etc.) were
also tested after the change. The plan is to convert all drivers using
parport_register_device() to parport_register_dev_model() first. Infact
i have systems with parallel port but no hardware to check the daisy
chain so I can not touch the old API related to that.
I will send you the modified patch after removing that temp device.

regards
sudip