| Message ID | 1494336614-2107-6-git-send-email-amit.karwar@redpinesignals.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Amitkumar Karwar <amitkarwar@gmail.com> writes: > From: Prameela Rani Garnepudi <prameela.j04cs@gmail.com> > > In functions usb read register and usb write register, dynamic allocation > of 4 bytes is used. This is removed as it is unncessary for local variable > and for such small data. > > Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@gmail.com> > Signed-off-by: Amitkumar Karwar <amit.karwar@redpinesignals.com> > --- > drivers/net/wireless/rsi/rsi_91x_usb.c | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c > index 73b01a8..8eb7407 100644 > --- a/drivers/net/wireless/rsi/rsi_91x_usb.c > +++ b/drivers/net/wireless/rsi/rsi_91x_usb.c > @@ -157,12 +157,8 @@ static int rsi_usb_reg_read(struct usb_device *usbdev, > u16 *value, > u16 len) > { > - u8 *buf; > - int status = -ENOMEM; > - > - buf = kmalloc(0x04, GFP_KERNEL); > - if (!buf) > - return status; > + u8 buf[4]; > + int status; > > status = usb_control_msg(usbdev, > usb_rcvctrlpipe(usbdev, 0), Recently I got a patch to orinoco_usb which did exactly the opposite (unless I'm missing something): https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=2f6ae79cb04bb7f9b4be3f1c32b6fda35bf976bc The documentation for usb_control_msg() does not mention anything if it's possible to use stack memory, but AFAIU it's not possible to use stack memory with DMA. Can anyone clarify?
On 5/11/2017 8:28 PM, Kalle Valo wrote: > Amitkumar Karwar <amitkarwar@gmail.com> writes: > >> From: Prameela Rani Garnepudi <prameela.j04cs@gmail.com> >> >> In functions usb read register and usb write register, dynamic allocation >> of 4 bytes is used. This is removed as it is unncessary for local variable >> and for such small data. >> >> Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@gmail.com> >> Signed-off-by: Amitkumar Karwar <amit.karwar@redpinesignals.com> >> --- >> drivers/net/wireless/rsi/rsi_91x_usb.c | 18 ++++-------------- >> 1 file changed, 4 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c >> index 73b01a8..8eb7407 100644 >> --- a/drivers/net/wireless/rsi/rsi_91x_usb.c >> +++ b/drivers/net/wireless/rsi/rsi_91x_usb.c >> @@ -157,12 +157,8 @@ static int rsi_usb_reg_read(struct usb_device *usbdev, >> u16 *value, >> u16 len) >> { >> - u8 *buf; >> - int status = -ENOMEM; >> - >> - buf = kmalloc(0x04, GFP_KERNEL); >> - if (!buf) >> - return status; >> + u8 buf[4]; >> + int status; >> >> status = usb_control_msg(usbdev, >> usb_rcvctrlpipe(usbdev, 0), > > Recently I got a patch to orinoco_usb which did exactly the opposite > (unless I'm missing something): > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=2f6ae79cb04bb7f9b4be3f1c32b6fda35bf976bc > > The documentation for usb_control_msg() does not mention anything if > it's possible to use stack memory, but AFAIU it's not possible to use > stack memory with DMA. Can anyone clarify? After private message I sent to Kalle here my public response :-p According to Greg this has been a USB core requirement for a long, long time (see below). Regards, Arend [1] http://marc.info/?l=linux-kernel&m=149391173014176&w=2 -------- Forwarded Message -------- Subject: Re: [PATCH] i2c: i2c-tiny-usb: fix buffer not being DMA capable Date: Thu, 4 May 2017 08:27:09 -0700 From: Greg KH <gregkh@linuxfoundation.org> To: Sebastian Reichel <sebastian.reichel@collabora.co.uk> CC: Sebastian Reichel <sre@kernel.org>, Till Harbaum <till@harbaum.org>, Wolfram Sang <wsa@the-dreams.de>, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, # Kernel 4 . 9+ <stable@vger.kernel.org> On Thu, May 04, 2017 at 02:56:32PM +0200, Sebastian Reichel wrote: > The i2c-tiny-usb driver generates the following trace and fails to > read or write data on a v4.9 or newer kernels. This patch avoids the > problem by allocating a DMA capable buffer for data transfer. [snip] This has been a USB core requirement since the 2.2 days, it's only since 4.9 that most people have started to notice this. So backporting it to anything that it can go to is best to have happen :) > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > drivers/i2c/busses/i2c-tiny-usb.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-)
On Fri, May 12, 2017 at 1:58 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > On 5/11/2017 8:28 PM, Kalle Valo wrote: >> >> Amitkumar Karwar <amitkarwar@gmail.com> writes: >> >>> From: Prameela Rani Garnepudi <prameela.j04cs@gmail.com> >>> >>> In functions usb read register and usb write register, dynamic allocation >>> of 4 bytes is used. This is removed as it is unncessary for local >>> variable >>> and for such small data. >>> >>> Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@gmail.com> >>> Signed-off-by: Amitkumar Karwar <amit.karwar@redpinesignals.com> >>> --- >>> drivers/net/wireless/rsi/rsi_91x_usb.c | 18 ++++-------------- >>> 1 file changed, 4 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c >>> b/drivers/net/wireless/rsi/rsi_91x_usb.c >>> index 73b01a8..8eb7407 100644 >>> --- a/drivers/net/wireless/rsi/rsi_91x_usb.c >>> +++ b/drivers/net/wireless/rsi/rsi_91x_usb.c >>> @@ -157,12 +157,8 @@ static int rsi_usb_reg_read(struct usb_device >>> *usbdev, >>> u16 *value, >>> u16 len) >>> { >>> - u8 *buf; >>> - int status = -ENOMEM; >>> - >>> - buf = kmalloc(0x04, GFP_KERNEL); >>> - if (!buf) >>> - return status; >>> + u8 buf[4]; >>> + int status; >>> status = usb_control_msg(usbdev, >>> usb_rcvctrlpipe(usbdev, 0), >> >> >> Recently I got a patch to orinoco_usb which did exactly the opposite >> (unless I'm missing something): >> >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=2f6ae79cb04bb7f9b4be3f1c32b6fda35bf976bc >> >> The documentation for usb_control_msg() does not mention anything if >> it's possible to use stack memory, but AFAIU it's not possible to use >> stack memory with DMA. Can anyone clarify? > > > After private message I sent to Kalle here my public response :-p > According to Greg this has been a USB core requirement for a long, long time > (see below). > Thanks Arend. I have dropped this patch in V3 series. Regards, Amitkumar Karwar
diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c index 73b01a8..8eb7407 100644 --- a/drivers/net/wireless/rsi/rsi_91x_usb.c +++ b/drivers/net/wireless/rsi/rsi_91x_usb.c @@ -157,12 +157,8 @@ static int rsi_usb_reg_read(struct usb_device *usbdev, u16 *value, u16 len) { - u8 *buf; - int status = -ENOMEM; - - buf = kmalloc(0x04, GFP_KERNEL); - if (!buf) - return status; + u8 buf[4]; + int status; status = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0), @@ -179,7 +175,6 @@ static int rsi_usb_reg_read(struct usb_device *usbdev, "%s: Reg read failed with error code :%d\n", __func__, status); } - kfree(buf); return status; } @@ -199,12 +194,8 @@ static int rsi_usb_reg_write(struct usb_device *usbdev, u16 value, u16 len) { - u8 *usb_reg_buf; - int status = -ENOMEM; - - usb_reg_buf = kmalloc(0x04, GFP_KERNEL); - if (!usb_reg_buf) - return status; + u8 usb_reg_buf[4]; + int status; usb_reg_buf[0] = (value & 0x00ff); usb_reg_buf[1] = (value & 0xff00) >> 8; @@ -225,7 +216,6 @@ static int rsi_usb_reg_write(struct usb_device *usbdev, "%s: Reg write failed with error code :%d\n", __func__, status); } - kfree(usb_reg_buf); return status; }