Re: [PATCH v2 3/4] USB: Add a function to obtain USB version independent maximum bpi value

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

Thank you for the review.

On Fri, Jul 11, 2025 at 03:44:21PM +0200, Hans de Goede wrote:
> Hi Sarari,
> 
> On 11-Jul-25 10:34 AM, Sakari Ailus wrote:
> > From: "Rai, Amardeep" <amardeep.rai@intel.com>
> > 
> > Add usb_endpoint_max_isoc_bpi() to obtain maximum bytes per interval for
> > isochronous endpoints in a USB version independent way.
> 
> Nice, thank you for adding a generic helper for this.
> 
> > Signed-off-by: Rai, Amardeep <amardeep.rai@intel.com>
> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Co-developed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/linux/usb.h | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 68166718ab30..bd70bd5ca82d 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -2038,6 +2038,28 @@ static inline int usb_translate_errors(int error_code)
> >  	}
> >  }
> >  
> > +static inline u32 usb_endpoint_max_isoc_bpi(struct usb_device *dev,
> > +					    const struct usb_host_endpoint *ep)
> > +{
> > +	if (usb_endpoint_type(&ep->desc) != USB_ENDPOINT_XFER_ISOC)
> > +		return 0;
> > +
> > +	switch (dev->speed) {
> > +	case USB_SPEED_SUPER_PLUS:
> > +		if (USB_SS_SSP_ISOC_COMP(ep->ss_ep_comp.bmAttributes))
> > +			return le32_to_cpu(ep->ssp_isoc_ep_comp.dwBytesPerInterval);
> > +		fallthrough;
> > +	case USB_SPEED_SUPER:
> > +		return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
> > +	case USB_SPEED_HIGH:
> > +		if (!usb_endpoint_maxp(&ep->desc) && le16_to_cpu(dev->descriptor.bcdUSB) == 0x220)
> > +			return le32_to_cpu(ep->eusb2_isoc_ep_comp.dwBytesPerInterval);
> 
> Shouldn't there be a check here that ep->eusb2_isoc_ep_comp is filled?
> 
> Like how the USB_SPEED_SUPER_PLU code above checks
> USB_SS_SSP_ISOC_COMP(ep->ss_ep_comp.bmAttributes)?
> 
> I know you check the bcdUSB, but in my experience that field sometimes
> contains made up numbers, so I was wondering if there is an extra check
> we can do here ?

In the case of eUSB2, there's no such flag as for the SuperSpeedPlus
Isochronous Endpoint Companion. The eUSB2 Isochronous Endpoint Companion
Descriptor is simply expected to be present on eUSB2 (bcdUSB check)
isochronous IN endpoints that support more than 3KB per microframe.

Also what the USB_SS_SSP_ISOC_COMP() macro returns actually dependens on
the device telling there's such a descriptor but it's still different from
the device actually providing one. But what would you do if the device
indicates it provides no SSP_ISOC_COMP descriptor but still does provide
one?

How about adding a flag (or maybe a bit field?) to tell which endpoint
descriptors have been actually filled in struct usb_host_endpoint? I might
do that as a separate patch on top...

I wonder what Mathias thinks, too.

-- 
Kind regards,

Sakari Ailus