[RFC] ftdi_sio driver: use altsetting or cur_altsetting?

[RFC] ftdi_sio driver: use altsetting or cur_altsetting?

[Mychaela Falconia]

Hello esteemed linux-usb folks,

While I am trying to get a little quirk for my custom hardware device
added to the ftdi_sio driver, I noticed an inconsistency between use
of interface->altsetting vs. interface->cur_altsetting in the existing
driver code.  Specifically:

* ftdi_determine_type() function uses this construct to get the number
  of the interface it is operating on:

  inter = serial->interface->altsetting->desc.bInterfaceNumber;

* ftdi_set_max_packet_size() uses interface->cur_altsetting to get to
  desc.bNumEndpoints and endpoint[i].desc

* The JTAG cleanup patch which Johan just applied uses
  intf->cur_altsetting->desc.bInterfaceNumber

So which is the right one to use, intf->altsetting or intf->cur_altsetting?
The comments in include/linux/usb.h say that the altsetting member
points to an array of struct usb_host_interface stored in no particular
order, so using that pointer in ftdi_determine_type() seems wrong -
but then I am a total novice in this area, hence I am hoping that
someone more knowledgeable could confirm.

The most recent version of my DUART28C quirk adding patch uses
serial->interface->altsetting, copied from ftdi_determine_type() -
should I change my proposed patch to use cur_altsetting instead?
Should I also make a patch to ftdi_determine_type() changing it to use
cur_altsetting as well?

TIA,
Mychaela
she/her/hers

Re: [RFC] ftdi_sio driver: use altsetting or cur_altsetting?

[Johan Hovold]

On Thu, Oct 01, 2020 at 11:20:08AM -0800, Mychaela Falconia wrote:
> Hello esteemed linux-usb folks,
> 
> While I am trying to get a little quirk for my custom hardware device
> added to the ftdi_sio driver, I noticed an inconsistency between use
> of interface->altsetting vs. interface->cur_altsetting in the existing
> driver code.  Specifically:
> 
> * ftdi_determine_type() function uses this construct to get the number
>   of the interface it is operating on:
> 
>   inter = serial->interface->altsetting->desc.bInterfaceNumber;
> 
> * ftdi_set_max_packet_size() uses interface->cur_altsetting to get to
>   desc.bNumEndpoints and endpoint[i].desc
> 
> * The JTAG cleanup patch which Johan just applied uses
>   intf->cur_altsetting->desc.bInterfaceNumber

Yeah, I noticed that too.

> So which is the right one to use, intf->altsetting or intf->cur_altsetting?
> The comments in include/linux/usb.h say that the altsetting member
> points to an array of struct usb_host_interface stored in no particular
> order, so using that pointer in ftdi_determine_type() seems wrong -
> but then I am a total novice in this area, hence I am hoping that
> someone more knowledgeable could confirm.

As long as you only access bNumInterfaces, which by definition is
identical for all altsettings, it's not wrong per se. But in general,
drivers should use cur_altsetting to not have to worry about such
subtleties and as a safe guard in case further fields are ever needed.
 
> The most recent version of my DUART28C quirk adding patch uses
> serial->interface->altsetting, copied from ftdi_determine_type() -
> should I change my proposed patch to use cur_altsetting instead?

Yes, please use that for new code.

> Should I also make a patch to ftdi_determine_type() changing it to use
> cur_altsetting as well?

Sure; it's not needed for correctness, but let's do it for consistency.

Johan

Re: [RFC] ftdi_sio driver: use altsetting or cur_altsetting?

[Mychaela Falconia]

Hi Johan,

> As long as you only access bNumInterfaces, which by definition is
> identical for all altsettings, it's not wrong per se.

But the code in ftdi_determine_type() that uses the altsetting pointer
is not looking at bNumInterfaces, it is looking at bInterfaceNumber
instead:

  inter = serial->interface->altsetting->desc.bInterfaceNumber;

> Yes, please use that for new code.

OK, I will make a new version of my DUART28 patch with cur_altsetting
instead of altsetting.

> > Should I also make a patch to ftdi_determine_type() changing it to use
> > cur_altsetting as well?
>
> Sure; it's not needed for correctness, but let's do it for consistency.

OK, I will include that patch as well in the next version of my series.

M~

Re: [RFC] ftdi_sio driver: use altsetting or cur_altsetting?

[Johan Hovold]

On Fri, Oct 02, 2020 at 12:37:58AM -0800, Mychaela Falconia wrote:
> Hi Johan,
> 
> > As long as you only access bNumInterfaces, which by definition is
> > identical for all altsettings, it's not wrong per se.
> 
> But the code in ftdi_determine_type() that uses the altsetting pointer
> is not looking at bNumInterfaces, it is looking at bInterfaceNumber
> instead:
> 
>   inter = serial->interface->altsetting->desc.bInterfaceNumber;

Sorry, I meant bInterfaceNumber above.

Johan