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

[Milan Broz <gmazyland@gmail.com>]

On 10/30/23 18:40, Alan Stern wrote:
> On Sat, Oct 28, 2023 at 07:41:45PM +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 driver_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 flags.
>>
>> For 64-bit platforms, where unsigned long is 64-bit, we do not need to
>> convert quirk flags to 32-bit index; the translation function there uses
>> flags directly.
>>
>> Signed-off-by: Milan Broz <gmazyland@gmail.com>
>> ---
> 
>> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
>> index 46635fa4a340..b8c5daeb8ff3 100644
>> --- a/drivers/usb/storage/Makefile
>> +++ b/drivers/usb/storage/Makefile
>> @@ -45,3 +45,34 @@ 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 values, as USB driver_info
>> +# (where the value is stored) can be only 32-bit.
>> +# The most significant bit means it is index to 64-bit pre-computed table
>> +# generated by mkflags host-compiled program.
>> +# Currently used only by mass-storage and uas driver.
>> +
>> +$(obj)/usual-tables.o:	$(obj)/usb-ids.c
>> +$(obj)/uas.o:		$(obj)/usb-ids-uas.c
> 
> Quick test: After those two lines were commented out from the Makefile,
> the compiler still knew to rebuild unusual-tables.o and uas.o when
> usb-ids.c and usb-ids-uas.c were changed.  So these lines aren't needed.

ok, thx, this is perhaps relict of some previous try (I tried different
approaches) - will remove it.


> Apart from this, I tried running the patch through checkpatch.pl, and it
> flagged a bunch of issues.  Some of them were false positives, but
> others really should be changed to match the kernel's style:
> 
> 	The SPDX license line in .c files is supposed to be a
> 	C++-style comment, i.e., use // instead of /* ... */.

Perhaps just copied from header file, it is different format there.
(And my bad, I forget to run checkpatch after many last changes...)

> 
> 	We aren't supposed to add new typedefs.  Instead of writing:
> 
> 		typedef enum {...} dev_type;
> 
> 	write:
> 
> 		enum dev_type {...};
> 
> 	and the same for dev_flags_set.
> 
> 	checkpatch would like the FLAGS_END macro value to be enclosed
> 	in parentheses, since it's a complex expression.  Same for the
> 	HI32 macro.  These don't really matter, but you may as well do
> 	it just to get rid of the warnings.
> 
> 	Quoted strings (line 117 in mkflags.c) aren't supposed to be
> 	broken across lines.  It should just be one very long line.
> 
> 	Contrariwise, some other lines are longer than checkpatch likes
> 	to see (its maximum is 100 columns).  They should be wrapped.
> 
> These issues ought to be fixed.  But it's all stylistic stuff; I don't
> see any actual errors or things to improve in the patch.

ok, I'll send next version once I have time to do it.

Thanks,
Milan