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

[Milan Broz <gmazyland@gmail.com>]

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