Message-ID: <20230907001322.w4hpfmkzu323nkh7@synopsys.com>
Date:   Thu, 7 Sep 2023 00:13:29 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Dingyan Li <18500469033@....com>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "mathias.nyman@...el.com" <mathias.nyman@...el.com>,
        "pawell@...ence.com" <pawell@...ence.com>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "brcm80211-dev-list.pdl@...adcom.com" 
        <brcm80211-dev-list.pdl@...adcom.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mina86@...a86.com" <mina86@...a86.com>,
        "clemens@...isch.de" <clemens@...isch.de>,
        "ruslan.bilovol@...il.com" <ruslan.bilovol@...il.com>,
        "bigeasy@...utronix.de" <bigeasy@...utronix.de>,
        "treding@...dia.com" <treding@...dia.com>,
        "oneukum@...e.com" <oneukum@...e.com>,
        "laurent.pinchart@...asonboard.com" 
        <laurent.pinchart@...asonboard.com>,
        "nic_swsd@...ltek.com" <nic_swsd@...ltek.com>,
        "marcel@...tmann.org" <marcel@...tmann.org>
Subject: Re: [PATCH v3] USB: Extend usb_device_speed with new value
 USB_SPEED_SUPER_PLUS_BY2

On Sun, Sep 03, 2023, Dingyan Li wrote:
> 
> At 2023-09-02 06:33:30, "Thinh Nguyen" <Thinh.Nguyen@...opsys.com> wrote:
> >Hi,
> >
> >Please Cc me for changes related to dwc3.
> >
> Sure, thanks for letting me know.
> 
> >On Sat, Sep 02, 2023, Dingyan Li wrote:
> >> Currently there are there major generations when speaking of
> >> USB_SPEED_SUPER_PLUS devices. However, they actually stands
> >> for different physical speeds. GEN_2x2 means 20Gbps, while
> >> the others mean 10Gbps. So in order to confirm 20Gbps, both
> >> speed and generation need ti be checked. This causes a trouble
> >> for ioctl USBDEVFS_GET_SPEED since it can only return speed
> >> value to userspace.
> >> 
> >> In order not to add a new ioctl to get ssp generation, extending
> >> usb_device_speed is a good option. The side effect is that
> >> USB_SPEED_SUPER_PLUS has been used in lots of places and
> >> it also takes some effort to go through all of them and check
> >> whether the new speed should be used too.
> >> 
> >> Besides, as suggested by Alen, ssp_rate is not a proper name,
> >> change it to ssp_gen. And change all functions/struct fileds
> >> ended with ssp_rate to ssp_gen. And there is also some code
> >> refactor to reduce duplicate definition of strings and increase
> >> the utilization of commonly defined utilities.
> >> 
> >> Signed-off-by: Dingyan Li <18500469033@....com>
> >> ---
> >
> >Can we spell out the whole thing instead of USB_SPEED_SUPER_PLUS_BY2
> >(ie. USB_SPEED_SUPER_PLUS_GEN_2x2 as you intended) instead of just the
> >lane count.
> >
> Here is a little bit more context at
> https://urldefense.com/v3/__https://lore.kernel.org/lkml/07c821ae-2391-474c-aec9-65f47d3fecf2@rowland.harvard.edu/__;!!A4F2R9G_pg!be_iVrJNFXagks2td2lrjlK6RvWnQ8P8wXkBJwQULZNbVDKcLDOBw_45IPNVe8Lm_i-CU3eq0D4hkPWUNTatmVJIVQ$ 
> What I'm trying to do is that in enum usb_device_speed, we only care about
> overall speed instead of stuff like lanes. For example, GEN_1x2 and
> GEN_2X1 are both 10Gbps, so we can use USB_SPEED_SUPER_PLUS
> to represent 10Gbps. GEN_2x2 represent 20Gbps, we can use
> USB_SPEED_SUPER_PLUS_BY2. There is no need to append generation
> and lane info when we only want to talk about overall speed. By the way,
> Apple also uses a similar way to declare speed enums and the new
> speed name is kind of borrowed from it.

The USB speed naming convention is already confusing as is. Now if you
introduce a new version of calling SuperSpeed Plus Gen 2x2 as
USB_SPEED_SUPER_PLUS_BY2, it's even more confusing.

Probably most class/function drivers don't care about the Gen or lane
count. However, the controller drivers use that to properly configure
the controller and report the speeds. If you really don't care about
the gen and the lane count, we may as well report everything as
SuperSpeed Plus. But that's not the case right? Your use case is that
you only care about 10Gbps or 20Gbps. Why limit to only your use case?

> 
> >There are SuperSpeed Plus generation _and_ lane count. That's why I
> >didn't name "usb_ssp_gen" that only reflects the generation and not the
> >
> Still, I think "usb_ssp_gen" is slightly better than "usb_ssp_rate". As for
> the lanes, I think it's OK to not mention it in the name since there are
> already comments to explain what this enum is for. Besides, the word
> "rate" is kind of like "speed", which should be covered by usb_device_speed
> instead of in the enum for generation and lanes.

Sure, but we could argue that it shares how the usb spec indicates Gen
XxY as where X is the "rate" of signaling on the wire.

But then again, I had trouble coming up with a better name. :/

> 
> >lane count. Also, I didn't extend usb_device_speed because gen XxY are
> >all a single speed: SuperSpeed Plus.
> >
> Again, I think it depends on what we really mean when speaking of speed.
> Clearly GEN_2x2 and GEN_2x1 have different overall speed. Let's not hide
> the fact under SuperSpeed Plus.

Well, the fact is that the current enum USB_SPEED_SUPER_PLUS can no
longer represent just Gen2x1. If we keep this enum USB_SPEED_SUPER_PLUS
in usb_device_speed, then this remains unclear, and you're hiding all
the Gen XxY under the same enum.

We attempted to extend and introduce usb_ssp_rate to help get around
this obscurity.

> 
> >If you're planning to do it this way, why not add the other speeds (such
> >as gen 1x2) to usb_device_speed enum too? Then we can drop the
> >
> Like I said above, if we only care about the overall speed, there is no need
> to tell GEN_1x2 from GEN_2x1 since both are 10Gbps. But like in dwc3 code,
> there are some different behaviors based on generation and lanes, so
> the enum still needs to be kept.

As I've mentioned, if you plan to do it this way, then introduce all the
Gen XxY to usb_device_speed. Otherwise, we now duplicating what the
usb_ssp_rate/gen enum was intended for.

> 
> >usb_ssp_rate enum. If we're going to check multiple enum for SuperSpeed
> >Plus, we probably need a usb_device_is_superspeed_plus() function.
> >
> >Now we need to audit all the greater/lesser speed checks that use < or >
> >to make sure that they are used how they were intended to.
> >
> After going through all places where USB_SPEED_SUPER_PLUS is used,
> for switch statements, I choose to add  an extra case. For if statement,
> change "==" to ">=". But I'm not completely sure they are all correct,
> which need to be further checked by maintainers.

The downside to introducing these enums to usb_device_speed is that we
need to audit all the drivers that checks for USB_SPEED_SUPER_PLUS.

> 
> >Since these changes are not simple and will touch on multiple places,
> >please split this patch out.
> >
> Okay, I can try to split the patch once we reach an agreement on
> how to handle SSP speed and generation. Besides, there is almost
> one file for each different module and it's not good to send one patch
> for each of them, right? There would be many in this way.
> 

Why is it not good? If you separate these changes, then
* You can keep the Acks for the patches that are already reviewed
* The maintainers don't have to review the large patch over and over
* We can keep track of the small changes
* If there's an issue, we don't have to revert the entire thing

Thanks,
Thinh

Powered by blists - more mailing lists