Re: [PATCH v4] [RFC] NEXIO (or iNexio) support for usbtouchscreen

[Ondrej Zary]

On Friday 20 November 2009 19:43:46 Oliver Neukum wrote:
> Am Freitag, 20. November 2009 10:21:43 schrieb Ondrej Zary:
> > On Thursday 19 November 2009, Oliver Neukum wrote:
> > > > +struct nexio_priv {
> > > > +	struct urb *ack;
> > > > +	char ack_buf[2];
> > > > +};
> > >
> > > No. Every buffer needs to have an exclusive cache line for DMA
> > > to work on the incoherent archotectures. Therefore you must allocate
> > > each buffer with its own kmalloc.
> >
> > OK, thanks for your patience.
>
> No problem. I should have explained better.
>
> > > > +	/* read replies */
> > > > +	for (i = 0; i < 3; i++) {
> > > > +		memset(buf, 0, NEXIO_BUFSIZE);
> > > > +		ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, NEXIO_INPUT_EP),
> > > > +				   buf, NEXIO_BUFSIZE, &actual_len,
> > > > +				   NEXIO_TIMEOUT);
> > > > +		if (ret < 0 || actual_len < 1 || buf[1] != actual_len)
> > > > +			continue;
> > > > +		switch (buf[0]) {
> > > > +			case 0x83:	/* firmware version */
> > > > +				firmware_ver = kstrdup(&buf[2], GFP_KERNEL);
>
> On second thought this is not nice. If a device is broken enough to report
> a name or a firmware version twice, you produce a memory leak.
> Do you know very buggy devices to exist?

Oh yes, that might be a problem - I'll add a NULL check before kstrdup. And 
maybe it should not be hardcoded to 3 reads - had to check this.

> > > > +				break;
> > > > +			case 0x84:	/* device name */
> > > > +				device_name = kstrdup(&buf[2], GFP_KERNEL);
> > > > +				break;
> > > > +		}
> > > > +	}
> > > > +	printk(KERN_INFO "Nexio device: %s, firmware version: %s\n",
> > > > +	       device_name, firmware_ver);
> > >
> > > How do you know device_name and firmware_ver are not NULL?
> >
> > printk works fine with NULL, it prints <NULL>. Is it necessary to add
> > more code only to make the output nice?
>
> No, for niceness it is not necessary. The question is whether you want
> to treat this as an error or print a warning. That is a matter of taste.

As there's no datasheet and I have only one device (with one firmware 
version), ignoring it looks like the best "solution".

I'll also want to remove NEXIO_INPUT_EP and NEXIO_OUTPUT_EP constants - the 
endpoint addresses can be found at runtine (there's only one input and one 
output endpoint). I think that to do this in nexio_init(), it needs to 
know "struct usb_interface *" instead of "struct usb_device *". I have a 
patch ready (but forgot to take it so it needs to wait until next week) that 
replaces "struct usb_device *udev" in struct usbtouch_usb with "struct 
usb_interface *interface" - is it a good idea?

>
> > Looks like a bug in the original usbtouchscreen code. There's no locking.
> > Will a spinlock in usbtouch_open() and usbtouch_disconnect() fix it? Or
> > do you see more problems here?
>
> You must not call usb_kill_urb() with a spinlock held.
> I'll lokk at the usbtouchscreen code.
> The new version looks good to me.
>
> 	Regards
> 		Oliver
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Ondrej Zary
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/