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: <2a82ba01-38dd-fad9-98b9-ac8591107921@redhat.com>
Date:   Wed, 26 Jul 2023 16:39:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] USB: add usbfs ioctl to get specific superspeedplus rates
Content-Language: en-US, nl
To:     Xiaofan Chen <xiaofanc@gmail.com>,
        Oliver Neukum <oneukum@suse.com>,
        Tormod Volden <lists.tormod@gmail.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Dingyan Li <18500469033@163.com>, stern@rowland.harvard.edu,
        sebastian.reichel@collabora.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230721084039.9728-1-18500469033@163.com>
 <2023072105-lethargic-saddling-ad97@gregkh>
 <130b453c.5c8f.1897872ce54.Coremail.18500469033@163.com>
 <2023072159-carol-underfeed-43eb@gregkh>
 <781b3f95-96e7-af83-e089-887ec7f2d255@suse.com>
 <2023072546-denture-half-5ceb@gregkh>
 <CAGjSPUCQ892adFdYm7zCuMpWujwzwoQtYimvp3xXrnnCCyN47w@mail.gmail.com>
 <da536c80-7398-dae0-a22c-16e521be697a@suse.com>
 <CAGjSPUDtn-YMqmNrmku+hkyXCKys4XRue4wdzQwr+yGxe2cJSQ@mail.gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <CAGjSPUDtn-YMqmNrmku+hkyXCKys4XRue4wdzQwr+yGxe2cJSQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi All,

On 7/26/23 05:20, Xiaofan Chen wrote:
> On Wed, Jul 26, 2023 at 5:38 PM Oliver Neukum <oneukum@suse.com> wrote:
>>
>> On 26.07.23 03:37, Xiaofan Chen wrote:
>>> On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> Hi,
>>
>>>> So unless there is some actual need from userspace tools like libusb (or
>>>> anything else?) that requires this new ioctl, let's not add it otherwise
>>>> we are signing ourselves up to support it for forever.
>>>
>>> Interestingly there is PR in libusb now, which uses sysfs for 20Gbps.
>>
>> True. Now would you write a patch for libusb?
>> This looks to be turning into a chicken and egg problem.
>>
>>> Maybe this new usbfs IOCTL is indeed good to have if we can not extend
>>
>> Looking at the code of libusb you can see that libusb has two modes
>> of operation. Either it finds sysfs, then it uses it, if not it
>> goes for the ioctl.
>>
>> Now, how well shall it work without sysfs? That is a design decision
>> and we should not be having this discussion again and again.
>>
>> BTW, that is not aimed at anybody personally, we are just trying to
>> avoid a basic decision and it will come back.
>>
>>> the existing IOCTL USBDEVFS_GET_SPEED (but why not?).
>>
>> It does not include the lane count.
>> And sort of fudging this into speed is a bad idea in the long
>> run because we are likely to have collisions in the future.
>>
>> We have a basic issue here. Do we require libusb to use sysfs or not?
> 
> Adding Hans de Goede and Tormod Volder (libusb admins) here in the discussions
> as I am more into the testing and support side of libusb and not a
> real developer.
> 
> libusb does work with or without sysfs and there are multiple commits related
> to sysfs vs usbfs.
> 
> An example commit from Hans in Sept 202 which is related to this discussion.
> https://github.com/libusb/libusb/commit/f6068e83c4f5e5fba16b23b6a87f1f6d7ab7200a
> ++++++++++++++++
> linux: Fix libusb_get_device_speed() not working on wrapped devices
> 
> We don't have a sysfs_dir for wrapped devices, so we cannot read the speed
> from sysfs.
> 
> The Linux kernel has supported a new ioctl to get the speed directly from
> the fd for a while now, use that when we don't have sysfs access.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1871818
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> +++++++++++++++++
> 
> To Hans and Tormod:
> Discussion thread for reference:
> https://lore.kernel.org/linux-usb/da536c80-7398-dae0-a22c-16e521be697a@suse.com/T/#t

Right, so the reason why IOCTL USBDEVFS_GET_SPEED was added is so that a confined qemu process which gets just a fd for a /dev/bus/usb/ device passed by a more privileged process can still get the speed despite it not having sysfs access. This is necessary for correct pass-through of USB devices.

Since USBDEVFS_GET_SPEED now no longer tells the full story I believe that the proposed USBDEVFS_GET_SSP_RATE ioctl makes sense.

The current patch however misses moving the enum usb_ssp_rate declaration from include/linux/usb/ch9.h to include/uapi/linux/usb/ch9.h so that needs to be fixed in a version 2. Assuming that with the above explanation of why this is necessary Greg and Alan are ok with adding the ioctl.

Regards,

Hans