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

[Alan Stern <stern@rowland.harvard.edu>]

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.

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 /* ... */.

	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.

Alan Stern