| Message ID | 20231006125445.122380-5-gmazyland@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb-storage,uas,scsi: Support OPAL commands on USB attached devices. | expand |
Okay, this one is a bit of a mess. Unavoidably so, I'm afraid. On Fri, Oct 06, 2023 at 02:54:43PM +0200, Milan Broz wrote: > The USB mass storage quirks flags are stored in driver_info, > 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 USB list > https://lore.kernel.org/linux-usb/f9e8acb5-32d5-4a30-859f-d4336a86b31a@gmail.com/ > > The initial solution to use 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 index and not actual value. > > This patch adds a host-compiled program that processes unusual_devs.h > (and unusual_uas.h) and generates files unusual-flags.c and unusual-flags-uas.c > (for pre-processed USB device table with 32 bit device info) and unusual-flags.h > with function to translate flags to 64-bits from device-info. > > The separate generated header file is needed as storage and UAS drivers headers > are tightly bound together and any ohter solution seems to be more pervasive. > > 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. > (Also, I did not find a reliable way a host-compiled program can detect > that the target platform has 32-bit unsigned long (usual macros do not > work here!). How about testing CONFIG_64BIT? Would that not do what you want? However, I agree that it's better to keep things simple by using the same code base for 32-bit and 64-bit kernels. > > Signed-off-by: Milan Broz <gmazyland@gmail.com> > --- > > drivers/usb/storage/Makefile | 25 ++++ > drivers/usb/storage/mkflags.c | 212 +++++++++++++++++++++++++++++ > drivers/usb/storage/uas-detect.h | 2 +- > drivers/usb/storage/uas.c | 17 +-- > drivers/usb/storage/usb.c | 7 +- > drivers/usb/storage/usual-tables.c | 23 +--- > 6 files changed, 248 insertions(+), 38 deletions(-) > create mode 100644 drivers/usb/storage/mkflags.c > > diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile > index 46635fa4a340..1eacdbb387cd 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 > + Suggestion: Add a comment here, explaining what the following code does and why it is necessary. > +$(obj)/usb.o: $(obj)/unusual-flags.h > +$(obj)/usual-tables.o: $(obj)/unusual-flags.c > +$(obj)/uas.o: $(obj)/unusual-flags.h $(obj)/unusual-flags-uas.c > +clean-files := unusual-flags.h unusual-flags.c unusual-flags-uas.c > +HOSTCFLAGS_mkflags.o := -I $(srctree)/include/ > +hostprogs += mkflags > + > +quiet_cmd_mkflag_flags = FLAGS $@ > + cmd_mkflag_flags = $(obj)/mkflags flags > $@ > + > +quiet_cmd_mkflag_storage = FLAGS $@ > + cmd_mkflag_storage = $(obj)/mkflags storage > $@ > + > +quiet_cmd_mkflag_uas = FLAGS $@ > + cmd_mkflag_uas = $(obj)/mkflags uas > $@ > + > +$(obj)/unusual-flags.h: $(obj)/mkflags FORCE > + $(call if_changed,mkflag_flags) > + > +$(obj)/unusual-flags.c: $(obj)/mkflags FORCE > + $(call if_changed,mkflag_storage) > + > +$(obj)/unusual-flags-uas.c: $(obj)/mkflags FORCE > + $(call if_changed,mkflag_uas) My make-fu isn't so hot. Do you really need to use this indirect way of specifying whether and how to rebuild the new files? > diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c > new file mode 100644 > index 000000000000..11aa6579e7e1 > --- /dev/null > +++ b/drivers/usb/storage/mkflags.c > @@ -0,0 +1,212 @@ > +// SPDX-License-Identifier: GPL-2.0+ There needs to be a big comment here, explaining why this program is needed and exactly what it does. > + > +#include <stdio.h> > +#include <string.h> > + > +/* > + * Cannot use userspace <inttypes.h> as code below > + * processes internal kernel headers > + */ > +#include <linux/types.h> > + > +/* > + * Silence warning for definitions in headers we do not use > + */ > +struct usb_device_id {}; > +struct usb_interface; > + > +#include <linux/usb_usual.h> > + > +struct svals { > + unsigned int type; > + > + /*interface */ > + uint8_t bDeviceSubClass; > + uint8_t bDeviceProtocol; > + > + /* device */ > + uint16_t idVendor; > + uint16_t idProduct; > + uint16_t bcdDevice_lo; > + uint16_t bcdDevice_hi; > + > + uint64_t flags; > + unsigned int set; > + unsigned int idx; > +}; > + > +enum { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS }; > +enum { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE }; > +#define FLAGS_END (uint64_t)-1 > + > +#define IS_ENABLED(x) 0 > + > +static struct svals vals[] = { > +#define USUAL_DEV(useProto, useTrans) \ > +{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 } > + > +#define COMPLIANT_DEV UNUSUAL_DEV > +#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ > + vendorName, productName, useProtocol, useTransport, \ > + initFunction, flags) \ > +{ TYPE_DEVICE_STORAGE, 0, 0, id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, flags, FLAGS_NOT_SET, 0 } > + > +#include "unusual_devs.h" > + > +/* UAS */ If you're going to put this comment line here, why isn't there a similar comment line "/* Mass-Storage */" at the start of the structure initializer? > +#undef UNUSUAL_DEV > +#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ > + vendorName, productName, useProtocol, useTransport, \ > + initFunction, flags) \ > +{ TYPE_DEVICE_UAS, 0, 0, id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, flags, FLAGS_NOT_SET, 0 } > + > +#include "unusual_uas.h" > + > +{ .flags = FLAGS_END } > +}; > +#undef UNUSUAL_DEV > +#undef USUAL_DEV > +#undef COMPLIANT_DEV > +#undef IS_ENABLED > + > +#define HI32 (uint32_t)0x80000000 > + > +static unsigned long get_device_info(uint64_t flags, unsigned int idx) > +{ > + if (flags < HI32) > + return (unsigned long)flags; > + > + /* Use index that will be processed in usb_stor_di2flags */ > + return HI32 + idx; > +} > + > +static void print_class(uint8_t bDeviceSubClass, uint8_t bDeviceProtocol) > +{ > + printf("\t{ .match_flags = USB_DEVICE_ID_MATCH_INT_INFO, "); > + printf(".bInterfaceClass = USB_CLASS_MASS_STORAGE, "); > + printf(".bInterfaceSubClass = 0x%x, .bInterfaceProtocol = 0x%x },\n", > + bDeviceSubClass, bDeviceProtocol); > +} > +static void print_type(unsigned int type) > +{ > + for (int i = 0; vals[i].flags != FLAGS_END; i++) { > + if (vals[i].type != type) > + continue; > + > + if (type == TYPE_DEVICE_STORAGE || type == TYPE_DEVICE_UAS) { > + printf("\t{ .match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION, "); > + printf(".idVendor = 0x%x, .idProduct =0x%x, " > + ".bcdDevice_lo = 0x%x, .bcdDevice_hi = 0x%x, .driver_info = 0x%lx },\n", > + vals[i].idVendor, vals[i].idProduct, > + vals[i].bcdDevice_lo, vals[i].bcdDevice_hi, > + get_device_info(vals[i].flags, vals[i].idx)); > + } else if (type == TYPE_CLASS) > + print_class(vals[i].bDeviceSubClass, vals[i].bDeviceProtocol); > + } > +} > + > +static void print_usb_flags(void) > +{ > + int i; > + > + printf("#include <linux/types.h>\n\n"); > + > + /* usb_stor_di2flags */ > + printf("static u64 usb_stor_di2flags(unsigned long idx)\n{\n"); > + printf("\tu64 flags = 0;\n\n"); > + printf("\tif (idx < 0x%x) \n\t\treturn idx;\n\n", HI32); > + printf("\tswitch(idx - 0x%x) {\n", HI32); > + for (i = 0; vals[i].flags != FLAGS_END; i++) { > + if (vals[i].set == FLAGS_SET) > + printf("\tcase %u: flags = 0x%llx; break;\n", vals[i].idx, vals[i].flags); > + } > + printf("\t}\n\n"); > + printf("\treturn flags;\n"); > + printf("}\n"); > +} I suspect the usb_stor_di2flags() function doesn't have to be created by this preprocessor. It ought to be possible to put a slightly altered version directly into uas-detect.h or some similar place (again, along with a comment explaining just what it does and why), and then generate here a simple array of 64-bit flags values which the function can index into rather than looking values up in a large "switch" statement. > +static void print_usb_storage(void) > +{ > + printf("#include <linux/usb.h>\n\n"); > + > + /* usb_storage_usb_ids */ > + printf("const struct usb_device_id usb_storage_usb_ids[] = {\n"); > + > + /* USB storage devices */ > + print_type(TYPE_DEVICE_STORAGE); > + > + /* UAS storage devices */ > + printf("#if IS_ENABLED(CONFIG_USB_UAS)\n"); > + print_type(TYPE_DEVICE_UAS); > + printf("#endif\n"); > + > + /* transport subclasses */ > + print_type(TYPE_CLASS); > + > + printf("\t{ }\t\t/* Terminating entry */\n};\n"); > + printf("MODULE_DEVICE_TABLE(usb, usb_storage_usb_ids);\n"); > +} > + > +static void print_usb_uas(void) > +{ > + printf("#include <linux/usb.h>\n\n"); > + > + /* uas_usb_ids */ > + printf("const struct usb_device_id uas_usb_ids[] = {\n"); > + > + /* UAS storage devices */ > + print_type(TYPE_DEVICE_UAS); > + > + /* transport subclasses */ > + print_class(USB_SC_SCSI, USB_PR_BULK); > + print_class(USB_SC_SCSI, USB_PR_UAS); > + > + printf("\t{ }\t\t/* Terminating entry */\n};\n"); > + printf("MODULE_DEVICE_TABLE(usb, uas_usb_ids);\n"); > +} > + > +int main(int argc, char *argv[]) > +{ > + int i, j, idx = 0, idx_old, skip = 0; > + > + if (argc != 2 || (strcmp(argv[1], "flags") && > + strcmp(argv[1], "storage") && strcmp(argv[1], "uas"))) { > + printf("Please specify type: storage, uas or flags.\n"); > + return 1; > + } > + > + for (i = 0; vals[i].flags != FLAGS_END; i++) { > + if (vals[i].type == TYPE_CLASS) > + continue; > + skip = 0; > + if (vals[i].flags >= HI32) { > + for (j = 0; j < i; j++) { > + if (vals[j].flags == vals[i].flags && > + vals[j].set == FLAGS_SET) { > + skip = 1; > + idx_old = vals[j].idx; > + break; > + } > + } This de-duplication may be a little premature. But I guess it doesn't hurt. > + if (skip) { > + vals[i].idx = idx_old; > + vals[i].set = FLAGS_DUPLICATE; > + } else { > + vals[i].idx = idx; > + vals[i].set = FLAGS_SET; > + idx++; > + } > + } > + } > + > + if (!strcmp(argv[1], "flags")) > + print_usb_flags(); > + else if (!strcmp(argv[1], "storage")) > + print_usb_storage(); > + else if (!strcmp(argv[1], "uas")) > + print_usb_uas(); > + else > + return 1; > + > + return 0; > +} The rest of the patch looks pretty straightforward. Alan Stern
On 10/6/23 20:44, Alan Stern wrote: > Okay, this one is a bit of a mess. Unavoidably so, I'm afraid. yes. What I need to know if it is acceptable approach (I spent quite a lot of time on it and still have no better idea... At least with a patch that is not too invasive). Here I compared generated tables with old pre-processor generated and it looks the same. (Also I keep it on kernel.org branch, so 0-day bot reports obvious mistakes.) ... >> This translation is unnecessary for a 64-bit system, but I keep it >> in place for simplicity. >> (Also, I did not find a reliable way a host-compiled program can detect >> that the target platform has 32-bit unsigned long (usual macros do not >> work here!). > > How about testing CONFIG_64BIT? Would that not do what you want? Yes, that was my last idea too, but I am not sure if it correct (and I have no longer access to more exotic platforms to check it). Also using kernel config defines in host-compiled code is tricky, but it should be possible. I will try to ask my former colleagues, though. > However, I agree that it's better to keep things simple by using the > same code base for 32-bit and 64-bit kernels. Yes, that was my plan for now. So you want to keep it as it is? We can add optimization for 64-bit with additional patch later, it should be pretty easy once I know how to detect that target platform really has 64-bit unsigned long so no translation is needed. Thanks, Milan > >> >> Signed-off-by: Milan Broz <gmazyland@gmail.com> >> --- >> >> drivers/usb/storage/Makefile | 25 ++++ >> drivers/usb/storage/mkflags.c | 212 +++++++++++++++++++++++++++++ >> drivers/usb/storage/uas-detect.h | 2 +- >> drivers/usb/storage/uas.c | 17 +-- >> drivers/usb/storage/usb.c | 7 +- >> drivers/usb/storage/usual-tables.c | 23 +--- >> 6 files changed, 248 insertions(+), 38 deletions(-) >> create mode 100644 drivers/usb/storage/mkflags.c >> >> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile >> index 46635fa4a340..1eacdbb387cd 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 >> + > > Suggestion: Add a comment here, explaining what the following code does > and why it is necessary. > >> +$(obj)/usb.o: $(obj)/unusual-flags.h >> +$(obj)/usual-tables.o: $(obj)/unusual-flags.c >> +$(obj)/uas.o: $(obj)/unusual-flags.h $(obj)/unusual-flags-uas.c >> +clean-files := unusual-flags.h unusual-flags.c unusual-flags-uas.c >> +HOSTCFLAGS_mkflags.o := -I $(srctree)/include/ >> +hostprogs += mkflags >> + >> +quiet_cmd_mkflag_flags = FLAGS $@ >> + cmd_mkflag_flags = $(obj)/mkflags flags > $@ >> + >> +quiet_cmd_mkflag_storage = FLAGS $@ >> + cmd_mkflag_storage = $(obj)/mkflags storage > $@ >> + >> +quiet_cmd_mkflag_uas = FLAGS $@ >> + cmd_mkflag_uas = $(obj)/mkflags uas > $@ >> + >> +$(obj)/unusual-flags.h: $(obj)/mkflags FORCE >> + $(call if_changed,mkflag_flags) >> + >> +$(obj)/unusual-flags.c: $(obj)/mkflags FORCE >> + $(call if_changed,mkflag_storage) >> + >> +$(obj)/unusual-flags-uas.c: $(obj)/mkflags FORCE >> + $(call if_changed,mkflag_uas) > > My make-fu isn't so hot. Do you really need to use this indirect way of > specifying whether and how to rebuild the new files? > >> diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c >> new file mode 100644 >> index 000000000000..11aa6579e7e1 >> --- /dev/null >> +++ b/drivers/usb/storage/mkflags.c >> @@ -0,0 +1,212 @@ >> +// SPDX-License-Identifier: GPL-2.0+ > > There needs to be a big comment here, explaining why this program is > needed and exactly what it does. > >> + >> +#include <stdio.h> >> +#include <string.h> >> + >> +/* >> + * Cannot use userspace <inttypes.h> as code below >> + * processes internal kernel headers >> + */ >> +#include <linux/types.h> >> + >> +/* >> + * Silence warning for definitions in headers we do not use >> + */ >> +struct usb_device_id {}; >> +struct usb_interface; >> + >> +#include <linux/usb_usual.h> >> + >> +struct svals { >> + unsigned int type; >> + >> + /*interface */ >> + uint8_t bDeviceSubClass; >> + uint8_t bDeviceProtocol; >> + >> + /* device */ >> + uint16_t idVendor; >> + uint16_t idProduct; >> + uint16_t bcdDevice_lo; >> + uint16_t bcdDevice_hi; >> + >> + uint64_t flags; >> + unsigned int set; >> + unsigned int idx; >> +}; >> + >> +enum { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS }; >> +enum { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE }; >> +#define FLAGS_END (uint64_t)-1 >> + >> +#define IS_ENABLED(x) 0 >> + >> +static struct svals vals[] = { >> +#define USUAL_DEV(useProto, useTrans) \ >> +{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 } >> + >> +#define COMPLIANT_DEV UNUSUAL_DEV >> +#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ >> + vendorName, productName, useProtocol, useTransport, \ >> + initFunction, flags) \ >> +{ TYPE_DEVICE_STORAGE, 0, 0, id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, flags, FLAGS_NOT_SET, 0 } >> + >> +#include "unusual_devs.h" >> + >> +/* UAS */ > > If you're going to put this comment line here, why isn't there a similar > comment line "/* Mass-Storage */" at the start of the structure > initializer? > >> +#undef UNUSUAL_DEV >> +#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ >> + vendorName, productName, useProtocol, useTransport, \ >> + initFunction, flags) \ >> +{ TYPE_DEVICE_UAS, 0, 0, id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, flags, FLAGS_NOT_SET, 0 } >> + >> +#include "unusual_uas.h" >> + >> +{ .flags = FLAGS_END } >> +}; >> +#undef UNUSUAL_DEV >> +#undef USUAL_DEV >> +#undef COMPLIANT_DEV >> +#undef IS_ENABLED >> + >> +#define HI32 (uint32_t)0x80000000 >> + >> +static unsigned long get_device_info(uint64_t flags, unsigned int idx) >> +{ >> + if (flags < HI32) >> + return (unsigned long)flags; >> + >> + /* Use index that will be processed in usb_stor_di2flags */ >> + return HI32 + idx; >> +} >> + >> +static void print_class(uint8_t bDeviceSubClass, uint8_t bDeviceProtocol) >> +{ >> + printf("\t{ .match_flags = USB_DEVICE_ID_MATCH_INT_INFO, "); >> + printf(".bInterfaceClass = USB_CLASS_MASS_STORAGE, "); >> + printf(".bInterfaceSubClass = 0x%x, .bInterfaceProtocol = 0x%x },\n", >> + bDeviceSubClass, bDeviceProtocol); >> +} >> +static void print_type(unsigned int type) >> +{ >> + for (int i = 0; vals[i].flags != FLAGS_END; i++) { >> + if (vals[i].type != type) >> + continue; >> + >> + if (type == TYPE_DEVICE_STORAGE || type == TYPE_DEVICE_UAS) { >> + printf("\t{ .match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION, "); >> + printf(".idVendor = 0x%x, .idProduct =0x%x, " >> + ".bcdDevice_lo = 0x%x, .bcdDevice_hi = 0x%x, .driver_info = 0x%lx },\n", >> + vals[i].idVendor, vals[i].idProduct, >> + vals[i].bcdDevice_lo, vals[i].bcdDevice_hi, >> + get_device_info(vals[i].flags, vals[i].idx)); >> + } else if (type == TYPE_CLASS) >> + print_class(vals[i].bDeviceSubClass, vals[i].bDeviceProtocol); >> + } >> +} >> + >> +static void print_usb_flags(void) >> +{ >> + int i; >> + >> + printf("#include <linux/types.h>\n\n"); >> + >> + /* usb_stor_di2flags */ >> + printf("static u64 usb_stor_di2flags(unsigned long idx)\n{\n"); >> + printf("\tu64 flags = 0;\n\n"); >> + printf("\tif (idx < 0x%x) \n\t\treturn idx;\n\n", HI32); >> + printf("\tswitch(idx - 0x%x) {\n", HI32); >> + for (i = 0; vals[i].flags != FLAGS_END; i++) { >> + if (vals[i].set == FLAGS_SET) >> + printf("\tcase %u: flags = 0x%llx; break;\n", vals[i].idx, vals[i].flags); >> + } >> + printf("\t}\n\n"); >> + printf("\treturn flags;\n"); >> + printf("}\n"); >> +} > > I suspect the usb_stor_di2flags() function doesn't have to be created by > this preprocessor. It ought to be possible to put a slightly altered > version directly into uas-detect.h or some similar place (again, along > with a comment explaining just what it does and why), and then generate > here a simple array of 64-bit flags values which the function can index > into rather than looking values up in a large "switch" statement. > >> +static void print_usb_storage(void) >> +{ >> + printf("#include <linux/usb.h>\n\n"); >> + >> + /* usb_storage_usb_ids */ >> + printf("const struct usb_device_id usb_storage_usb_ids[] = {\n"); >> + >> + /* USB storage devices */ >> + print_type(TYPE_DEVICE_STORAGE); >> + >> + /* UAS storage devices */ >> + printf("#if IS_ENABLED(CONFIG_USB_UAS)\n"); >> + print_type(TYPE_DEVICE_UAS); >> + printf("#endif\n"); >> + >> + /* transport subclasses */ >> + print_type(TYPE_CLASS); >> + >> + printf("\t{ }\t\t/* Terminating entry */\n};\n"); >> + printf("MODULE_DEVICE_TABLE(usb, usb_storage_usb_ids);\n"); >> +} >> + >> +static void print_usb_uas(void) >> +{ >> + printf("#include <linux/usb.h>\n\n"); >> + >> + /* uas_usb_ids */ >> + printf("const struct usb_device_id uas_usb_ids[] = {\n"); >> + >> + /* UAS storage devices */ >> + print_type(TYPE_DEVICE_UAS); >> + >> + /* transport subclasses */ >> + print_class(USB_SC_SCSI, USB_PR_BULK); >> + print_class(USB_SC_SCSI, USB_PR_UAS); >> + >> + printf("\t{ }\t\t/* Terminating entry */\n};\n"); >> + printf("MODULE_DEVICE_TABLE(usb, uas_usb_ids);\n"); >> +} >> + >> +int main(int argc, char *argv[]) >> +{ >> + int i, j, idx = 0, idx_old, skip = 0; >> + >> + if (argc != 2 || (strcmp(argv[1], "flags") && >> + strcmp(argv[1], "storage") && strcmp(argv[1], "uas"))) { >> + printf("Please specify type: storage, uas or flags.\n"); >> + return 1; >> + } >> + >> + for (i = 0; vals[i].flags != FLAGS_END; i++) { >> + if (vals[i].type == TYPE_CLASS) >> + continue; >> + skip = 0; >> + if (vals[i].flags >= HI32) { >> + for (j = 0; j < i; j++) { >> + if (vals[j].flags == vals[i].flags && >> + vals[j].set == FLAGS_SET) { >> + skip = 1; >> + idx_old = vals[j].idx; >> + break; >> + } >> + } > > This de-duplication may be a little premature. But I guess it doesn't > hurt. > >> + if (skip) { >> + vals[i].idx = idx_old; >> + vals[i].set = FLAGS_DUPLICATE; >> + } else { >> + vals[i].idx = idx; >> + vals[i].set = FLAGS_SET; >> + idx++; >> + } >> + } >> + } >> + >> + if (!strcmp(argv[1], "flags")) >> + print_usb_flags(); >> + else if (!strcmp(argv[1], "storage")) >> + print_usb_storage(); >> + else if (!strcmp(argv[1], "uas")) >> + print_usb_uas(); >> + else >> + return 1; >> + >> + return 0; >> +} > > The rest of the patch looks pretty straightforward. > > Alan Stern
On Sun, Oct 08, 2023 at 12:41:42PM +0200, Milan Broz wrote: > On 10/6/23 20:44, Alan Stern wrote: > > Okay, this one is a bit of a mess. Unavoidably so, I'm afraid. > > yes. What I need to know if it is acceptable approach (I spent quite > a lot of time on it and still have no better idea... At least with > a patch that is not too invasive). Yes, the basic idea is acceptable (subject to the comments in my earlier email). In fact, it's probably the best we can do, given the constraints we face. > Here I compared generated tables with old pre-processor generated > and it looks the same. (Also I keep it on kernel.org branch, so > 0-day bot reports obvious mistakes.) > > ... > > > > This translation is unnecessary for a 64-bit system, but I keep it > > > in place for simplicity. > > > (Also, I did not find a reliable way a host-compiled program can detect > > > that the target platform has 32-bit unsigned long (usual macros do not > > > work here!). > > > > How about testing CONFIG_64BIT? Would that not do what you want? > > Yes, that was my last idea too, but I am not sure if it correct (and I have > no longer access to more exotic platforms to check it). I'm reasonably sure that it's the right thing to check. > Also using kernel config defines in host-compiled code is tricky, but > it should be possible. Yeah; I'm not certain about how to do it. One possibility is simply to parse the .config file directly at runtime, if the Kbuild system doesn't provide the CONFIG_* macros when compiling a host program. > I will try to ask my former colleagues, though. > > > However, I agree that it's better to keep things simple by using the > > same code base for 32-bit and 64-bit kernels. > > Yes, that was my plan for now. So you want to keep it as it is? > > We can add optimization for 64-bit with additional patch later, it should be > pretty easy once I know how to detect that target platform really has > 64-bit unsigned long so no translation is needed. Yes, I agree that this is the approach we should take. Alan Stern
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile index 46635fa4a340..1eacdbb387cd 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 + +$(obj)/usb.o: $(obj)/unusual-flags.h +$(obj)/usual-tables.o: $(obj)/unusual-flags.c +$(obj)/uas.o: $(obj)/unusual-flags.h $(obj)/unusual-flags-uas.c +clean-files := unusual-flags.h unusual-flags.c unusual-flags-uas.c +HOSTCFLAGS_mkflags.o := -I $(srctree)/include/ +hostprogs += mkflags + +quiet_cmd_mkflag_flags = FLAGS $@ + cmd_mkflag_flags = $(obj)/mkflags flags > $@ + +quiet_cmd_mkflag_storage = FLAGS $@ + cmd_mkflag_storage = $(obj)/mkflags storage > $@ + +quiet_cmd_mkflag_uas = FLAGS $@ + cmd_mkflag_uas = $(obj)/mkflags uas > $@ + +$(obj)/unusual-flags.h: $(obj)/mkflags FORCE + $(call if_changed,mkflag_flags) + +$(obj)/unusual-flags.c: $(obj)/mkflags FORCE + $(call if_changed,mkflag_storage) + +$(obj)/unusual-flags-uas.c: $(obj)/mkflags FORCE + $(call if_changed,mkflag_uas) diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c new file mode 100644 index 000000000000..11aa6579e7e1 --- /dev/null +++ b/drivers/usb/storage/mkflags.c @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <stdio.h> +#include <string.h> + +/* + * Cannot use userspace <inttypes.h> as code below + * processes internal kernel headers + */ +#include <linux/types.h> + +/* + * Silence warning for definitions in headers we do not use + */ +struct usb_device_id {}; +struct usb_interface; + +#include <linux/usb_usual.h> + +struct svals { + unsigned int type; + + /*interface */ + uint8_t bDeviceSubClass; + uint8_t bDeviceProtocol; + + /* device */ + uint16_t idVendor; + uint16_t idProduct; + uint16_t bcdDevice_lo; + uint16_t bcdDevice_hi; + + uint64_t flags; + unsigned int set; + unsigned int idx; +}; + +enum { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS }; +enum { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE }; +#define FLAGS_END (uint64_t)-1 + +#define IS_ENABLED(x) 0 + +static struct svals vals[] = { +#define USUAL_DEV(useProto, useTrans) \ +{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 } + +#define COMPLIANT_DEV UNUSUAL_DEV +#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ + vendorName, productName, useProtocol, useTransport, \ + initFunction, flags) \ +{ TYPE_DEVICE_STORAGE, 0, 0, id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, flags, FLAGS_NOT_SET, 0 } + +#include "unusual_devs.h" + +/* UAS */ +#undef UNUSUAL_DEV +#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ + vendorName, productName, useProtocol, useTransport, \ + initFunction, flags) \ +{ TYPE_DEVICE_UAS, 0, 0, id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, flags, FLAGS_NOT_SET, 0 } + +#include "unusual_uas.h" + +{ .flags = FLAGS_END } +}; +#undef UNUSUAL_DEV +#undef USUAL_DEV +#undef COMPLIANT_DEV +#undef IS_ENABLED + +#define HI32 (uint32_t)0x80000000 + +static unsigned long get_device_info(uint64_t flags, unsigned int idx) +{ + if (flags < HI32) + return (unsigned long)flags; + + /* Use index that will be processed in usb_stor_di2flags */ + return HI32 + idx; +} + +static void print_class(uint8_t bDeviceSubClass, uint8_t bDeviceProtocol) +{ + printf("\t{ .match_flags = USB_DEVICE_ID_MATCH_INT_INFO, "); + printf(".bInterfaceClass = USB_CLASS_MASS_STORAGE, "); + printf(".bInterfaceSubClass = 0x%x, .bInterfaceProtocol = 0x%x },\n", + bDeviceSubClass, bDeviceProtocol); +} +static void print_type(unsigned int type) +{ + for (int i = 0; vals[i].flags != FLAGS_END; i++) { + if (vals[i].type != type) + continue; + + if (type == TYPE_DEVICE_STORAGE || type == TYPE_DEVICE_UAS) { + printf("\t{ .match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION, "); + printf(".idVendor = 0x%x, .idProduct =0x%x, " + ".bcdDevice_lo = 0x%x, .bcdDevice_hi = 0x%x, .driver_info = 0x%lx },\n", + vals[i].idVendor, vals[i].idProduct, + vals[i].bcdDevice_lo, vals[i].bcdDevice_hi, + get_device_info(vals[i].flags, vals[i].idx)); + } else if (type == TYPE_CLASS) + print_class(vals[i].bDeviceSubClass, vals[i].bDeviceProtocol); + } +} + +static void print_usb_flags(void) +{ + int i; + + printf("#include <linux/types.h>\n\n"); + + /* usb_stor_di2flags */ + printf("static u64 usb_stor_di2flags(unsigned long idx)\n{\n"); + printf("\tu64 flags = 0;\n\n"); + printf("\tif (idx < 0x%x) \n\t\treturn idx;\n\n", HI32); + printf("\tswitch(idx - 0x%x) {\n", HI32); + for (i = 0; vals[i].flags != FLAGS_END; i++) { + if (vals[i].set == FLAGS_SET) + printf("\tcase %u: flags = 0x%llx; break;\n", vals[i].idx, vals[i].flags); + } + printf("\t}\n\n"); + printf("\treturn flags;\n"); + printf("}\n"); +} + +static void print_usb_storage(void) +{ + printf("#include <linux/usb.h>\n\n"); + + /* usb_storage_usb_ids */ + printf("const struct usb_device_id usb_storage_usb_ids[] = {\n"); + + /* USB storage devices */ + print_type(TYPE_DEVICE_STORAGE); + + /* UAS storage devices */ + printf("#if IS_ENABLED(CONFIG_USB_UAS)\n"); + print_type(TYPE_DEVICE_UAS); + printf("#endif\n"); + + /* transport subclasses */ + print_type(TYPE_CLASS); + + printf("\t{ }\t\t/* Terminating entry */\n};\n"); + printf("MODULE_DEVICE_TABLE(usb, usb_storage_usb_ids);\n"); +} + +static void print_usb_uas(void) +{ + printf("#include <linux/usb.h>\n\n"); + + /* uas_usb_ids */ + printf("const struct usb_device_id uas_usb_ids[] = {\n"); + + /* UAS storage devices */ + print_type(TYPE_DEVICE_UAS); + + /* transport subclasses */ + print_class(USB_SC_SCSI, USB_PR_BULK); + print_class(USB_SC_SCSI, USB_PR_UAS); + + printf("\t{ }\t\t/* Terminating entry */\n};\n"); + printf("MODULE_DEVICE_TABLE(usb, uas_usb_ids);\n"); +} + +int main(int argc, char *argv[]) +{ + int i, j, idx = 0, idx_old, skip = 0; + + if (argc != 2 || (strcmp(argv[1], "flags") && + strcmp(argv[1], "storage") && strcmp(argv[1], "uas"))) { + printf("Please specify type: storage, uas or flags.\n"); + return 1; + } + + for (i = 0; vals[i].flags != FLAGS_END; i++) { + if (vals[i].type == TYPE_CLASS) + continue; + skip = 0; + if (vals[i].flags >= HI32) { + for (j = 0; j < i; j++) { + if (vals[j].flags == vals[i].flags && + vals[j].set == FLAGS_SET) { + skip = 1; + idx_old = vals[j].idx; + break; + } + } + if (skip) { + vals[i].idx = idx_old; + vals[i].set = FLAGS_DUPLICATE; + } else { + vals[i].idx = idx; + vals[i].set = FLAGS_SET; + idx++; + } + } + } + + if (!strcmp(argv[1], "flags")) + print_usb_flags(); + else if (!strcmp(argv[1], "storage")) + print_usb_storage(); + else if (!strcmp(argv[1], "uas")) + print_usb_uas(); + else + return 1; + + return 0; +} diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h index 4d3b49e5b87a..701621aaa9e7 100644 --- a/drivers/usb/storage/uas-detect.h +++ b/drivers/usb/storage/uas-detect.h @@ -59,7 +59,7 @@ static int uas_use_uas_driver(struct usb_interface *intf, struct usb_host_endpoint *eps[4] = { }; struct usb_device *udev = interface_to_usbdev(intf); struct usb_hcd *hcd = bus_to_hcd(udev->bus); - u64 flags = id->driver_info; + u64 flags = usb_stor_di2flags(id->driver_info); struct usb_host_interface *alt; int r; diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 696bb0b23599..f6e293daabf4 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -26,6 +26,7 @@ #include <scsi/scsi_host.h> #include <scsi/scsi_tcq.h> +#include "unusual-flags.h" #include "uas-detect.h" #include "scsiglue.h" @@ -909,21 +910,7 @@ static const struct scsi_host_template uas_host_template = { .cmd_size = sizeof(struct uas_cmd_info), }; -#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ - vendorName, productName, useProtocol, useTransport, \ - initFunction, flags) \ -{ USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax), \ - .driver_info = (flags) } - -static struct usb_device_id uas_usb_ids[] = { -# include "unusual_uas.h" - { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_BULK) }, - { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_UAS) }, - { } -}; -MODULE_DEVICE_TABLE(usb, uas_usb_ids); - -#undef UNUSUAL_DEV +#include "unusual-flags-uas.c" static int uas_switch_interface(struct usb_device *udev, struct usb_interface *intf) diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 72b48b94aa5f..f3a53c3eeb45 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -56,6 +56,8 @@ #include "sierra_ms.h" #include "option_ms.h" +#include "unusual-flags.h" + #if IS_ENABLED(CONFIG_USB_UAS) #include "uas-detect.h" #endif @@ -589,7 +591,10 @@ static int get_device_info(struct us_data *us, const struct usb_device_id *id, us->protocol = (unusual_dev->useTransport == USB_PR_DEVICE) ? idesc->bInterfaceProtocol : unusual_dev->useTransport; - us->fflags = id->driver_info; + if (fflags_use_index) + us->fflags = usb_stor_di2flags(id->driver_info); + else + us->fflags = id->driver_info; usb_stor_adjust_quirks(us->pusb_dev, &us->fflags); diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c index a26029e43dfd..c5871c1c3915 100644 --- a/drivers/usb/storage/usual-tables.c +++ b/drivers/usb/storage/usual-tables.c @@ -13,28 +13,9 @@ /* - * The table of devices + * The table of devices is pre-generated in unusual-flags.c */ -#define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \ - vendorName, productName, useProtocol, useTransport, \ - initFunction, flags) \ -{ USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax), \ - .driver_info = (kernel_ulong_t)(flags) } - -#define COMPLIANT_DEV UNUSUAL_DEV - -#define USUAL_DEV(useProto, useTrans) \ -{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans) } - -const struct usb_device_id usb_storage_usb_ids[] = { -# include "unusual_devs.h" - { } /* Terminating entry */ -}; -MODULE_DEVICE_TABLE(usb, usb_storage_usb_ids); - -#undef UNUSUAL_DEV -#undef COMPLIANT_DEV -#undef USUAL_DEV +#include "unusual-flags.c" /* * The table of devices to ignore
The USB mass storage quirks flags are stored in driver_info, 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 USB list https://lore.kernel.org/linux-usb/f9e8acb5-32d5-4a30-859f-d4336a86b31a@gmail.com/ The initial solution to use 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 index and not actual value. This patch adds a host-compiled program that processes unusual_devs.h (and unusual_uas.h) and generates files unusual-flags.c and unusual-flags-uas.c (for pre-processed USB device table with 32 bit device info) and unusual-flags.h with function to translate flags to 64-bits from device-info. The separate generated header file is needed as storage and UAS drivers headers are tightly bound together and any ohter solution seems to be more pervasive. 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. (Also, I did not find a reliable way a host-compiled program can detect that the target platform has 32-bit unsigned long (usual macros do not work here!). Signed-off-by: Milan Broz <gmazyland@gmail.com> --- drivers/usb/storage/Makefile | 25 ++++ drivers/usb/storage/mkflags.c | 212 +++++++++++++++++++++++++++++ drivers/usb/storage/uas-detect.h | 2 +- drivers/usb/storage/uas.c | 17 +-- drivers/usb/storage/usb.c | 7 +- drivers/usb/storage/usual-tables.c | 23 +--- 6 files changed, 248 insertions(+), 38 deletions(-) create mode 100644 drivers/usb/storage/mkflags.c