linux-kernel - Re: [PATCH v7] 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: <ZIl8KeaePdKHCnVY@kuha.fi.intel.com>
Date:   Wed, 14 Jun 2023 11:36:57 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Prashanth K <quic_prashk@...cinc.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] usb: common: usb-conn-gpio: Set last role to unknown
 before initial detection

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,

-- 
heikki