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: <a31ee75e-6a8e-fe7b-aadc-f118d9ab60f9@redhat.com>
Date:   Thu, 3 Aug 2023 17:39:56 +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
To:     Alan Stern <stern@rowland.harvard.edu>,
        Dingyan Li <18500469033@163.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Xiaofan Chen <xiaofanc@gmail.com>,
        Oliver Neukum <oneukum@suse.com>,
        Tormod Volden <lists.tormod@gmail.com>,
        sebastian.reichel@collabora.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <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>
 <2a82ba01-38dd-fad9-98b9-ac8591107921@redhat.com>
 <151a5748.3e99.189ba07b110.Coremail.18500469033@163.com>
 <51926ee6-ee81-4543-a1f7-338e65a26670@rowland.harvard.edu>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <51926ee6-ee81-4543-a1f7-338e65a26670@rowland.harvard.edu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 8/3/23 17:10, Alan Stern wrote:
> On Thu, Aug 03, 2023 at 02:13:33PM +0800, Dingyan Li wrote:
>>
>> At 2023-07-26 22:39:32, "Hans de Goede" <hdegoede@redhat.com> wrote:
> 
>>> 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
>>>
>>
>> Hi Greg and Alan,
>>
>> Could you please share your opinions about Hans' justification?
> 
> Instead of adding a new ioctl or modifying an existing one, how about 
> increasing the set of constants in enum usb_device_speed?  Then the 
> existing ioctls could return the newly defined values when appropriate, 
> with no other changes necessary.

Right, I was thinking along the same lines but I was not entirely
sure this would work because I looked at the wrong bits of
include/uapi/linux/usb/ch9.h while writing my first email on this.

Looking again I see we already have a straight forward
enum usb_device_speed for this which can easily be extended.

> (This doesn't mean just moving the definition of usb_ssp_rate from one 
> header file to the other.  The usb_device_speed enumeration should be 
> extended with the new members.  Perhaps omitting USB_SSP_GEN_UNKNOWN 
> since we already have USB_SPEED_SUPER_PLUS, or setting the first equal 
> to the second.)
> 
> I don't think there was ever a requirement in the API that the set of 
> values in usb_device_speed could never increase (and in fact it has 
> increased in the past).  Such a requirement wouldn't make any sense, 
> given how the USB-IF keeps defining newer and faster USB bus 
> implementations.
> 
> Hans, would that play well with libusb?

It should, this is how libusb uses the USBDEVFS_GET_SPEED ioctl:

static enum libusb_speed usbfs_get_speed(struct libusb_context *ctx, int fd)
{
	int r;

	r = ioctl(fd, IOCTL_USBFS_GET_SPEED, NULL);
	switch (r) {
	case USBFS_SPEED_UNKNOWN:	return LIBUSB_SPEED_UNKNOWN;
	case USBFS_SPEED_LOW:		return LIBUSB_SPEED_LOW;
	case USBFS_SPEED_FULL:		return LIBUSB_SPEED_FULL;
	case USBFS_SPEED_HIGH:		return LIBUSB_SPEED_HIGH;
	case USBFS_SPEED_WIRELESS:	return LIBUSB_SPEED_HIGH;
	case USBFS_SPEED_SUPER:		return LIBUSB_SPEED_SUPER;
	case USBFS_SPEED_SUPER_PLUS:	return LIBUSB_SPEED_SUPER_PLUS;
	default:
		usbi_warn(ctx, "Error getting device speed: %d", r);
	}

	return LIBUSB_SPEED_UNKNOWN;
}

I think that GEN_2x1 should probably be mapped to
USBFS_SPEED_SUPER_PLUS so as to not break this most common case
and to keep apps reporting either Super Speed Plus or 10Gbps
(more common) for this.

GEN_1x2 + GEN_2x2 can then be mapped to new values, which will
cause libusb to log a warning + return LIBUSB_SPEED_UNKNOWN
until libusb is updated which seems harmless enough.

Regards,

Hans