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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2117b7c4-d164-de17-5a2d-ef3d51304983@collabora.com>
Date:   Thu, 25 May 2023 10:45:07 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Prashanth K <quic_prashk@...cinc.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Matthias Brugger <matthias.bgg@...il.com>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] usb: common: usb-conn-gpio: Set last role to unknown
 before initial detection

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@...cinc.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,