[v7] 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, intel-xhci-usb-role-switch & musb/jz4740 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>
---
v7: Added default case in musb/jz4740.c & intel-xhci-usb-role-switch.c to
    avoid build warnings.
v6: Moved USB_ROLE_UNKNOWN towards the end of enum usb_role.
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 +++
 drivers/usb/musb/jz4740.c                      | 2 ++
 drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
 include/linux/usb/role.h                       | 1 +
 5 files changed, 10 insertions(+)
  

On 31-05-23 08:11 pm, Prashanth K wrote:
>   
> diff --git a/drivers/usb/musb/jz4740.c b/drivers/usb/musb/jz4740.c
> index 5aabdd7..6d880c4 100644
> --- a/drivers/usb/musb/jz4740.c
> +++ b/drivers/usb/musb/jz4740.c
> @@ -95,6 +95,8 @@ static int jz4740_musb_role_switch_set(struct usb_role_switch *sw,
>   	case USB_ROLE_HOST:
>   		atomic_notifier_call_chain(&phy->notifier, USB_EVENT_ID, phy);
>   		break;
> +	default:
> +		break;
>   	}
>   
>   	return 0;
> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> index 5c96e92..4d6a3dd 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -97,6 +97,8 @@ static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
>   		val |= SW_VBUS_VALID;
>   		drd_config = DRD_CONFIG_STATIC_DEVICE;
>   		break;
> +	default:
> +		break;
>   	}
>   	val |= SW_IDPIN_EN;
>   	if (data->enable_sw_switch) {
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index b5deafd..65e790a 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -11,6 +11,7 @@ enum usb_role {
>   	USB_ROLE_NONE,
>   	USB_ROLE_HOST,
>   	USB_ROLE_DEVICE,
> +	USB_ROLE_UNKNOWN,
>   };
>   
>   typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,

Hi Greg, I have fixed the drivers that doesn't have default case while 
using usb_role enum. Added the same on intel-xhci-usb-role-switch.c & 
musb/jz4740.c files. I was able to compile successfully. Please check 
once if this fixed the build issue.

Thanks in advance,
Prashanth K
  

On Wed, May 31, 2023 at 08:17:23PM +0530, Prashanth K wrote:
> 
> 
> On 31-05-23 08:11 pm, Prashanth K wrote:
> > diff --git a/drivers/usb/musb/jz4740.c b/drivers/usb/musb/jz4740.c
> > index 5aabdd7..6d880c4 100644
> > --- a/drivers/usb/musb/jz4740.c
> > +++ b/drivers/usb/musb/jz4740.c
> > @@ -95,6 +95,8 @@ static int jz4740_musb_role_switch_set(struct usb_role_switch *sw,
> >   	case USB_ROLE_HOST:
> >   		atomic_notifier_call_chain(&phy->notifier, USB_EVENT_ID, phy);
> >   		break;
> > +	default:
> > +		break;
> >   	}
> >   	return 0;
> > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > index 5c96e92..4d6a3dd 100644
> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > @@ -97,6 +97,8 @@ static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
> >   		val |= SW_VBUS_VALID;
> >   		drd_config = DRD_CONFIG_STATIC_DEVICE;
> >   		break;
> > +	default:
> > +		break;
> >   	}
> >   	val |= SW_IDPIN_EN;
> >   	if (data->enable_sw_switch) {
> > diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> > index b5deafd..65e790a 100644
> > --- a/include/linux/usb/role.h
> > +++ b/include/linux/usb/role.h
> > @@ -11,6 +11,7 @@ enum usb_role {
> >   	USB_ROLE_NONE,
> >   	USB_ROLE_HOST,
> >   	USB_ROLE_DEVICE,
> > +	USB_ROLE_UNKNOWN,
> >   };
> >   typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,
> 
> Hi Greg, I have fixed the drivers that doesn't have default case while using
> usb_role enum. Added the same on intel-xhci-usb-role-switch.c &
> musb/jz4740.c files. I was able to compile successfully. Please check once
> if this fixed the build issue.

Looks good, thanks!

greg k-h
  

Hi,

On Wed, May 31, 2023 at 08:11:14PM +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.

So why can't you fix this by just always setting the role
unconditionally to USB_ROLE_NONE in your probe function before the
initial detection?

> While at it, also handle default case for the usb_role switch
> in cdns3, intel-xhci-usb-role-switch & musb/jz4740 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>
> ---
> v7: Added default case in musb/jz4740.c & intel-xhci-usb-role-switch.c to
>     avoid build warnings.
> v6: Moved USB_ROLE_UNKNOWN towards the end of enum usb_role.
> 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 +++
>  drivers/usb/musb/jz4740.c                      | 2 ++
>  drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
>  include/linux/usb/role.h                       | 1 +
>  5 files changed, 10 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;
>  	}
>  
>  	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;
> +
>  	/* Perform initial detection */
>  	usb_conn_queue_dwork(info, 0);
>  
> diff --git a/drivers/usb/musb/jz4740.c b/drivers/usb/musb/jz4740.c
> index 5aabdd7..6d880c4 100644
> --- a/drivers/usb/musb/jz4740.c
> +++ b/drivers/usb/musb/jz4740.c
> @@ -95,6 +95,8 @@ static int jz4740_musb_role_switch_set(struct usb_role_switch *sw,
>  	case USB_ROLE_HOST:
>  		atomic_notifier_call_chain(&phy->notifier, USB_EVENT_ID, phy);
>  		break;
> +	default:
> +		break;
>  	}
>  
>  	return 0;
> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> index 5c96e92..4d6a3dd 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -97,6 +97,8 @@ static int intel_xhci_usb_set_role(struct usb_role_switch *sw,
>  		val |= SW_VBUS_VALID;
>  		drd_config = DRD_CONFIG_STATIC_DEVICE;
>  		break;
> +	default:
> +		break;
>  	}
>  	val |= SW_IDPIN_EN;
>  	if (data->enable_sw_switch) {
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index b5deafd..65e790a 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -11,6 +11,7 @@ enum usb_role {
>  	USB_ROLE_NONE,
>  	USB_ROLE_HOST,
>  	USB_ROLE_DEVICE,
> +	USB_ROLE_UNKNOWN,
>  };
>  
>  typedef int (*usb_role_switch_set_t)(struct usb_role_switch *sw,
> -- 
> 2.7.4
  

On 13-06-23 04:40 pm, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, May 31, 2023 at 08:11:14PM +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.
> 
> So why can't you fix this by just always setting the role
> unconditionally to USB_ROLE_NONE in your probe function before the
> initial detection?
> 
Hi Heikki, thats exactly what we are doing here.

+	/* 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);

Thanks,
Prashanth K
  

On Wed, Jun 14, 2023 at 09:55:10AM +0530, Prashanth K wrote:
> 
> 
> On 13-06-23 04:40 pm, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Wed, May 31, 2023 at 08:11:14PM +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.
> > 
> > So why can't you fix this by just always setting the role
> > unconditionally to USB_ROLE_NONE in your probe function before the
> > initial detection?
> > 
> Hi Heikki, thats exactly what we are doing here.
> 
> +	/* Set last role to unknown before performing the initial detection */
> +	info->last_role = USB_ROLE_UNKNOWN;

No, I'm asking why can't you just call set_role(USB_ROLE_NONE)
(together with any other steps that you need to take in order to fix
you issue) directly from your probe function?

That USB_ROLE_UNKNOWN as a global is not acceptable - there is no
difference between USB_ROLE_UNKNOWN and USB_ROLE_NONE from the role
switch PoW. So if you want to use something like that, you have to
confine it to your driver.

But I honestly don't think you need it at all. You should be able to
refactor your driver in order to solve the issue described in the
commit message without any need for it.

Note! I just realised that you are not modifying
drivers/usb/roles/class.c, so this patch is actually broken.

thanks,
  

On Wed, May 31, 2023 at 08:11:14PM +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, intel-xhci-usb-role-switch & musb/jz4740 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>
> ---
> v7: Added default case in musb/jz4740.c & intel-xhci-usb-role-switch.c to
>     avoid build warnings.
> v6: Moved USB_ROLE_UNKNOWN towards the end of enum usb_role.
> 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 +++
>  drivers/usb/musb/jz4740.c                      | 2 ++
>  drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
>  include/linux/usb/role.h                       | 1 +
>  5 files changed, 10 insertions(+)

Just to be clear to everybody, that USB_ROLE_UNKNOWN is not handled in
drivers/usb/roles/class.c, so this patch is broken.

But the whole approach is wrong. That USB_ROLE_UNKNOWN is clearly a
flag where the other values in enum usb_role are actual switch states.
So it does not belong there.

In general, adding globals states like that just in order to work
around issues in single drivers is never a good idea IMO.

thanks,
  

On Wed, Jun 14, 2023 at 12:14:08PM +0300, Heikki Krogerus wrote:
> On Wed, May 31, 2023 at 08:11:14PM +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, intel-xhci-usb-role-switch & musb/jz4740 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>
> > ---
> > v7: Added default case in musb/jz4740.c & intel-xhci-usb-role-switch.c to
> >     avoid build warnings.
> > v6: Moved USB_ROLE_UNKNOWN towards the end of enum usb_role.
> > 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 +++
> >  drivers/usb/musb/jz4740.c                      | 2 ++
> >  drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
> >  include/linux/usb/role.h                       | 1 +
> >  5 files changed, 10 insertions(+)
> 
> Just to be clear to everybody, that USB_ROLE_UNKNOWN is not handled in
> drivers/usb/roles/class.c, so this patch is broken.
> 
> But the whole approach is wrong. That USB_ROLE_UNKNOWN is clearly a
> flag where the other values in enum usb_role are actual switch states.
> So it does not belong there.
> 
> In general, adding globals states like that just in order to work
> around issues in single drivers is never a good idea IMO.

Ok, let me go revert this from my tree, thanks for the review.

greg k-h
  

On 15-06-23 03:00 pm, Greg Kroah-Hartman wrote:
> On Wed, Jun 14, 2023 at 12:14:08PM +0300, Heikki Krogerus wrote:
>> On Wed, May 31, 2023 at 08:11:14PM +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, intel-xhci-usb-role-switch & musb/jz4740 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>
>>> ---
>>> v7: Added default case in musb/jz4740.c & intel-xhci-usb-role-switch.c to
>>>      avoid build warnings.
>>> v6: Moved USB_ROLE_UNKNOWN towards the end of enum usb_role.
>>> 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 +++
>>>   drivers/usb/musb/jz4740.c                      | 2 ++
>>>   drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
>>>   include/linux/usb/role.h                       | 1 +
>>>   5 files changed, 10 insertions(+)
>>
>> Just to be clear to everybody, that USB_ROLE_UNKNOWN is not handled in
>> drivers/usb/roles/class.c, so this patch is broken.
>>
>> But the whole approach is wrong. That USB_ROLE_UNKNOWN is clearly a
>> flag where the other values in enum usb_role are actual switch states.
>> So it does not belong there.
>>
>> In general, adding globals states like that just in order to work
>> around issues in single drivers is never a good idea IMO.
> 
> Ok, let me go revert this from my tree, thanks for the review.
> 
> greg k-h

In that case, can I resubmit v1 of this patch again, where I have used a 
macro in usb-conn-gpio driver ? something like this.

@@ -27,6 +27,8 @@
  #define USB_CONN_IRQF	\
  	(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)

+#define USB_ROLE_UNKNOWN (USB_ROLE_NONE -1)
+
  struct usb_conn_info {
  	struct device *dev;
  	struct usb_role_switch *role_sw;
@@ -257,6 +259,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);

Thanks,
Prashanth K
  

On Thu, Jun 15, 2023 at 07:52:32PM +0530, Prashanth K wrote:
> 
> 
> On 15-06-23 03:00 pm, Greg Kroah-Hartman wrote:
> > On Wed, Jun 14, 2023 at 12:14:08PM +0300, Heikki Krogerus wrote:
> > > On Wed, May 31, 2023 at 08:11:14PM +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, intel-xhci-usb-role-switch & musb/jz4740 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>
> > > > ---
> > > > v7: Added default case in musb/jz4740.c & intel-xhci-usb-role-switch.c to
> > > >      avoid build warnings.
> > > > v6: Moved USB_ROLE_UNKNOWN towards the end of enum usb_role.
> > > > 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 +++
> > > >   drivers/usb/musb/jz4740.c                      | 2 ++
> > > >   drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
> > > >   include/linux/usb/role.h                       | 1 +
> > > >   5 files changed, 10 insertions(+)
> > > 
> > > Just to be clear to everybody, that USB_ROLE_UNKNOWN is not handled in
> > > drivers/usb/roles/class.c, so this patch is broken.
> > > 
> > > But the whole approach is wrong. That USB_ROLE_UNKNOWN is clearly a
> > > flag where the other values in enum usb_role are actual switch states.
> > > So it does not belong there.
> > > 
> > > In general, adding globals states like that just in order to work
> > > around issues in single drivers is never a good idea IMO.
> > 
> > Ok, let me go revert this from my tree, thanks for the review.
> > 
> > greg k-h
> 
> In that case, can I resubmit v1 of this patch again, where I have used a
> macro in usb-conn-gpio driver ? something like this.
> 
> @@ -27,6 +27,8 @@
>  #define USB_CONN_IRQF	\
>  	(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
> 
> +#define USB_ROLE_UNKNOWN (USB_ROLE_NONE -1)

Are you referencing an existing enum here and assuming it is a specific
value?

For obvious reasons, that can never work :)

thanks,

greg k-h
  

On 15-06-23 08:06 pm, Greg Kroah-Hartman wrote:
> On Thu, Jun 15, 2023 at 07:52:32PM +0530, Prashanth K wrote:
>>
>> In that case, can I resubmit v1 of this patch again, where I have used a
>> macro in usb-conn-gpio driver ? something like this.
>>
>> @@ -27,6 +27,8 @@
>>   #define USB_CONN_IRQF	\
>>   	(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
>>
>> +#define USB_ROLE_UNKNOWN (USB_ROLE_NONE -1)
> 
> Are you referencing an existing enum here and assuming it is a specific
> value?

I' not assuming UBS_ROLE_NONE to be a specific value, but I want an 
integer (for macro) which is not equal to USB_ROLE_NONE/DEVICE/HOST, 
that's why I'm using (USB_ROLE_NONE - 1), assuming enumerators NONE, 
DEVICE & HOST will be having adjacent integer values. Wouldn't that help?

Thanks,
Prashanth K
  

On Thu, Jun 15, 2023 at 08:28:13PM +0530, Prashanth K wrote:
> 
> 
> On 15-06-23 08:06 pm, Greg Kroah-Hartman wrote:
> > On Thu, Jun 15, 2023 at 07:52:32PM +0530, Prashanth K wrote:
> > > 
> > > In that case, can I resubmit v1 of this patch again, where I have used a
> > > macro in usb-conn-gpio driver ? something like this.
> > > 
> > > @@ -27,6 +27,8 @@
> > >   #define USB_CONN_IRQF	\
> > >   	(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
> > > 
> > > +#define USB_ROLE_UNKNOWN (USB_ROLE_NONE -1)
> > 
> > Are you referencing an existing enum here and assuming it is a specific
> > value?
> 
> I' not assuming UBS_ROLE_NONE to be a specific value, but I want an integer
> (for macro) which is not equal to USB_ROLE_NONE/DEVICE/HOST, that's why I'm
> using (USB_ROLE_NONE - 1), assuming enumerators NONE, DEVICE & HOST will be
> having adjacent integer values. Wouldn't that help?

You can't do "math" on an enumerated type and expect the result to be
anything constant over time.

And yes, you can hope that enumerated types are sequential, and the spec
says so, but please never rely on that as what happens if someone adds a
new one in the list without you ever noticing it.

Pleasae treat enumerated types as an opaque thing that you never know
the real value of, it's a symbol only.

thanks,

greg k-h
  

On 15-06-23 08:35 pm, Greg Kroah-Hartman wrote:
> On Thu, Jun 15, 2023 at 08:28:13PM +0530, Prashanth K wrote:
>>
>>
>> On 15-06-23 08:06 pm, Greg Kroah-Hartman wrote:
>>> On Thu, Jun 15, 2023 at 07:52:32PM +0530, Prashanth K wrote:
>>>>
>>>> In that case, can I resubmit v1 of this patch again, where I have used a
>>>> macro in usb-conn-gpio driver ? something like this.
>>>>
>>>> @@ -27,6 +27,8 @@
>>>>    #define USB_CONN_IRQF	\
>>>>    	(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
>>>>
>>>> +#define USB_ROLE_UNKNOWN (USB_ROLE_NONE -1)
>>>
>>> Are you referencing an existing enum here and assuming it is a specific
>>> value?
>>
>> I' not assuming UBS_ROLE_NONE to be a specific value, but I want an integer
>> (for macro) which is not equal to USB_ROLE_NONE/DEVICE/HOST, that's why I'm
>> using (USB_ROLE_NONE - 1), assuming enumerators NONE, DEVICE & HOST will be
>> having adjacent integer values. Wouldn't that help?
> 
> You can't do "math" on an enumerated type and expect the result to be
> anything constant over time.
> 
> And yes, you can hope that enumerated types are sequential, and the spec
> says so, but please never rely on that as what happens if someone adds a
> new one in the list without you ever noticing it.
> 
> Pleasae treat enumerated types as an opaque thing that you never know
> the real value of, it's a symbol only.
> 
> thanks,
> 
> greg k-h

Then we can go ahead a different approach using a flag. But that would 
require us to add a new member to usb_conn_info struct. What do you suggest?

@@ -42,6 +42,7 @@ struct usb_conn_info {

         struct power_supply_desc desc;
         struct power_supply *charger;
+       bool initial_det;
  };

  /*
@@ -86,11 +87,13 @@ static void usb_conn_detect_cable(struct work_struct 
*work)
         dev_dbg(info->dev, "role %s -> %s, gpios: id %d, vbus %d\n",
                 usb_role_string(info->last_role), 
usb_role_string(role), id, vbus);

-       if (info->last_role == role) {
+       if (!info->initial_det && (info->last_role == role)) {
                 dev_warn(info->dev, "repeated role: %s\n", 
usb_role_string(role));
                 return;
         }

+       info->initial_det = false;
+
         if (info->last_role == USB_ROLE_HOST && info->vbus)
                 regulator_disable(info->vbus);

@@ -258,6 +261,7 @@ static int usb_conn_probe(struct platform_device *pdev)
         device_set_wakeup_capable(&pdev->dev, true);

         /* Perform initial detection */
+       info->initial_det = true;
         usb_conn_queue_dwork(info, 0);

Regards,
Prashanth K