Currently if we bootup a device without cable connected, then
usb-conn-gpio won't call set_role() since last_role is same as
current role. This happens because during probe last_role gets
initialized to zero.
To avoid this, added a new constant in enum usb_role, last_role
is set to USB_ROLE_UNKNOWN before performing initial detection.
Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
Signed-off-by: Prashanth K <[email protected]>
---
v2: Added USB_ROLE_UNKNWON to enum usb_role
drivers/usb/common/usb-conn-gpio.c | 3 +++
include/linux/usb/role.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index e20874c..30bdb81 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -257,6 +257,9 @@ static int usb_conn_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, info);
device_set_wakeup_capable(&pdev->dev, true);
+ /* Set last role to unknown before performing the initial detection */
+ info->last_role = USB_ROLE_UNKNOWN;
+
/* Perform initial detection */
usb_conn_queue_dwork(info, 0);
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index b5deafd..221d462 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -8,6 +8,7 @@
struct usb_role_switch;
enum usb_role {
+ USB_ROLE_UNKNOWN = -1,
USB_ROLE_NONE,
USB_ROLE_HOST,
USB_ROLE_DEVICE,
--
2.7.4
Hi Prashanth,
kernel test robot noticed the following build warnings:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.4-rc3 next-20230524]
[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-common-usb-conn-gpio-Set-last-role-to-unknown-before-initial-detection/20230524-215109
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1684936207-23529-1-git-send-email-quic_prashk%40quicinc.com
patch subject: [PATCH v2] usb: common: usb-conn-gpio: Set last role to unknown before initial detection
config: sparc-allyesconfig
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
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/263bb2a82451e76309b0c0f4d6f6194dd85954ca
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Prashanth-K/usb-common-usb-conn-gpio-Set-last-role-to-unknown-before-initial-detection/20230524-215109
git checkout 263bb2a82451e76309b0c0f4d6f6194dd85954ca
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/usb/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/usb/cdns3/core.c: In function 'cdns_hw_role_state_machine':
>> drivers/usb/cdns3/core.c:236:9: warning: enumeration value 'USB_ROLE_UNKNOWN' not handled in switch [-Wswitch]
236 | switch (role) {
| ^~~~~~
vim +/USB_ROLE_UNKNOWN +236 drivers/usb/cdns3/core.c
7733f6c32e36ff Pawel Laszczak 2019-08-26 204
7733f6c32e36ff Pawel Laszczak 2019-08-26 205 /**
0b490046d8d7c0 Pawel Laszczak 2020-12-07 206 * cdns_hw_role_state_machine - role switch state machine based on hw events.
7733f6c32e36ff Pawel Laszczak 2019-08-26 207 * @cdns: Pointer to controller structure.
7733f6c32e36ff Pawel Laszczak 2019-08-26 208 *
7733f6c32e36ff Pawel Laszczak 2019-08-26 209 * Returns next role to be entered based on hw events.
7733f6c32e36ff Pawel Laszczak 2019-08-26 210 */
0b490046d8d7c0 Pawel Laszczak 2020-12-07 211 static enum usb_role cdns_hw_role_state_machine(struct cdns *cdns)
7733f6c32e36ff Pawel Laszczak 2019-08-26 212 {
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 213 enum usb_role role = USB_ROLE_NONE;
7733f6c32e36ff Pawel Laszczak 2019-08-26 214 int id, vbus;
7733f6c32e36ff Pawel Laszczak 2019-08-26 215
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 216 if (cdns->dr_mode != USB_DR_MODE_OTG) {
0b490046d8d7c0 Pawel Laszczak 2020-12-07 217 if (cdns_is_host(cdns))
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 218 role = USB_ROLE_HOST;
0b490046d8d7c0 Pawel Laszczak 2020-12-07 219 if (cdns_is_device(cdns))
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 220 role = USB_ROLE_DEVICE;
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 221
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 222 return role;
5c2cf30f14cc39 Pawel Laszczak 2020-07-13 223 }
7733f6c32e36ff Pawel Laszczak 2019-08-26 224
0b490046d8d7c0 Pawel Laszczak 2020-12-07 225 id = cdns_get_id(cdns);
0b490046d8d7c0 Pawel Laszczak 2020-12-07 226 vbus = cdns_get_vbus(cdns);
7733f6c32e36ff Pawel Laszczak 2019-08-26 227
7733f6c32e36ff Pawel Laszczak 2019-08-26 228 /*
7733f6c32e36ff Pawel Laszczak 2019-08-26 229 * Role change state machine
7733f6c32e36ff Pawel Laszczak 2019-08-26 230 * Inputs: ID, VBUS
7733f6c32e36ff Pawel Laszczak 2019-08-26 231 * Previous state: cdns->role
7733f6c32e36ff Pawel Laszczak 2019-08-26 232 * Next state: role
7733f6c32e36ff Pawel Laszczak 2019-08-26 233 */
7733f6c32e36ff Pawel Laszczak 2019-08-26 234 role = cdns->role;
7733f6c32e36ff Pawel Laszczak 2019-08-26 235
7733f6c32e36ff Pawel Laszczak 2019-08-26 @236 switch (role) {
7733f6c32e36ff Pawel Laszczak 2019-08-26 237 case USB_ROLE_NONE:
7733f6c32e36ff Pawel Laszczak 2019-08-26 238 /*
7733f6c32e36ff Pawel Laszczak 2019-08-26 239 * Driver treats USB_ROLE_NONE synonymous to IDLE state from
7733f6c32e36ff Pawel Laszczak 2019-08-26 240 * controller specification.
7733f6c32e36ff Pawel Laszczak 2019-08-26 241 */
7733f6c32e36ff Pawel Laszczak 2019-08-26 242 if (!id)
7733f6c32e36ff Pawel Laszczak 2019-08-26 243 role = USB_ROLE_HOST;
7733f6c32e36ff Pawel Laszczak 2019-08-26 244 else if (vbus)
7733f6c32e36ff Pawel Laszczak 2019-08-26 245 role = USB_ROLE_DEVICE;
7733f6c32e36ff Pawel Laszczak 2019-08-26 246 break;
7733f6c32e36ff Pawel Laszczak 2019-08-26 247 case USB_ROLE_HOST: /* from HOST, we can only change to NONE */
7733f6c32e36ff Pawel Laszczak 2019-08-26 248 if (id)
7733f6c32e36ff Pawel Laszczak 2019-08-26 249 role = USB_ROLE_NONE;
7733f6c32e36ff Pawel Laszczak 2019-08-26 250 break;
7733f6c32e36ff Pawel Laszczak 2019-08-26 251 case USB_ROLE_DEVICE: /* from GADGET, we can only change to NONE*/
7733f6c32e36ff Pawel Laszczak 2019-08-26 252 if (!vbus)
7733f6c32e36ff Pawel Laszczak 2019-08-26 253 role = USB_ROLE_NONE;
7733f6c32e36ff Pawel Laszczak 2019-08-26 254 break;
7733f6c32e36ff Pawel Laszczak 2019-08-26 255 }
7733f6c32e36ff Pawel Laszczak 2019-08-26 256
7733f6c32e36ff Pawel Laszczak 2019-08-26 257 dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
7733f6c32e36ff Pawel Laszczak 2019-08-26 258
7733f6c32e36ff Pawel Laszczak 2019-08-26 259 return role;
7733f6c32e36ff Pawel Laszczak 2019-08-26 260 }
7733f6c32e36ff Pawel Laszczak 2019-08-26 261
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, 2023-05-24 at 19:20 +0530, Prashanth K wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Currently if we bootup a device without cable connected, then
> usb-conn-gpio won't call set_role() since last_role is same as
> current role. This happens because during probe last_role gets
> initialized to zero.
>
> To avoid this, added a new constant in enum usb_role, last_role
> is set to USB_ROLE_UNKNOWN before performing initial detection.
>
> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection
> detection driver")
> Signed-off-by: Prashanth K <[email protected]>
> ---
> v2: Added USB_ROLE_UNKNWON to enum usb_role
>
> drivers/usb/common/usb-conn-gpio.c | 3 +++
> include/linux/usb/role.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/usb/common/usb-conn-gpio.c
> b/drivers/usb/common/usb-conn-gpio.c
> index e20874c..30bdb81 100644
> --- a/drivers/usb/common/usb-conn-gpio.c
> +++ b/drivers/usb/common/usb-conn-gpio.c
> @@ -257,6 +257,9 @@ static int usb_conn_probe(struct platform_device
> *pdev)
> platform_set_drvdata(pdev, info);
> device_set_wakeup_capable(&pdev->dev, true);
>
> + /* Set last role to unknown before performing the initial
> detection */
> + info->last_role = USB_ROLE_UNKNOWN;
Do you only use vbus-pin?
This driver assumes that the gadget driver's default role is none.
> +
> /* Perform initial detection */
> usb_conn_queue_dwork(info, 0);
>
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index b5deafd..221d462 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -8,6 +8,7 @@
> struct usb_role_switch;
>
> enum usb_role {
> + USB_ROLE_UNKNOWN = -1,
> USB_ROLE_NONE,
> USB_ROLE_HOST,
> USB_ROLE_DEVICE,
> --
> 2.7.4
>
Il 24/05/23 15:50, Prashanth K ha scritto:
> Currently if we bootup a device without cable connected, then
> usb-conn-gpio won't call set_role() since last_role is same as
> current role. This happens because during probe last_role gets
> initialized to zero.
>
> To avoid this, added a new constant in enum usb_role, last_role
> is set to USB_ROLE_UNKNOWN before performing initial detection.
>
> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
> Signed-off-by: Prashanth K <[email protected]>
There's an issue with drivers/usb/cdns3/core.c as pointed out by the
test robot; the solution is to handle `default` in the switch, I'd say
that it would be safe to handle it as
default:
break;
after solving that:
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> v2: Added USB_ROLE_UNKNWON to enum usb_role
>
> drivers/usb/common/usb-conn-gpio.c | 3 +++
> include/linux/usb/role.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> index e20874c..30bdb81 100644
> --- a/drivers/usb/common/usb-conn-gpio.c
> +++ b/drivers/usb/common/usb-conn-gpio.c
> @@ -257,6 +257,9 @@ static int usb_conn_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, info);
> device_set_wakeup_capable(&pdev->dev, true);
>
> + /* Set last role to unknown before performing the initial detection */
> + info->last_role = USB_ROLE_UNKNOWN;
> +
> /* Perform initial detection */
> usb_conn_queue_dwork(info, 0);
>
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index b5deafd..221d462 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -8,6 +8,7 @@
> struct usb_role_switch;
>
> enum usb_role {
> + USB_ROLE_UNKNOWN = -1,
> USB_ROLE_NONE,
> USB_ROLE_HOST,
> USB_ROLE_DEVICE,
On 25-05-23 12:27 pm, Chunfeng Yun (云春峰) wrote:
> On Wed, 2023-05-24 at 19:20 +0530, Prashanth K wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Currently if we bootup a device without cable connected, then
>> usb-conn-gpio won't call set_role() since last_role is same as
>> current role. This happens because during probe last_role gets
>> initialized to zero.
>>
>> To avoid this, added a new constant in enum usb_role, last_role
>> is set to USB_ROLE_UNKNOWN before performing initial detection.
>>
>> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection
>> detection driver")
>> Signed-off-by: Prashanth K <[email protected]>
>> ---
>> v2: Added USB_ROLE_UNKNWON to enum usb_role
>>
>> drivers/usb/common/usb-conn-gpio.c | 3 +++
>> include/linux/usb/role.h | 1 +
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/common/usb-conn-gpio.c
>> b/drivers/usb/common/usb-conn-gpio.c
>> index e20874c..30bdb81 100644
>> --- a/drivers/usb/common/usb-conn-gpio.c
>> +++ b/drivers/usb/common/usb-conn-gpio.c
>> @@ -257,6 +257,9 @@ static int usb_conn_probe(struct platform_device
>> *pdev)
>> platform_set_drvdata(pdev, info);
>> device_set_wakeup_capable(&pdev->dev, true);
>>
>> + /* Set last role to unknown before performing the initial
>> detection */
>> + info->last_role = USB_ROLE_UNKNOWN;
>
> Do you only use vbus-pin?
This driver has support for both Vbus and ID GPIOs.
>
> This driver assumes that the gadget driver's default role is none.
No, after probe it calls set role based on the state of Vbus and ID pin.
If Vbus is low, then it should issue none role to the gadget. But
currently it doesnt call set_role if initial role is none.
Regards
On 25-05-23 01:37 pm, AngeloGioacchino Del Regno wrote:
> Il 24/05/23 15:50, Prashanth K ha scritto:
>> Currently if we bootup a device without cable connected, then
>> usb-conn-gpio won't call set_role() since last_role is same as
>> current role. This happens because during probe last_role gets
>> initialized to zero.
>>
>> To avoid this, added a new constant in enum usb_role, last_role
>> is set to USB_ROLE_UNKNOWN before performing initial detection.
>>
>> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection
>> detection driver")
>> Signed-off-by: Prashanth K <[email protected]>
>
> There's an issue with drivers/usb/cdns3/core.c as pointed out by the
> test robot; the solution is to handle `default` in the switch, I'd say
> that it would be safe to handle it as
>
> default:
> break;
>
> after solving that:
>
> Reviewed-by: AngeloGioacchino Del Regno
> <[email protected]>
Yea sure, thanks for the suggestion Agnelo
Regards,
Prashanth K