Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2117b7c4-d164-de17-5a2d-ef3d51304983@collabora.com>
Date:   Thu, 25 May 2023 10:45:07 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v3] usb: common: usb-conn-gpio: Set last role to unknown
 before initial detection
Content-Language: en-US
To:     Prashanth K <quic_prashk@quicinc.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Matthias Brugger <matthias.bgg@gmail.com>
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1685004036-17273-1-git-send-email-quic_prashk@quicinc.com>
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <1685004036-17273-1-git-send-email-quic_prashk@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Il 25/05/23 10:40, 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
> 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.
> 
> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>

I'm sorry to make a call for v4, but you have to mention that you're touching
the cdns3 driver in the commit description, if you want to keep the entire
change set in one commit... otherwise you'll have to split it in two, one adding
the new entry to the enum and fixing cdns3; the other setting the last role to
unknown in usb-conn-gpio.c.

I can suggest text for keeping that in one commit, but the choice is yours;

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

> ---
> 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;
>   	}
>   
>   	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/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,