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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150731203618.GA24494@kroah.com>
Date:	Fri, 31 Jul 2015 13:36:18 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH] Input: drivers/joystick: use parallel port device model

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@...torindia.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-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/