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

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/ (local)

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.

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?

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