Message-ID: <58c0090e.160e.18a5a531364.Coremail.18500469033@163.com>
Date:   Sun, 3 Sep 2023 17:15:08 +0800 (CST)
From:   "Dingyan Li" <18500469033@....com>
To:     "Thinh Nguyen" <Thinh.Nguyen@...opsys.com>
Cc:     "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:Re: [PATCH v3] USB: Extend usb_device_speed with new value
 USB_SPEED_SUPER_PLUS_BY2


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://lore.kernel.org/lkml/07c821ae-2391-474c-aec9-65f47d3fecf2@rowland.harvard.edu/
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.

>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.

>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.

>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.

>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.

>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.

Regards,
Dingyan

Powered by blists - more mailing lists