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

  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
initialised 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.

While at it, also handle default case for the usb_role switch
in cdns3 to avoid build warnings.

Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
v5: Update commit text to mention the changes made in cdns3 driver.
v4: Added Reviewed-by tag.
v3: Added a default case in drivers/usb/cdns3/core.c as pointed out by
    the test robot.
v2: Added USB_ROLE_UNKNWON to enum usb_role.

 drivers/usb/cdns3/core.c           | 2 ++
 drivers/usb/common/usb-conn-gpio.c | 3 +++
 include/linux/usb/role.h           | 1 +
 3 files changed, 6 insertions(+)
  

On Thu, May 25, 2023 at 02:23:45PM +0530, Prashanth K wrote:
> 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
> initialised 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.
> 
> While at it, also handle default case for the usb_role switch
> in cdns3 to avoid build warnings.
> 
> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
> v5: Update commit text to mention the changes made in cdns3 driver.
> v4: Added Reviewed-by tag.
> v3: Added a default case in drivers/usb/cdns3/core.c as pointed out by
>     the test robot.
> v2: Added USB_ROLE_UNKNWON to enum usb_role.
> 
>  drivers/usb/cdns3/core.c           | 2 ++
>  drivers/usb/common/usb-conn-gpio.c | 3 +++
>  include/linux/usb/role.h           | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index dbcdf3b..69d2921 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -252,6 +252,8 @@ static enum usb_role cdns_hw_role_state_machine(struct cdns *cdns)
>  		if (!vbus)
>  			role = USB_ROLE_NONE;
>  		break;
> +	default:
> +		break;

No error if this happens?

>  	}
>  
>  	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
> 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;

Shouldn't last_role have already been set to 0?  If so, why not just
have this enum value be 0?


> +
>  	/* 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,

Why is this explicitly set to a value?  What is magic about -1?  Why not
0x42?  Or something else?  Or as I mention above, 0?

thanks,

greg k-h
  

On 25-05-23 10:04 pm, Greg Kroah-Hartman wrote:
> On Thu, May 25, 2023 at 02:23:45PM +0530, Prashanth K wrote:
>> 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
>> initialised 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.
>>
>> While at it, also handle default case for the usb_role switch
>> in cdns3 to avoid build warnings.
>>
>> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>> v5: Update commit text to mention the changes made in cdns3 driver.
>> v4: Added Reviewed-by tag.
>> v3: Added a default case in drivers/usb/cdns3/core.c as pointed out by
>>      the test robot.
>> v2: Added USB_ROLE_UNKNWON to enum usb_role.
>>
>>   drivers/usb/cdns3/core.c           | 2 ++
>>   drivers/usb/common/usb-conn-gpio.c | 3 +++
>>   include/linux/usb/role.h           | 1 +
>>   3 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index dbcdf3b..69d2921 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -252,6 +252,8 @@ static enum usb_role cdns_hw_role_state_machine(struct cdns *cdns)
>>   		if (!vbus)
>>   			role = USB_ROLE_NONE;
>>   		break;
>> +	default:
>> +		break;
> 
> No error if this happens?
It wouldn't come to default case in as no one sets the role to 
USB_ROLE_UNKNOWN in cdns3 driver. Moreover it would work the same
without the default case also (we have added it just to address a 
warning pointed out be test-robot).
> 
>>   	}
>>   
>>   	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
>> 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;
> 
> Shouldn't last_role have already been set to 0?  If so, why not just
> have this enum value be 0?
Last role would be 0 during first detection, that's the problem here.
During initial detection, if the the new role is detected as 
USB_ROLE_NONE (0), then we wouldn't call the set_role(). But it should 
send the current role to gadget after the inital detection.

if (info->last_role == role) {
		dev_warn(info->dev, "repeated role: %s\n", usb_role_string(role));
		return;
	}
> 
> 
>> +
>>   	/* 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,
> 
> Why is this explicitly set to a value?  What is magic about -1?  Why not
> 0x42?  Or something else?  Or as I mention above, 0?
> I just chose -1 as the currently 0,1,2 are used for NONE, Device and 
Host roles. Didn't make the USB_ROLE_UNKNOWN = 0 because i didn't want 
to break the existing logic in other drivers. Do you have any 
suggestion? Please let me know.

Thanks,
Prashanth K
  

On Fri, May 26, 2023 at 10:15:43AM +0530, Prashanth K wrote:
> 
> 
> On 25-05-23 10:04 pm, Greg Kroah-Hartman wrote:
> > On Thu, May 25, 2023 at 02:23:45PM +0530, Prashanth K wrote:
> > > 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
> > > initialised 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.
> > > 
> > > While at it, also handle default case for the usb_role switch
> > > in cdns3 to avoid build warnings.
> > > 
> > > Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
> > > Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > ---
> > > v5: Update commit text to mention the changes made in cdns3 driver.
> > > v4: Added Reviewed-by tag.
> > > v3: Added a default case in drivers/usb/cdns3/core.c as pointed out by
> > >      the test robot.
> > > v2: Added USB_ROLE_UNKNWON to enum usb_role.
> > > 
> > >   drivers/usb/cdns3/core.c           | 2 ++
> > >   drivers/usb/common/usb-conn-gpio.c | 3 +++
> > >   include/linux/usb/role.h           | 1 +
> > >   3 files changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > index dbcdf3b..69d2921 100644
> > > --- a/drivers/usb/cdns3/core.c
> > > +++ b/drivers/usb/cdns3/core.c
> > > @@ -252,6 +252,8 @@ static enum usb_role cdns_hw_role_state_machine(struct cdns *cdns)
> > >   		if (!vbus)
> > >   			role = USB_ROLE_NONE;
> > >   		break;
> > > +	default:
> > > +		break;
> > 
> > No error if this happens?
> It wouldn't come to default case in as no one sets the role to
> USB_ROLE_UNKNOWN in cdns3 driver. Moreover it would work the same
> without the default case also (we have added it just to address a warning
> pointed out be test-robot).
> > 
> > >   	}
> > >   	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
> > > 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;
> > 
> > Shouldn't last_role have already been set to 0?  If so, why not just
> > have this enum value be 0?
> Last role would be 0 during first detection, that's the problem here.
> During initial detection, if the the new role is detected as USB_ROLE_NONE
> (0), then we wouldn't call the set_role(). But it should send the current
> role to gadget after the inital detection.

So you are hoping that the old enum type is still assigned to 0?  That's
brave, please make it explicit otherwise it's very hard to follow or
ensure that this really will happen.  And most of all, document it so
that that value remains 0 in the future, otherwise a list of enum types
without explicit values are seen as if the values do not matter.

thanks,

greg k-h
  

On 28-05-23 05:03 pm, Greg Kroah-Hartman wrote:
>>>> 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;
>>>
>>> Shouldn't last_role have already been set to 0?  If so, why not just
>>> have this enum value be 0?
>> Last role would be 0 during first detection, that's the problem here.
>> During initial detection, if the the new role is detected as USB_ROLE_NONE
>> (0), then we wouldn't call the set_role(). But it should send the current
>> role to gadget after the inital detection.
> 
> So you are hoping that the old enum type is still assigned to 0?  That's
> brave, please make it explicit otherwise it's very hard to follow or
> ensure that this really will happen.  And most of all, document it so
> that that value remains 0 in the future, otherwise a list of enum types
> without explicit values are seen as if the values do not matter.
> 
> thanks,
> 
> greg k-h

So I think it would be better to add USB_ROLE_UNKNOWN towards the end of 
enum usb_role, so that we can avoid explicit declaration. Is that fine?

  enum usb_role {
  	USB_ROLE_NONE,
  	USB_ROLE_HOST,
  	USB_ROLE_DEVICE,
+	USB_ROLE_UNKNOWN,
   }

Thanks,
Prashanth K
  

On Tue, May 30, 2023 at 12:00:15AM +0530, Prashanth K wrote:
> 
> 
> On 28-05-23 05:03 pm, Greg Kroah-Hartman wrote:
> > > > > 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;
> > > > 
> > > > Shouldn't last_role have already been set to 0?  If so, why not just
> > > > have this enum value be 0?
> > > Last role would be 0 during first detection, that's the problem here.
> > > During initial detection, if the the new role is detected as USB_ROLE_NONE
> > > (0), then we wouldn't call the set_role(). But it should send the current
> > > role to gadget after the inital detection.
> > 
> > So you are hoping that the old enum type is still assigned to 0?  That's
> > brave, please make it explicit otherwise it's very hard to follow or
> > ensure that this really will happen.  And most of all, document it so
> > that that value remains 0 in the future, otherwise a list of enum types
> > without explicit values are seen as if the values do not matter.
> > 
> > thanks,
> > 
> > greg k-h
> 
> So I think it would be better to add USB_ROLE_UNKNOWN towards the end of
> enum usb_role, so that we can avoid explicit declaration. Is that fine?
> 
>  enum usb_role {
>  	USB_ROLE_NONE,
>  	USB_ROLE_HOST,
>  	USB_ROLE_DEVICE,
> +	USB_ROLE_UNKNOWN,

Either is fine, be explicit, or not, just don't mix the two please.

thanks,

greg k-h
  

On 30-05-23 12:31 am, Greg Kroah-Hartman wrote:
> On Tue, May 30, 2023 at 12:00:15AM +0530, Prashanth K wrote:
>>
>>
>> On 28-05-23 05:03 pm, Greg Kroah-Hartman wrote:
>>>>>> 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;
>>>>>
>>>>> Shouldn't last_role have already been set to 0?  If so, why not just
>>>>> have this enum value be 0?
>>>> Last role would be 0 during first detection, that's the problem here.
>>>> During initial detection, if the the new role is detected as USB_ROLE_NONE
>>>> (0), then we wouldn't call the set_role(). But it should send the current
>>>> role to gadget after the inital detection.
>>>
>>> So you are hoping that the old enum type is still assigned to 0?  That's
>>> brave, please make it explicit otherwise it's very hard to follow or
>>> ensure that this really will happen.  And most of all, document it so
>>> that that value remains 0 in the future, otherwise a list of enum types
>>> without explicit values are seen as if the values do not matter.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> So I think it would be better to add USB_ROLE_UNKNOWN towards the end of
>> enum usb_role, so that we can avoid explicit declaration. Is that fine?
>>
>>   enum usb_role {
>>   	USB_ROLE_NONE,
>>   	USB_ROLE_HOST,
>>   	USB_ROLE_DEVICE,
>> +	USB_ROLE_UNKNOWN,
> 
> Either is fine, be explicit, or not, just don't mix the two please.
> 
> thanks,
> 
> greg k-h

Thanks for the suggestion, will update it in next patch.

Regards,
Prashanth K