| Message ID | a6ccf6adbcfeaad8c4ed24e94c50b2dd3db57c15.1627311383.git.paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: fix same memory leaks in can drivers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Pavel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkl-can-next/testing]
[also build test ERROR on v5.14-rc3 next-20210723]
[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/Pavel-Skripkin/can-fix-same-memory-leaks-in-can-drivers/20210726-233224
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 10.3.0
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
# https://github.com/0day-ci/linux/commit/14373e3834eb74f943720146607c709613b93e95
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Pavel-Skripkin/can-fix-same-memory-leaks-in-can-drivers/20210726-233224
git checkout 14373e3834eb74f943720146607c709613b93e95
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash drivers/net/can/usb/
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/can/usb/esd_usb2.c: In function 'esd_usb2_setup_rx_urbs':
>> drivers/net/can/usb/esd_usb2.c:582:4: error: label 'freeusrb' used but not defined
582 | goto freeusrb;
| ^~~~
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
Selected by
- SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
- SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC
vim +/freeusrb +582 drivers/net/can/usb/esd_usb2.c
539
540 static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev)
541 {
542 int i, err = 0;
543
544 if (dev->rxinitdone)
545 return 0;
546
547 for (i = 0; i < MAX_RX_URBS; i++) {
548 struct urb *urb = NULL;
549 u8 *buf = NULL;
550 dma_addr_t buf_dma;
551
552 /* create a URB, and a buffer for it */
553 urb = usb_alloc_urb(0, GFP_KERNEL);
554 if (!urb) {
555 err = -ENOMEM;
556 break;
557 }
558
559 buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
560 &buf_dma);
561 if (!buf) {
562 dev_warn(dev->udev->dev.parent,
563 "No memory left for USB buffer\n");
564 err = -ENOMEM;
565 goto freeurb;
566 }
567
568 urb->transfer_dma = buf_dma;
569
570 usb_fill_bulk_urb(urb, dev->udev,
571 usb_rcvbulkpipe(dev->udev, 1),
572 buf, RX_BUFFER_SIZE,
573 esd_usb2_read_bulk_callback, dev);
574 urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
575 usb_anchor_urb(urb, &dev->rx_submitted);
576
577 err = usb_submit_urb(urb, GFP_KERNEL);
578 if (err) {
579 usb_unanchor_urb(urb);
580 usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
581 urb->transfer_dma);
> 582 goto freeusrb;
583 }
584
585 dev->rxbuf[i] = buf;
586 dev->rxbuf_dma[i] = buf_dma;
587
588 freeurb:
589 /* Drop reference, USB core will take care of freeing it */
590 usb_free_urb(urb);
591 if (err)
592 break;
593 }
594
595 /* Did we submit any URBs */
596 if (i == 0) {
597 dev_err(dev->udev->dev.parent, "couldn't setup read URBs\n");
598 return err;
599 }
600
601 /* Warn if we've couldn't transmit all the URBs */
602 if (i < MAX_RX_URBS) {
603 dev_warn(dev->udev->dev.parent,
604 "rx performance may be slow\n");
605 }
606
607 dev->rxinitdone = 1;
608 return 0;
609 }
610
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Pavel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkl-can-next/testing]
[also build test ERROR on v5.14-rc3 next-20210727]
[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/Pavel-Skripkin/can-fix-same-memory-leaks-in-can-drivers/20210726-233224
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing
config: x86_64-randconfig-a012-20210728 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project c49df15c278857adecd12db6bb1cdc96885f7079)
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
# https://github.com/0day-ci/linux/commit/14373e3834eb74f943720146607c709613b93e95
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Pavel-Skripkin/can-fix-same-memory-leaks-in-can-drivers/20210726-233224
git checkout 14373e3834eb74f943720146607c709613b93e95
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=x86_64 SHELL=/bin/bash
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/can/usb/esd_usb2.c:582:9: error: use of undeclared label 'freeusrb'
goto freeusrb;
^
1 error generated.
vim +/freeusrb +582 drivers/net/can/usb/esd_usb2.c
539
540 static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev)
541 {
542 int i, err = 0;
543
544 if (dev->rxinitdone)
545 return 0;
546
547 for (i = 0; i < MAX_RX_URBS; i++) {
548 struct urb *urb = NULL;
549 u8 *buf = NULL;
550 dma_addr_t buf_dma;
551
552 /* create a URB, and a buffer for it */
553 urb = usb_alloc_urb(0, GFP_KERNEL);
554 if (!urb) {
555 err = -ENOMEM;
556 break;
557 }
558
559 buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
560 &buf_dma);
561 if (!buf) {
562 dev_warn(dev->udev->dev.parent,
563 "No memory left for USB buffer\n");
564 err = -ENOMEM;
565 goto freeurb;
566 }
567
568 urb->transfer_dma = buf_dma;
569
570 usb_fill_bulk_urb(urb, dev->udev,
571 usb_rcvbulkpipe(dev->udev, 1),
572 buf, RX_BUFFER_SIZE,
573 esd_usb2_read_bulk_callback, dev);
574 urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
575 usb_anchor_urb(urb, &dev->rx_submitted);
576
577 err = usb_submit_urb(urb, GFP_KERNEL);
578 if (err) {
579 usb_unanchor_urb(urb);
580 usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
581 urb->transfer_dma);
> 582 goto freeusrb;
583 }
584
585 dev->rxbuf[i] = buf;
586 dev->rxbuf_dma[i] = buf_dma;
587
588 freeurb:
589 /* Drop reference, USB core will take care of freeing it */
590 usb_free_urb(urb);
591 if (err)
592 break;
593 }
594
595 /* Did we submit any URBs */
596 if (i == 0) {
597 dev_err(dev->udev->dev.parent, "couldn't setup read URBs\n");
598 return err;
599 }
600
601 /* Warn if we've couldn't transmit all the URBs */
602 if (i < MAX_RX_URBS) {
603 dev_warn(dev->udev->dev.parent,
604 "rx performance may be slow\n");
605 }
606
607 dev->rxinitdone = 1;
608 return 0;
609 }
610
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c index 65b58f8fc328..303560abe2b0 100644 --- a/drivers/net/can/usb/esd_usb2.c +++ b/drivers/net/can/usb/esd_usb2.c @@ -195,6 +195,8 @@ struct esd_usb2 { int net_count; u32 version; int rxinitdone; + void *rxbuf[MAX_RX_URBS]; + dma_addr_t rxbuf_dma[MAX_RX_URBS]; }; struct esd_usb2_net_priv { @@ -545,6 +547,7 @@ static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev) for (i = 0; i < MAX_RX_URBS; i++) { struct urb *urb = NULL; u8 *buf = NULL; + dma_addr_t buf_dma; /* create a URB, and a buffer for it */ urb = usb_alloc_urb(0, GFP_KERNEL); @@ -554,7 +557,7 @@ static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev) } buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL, - &urb->transfer_dma); + &buf_dma); if (!buf) { dev_warn(dev->udev->dev.parent, "No memory left for USB buffer\n"); @@ -562,6 +565,8 @@ static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev) goto freeurb; } + urb->transfer_dma = buf_dma; + usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1), buf, RX_BUFFER_SIZE, @@ -574,8 +579,12 @@ static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev) usb_unanchor_urb(urb); usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf, urb->transfer_dma); + goto freeusrb; } + dev->rxbuf[i] = buf; + dev->rxbuf_dma[i] = buf_dma; + freeurb: /* Drop reference, USB core will take care of freeing it */ usb_free_urb(urb); @@ -663,6 +672,11 @@ static void unlink_all_urbs(struct esd_usb2 *dev) int i, j; usb_kill_anchored_urbs(&dev->rx_submitted); + + for (i = 0; i < MAX_RX_URBS; ++i) + usb_free_coherent(dev->udev, RX_BUFFER_SIZE, + dev->rxbuf[i], dev->rxbuf_dma[i]); + for (i = 0; i < dev->net_count; i++) { priv = dev->nets[i]; if (priv) {
In esd_usb2_setup_rx_urbs() MAX_RX_URBS coherent buffers are allocated and there is nothing, that frees them: 1) In callback function the urb is resubmitted and that's all 2) In disconnect function urbs are simply killed, but URB_FREE_BUFFER is not set (see esd_usb2_setup_rx_urbs) and this flag cannot be used with coherent buffers. So, all allocated buffers should be freed with usb_free_coherent() explicitly. Side note: This code looks like a copy-paste of other can drivers. The same patch was applied to mcba_usb driver and it works nice with real hardware. There is no change in functionality, only clean-up code for coherent buffers Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device") Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- drivers/net/can/usb/esd_usb2.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)