linux-kernel - Re: [alsa-devel] [PATCH 6/6] [ALSA] portman2x4

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:	Thu, 07 Jan 2016 12:45:51 +0100
From:	Takashi Iwai <tiwai@...e.de>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Jaroslav Kysela <perex@...ex.cz>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: [alsa-devel] [PATCH 6/6] [ALSA] portman2x4 - use new parport device	model

On Thu, 07 Jan 2016 12:19:36 +0100,
Sudip Mukherjee wrote:
> 
> On Thu, Jan 07, 2016 at 04:31:27PM +0530, Sudip Mukherjee wrote:
> > On Thu, Jan 07, 2016 at 11:50:15AM +0100, Takashi Iwai wrote:
> > > On Thu, 07 Jan 2016 11:44:34 +0100,
> > > Sudip Mukherjee wrote:
> > > > 
> > > > On Thu, Jan 07, 2016 at 11:26:44AM +0100, Takashi Iwai wrote:
> > > > > On Thu, 07 Jan 2016 08:15:51 +0100,
> > > > > Sudip Mukherjee wrote:
> > > > > > 
> <snip>
> > > > > 
> > > > > > +
> > > > > > +	pardev = parport_register_dev_model(p,		   /* port */
> > > > > > +					    DRIVER_NAME,   /* name */
> > > > > > +					    &portman_cb,   /* callbacks */
> > > > > > +					    device_count); /* device number */
> > > > > 
> > > > > Does device_count really work similarly for
> > > > > parport_register_dev_model()?  I supposed the argument being the
> > > > > device id number while you're passing the number of devices to
> > > > > create.
> > > > 
> > > > This device_count is actually used for the device name in
> > > > /sys/bus/parport/devices. Something like DRIVER_NAME.device_count.
> > > 
> > > Well, but device_count is incremented in snd_portman_attach().  The
> > > management of device_count should be moved around the caller side, if
> > > we use this as the id (and use the assigned id instead of device_count
> > > in snd_portman_attach()).
> 
> did you mean something like this: (on top of my patch)
> 
> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> index 88b25ca..d749786 100644
> --- a/sound/drivers/portman2x4.c
> +++ b/sound/drivers/portman2x4.c
> @@ -688,14 +688,8 @@ static void snd_portman_attach(struct parport *p)
>  
>  	/* Since we dont get the return value of probe
>  	 * We need to check if device probing succeeded or not */
> -	if (!platform_get_drvdata(device)) {
> +	if (!platform_get_drvdata(device))
>  		platform_device_unregister(device);
> -		return;
> -	}
> -
> -	/* register device in global table */
> -	platform_devices[device_count] = device;
> -	device_count++;
>  }
>  
>  static void snd_portman_detach(struct parport *p)
> @@ -768,7 +762,7 @@ static int snd_portman_probe(struct platform_device *pdev)
>  	pardev = parport_register_dev_model(p,		   /* port */
>  					    DRIVER_NAME,   /* name */
>  					    &portman_cb,   /* callbacks */
> -					    device_count); /* device number */
> +					    pdev->id);	   /* device number */
>  	if (!pardev) {
>  		snd_printd("Cannot register pardevice\n");
>  		err = -EIO;
> @@ -812,6 +806,10 @@ static int snd_portman_probe(struct platform_device *pdev)
>  		goto __err;
>  	}
>  
> +	/* register device in global table */
> +	platform_devices[device_count] = device;
> +	device_count++;
> +
>  	snd_printk(KERN_INFO "Portman 2x4 on 0x%lx\n", p->base);
>  	return 0;

Hmm, this doesn't look better either.
I think the necessary change is just the access of device_count in
partport_register_dev_model().  This can be replaced with pdev->id, so
that you don't touch device_count at all there.

In anyway, these patch series (also for mst) are too late for 4.5.
These are neither serious bug fix nor function improvement, and the
drivers you're modifying are for the very minor devices.
So, please resubmit after the next merge window is closed.

While we're at it, some comments about other patches in the series:
I see no big merit to split several whitespace and blank line patches.
These are basically all whitespace fixes, so smash as a single patch.
The most important point is that this won't change any code context at
all but just a matter of white spaces.  (And write it clearly in the
change log -- so that reader can ignore this e.g. while bisecting.)

The NULL check is a matter of taste and isn't worth to fix at all.

The assignment in if is good to fix in general, so this can be a
separate patch indeed.


thanks,

Takashi
--
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/