Re: [PATCH 4.4 02/15] [media] siano: make it work again with CONFIG_VMAP_STACK

[Mauro Carvalho Chehab]

Em Wed, 22 Feb 2017 21:07:24 +0000
Eddie Chapman <eddie@xxxxxxxx> escreveu:

> On 21/02/17 13:01, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> >
> > ------------------
> >
> > From: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> >
> > commit f9c85ee67164b37f9296eab3b754e543e4e96a1c upstream.
> >
> > Reported as a Kaffeine bug:
> > 	https://bugs.kde.org/show_bug.cgi?id=375811
> >
> > The USB control messages require DMA to work. We cannot pass
> > a stack-allocated buffer, as it is not warranted that the
> > stack would be into a DMA enabled area.
> >
> > On Kernel 4.9, the default is to not accept DMA on stack anymore
> > on x86 architecture. On other architectures, this has been a
> > requirement since Kernel 2.2. So, after this patch, this driver
> > should likely work fine on all archs.
> >
> > Tested with USB ID 2040:5510: Hauppauge Windham
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >
> > ---
> >  drivers/media/usb/siano/smsusb.c |   18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > --- a/drivers/media/usb/siano/smsusb.c
> > +++ b/drivers/media/usb/siano/smsusb.c
> > @@ -200,22 +200,30 @@ static int smsusb_start_streaming(struct
> >  static int smsusb_sendrequest(void *context, void *buffer, size_t size)
> >  {
> >  	struct smsusb_device_t *dev = (struct smsusb_device_t *) context;
> > -	struct sms_msg_hdr *phdr = (struct sms_msg_hdr *) buffer;
> > -	int dummy;
> > +	struct sms_msg_hdr *phdr;
> > +	int dummy, ret;
> >
> >  	if (dev->state != SMSUSB_ACTIVE) {
> >  		pr_debug("Device not active yet\n");
> >  		return -ENOENT;
> >  	}
> >
> > +	phdr = kmalloc(size, GFP_KERNEL);
> > +	if (!phdr)
> > +		return -ENOMEM;
> > +	memcpy(phdr, buffer, size);
> > +
> >  	pr_debug("sending %s(%d) size: %d\n",
> >  		  smscore_translate_msg(phdr->msg_type), phdr->msg_type,
> >  		  phdr->msg_length);
> >
> >  	smsendian_handle_tx_message((struct sms_msg_data *) phdr);
> > -	smsendian_handle_message_header((struct sms_msg_hdr *)buffer);
> > -	return usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 2),
> > -			    buffer, size, &dummy, 1000);
> > +	smsendian_handle_message_header((struct sms_msg_hdr *)phdr);
> > +	ret = usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 2),
> > +			    phdr, size, &dummy, 1000);
> > +
> > +	kfree(phdr);
> > +	return ret;
> >  }
> >
> >  static char *smsusb1_fw_lkup[] = {  
> 
> Hello Greg,
> 
> According to the bug report linked to in the commit message this is only 
> needed in 4.9 and higher.

Actually, it makes this device work on ARM platforms too.

The thing is, although forbidden since Kernel 2.2, on x86, using
the stack used to work (on most cases). So, there are many drivers
that were written without taking care of not using the stack before
sending an URB.

So, the best is to backport this patch to -stable Kernels as well,
as it will avoid potential (rare) issues on x86, but it will also
make the driver to work on non-x86 archs.

I updated the patch description to reflect it, but maybe I was not
too clear ;)


Thanks,
Mauro