linux-kernel - Re: [PATCH v5 WIP 5/5] paride: use new parport 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]

Date:	Wed, 20 May 2015 10:07:48 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 WIP 5/5] paride: use new parport device model

Hi Sudip,

On Wed,  6 May 2015 15:46:17 +0530, Sudip Mukherjee wrote:
> modify paride driver to use the new parallel port device model.

Leading capital please ;-)

> 
> Signed-off-by: Sudip Mukherjee <sudip@...torindia.org>
> ---
> 
> in v4 of i2c-parport patch Jean mentioned to use the full name while
> in probe. That has been done in other drivers except this one.
> The higher layer drivers (pcd , pd etc.) are appending the unit number
> to the name before the name is being sent to the lower level (paride).

Mmmpf. If not all drivers can stick to the convention, this kind of
voids my original point.

Anyway... Again not a complete review, just two things which caught my
eye:

> -static int pi_register_parport(PIA * pi, int verbose)
> +static int pi_register_parport(PIA *pi, int verbose, int unit)
>  {
>  	struct parport *port;
> +	struct pardev_cb *par_cb;
>  
>  	port = parport_find_base(pi->port);
>  	if (!port)
>  		return 0;
> -
> -	pi->pardev = parport_register_device(port,
> -					     pi->device, NULL,
> -					     pi_wake_up, NULL, 0, (void *) pi);
> +	par_cb = kzalloc(sizeof(*par_cb), GFP_KERNEL);
> +	if (!par_cb) {
> +		parport_put_port(port);
> +		return 0;
> +	}
> +	par_cb->flags = 0;
> +	par_cb->wakeup = pi_wake_up;
> +	par_cb->private = (void *)pi;
> +	pi->pardev = parport_register_dev_model(port, pi->device, par_cb,
> +						unit);

If pi->device already includes the device number, and you are passing
it again as unit, won't you end up with odd names like pcd0.0, pcd1.1,
pcd2.2 etc?

>  	parport_put_port(port);
> +	kfree(par_cb);

If you don't need par_cb anywhere else then you shouldn't be allocating
it dynamically. It's small enough to fit on the stack really.

-- 
Jean Delvare
SUSE L3 Support
--
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/