diff mbox series

[v2] usb: common: usb-conn-gpio: Set last role to unknown before initial detection

Message ID 1684936207-23529-1-git-send-email-quic_prashk@quicinc.com
State New
Headers
Series [v2] usb: common: usb-conn-gpio: Set last role to unknown before initial detection |

Commit Message

Prashanth K May 24, 2023, 1:50 p.m. UTC 
  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 <quic_prashk@quicinc.com>
---
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(+)

Comments

kernel test robot May 24, 2023, 8:07 p.m. UTC | #1 
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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305250319.A6ppbHdS-lkp@intel.com/

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
Chunfeng Yun (云春峰) May 25, 2023, 6:57 a.m. UTC | #2 
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 <quic_prashk@quicinc.com>
> ---
> 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
>
AngeloGioacchino Del Regno May 25, 2023, 8:07 a.m. UTC | #3 
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 <quic_prashk@quicinc.com>

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 <angelogioacchino.delregno@collabora.com>

> ---
> 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,
Prashanth K May 25, 2023, 8:26 a.m. UTC | #4 
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 <quic_prashk@quicinc.com>
>> ---
>> 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
Prashanth K May 25, 2023, 8:27 a.m. UTC | #5 
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 <quic_prashk@quicinc.com>
> 
> 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 
> <angelogioacchino.delregno@collabora.com>

Yea sure, thanks for the suggestion Agnelo

Regards,
Prashanth K
diff mbox series

Patch

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,