| Message ID | 20201010065623.10189-1-anant.thazhemadam@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API | expand |
On Sat, 10 Oct 2020 12:26:23 +0530 Anant Thazhemadam wrote:
> GPF_KERNEL
You haven't even built this, let alone tested :/
On 10/10/20 10:33 pm, Jakub Kicinski wrote: > On Sat, 10 Oct 2020 12:26:23 +0530 Anant Thazhemadam wrote: >> GPF_KERNEL > You haven't even built this, let alone tested :/ I'm really sorry about this. Turns out, my .config wasn't set generated by make allyesconfig, and thus this regression went undetected. I can submit a v2 that doesn't break the build, and which is build tested properly. However, I'm not clear on how else I could locally run unit tests pertaining to this patch. I understand the critical requirement for testing changes and would really appreciate it if someone could direct me towards some resource or another that could help me figure that out too. Once again, I'm really sorry for this oversight. Thank you for your time. Thanks, Anant
Hi Anant,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.9 next-20201012]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Anant-Thazhemadam/net-usb-usbnet-update-__usbnet_-read-write-_cmd-to-use-new-API/20201010-145950
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6f2f486d57c4d562cdf4932320b66fbb878ab1c4
config: x86_64-randconfig-a005-20201012 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e72d3eaf38f217698f72cb8fdc969a6e72dad3a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/77da62a1a1fcc73c3d8309c273dfd4d497918c6e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Anant-Thazhemadam/net-usb-usbnet-update-__usbnet_-read-write-_cmd-to-use-new-API/20201010-145950
git checkout 77da62a1a1fcc73c3d8309c273dfd4d497918c6e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/usb/usbnet.c:1990:9: error: implicit declaration of function 'usb_control_msg_recv' [-Werror,-Wimplicit-function-declaration]
return usb_control_msg_recv(dev->udev, 0,
^
drivers/net/usb/usbnet.c:1990:9: note: did you mean 'usb_control_msg'?
include/linux/usb.h:1794:12: note: 'usb_control_msg' declared here
extern int usb_control_msg(struct usb_device *dev, unsigned int pipe,
^
drivers/net/usb/usbnet.c:2008:9: error: implicit declaration of function 'usb_control_msg_send' [-Werror,-Wimplicit-function-declaration]
return usb_control_msg_send(dev->udev, 0,
^
drivers/net/usb/usbnet.c:2008:9: note: did you mean 'usb_control_msg'?
include/linux/usb.h:1794:12: note: 'usb_control_msg' declared here
extern int usb_control_msg(struct usb_device *dev, unsigned int pipe,
^
>> drivers/net/usb/usbnet.c:2010:26: error: use of undeclared identifier 'GPF_KERNEL'
USB_CTRL_SET_TIMEOUT, GPF_KERNEL);
^
3 errors generated.
vim +/GPF_KERNEL +2010 drivers/net/usb/usbnet.c
1994
1995 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
1996 u16 value, u16 index, const void *data,
1997 u16 size)
1998 {
1999 netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
2000 " value=0x%04x index=0x%04x size=%d\n",
2001 cmd, reqtype, value, index, size);
2002
2003 if (size && !data) {
2004 WARN_ON_ONCE(1);
2005 return -EINVAL;
2006 }
2007
2008 return usb_control_msg_send(dev->udev, 0,
2009 cmd, reqtype, value, index, data, size,
> 2010 USB_CTRL_SET_TIMEOUT, GPF_KERNEL);
2011 }
2012
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 29/10/20 6:52 pm, Anant Thazhemadam wrote: > Currently, __usbnet_{read|write}_cmd() use usb_control_msg(), > and thus consider potential partial reads/writes being done to > be perfectly valid. > Quite a few callers of usbnet_{read|write}_cmd() don't enforce > checking for partial reads/writes into account either, automatically > assuming that a complete read/write occurs. > > However, the new usb_control_msg_{send|recv}() APIs don't allow partial > reads and writes. > Using the new APIs also relaxes the return value checking that must > be done after usbnet_{read|write}_cmd() is called. > > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> <mailto:anant.thazhemadam@gmail.com> > --- > Changes in v2: > * Fix build error > > This patch has been compile and build tested with a .config file that > was generated using make allyesconfig, and the build error has been > fixed. > Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing, > and would appreciate it if someone could do that. > > drivers/net/usb/usbnet.c | 52 ++++++++-------------------------------- > 1 file changed, 10 insertions(+), 42 deletions(-) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index bf6c58240bd4..2f7c7b7f4047 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change); > static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, > u16 value, u16 index, void *data, u16 size) > { > - void *buf = NULL; > - int err = -ENOMEM; > > netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x" > " value=0x%04x index=0x%04x size=%d\n", > cmd, reqtype, value, index, size); > > - if (size) { > - buf = kmalloc(size, GFP_KERNEL); > - if (!buf) > - goto out; > - } > - > - err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > - cmd, reqtype, value, index, buf, size, > - USB_CTRL_GET_TIMEOUT); > - if (err > 0 && err <= size) { > - if (data) > - memcpy(data, buf, err); > - else > - netdev_dbg(dev->net, > - "Huh? Data requested but thrown away.\n"); > - } > - kfree(buf); > -out: > - return err; > + return usb_control_msg_recv(dev->udev, 0, > + cmd, reqtype, value, index, data, size, > + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); > } > > static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, > u16 value, u16 index, const void *data, > u16 size) > { > - void *buf = NULL; > - int err = -ENOMEM; > - > netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x" > " value=0x%04x index=0x%04x size=%d\n", > cmd, reqtype, value, index, size); > > - if (data) { > - buf = kmemdup(data, size, GFP_KERNEL); > - if (!buf) > - goto out; > - } else { > - if (size) { > - WARN_ON_ONCE(1); > - err = -EINVAL; > - goto out; > - } > - } > - > - err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), > - cmd, reqtype, value, index, buf, size, > - USB_CTRL_SET_TIMEOUT); > - kfree(buf); > + if (size && !data) { > + WARN_ON_ONCE(1); > + return -EINVAL; > + } > > -out: > - return err; > + return usb_control_msg_send(dev->udev, 0, > + cmd, reqtype, value, index, data, size, > + USB_CTRL_SET_TIMEOUT, GFP_KERNEL); > } > > /* I had a v2 prepared and ready but was told to wait for a week before sending it in, since usb_control_msg_{send|recv}() that were being used were not present in the networking tree at the time, and all the trees would be converged by then. So, just to be on the safer side, I waited for two weeks. I checked the net tree, and found the APIs there too (defined in usb.h). However the build seems to fail here, https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/ I'm not entirely sure at this point why this is happening, and would appreciate it if someone could take the time to tell me if and how this might be an issue with my patch. Thanks, Anant
On 01/11/20 2:41 am, Jakub Kicinski wrote: > On Thu, 29 Oct 2020 18:52:56 +0530 Anant Thazhemadam wrote: >> + return usb_control_msg_recv(dev->udev, 0, >> + cmd, reqtype, value, index, data, size, >> + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); > Please align continuation lines after the opening bracket. I will do that, and send in a v3 right away. Thanks, Anant
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index bf6c58240bd4..dd9fe530a374 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change); static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value, u16 index, void *data, u16 size) { - void *buf = NULL; - int err = -ENOMEM; netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x" " value=0x%04x index=0x%04x size=%d\n", cmd, reqtype, value, index, size); - if (size) { - buf = kmalloc(size, GFP_KERNEL); - if (!buf) - goto out; - } - - err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), - cmd, reqtype, value, index, buf, size, - USB_CTRL_GET_TIMEOUT); - if (err > 0 && err <= size) { - if (data) - memcpy(data, buf, err); - else - netdev_dbg(dev->net, - "Huh? Data requested but thrown away.\n"); - } - kfree(buf); -out: - return err; + return usb_control_msg_recv(dev->udev, 0, + cmd, reqtype, value, index, data, size, + USB_CTRL_GET_TIMEOUT, GFP_KERNEL); } static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value, u16 index, const void *data, u16 size) { - void *buf = NULL; - int err = -ENOMEM; - netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x" " value=0x%04x index=0x%04x size=%d\n", cmd, reqtype, value, index, size); - if (data) { - buf = kmemdup(data, size, GFP_KERNEL); - if (!buf) - goto out; - } else { - if (size) { - WARN_ON_ONCE(1); - err = -EINVAL; - goto out; - } - } - - err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), - cmd, reqtype, value, index, buf, size, - USB_CTRL_SET_TIMEOUT); - kfree(buf); + if (size && !data) { + WARN_ON_ONCE(1); + return -EINVAL; + } -out: - return err; + return usb_control_msg_send(dev->udev, 0, + cmd, reqtype, value, index, data, size, + USB_CTRL_SET_TIMEOUT, GPF_KERNEL); } /*
Currently, __usbnet_{read|write}_cmd() use usb_control_msg(). However, this could lead to potential partial reads/writes being considered valid, and since most of the callers of usbnet_{read|write}_cmd() don't take partial reads/writes into account (only checking for negative error number is done), and this can lead to issues. However, the new usb_control_msg_{send|recv}() APIs don't allow partial reads and writes. Using the new APIs also relaxes the return value checking that must be done after usbnet_{read|write}_cmd() is called. Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> --- Since not all callers of usbnet_{read|write}_cmd() check if a complete read/write happened, partial reads can go unnoticed. This issue was briefly mentioned here. https://lore.kernel.org/linux-usb/1565777764.25764.4.camel@suse.com/ Using the new API in place of the old one doesn't break anything. This is mainly because usb_control_msg_{send|recv}() returns 0 on success and a negative error number on failure (which includes partial reads/writes). Thus, the error checking condition provided by the present callers of usbnet_{read|write}_cmd() for failure (return value < 0 is considered as an error) will hold. And similarly, the condition checked by some callers for 'success' (return value >= 0 && return value < length/size) will also hold. However, if I have missed out on any caller that this might cause problems with, please let me know, and I will fix that up as well. drivers/net/usb/usbnet.c | 52 ++++++++-------------------------------- 1 file changed, 10 insertions(+), 42 deletions(-)