On Thu, Oct 26, 2023 at 12:16:15PM +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@xxxxxxxxx/ > > 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@xxxxxxxxx> > --- Just a few minor comments. > diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile > index 46635fa4a340..9c09d83769e3 100644 > --- a/drivers/usb/storage/Makefile > +++ b/drivers/usb/storage/Makefile > @@ -45,3 +45,35 @@ 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 It would look better to put tabs after the ':'s in these two lines, so that the second field aligns with the lines below. > +clean-files := usb-ids.c usb-ids-uas.c > +HOSTCFLAGS_mkflags.o := -I $(srctree)/include/ > +ifdef CONFIG_64BIT > +HOSTCFLAGS_mkflags.o += -D CONFIG_64BIT=1 > +else > +HOSTCFLAGS_mkflags.o += -D CONFIG_64BIT=0 > +endif > +hostprogs += mkflags > + > +quiet_cmd_mkflag_storage = FLAGS $@ > +cmd_mkflag_storage = $(obj)/mkflags storage > $@ > + > +quiet_cmd_mkflag_uas = FLAGS $@ > +cmd_mkflag_uas = $(obj)/mkflags uas > $@ > + > +# mkflags always need to include unusual_devs.h and unusual_uas.h > +$(obj)/usb-ids.c: $(obj)/mkflags $(obj)/unusual_devs.h $(obj)/unusual_uas.h > + $(call cmd,mkflag_storage) > + > +$(obj)/usb-ids-uas.c: $(obj)/mkflags $(obj)/unusual_devs.h $(obj)/unusual_uas.h > + $(call cmd,mkflag_uas) I don't think these dependencies are quite right. usb-ids.c and usb-ids-uas.c don't depend directly on unusual_devs.h or unusual_uas.h -- that is, the mkflags program doesn't read those header files when it runs. Rather, mkflags itself depends on those headers, and the compiler can figure this out by itself so the Makefile doesn't need to mention it. So instead you should say: $(obj)/usb-ids.c: $(obj)/mkflags $(call cmd,mkflag_storage) $(obj)/usb-ids-uas.c: $(obj)/mkflags $(call cmd,mkflag_uas) > diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h > new file mode 100644 > index 000000000000..d0359c572f33 > --- /dev/null > +++ b/drivers/usb/storage/usb-ids.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > + > +#ifndef _USB_STOR_IDS_H_ > +#define _USB_STOR_IDS_H_ > + > +#include <linux/types.h> > +#include <linux/bug.h> > + > +/* Conversion of 32-bit quirks flags for 32-bit platforms */ > +extern const unsigned long usb_stor_drv_info_u64_table_size; > +extern const unsigned long usb_uas_drv_info_u64_table_size; > +extern const u64 usb_stor_drv_info_u64_table[]; > +extern const u64 usb_uas_drv_info_u64_table[]; > + > +static u64 usb_stor_drv_info_to_flags(const u64 *drv_info_u64_table, > + unsigned long table_size, unsigned long idx) > +{ > +#if IS_ENABLED(CONFIG_64BIT) > + return idx; > +#else > + u64 flags = 0; > + > + if (idx < (1UL << 31)) > + return idx; > + > + idx -= (1UL << 31); > + > + if (idx < table_size) > + flags = drv_info_u64_table[idx]; > + else > + pr_warn_once("usb_stor_drv_info_u64_table not updated"); > + > + return flags; > +#endif > +} In order to avoid conditional macros within a function definition, this can be rewritten as: #if IS_ENABLED(CONFIG_64BIT) /* 64-bit systems don't need to use the drv_info_64_table */ static u64 usb_stor_drv_info_to_flags(const u64 *drv_info_u64_table, unsigned long table_size, unsigned long idx) { return idx; } #else /* 32-bit systems need to look up flags if bits 31 or beyond are used */ static u64 usb_stor_drv_info_to_flags(const u64 *drv_info_u64_table, unsigned long table_size, unsigned long idx) { ... } #endif Everything else looks okay. Alan Stern
