Re: usb-storage: how to extend quirks flags to 64bit?

[Alan Stern]

On Wed, Aug 30, 2023 at 10:39:07PM +0200, Milan Broz wrote:
> On 8/27/23 20:55, Alan Stern wrote:
> 
> ...
> 
> > > > > Someone will need a new quirks flag in the future anyway... :)
> > > > 
> > > > I can think of only one way to accomplish this on 32-bit systems: Change
> > > > the driver_info field from a bit array to an index into a static table
> > > > of 64-bit flags values.  Each unusual_devs structure would have its own
> > > > entry in this table.  As far as I can tell, the other unusual_*.h tables
> > > > could retain their current driver_info interpretations, since no new
> > > > quirk bits are likely to be relevant to them.
> > > > 
> > > > Making this change would be an awkward nuisance, but it should be
> > > > doable.
> > > 
> > > Hm, yes, thanks for the idea,that is a possible solution.
> > > It will need to modify all unusual macros, though. Just I am not sure I want
> > > to spent time patching all the drivers as I have not way how to test it.
> > 
> > I don't think it will be necessary to change all those macros, just the
> > ones in usual_tables.c.  And to create the new table containing the
> > actual flag values, of course.
> > 
> > There will also have to be a new argument to usb_stor_probe1()
> > specifying whether the id->driver_info field is standard (i.e., it
> > contains the flags directly) or is one of the new indirect index values.
> > 
> > And you'll have to figure out a comparable change to the dynamic device
> > ID table mechanism.
> > 
> > (If you want to be really fancy about it, you could design things in
> > such a way that the indirect flags approach is used only on 32-bit
> > systems.  64-bit systems can put the new flag bits directly into the
> > driver_info field.  However, it's probably best not to worry about this
> > initially.)
> 
> Hi Alan,
> 
> So, I really tried this approach, spent more time on in than I expected, but
> produced working code... that I am really not proud of :-]
> (Thus avoiding to send it here, for now.)
> 
> I pushed it to my dm-cryptsetup branch here
> https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/log/?h=dm-cryptsetup

I haven't had a chance to look at your changes yet, and I might not get 
around to it for a while.

> The last patch is the reason why I need it, just for reference.
> More comments in the patch headers.
> 
> Could you please check it if it is *really* what we want?
> If so, I'll rebase it for usb next tree and send as a patchset.
> 
> But the macro magic is crazy... and I really did not find the better way.

Another possibility is to create a simple pre-processor program that 
would read the unusual_devs files and massage the information into the 
form the driver will want.  Such a program could do things that the C 
preprocessor isn't capable of.

For example, with this approach you could keep the original flag bits in 
positions 0 - 30, and reserve bit 31 as an indicator that there are 
additional flags stored in an "overflow" table entry.  This extra table 
could be relatively short, since it would need to contain only a small 
number of entries.  I can't think of any way to get the C preprocessor 
to do this, but it would be easy to have a separate program do it.

> Anyway, it also uncovered some problems
>  - some macros need to be changed a little bit, there is even old one unused

It doesn't hurt to have someone with fresh eyes taking a look at this.  :-)

>  - duplicity of entries in UAS and mass-storage are strange (and complicates
>    the approach).
>    I guess the sorting is intentionally that mass-storage is included
>    before UAS, so the mass-storage quirk is found as the first (for non-UAS).
>    (While UAS drive includes only own header.)

It's a little tricky because the two drivers need to be aware, to some 
extent, of which devices either prefer to use UAS or can't work with UAS.

> - the patch significantly increases size of module for 32bit
>   (64bit system use the direct flag store approach)

The "overflow table" approach would help a lot with this.

> - I stored a pointer to the flags array, not the index. Perhaps it should be
>   index only (trivial change, though).

Alan Stern