Re: [PATCH 4/7] usb-storage,uas: use host helper to generate driver info

[Milan Broz <gmazyland@gmail.com>]

On 10/16/23 20:49, Alan Stern wrote:
> On Mon, Oct 16, 2023 at 09:26:01AM +0200, Milan Broz wrote:
>> The USB mass storage quirks flags can be stored in driver_info in
>> a 32-bit integer (unsigned long on 32-bit platforms).
>> As this attribute cannot be enlarged, we need to use some form
>> of translation of 64-bit quirk bits.
>>
>> This problem was discussed on the USB list
>> https://lore.kernel.org/linux-usb/f9e8acb5-32d5-4a30-859f-d4336a86b31a@gmail.com/
>>
>> The initial solution to use a static array extensively increased the size
>> of the kernel module, so I decided to try the second suggested solution:
>> generate a table by host-compiled program and use bit 31 to indicate
>> that the value is an index, not the actual value.
>>
>> This patch adds a host-compiled program that processes unusual_devs.h
>> (and unusual_uas.h) and generates files usb-ids.c and usb-ids-uas.c
>> (for pre-processed USB device table with 32-bit device info).
>> These files also contain a generated translation table for device_info
>> to 64-bit values.
>>
>> The translation function is used only in usb-storage and uas modules; all
>> other USB storage modules store flags directly, using only 32-bit integers.
>>
>> This translation is unnecessary for a 64-bit system, but I keep it
>> in place for simplicity in this patch.
>>
>> Signed-off-by: Milan Broz <gmazyland@gmail.com>
>> ---
>>   drivers/usb/storage/Makefile       |  25 ++++
>>   drivers/usb/storage/mkflags.c      | 226 +++++++++++++++++++++++++++++
>>   drivers/usb/storage/uas-detect.h   |   4 +-
>>   drivers/usb/storage/uas.c          |  20 +--
>>   drivers/usb/storage/usb-ids.h      |  33 +++++
>>   drivers/usb/storage/usb.c          |  10 +-
>>   drivers/usb/storage/usual-tables.c |  23 +--
>>   7 files changed, 301 insertions(+), 40 deletions(-)
>>   create mode 100644 drivers/usb/storage/mkflags.c
>>   create mode 100644 drivers/usb/storage/usb-ids.h
>>
>> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
>> index 46635fa4a340..612678f108d0 100644
>> --- a/drivers/usb/storage/Makefile
>> +++ b/drivers/usb/storage/Makefile
>> @@ -45,3 +45,28 @@ ums-realtek-y		:= realtek_cr.o
>>   ums-sddr09-y		:= sddr09.o
>>   ums-sddr55-y		:= sddr55.o
>>   ums-usbat-y		:= shuttle_usbat.o
>> +
>> +# The mkflags host-compiled generator produces usb-ids.c (usb-storage)
>> +# and usb-ids-uas.c (uas) with USB device tables.
>> +# These tables include pre-computed 32-bit flags as USB driver device_info
> 
> s/flags as/flags, as/
> 
> Otherwise this seems to say that the 32-bit flags are converted to USB
> driver device_info values -- an incorrect parsing that makes no sense
> and will confuse readers.  (It confused me at first.)
> 
> Also, don't you really mean "driver_info" rather than "driver
> device_info"?  That's the name of the field in struct usb_device_id.

Yes, not sure why I mixed these. Fixed in v3 patch (and now only one
patch is needed as 2 previous are merged in usb-next).

I hope I fixed all other comments too, thanks!

Milan