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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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