LinuxLists.cc - [PATCH v2 0/2] Fix vbus draw of dwc3 gadget

2023-02-23 08:58:04

Subject: [PATCH v2 0/2] Fix vbus draw of dwc3 gadget

Changes in v2
- Added min() calculation against CONFIG_USB_GADGET_VBUS_DRAW in case
of unconfigured state.

Currently dwc3 gadget processes the suspend interrupt event only
if the device is in configured state. But consider a case where
device is not configured and got suspend interrupt, in that case
our gadget would still use 100mA as composite_suspend didn't happen.
But battery charging specification (BC1.2) expects a downstream
device to draw less than 2.5mA when unconnected OR suspended.

And while resuming, the gadget can draw upto 100mA if its not
configured, but the current implementation of composite_resume
doesn't consider the case of unconfigured device. This series
addresses the above mentioned issues.

Prashanth K (2):
usb: dwc3: gadget: Change condition for processing suspend event
usb: gadget: composite: Draw 100mA current if not configured

drivers/usb/dwc3/gadget.c | 11 ++---------
drivers/usb/gadget/composite.c | 3 +++
2 files changed, 5 insertions(+), 9 deletions(-)

--
2.7.4

2023-02-23 08:58:09

by Prashanth K

[permalink] [raw]

Subject: [PATCH v2 1/2] usb: dwc3: gadget: Change condition for processing suspend event

Currently we process the suspend interrupt event only if the
device is in configured state. Consider a case where device
is not configured and got suspend interrupt, in that case our
gadget will still use 100mA as composite_suspend didn't happen.
But battery charging specification (BC1.2) expects a downstream
device to draw less than 2.5mA when unconnected OR suspended.

Fix this by removing the condition for processing suspend event,
and thus composite_resume would set vbus draw to 2.

Fixes: 72704f876f50 ("dwc3: gadget: Implement the suspend entry event handler")
Signed-off-by: Prashanth K <[email protected]>
---
drivers/usb/dwc3/gadget.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 89dcfac..a83f34e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4241,15 +4241,8 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
break;
case DWC3_DEVICE_EVENT_SUSPEND:
/* It changed to be suspend event for version 2.30a and above */
- if (!DWC3_VER_IS_PRIOR(DWC3, 230A)) {
- /*
- * Ignore suspend event until the gadget enters into
- * USB_STATE_CONFIGURED state.
- */
- if (dwc->gadget->state >= USB_STATE_CONFIGURED)
- dwc3_gadget_suspend_interrupt(dwc,
- event->event_info);
- }
+ if (!DWC3_VER_IS_PRIOR(DWC3, 230A))
+ dwc3_gadget_suspend_interrupt(dwc, event->event_info);
break;
case DWC3_DEVICE_EVENT_SOF:
case DWC3_DEVICE_EVENT_ERRATIC_ERROR:
--
2.7.4

2023-02-23 08:58:12

by Prashanth K

[permalink] [raw]

Subject: [PATCH v2 2/2] usb: gadget: composite: Draw 100mA current if not configured

Currently we don't change the current value if device isn't in
configured state. But the battery charging specification says,
device can draw up to 100mA of current if its in unconfigured
state. Hence add a Vbus_draw work in composite_resume to draw
100mA if the device isn't configured.

Signed-off-by: Prashanth K <[email protected]>
---
drivers/usb/gadget/composite.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 403563c..23b7347a8 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2449,6 +2449,9 @@ void composite_resume(struct usb_gadget *gadget)
usb_gadget_clear_selfpowered(gadget);

usb_gadget_vbus_draw(gadget, maxpower);
+ } else {
+ maxpower = min(CONFIG_USB_GADGET_VBUS_DRAW, 100U)
+ usb_gadget_vbus_draw(gadget, maxpower);
}

cdev->suspended = 0;
--
2.7.4

2023-02-23 11:58:07

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] usb: gadget: composite: Draw 100mA current if not configured

Hi Prashanth,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.2]
[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#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Prashanth-K/usb-dwc3-gadget-Change-condition-for-processing-suspend-event/20230223-165955
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1677142665-8686-3-git-send-email-quic_prashk%40quicinc.com
patch subject: [PATCH v2 2/2] usb: gadget: composite: Draw 100mA current if not configured
config: hexagon-randconfig-r005-20230222 (https://download.01.org/0day-ci/archive/20230223/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project db89896bbbd2251fff457699635acbbedeead27f)
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/intel-lab-lkp/linux/commit/19beaeb0554fc9c1556e8f7da85011f4267bd8fc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Prashanth-K/usb-dwc3-gadget-Change-condition-for-processing-suspend-event/20230223-165955
git checkout 19beaeb0554fc9c1556e8f7da85011f4267bd8fc
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/gadget/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All error/warnings (new ones prefixed by >>):

In file included from drivers/usb/gadget/composite.c:19:
In file included from include/linux/usb/composite.h:27:
In file included from include/linux/usb/gadget.h:24:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/usb/gadget/composite.c:19:
In file included from include/linux/usb/composite.h:27:
In file included from include/linux/usb/gadget.h:24:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/usb/gadget/composite.c:19:
In file included from include/linux/usb/composite.h:27:
In file included from include/linux/usb/gadget.h:24:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/usb/gadget/composite.c:2535:14: warning: comparison of distinct pointer types ('typeof (2) *' (aka 'int *') and 'typeof (100U) *' (aka 'unsigned int *')) [-Wcompare-distinct-pointer-types]
maxpower = min(CONFIG_USB_GADGET_VBUS_DRAW, 100U)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:67:19: note: expanded from macro 'min'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~~~~~~~
include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~~~~~~~
include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
>> drivers/usb/gadget/composite.c:2535:52: error: expected ';' after expression
maxpower = min(CONFIG_USB_GADGET_VBUS_DRAW, 100U)
^
;
7 warnings and 1 error generated.

vim +2535 drivers/usb/gadget/composite.c

2504
2505 void composite_resume(struct usb_gadget *gadget)
2506 {
2507 struct usb_composite_dev *cdev = get_gadget_data(gadget);
2508 struct usb_function *f;
2509 unsigned maxpower;
2510
2511 /* REVISIT: should we have config level
2512 * suspend/resume callbacks?
2513 */
2514 DBG(cdev, "resume\n");
2515 if (cdev->driver->resume)
2516 cdev->driver->resume(cdev);
2517 if (cdev->config) {
2518 list_for_each_entry(f, &cdev->config->functions, list) {
2519 if (f->resume)
2520 f->resume(f);
2521 }
2522
2523 maxpower = cdev->config->MaxPower ?
2524 cdev->config->MaxPower : CONFIG_USB_GADGET_VBUS_DRAW;
2525 if (gadget->speed < USB_SPEED_SUPER)
2526 maxpower = min(maxpower, 500U);
2527 else
2528 maxpower = min(maxpower, 900U);
2529
2530 if (maxpower > USB_SELF_POWER_VBUS_MAX_DRAW)
2531 usb_gadget_clear_selfpowered(gadget);
2532
2533 usb_gadget_vbus_draw(gadget, maxpower);
2534 } else {
> 2535 maxpower = min(CONFIG_USB_GADGET_VBUS_DRAW, 100U)
2536 usb_gadget_vbus_draw(gadget, maxpower);
2537 }
2538
2539 cdev->suspended = 0;
2540 }
2541

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-02-23 12:11:41

by kernel test robot

[permalink] [raw]