Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1197188 > unrolled thread

Re: [PATCH] Input: drivers/joystick: use parallel port device model

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Input: drivers/joystick: use parallel port device model Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-07-31 19:00 +0200
    Re: [PATCH] Input: drivers/joystick: use parallel port device model Pali Rohár <pali.rohar@gmail.com> - 2015-08-03 14:30 +0200
      Re: [PATCH] Input: drivers/joystick: use parallel port device model Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-08-03 14:40 +0200
        Re: [PATCH] Input: drivers/joystick: use parallel port device model Pali Rohár <pali.rohar@gmail.com> - 2015-08-03 14:50 +0200
          Re: [PATCH] Input: drivers/joystick: use parallel port device model Pali Rohár <pali.rohar@gmail.com> - 2015-08-04 20:40 +0200
    Re: [PATCH] Input: drivers/joystick: use parallel port device model Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-08-03 17:10 +0200
      Re: [PATCH] Input: drivers/joystick: use parallel port device model Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-08-04 20:50 +0200
        Re: [PATCH] Input: drivers/joystick: use parallel port device model Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-08-05 08:40 +0200

#1197188 — Re: [PATCH] Input: drivers/joystick: use parallel port device model

On Fri, Jul 31, 2015 at 03:45:25PM +0530, Sudip Mukherjee wrote:
> On Thu, Jul 30, 2015 at 09:53:23AM -0700, Dmitry Torokhov wrote:
> > Hi Sudip,
> Hi Dmitry,
> > 
> > On Thu, Jul 30, 2015 at 07:36:34PM +0530, Sudip Mukherjee wrote:
> > > Modify db9 driver to use the new Parallel Port device model.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> > > 
> > > It will generate a checkpatch warning about long line but I have not
> > > changed it as it was only 2 char more and for 2 char it is more readable
> > > now.
> > 
> > You can also write it as
> > 
> > 	if (!have_dev)
> > 		return -ENODEV;
> > 
> > 	return parport_register_driver(...);
> sure.
> > 
> > 
> > Could you please tell me how you tested this change? With these devices
> > becoming exceedingly rare just compile-tested changes may introduce
> > regressions that are not easily noticed.
> I dont have the device. It was just tested with module loading,
> unloading, bind and unbind and checking that it is getting attached
> properly. Also checked that with lp loaded, this driver fails to load.
> Since the modification is only in the init, exit and probe
> section so that should not affect the working of the driver. After the
> driver loads and gets access to the parport and binds to it then these
> sections have done their part. After that the db9_open and other old
> functions will be responsible and since I have not touched those
> functions so theoretically there should not be any regressions.
> > 
> <snip>
> > > 
> > > +
> > > +	for (i = 0; i < DB9_MAX_PORTS; i++) {
> > > +		if (db9_cfg[i].nargs == 0 ||
> > > +		    db9_cfg[i].args[DB9_ARG_PARPORT] < 0)
> > > +			continue;
> > > +
> > > +		if (db9_cfg[i].nargs < 2) {
> > > +			pr_warn("db9.c: Device type must be specified.\n");
> > > +			return;
> > > +		}
> > > +
> > > +		if (db9_cfg[i].args[DB9_ARG_PARPORT] == pp->number)
> > > +			break;
> > > +	}
> > > +
> > > +	if (i == DB9_MAX_PORTS) {
> > > +		pr_debug("Not using parport%d.\n", pp->number);
> > > +		return;
> > > +	}
> > 
> > Why do we validate module options in attach() instead of how we did this
> > in module init function? By doing this here we losing the ability to
> > abort module loading when parameters are invalid.
> It is there in the module_init. The same check was added here also.
> we can remove the check for db9_cfg[i].nargs < 2 from here.
> But the other one will be required to check for the port to which we
> need to register.
> > 
> > > +
> <snip>
> > >  
> > > -	parport_put_port(pp);
> > > -	return db9;
> > > +	db9_base[i] = db9;
> > 
> > Instead of using static array maybe store db9 in pardevice's private
> > pointer? Given that we are using parport exclusively on detach we can
> > just get the first pardevice and get db9 from it.
> Well, yes... Actually I wanted to do this with the minimum possible code
> change so that any chance of regression can be avoided. This should not
> have any problem, but I am a bit hesitant as this can not be tested on
> real hardware. If you confirm then I will make it this way in v2.
> By any chance, do you have the hardware?

No, unfortunately I do not.

Since neither of us can test the change what is the benefit of doing the
conversion? What will be gained by doing it? Are there plans for parport
subsystem to remove the old style initialization?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1198796

On Saturday 01 August 2015 18:13:48 Sudip Mukherjee wrote:
> On Fri, Jul 31, 2015 at 01:43:06PM -0700, Dmitry Torokhov wrote:
> > On Fri, Jul 31, 2015 at 01:36:18PM -0700, Greg KH wrote:
> > > On Fri, Jul 31, 2015 at 11:04:26PM +0530, Sudip Mukherjee wrote:
> > > > On Fri, Jul 31, 2015 at 09:54:27AM -0700, Dmitry Torokhov wrote:
> > > > > On Fri, Jul 31, 2015 at 03:45:25PM +0530, Sudip Mukherjee wrote:
> > > > > > On Thu, Jul 30, 2015 at 09:53:23AM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Sudip,
> > > > > > Hi Dmitry,
> > > > > > > 
> > > > > > > On Thu, Jul 30, 2015 at 07:36:34PM +0530, Sudip Mukherjee wrote:
> <snip>
> > > > > 
> > > > > No, unfortunately I do not.
> > > > > 
> > > > > Since neither of us can test the change what is the benefit of doing the
> > > > > conversion? What will be gained by doing it? Are there plans for parport
> > > > > subsystem to remove the old style initialization?
> > > > Yes, that is the plan. Well, if you are not comfortable with introducing
> > > > attach and detach functions then this can be done in another way where
> > > > there will be very minimum change in the code. But I will prefer to have
> > > > attach and detach then it can take advantage of the hotplug feature.
> > > > Adding Greg in To: list for his comments.
> > > 
> > > Converting to the "new" api is the end goal here, no need to keep the
> > > old one around anymore.
> > 
> > OK, then I guess we can do the conversion right (dropping db9_base
> > module-global) and see if anyone screams at us.
> I am working on it now to remove db9_base. But in the detach callback we
> will get struct parport * and from parport to get a pardevice we need to
> get it from port->physport->devices. Since it is having PARPORT_FLAG_EXCL
> it is ok, but should we really depend on and work with the internal data
> structures of the parport rather than working with the exported api?
> 
> regards
> sudip
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Hey! I believe you are not going to break my Atari joystick connected to
my desktop PC via special homemade reduction for parallel port... :-)
That Atari joystick uses db9.ko linux kernel driver and so any untested
changes to db9.c could break functionality!

-- 
Pali Rohár
pali.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198818

On Mon, Aug 03, 2015 at 02:26:48PM +0200, Pali Rohár wrote:
> On Saturday 01 August 2015 18:13:48 Sudip Mukherjee wrote:
> > 
> > 
<snip>
> 
> Hey! I believe you are not going to break my Atari joystick connected to
> my desktop PC via special homemade reduction for parallel port... :-)
> That Atari joystick uses db9.ko linux kernel driver and so any untested
> changes to db9.c could break functionality!
Can you please test the patch I am going to post in an hour or so? The
changes which I am making should not break any functionality.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198821

On Monday 03 August 2015 18:06:26 Sudip Mukherjee wrote:
> On Mon, Aug 03, 2015 at 02:26:48PM +0200, Pali Rohár wrote:
> > On Saturday 01 August 2015 18:13:48 Sudip Mukherjee wrote:
> > > 
> > > 
> <snip>
> > 
> > Hey! I believe you are not going to break my Atari joystick connected to
> > my desktop PC via special homemade reduction for parallel port... :-)
> > That Atari joystick uses db9.ko linux kernel driver and so any untested
> > changes to db9.c could break functionality!
> Can you please test the patch I am going to post in an hour or so? The
> changes which I am making should not break any functionality.
> 
> regards
> sudip

I can test db9.c patches, but not right now... Desktop PC with joystick
is thousands of miles from me. It will take some time, maybe weeks...

-- 
Pali Rohár
pali.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200220

[Multipart message — attachments visible in raw view] — view raw

On Monday 03 August 2015 14:41:27 Pali Rohár wrote:
> On Monday 03 August 2015 18:06:26 Sudip Mukherjee wrote:
> > On Mon, Aug 03, 2015 at 02:26:48PM +0200, Pali Rohár wrote:
> > > On Saturday 01 August 2015 18:13:48 Sudip Mukherjee wrote:
> > <snip>
> > 
> > > Hey! I believe you are not going to break my Atari joystick
> > > connected to my desktop PC via special homemade reduction for
> > > parallel port... :-) That Atari joystick uses db9.ko linux
> > > kernel driver and so any untested changes to db9.c could break
> > > functionality!
> > 
> > Can you please test the patch I am going to post in an hour or so?
> > The changes which I am making should not break any functionality.
> > 
> > regards
> > sudip
> 
> I can test db9.c patches, but not right now... Desktop PC with
> joystick is thousands of miles from me. It will take some time,
> maybe weeks...

Ok, so please send patches and I will try to test them via SSH and 
remote cooperation :-)

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1198934

On Fri, Jul 31, 2015 at 01:43:06PM -0700, Dmitry Torokhov wrote:
> > 
> > Converting to the "new" api is the end goal here, no need to keep the
> > old one around anymore.
> 
> OK, then I guess we can do the conversion right (dropping db9_base
> module-global) and see if anyone screams at us.
Hi Dmitry,
It might become a scream-able condition if we put the pointer to db9 in
the private of pardevice. I was doing par_dev->private = db9;
and while in detach I was doing
struct db9 *db9 = port->physport->devices->private;

Now consider these three situtaions:
1) We have two parallel ports. db9 is attached and registered with
parport0. Now parport1 is removed. So the removal of parport1 will be
announced to all the drivers which are registered with parallel port
bus. And so our detach callback is called with parport1 as the argument.
Now the detach function should check if it is using that port but db9
has the portnumber data and we have stored db9 in the private so we
try to do: struct db9 *db9 = port->physport->devices->private; and BANG
(we have not used parport1).

2) Again we have two parallel ports. we are connected to parport1 and
parport0 is empty. We try to remove db9 module.
parport_unregister_driver() will call the detach callback with all the
ports available with the parallel port bus. So db9_detach() is called
with parport0 and we try to do:
struct db9 *db9 = port->physport->devices->private; and OOPS, no device
is using parport0 and so physport->devices is still NULL.

3) We have one parallel port and lp is already loaded. We try to load
db9 and the driver is loaded but it is not able to register the device.
So we try to rmmod the module and the detach is called with the
parport0. But we still have not registred with parport0 and since we
donot have db9_base with us we have no way of knowing that we are
registered or not.

If you still want to remove db9_base we can do by checking the device
name in the detach function and by testing port->physport->devices for
NULL, but the code will get unnecessary complicated. And these are not
just hypothetical situtation I got them while testing.
I am ready to make the changes, just want your confirmation if it is
worth complicatng the code and taking risk just for removing one global
variable.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200231

On Mon, Aug 03, 2015 at 08:32:20PM +0530, Sudip Mukherjee wrote:
> On Fri, Jul 31, 2015 at 01:43:06PM -0700, Dmitry Torokhov wrote:
> > > 
> > > Converting to the "new" api is the end goal here, no need to keep the
> > > old one around anymore.
> > 
> > OK, then I guess we can do the conversion right (dropping db9_base
> > module-global) and see if anyone screams at us.
> Hi Dmitry,
> It might become a scream-able condition if we put the pointer to db9 in
> the private of pardevice. I was doing par_dev->private = db9;
> and while in detach I was doing
> struct db9 *db9 = port->physport->devices->private;
> 
> Now consider these three situtaions:
> 1) We have two parallel ports. db9 is attached and registered with
> parport0. Now parport1 is removed. So the removal of parport1 will be
> announced to all the drivers which are registered with parallel port
> bus. And so our detach callback is called with parport1 as the argument.
> Now the detach function should check if it is using that port but db9
> has the portnumber data and we have stored db9 in the private so we
> try to do: struct db9 *db9 = port->physport->devices->private; and BANG
> (we have not used parport1).
> 
> 2) Again we have two parallel ports. we are connected to parport1 and
> parport0 is empty. We try to remove db9 module.
> parport_unregister_driver() will call the detach callback with all the
> ports available with the parallel port bus. So db9_detach() is called
> with parport0 and we try to do:
> struct db9 *db9 = port->physport->devices->private; and OOPS, no device
> is using parport0 and so physport->devices is still NULL.
> 
> 3) We have one parallel port and lp is already loaded. We try to load
> db9 and the driver is loaded but it is not able to register the device.
> So we try to rmmod the module and the detach is called with the
> parport0. But we still have not registred with parport0 and since we
> donot have db9_base with us we have no way of knowing that we are
> registered or not.
> 
> If you still want to remove db9_base we can do by checking the device
> name in the detach function and by testing port->physport->devices for
> NULL, but the code will get unnecessary complicated. And these are not
> just hypothetical situtation I got them while testing.
> I am ready to make the changes, just want your confirmation if it is
> worth complicatng the code and taking risk just for removing one global
> variable.

Hmm, it is quite unexpected that detach() is called even if attach() did
not actually attach anything. Maybe it is not too late if attach() would
return a pointer to:

	struct pp_client {
		struct parport_driver *driver;
		struct list_head node;
	}

that drivers can embed into their structure.

Or maybe do something similar to input core with input_handle tying
input device with one or several input handlers.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200449

On Tue, Aug 04, 2015 at 11:41:28AM -0700, Dmitry Torokhov wrote:
> On Mon, Aug 03, 2015 at 08:32:20PM +0530, Sudip Mukherjee wrote:
> > On Fri, Jul 31, 2015 at 01:43:06PM -0700, Dmitry Torokhov wrote:
> > > > 
> > > > Converting to the "new" api is the end goal here, no need to keep the
> > > > old one around anymore.
> > > 
> > > OK, then I guess we can do the conversion right (dropping db9_base
> > > module-global) and see if anyone screams at us.
> > Hi Dmitry,
<snip>	
> > NULL, but the code will get unnecessary complicated. And these are not
> > just hypothetical situtation I got them while testing.
> > I am ready to make the changes, just want your confirmation if it is
> > worth complicatng the code and taking risk just for removing one global
> > variable.
> 
> Hmm, it is quite unexpected that detach() is called even if attach() did
> not actually attach anything.
But that is the way original parallel port used to work. And this adding
the device model code was mainly done keeping the original behaviour of
the code. In the original code the register_driver did:
list_for_each_entry(port, &portlist, list)
	drv->attach(port);
list_add(&drv->list, &drivers);

And the unregister_driver did:
list_for_each_entry(port, &portlist, list)
	drv->detach(port);

So, the original code never checked if attach succeded or not, and used
to call detach whenever driver used to unregister even if attach failed.
It was on the driver to check for and maintain the status.

> Maybe it is not too late if attach() would return a pointer to:
But that will change the way the parallel port client drivers used to
work. Maybe, for now we should stick to the old existing behaviour until
all the conversion is complete (hopefully without any regression). And
after we have fully converted to device model then we can plan to improve
it.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web