linux-kernel - Re: [PATCH] staging: comedi: remove duplicate pointer assignments in attach functions

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] [day] [month] [year] [list]

Date:	Tue, 29 Apr 2014 20:59:04 +0200
From:	Christian Engelmayer <cengelma@....at>
To:	Hartley Sweeten <HartleyS@...ionengravers.com>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>
Cc:	"abbotti@....co.uk" <abbotti@....co.uk>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"chase.southwood@...oo.com" <chase.southwood@...oo.com>,
	"unixed@...il.com" <unixed@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: comedi: remove duplicate pointer assignments
 in attach functions

On Mon, 28 Apr 2014 22:36:13 +0000, Hartley Sweeten <HartleyS@...ionengravers.com> wrote:
> Technically, these drivers are fine as-is.

They are. The proposed change falls under minor code maintenance only.

> They are all legacy comedi drivers and use the manual attach mechanism. The
> dev->board pointer is setup by the comedi core before calling the drivers
> (*attach) so the foo = comedi_board(dev) is getting the board pointer that
> was found by the core.

> Unlike most comedi legacy drivers, these drivers then do an additional "probe"
> to try and identify the board. This could result in the dev->board_ptr getting
> changed which requires updating the local variable for the board pointer.

The point is that while updating dev->board_ptr is necessary in case of the
manual attach use case, deriving the local pointer before dev->board_ptr is
decided is not. Furthermore it might be a bit risky to already have a local
pointer to a valid, but potentially wrong comedi struct preselected by the
core, although it cannot be used safely anyway until overwritten after the
manual probe is done.

Having had a short look over the comedi code I was under the impression that
the change would make the 4 affected functions consistent to the other parts
that seemingly follow the skeleton.

	static int skel_attach(struct comedi_device *dev, struct comedi_devconfig *it)
	{
		const struct skel_board *thisboard;
		struct skel_private *devpriv;

		/*
		* If you can probe the device to determine what device in a series
		* it is, this is the place to do it.  Otherwise, dev->board_ptr
		* should already be initialized.
		*/
		/* dev->board_ptr = skel_probe(dev, it); */

		thisboard = comedi_board(dev);

> These probe functions need to be looked at to see if they are actually needed.
> For now I would prefer that the existing code stay as-is.

That added about the intention of the patch, I'm fine if You want to question
the necessity of the probes as a whole and keep the legacy code meanwhile
untouched.

Regards,
Christian

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)