| Message ID | 1438265194-20366-1-git-send-email-sudipm.mukherjee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sudip, 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(...); 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. > > drivers/input/joystick/db9.c | 114 ++++++++++++++++++++++--------------------- > 1 file changed, 59 insertions(+), 55 deletions(-) > > diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c > index 8e7de5c..a69ab29 100644 > --- a/drivers/input/joystick/db9.c > +++ b/drivers/input/joystick/db9.c > @@ -48,7 +48,7 @@ struct db9_config { > }; > > #define DB9_MAX_PORTS 3 > -static struct db9_config db9_cfg[DB9_MAX_PORTS] __initdata; > +static struct db9_config db9_cfg[DB9_MAX_PORTS]; > > module_param_array_named(dev, db9_cfg[0].args, int, &db9_cfg[0].nargs, 0); > MODULE_PARM_DESC(dev, "Describes first attached device (<parport#>,<type>)"); > @@ -106,6 +106,7 @@ struct db9 { > struct pardevice *pd; > int mode; > int used; > + int parportno; > struct mutex mutex; > char phys[DB9_MAX_DEVICES][32]; > }; > @@ -553,54 +554,65 @@ static void db9_close(struct input_dev *dev) > mutex_unlock(&db9->mutex); > } > > -static struct db9 __init *db9_probe(int parport, int mode) > +static void db9_attach(struct parport *pp) > { > struct db9 *db9; > const struct db9_mode_data *db9_mode; > - struct parport *pp; > struct pardevice *pd; > struct input_dev *input_dev; > int i, j; > - int err; > + int mode; > + struct pardev_cb db9_parport_cb; > + > + 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. > + > + mode = db9_cfg[i].args[DB9_ARG_MODE]; > > if (mode < 1 || mode >= DB9_MAX_PAD || !db9_modes[mode].n_buttons) { > printk(KERN_ERR "db9.c: Bad device type %d\n", mode); > - err = -EINVAL; > - goto err_out; > + return; > } > > db9_mode = &db9_modes[mode]; > > - pp = parport_find_number(parport); > - if (!pp) { > - printk(KERN_ERR "db9.c: no such parport\n"); > - err = -ENODEV; > - goto err_out; > - } > - > if (db9_mode->bidirectional && !(pp->modes & PARPORT_MODE_TRISTATE)) { > printk(KERN_ERR "db9.c: specified parport is not bidirectional\n"); > - err = -EINVAL; > - goto err_put_pp; > + return; > } > > - pd = parport_register_device(pp, "db9", NULL, NULL, NULL, PARPORT_DEV_EXCL, NULL); > + db9_parport_cb.flags = PARPORT_FLAG_EXCL; > + > + pd = parport_register_dev_model(pp, "db9", &db9_parport_cb, i); > if (!pd) { > printk(KERN_ERR "db9.c: parport busy already - lp.o loaded?\n"); > - err = -EBUSY; > - goto err_put_pp; > + return; > } > > db9 = kzalloc(sizeof(struct db9), GFP_KERNEL); > - if (!db9) { > - printk(KERN_ERR "db9.c: Not enough memory\n"); > - err = -ENOMEM; > + if (!db9) > goto err_unreg_pardev; > - } > > mutex_init(&db9->mutex); > db9->pd = pd; > db9->mode = mode; > + db9->parportno = pp->number; > init_timer(&db9->timer); > db9->timer.data = (long) db9; > db9->timer.function = db9_timer; > @@ -610,7 +622,6 @@ static struct db9 __init *db9_probe(int parport, int mode) > db9->dev[i] = input_dev = input_allocate_device(); > if (!input_dev) { > printk(KERN_ERR "db9.c: Not enough memory for input device\n"); > - err = -ENOMEM; > goto err_unreg_devs; > } > > @@ -639,13 +650,12 @@ static struct db9 __init *db9_probe(int parport, int mode) > input_set_abs_params(input_dev, db9_abs[j], 1, 255, 0, 0); > } > > - err = input_register_device(input_dev); > - if (err) > + if (input_register_device(input_dev)) > goto err_free_dev; > } > > - 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. > + return; > > err_free_dev: > input_free_device(db9->dev[i]); > @@ -655,15 +665,22 @@ static struct db9 __init *db9_probe(int parport, int mode) > kfree(db9); > err_unreg_pardev: > parport_unregister_device(pd); > - err_put_pp: > - parport_put_port(pp); > - err_out: > - return ERR_PTR(err); > } > > -static void db9_remove(struct db9 *db9) > +static void db9_detach(struct parport *port) > { > int i; > + struct db9 *db9; > + > + for (i = 0; i < DB9_MAX_PORTS; i++) { > + if (db9_base[i] && db9_base[i]->parportno == port->number) > + break; > + } > + > + if (i == DB9_MAX_PORTS) > + return; > + > + db9 = db9_base[i]; > > for (i = 0; i < min(db9_modes[db9->mode].n_pads, DB9_MAX_DEVICES); i++) > input_unregister_device(db9->dev[i]); > @@ -671,11 +688,17 @@ static void db9_remove(struct db9 *db9) > kfree(db9); > } > > +static struct parport_driver db9_parport_driver = { > + .name = "db9", > + .match_port = db9_attach, > + .detach = db9_detach, > + .devmodel = true, > +}; > + > static int __init db9_init(void) > { > int i; > int have_dev = 0; > - int err = 0; > > for (i = 0; i < DB9_MAX_PORTS; i++) { > if (db9_cfg[i].nargs == 0 || db9_cfg[i].args[DB9_ARG_PARPORT] < 0) > @@ -683,37 +706,18 @@ static int __init db9_init(void) > > if (db9_cfg[i].nargs < 2) { > printk(KERN_ERR "db9.c: Device type must be specified.\n"); > - err = -EINVAL; > - break; > - } > - > - db9_base[i] = db9_probe(db9_cfg[i].args[DB9_ARG_PARPORT], > - db9_cfg[i].args[DB9_ARG_MODE]); > - if (IS_ERR(db9_base[i])) { > - err = PTR_ERR(db9_base[i]); > - break; > + return -EINVAL; > } > > have_dev = 1; > } > > - if (err) { > - while (--i >= 0) > - if (db9_base[i]) > - db9_remove(db9_base[i]); > - return err; > - } > - > - return have_dev ? 0 : -ENODEV; > + return have_dev ? parport_register_driver(&db9_parport_driver) : -ENODEV; > } > > static void __exit db9_exit(void) > { > - int i; > - > - for (i = 0; i < DB9_MAX_PORTS; i++) > - if (db9_base[i]) > - db9_remove(db9_base[i]); > + parport_unregister_driver(&db9_parport_driver); > } > > module_init(db9_init); > -- > 1.9.1 > Thanks.
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? 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
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.
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: > > > > 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? 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. 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
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: > > > > > 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? > 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. thanks, greg k-h -- 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
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: > > > > > > 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? > > 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. Thanks.
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
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!
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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :-)
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.
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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c index 8e7de5c..a69ab29 100644 --- a/drivers/input/joystick/db9.c +++ b/drivers/input/joystick/db9.c @@ -48,7 +48,7 @@ struct db9_config { }; #define DB9_MAX_PORTS 3 -static struct db9_config db9_cfg[DB9_MAX_PORTS] __initdata; +static struct db9_config db9_cfg[DB9_MAX_PORTS]; module_param_array_named(dev, db9_cfg[0].args, int, &db9_cfg[0].nargs, 0); MODULE_PARM_DESC(dev, "Describes first attached device (<parport#>,<type>)"); @@ -106,6 +106,7 @@ struct db9 { struct pardevice *pd; int mode; int used; + int parportno; struct mutex mutex; char phys[DB9_MAX_DEVICES][32]; }; @@ -553,54 +554,65 @@ static void db9_close(struct input_dev *dev) mutex_unlock(&db9->mutex); } -static struct db9 __init *db9_probe(int parport, int mode) +static void db9_attach(struct parport *pp) { struct db9 *db9; const struct db9_mode_data *db9_mode; - struct parport *pp; struct pardevice *pd; struct input_dev *input_dev; int i, j; - int err; + int mode; + struct pardev_cb db9_parport_cb; + + 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; + } + + mode = db9_cfg[i].args[DB9_ARG_MODE]; if (mode < 1 || mode >= DB9_MAX_PAD || !db9_modes[mode].n_buttons) { printk(KERN_ERR "db9.c: Bad device type %d\n", mode); - err = -EINVAL; - goto err_out; + return; } db9_mode = &db9_modes[mode]; - pp = parport_find_number(parport); - if (!pp) { - printk(KERN_ERR "db9.c: no such parport\n"); - err = -ENODEV; - goto err_out; - } - if (db9_mode->bidirectional && !(pp->modes & PARPORT_MODE_TRISTATE)) { printk(KERN_ERR "db9.c: specified parport is not bidirectional\n"); - err = -EINVAL; - goto err_put_pp; + return; } - pd = parport_register_device(pp, "db9", NULL, NULL, NULL, PARPORT_DEV_EXCL, NULL); + db9_parport_cb.flags = PARPORT_FLAG_EXCL; + + pd = parport_register_dev_model(pp, "db9", &db9_parport_cb, i); if (!pd) { printk(KERN_ERR "db9.c: parport busy already - lp.o loaded?\n"); - err = -EBUSY; - goto err_put_pp; + return; } db9 = kzalloc(sizeof(struct db9), GFP_KERNEL); - if (!db9) { - printk(KERN_ERR "db9.c: Not enough memory\n"); - err = -ENOMEM; + if (!db9) goto err_unreg_pardev; - } mutex_init(&db9->mutex); db9->pd = pd; db9->mode = mode; + db9->parportno = pp->number; init_timer(&db9->timer); db9->timer.data = (long) db9; db9->timer.function = db9_timer; @@ -610,7 +622,6 @@ static struct db9 __init *db9_probe(int parport, int mode) db9->dev[i] = input_dev = input_allocate_device(); if (!input_dev) { printk(KERN_ERR "db9.c: Not enough memory for input device\n"); - err = -ENOMEM; goto err_unreg_devs; } @@ -639,13 +650,12 @@ static struct db9 __init *db9_probe(int parport, int mode) input_set_abs_params(input_dev, db9_abs[j], 1, 255, 0, 0); } - err = input_register_device(input_dev); - if (err) + if (input_register_device(input_dev)) goto err_free_dev; } - parport_put_port(pp); - return db9; + db9_base[i] = db9; + return; err_free_dev: input_free_device(db9->dev[i]); @@ -655,15 +665,22 @@ static struct db9 __init *db9_probe(int parport, int mode) kfree(db9); err_unreg_pardev: parport_unregister_device(pd); - err_put_pp: - parport_put_port(pp); - err_out: - return ERR_PTR(err); } -static void db9_remove(struct db9 *db9) +static void db9_detach(struct parport *port) { int i; + struct db9 *db9; + + for (i = 0; i < DB9_MAX_PORTS; i++) { + if (db9_base[i] && db9_base[i]->parportno == port->number) + break; + } + + if (i == DB9_MAX_PORTS) + return; + + db9 = db9_base[i]; for (i = 0; i < min(db9_modes[db9->mode].n_pads, DB9_MAX_DEVICES); i++) input_unregister_device(db9->dev[i]); @@ -671,11 +688,17 @@ static void db9_remove(struct db9 *db9) kfree(db9); } +static struct parport_driver db9_parport_driver = { + .name = "db9", + .match_port = db9_attach, + .detach = db9_detach, + .devmodel = true, +}; + static int __init db9_init(void) { int i; int have_dev = 0; - int err = 0; for (i = 0; i < DB9_MAX_PORTS; i++) { if (db9_cfg[i].nargs == 0 || db9_cfg[i].args[DB9_ARG_PARPORT] < 0) @@ -683,37 +706,18 @@ static int __init db9_init(void) if (db9_cfg[i].nargs < 2) { printk(KERN_ERR "db9.c: Device type must be specified.\n"); - err = -EINVAL; - break; - } - - db9_base[i] = db9_probe(db9_cfg[i].args[DB9_ARG_PARPORT], - db9_cfg[i].args[DB9_ARG_MODE]); - if (IS_ERR(db9_base[i])) { - err = PTR_ERR(db9_base[i]); - break; + return -EINVAL; } have_dev = 1; } - if (err) { - while (--i >= 0) - if (db9_base[i]) - db9_remove(db9_base[i]); - return err; - } - - return have_dev ? 0 : -ENODEV; + return have_dev ? parport_register_driver(&db9_parport_driver) : -ENODEV; } static void __exit db9_exit(void) { - int i; - - for (i = 0; i < DB9_MAX_PORTS; i++) - if (db9_base[i]) - db9_remove(db9_base[i]); + parport_unregister_driver(&db9_parport_driver); } module_init(db9_init);
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. drivers/input/joystick/db9.c | 114 ++++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 55 deletions(-)