* [PATCH 1/7] usb-storage: remove UNUSUAL_VENDOR_INTF macro
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
@ 2023-10-16 7:25 ` Milan Broz
2023-10-16 7:25 ` [PATCH 2/7] usb-storage,uas: make internal quirks flags 64bit Milan Broz
` (6 subsequent siblings)
7 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:25 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
This patch removes macro that was used only
by commit that was reverted in commit ab4b71644a26
("USB: storage: fix Huawei mode switching regression")
Fixes: ab4b71644a26 ("USB: storage: fix Huawei mode switching regression")
Signed-off-by: Milan Broz <gmazyland@gmail.com>
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
---
drivers/usb/storage/usb.c | 12 ------------
drivers/usb/storage/usual-tables.c | 15 ---------------
2 files changed, 27 deletions(-)
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 7b36a3334fb3..bb1fbeddc5aa 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -110,17 +110,6 @@ MODULE_PARM_DESC(quirks, "supplemental list of device IDs and their quirks");
.useTransport = use_transport, \
}
-#define UNUSUAL_VENDOR_INTF(idVendor, cl, sc, pr, \
- vendor_name, product_name, use_protocol, use_transport, \
- init_function, Flags) \
-{ \
- .vendorName = vendor_name, \
- .productName = product_name, \
- .useProtocol = use_protocol, \
- .useTransport = use_transport, \
- .initFunction = init_function, \
-}
-
static const struct us_unusual_dev us_unusual_dev_list[] = {
# include "unusual_devs.h"
{ } /* Terminating entry */
@@ -132,7 +121,6 @@ static const struct us_unusual_dev for_dynamic_ids =
#undef UNUSUAL_DEV
#undef COMPLIANT_DEV
#undef USUAL_DEV
-#undef UNUSUAL_VENDOR_INTF
#ifdef CONFIG_LOCKDEP
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index 529512827d8f..b3c3ea04c11c 100644
--- a/drivers/usb/storage/usual-tables.c
+++ b/drivers/usb/storage/usual-tables.c
@@ -26,20 +26,6 @@
#define USUAL_DEV(useProto, useTrans) \
{ USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans) }
-/* Define the device is matched with Vendor ID and interface descriptors */
-#define UNUSUAL_VENDOR_INTF(id_vendor, cl, sc, pr, \
- vendorName, productName, useProtocol, useTransport, \
- initFunction, flags) \
-{ \
- .match_flags = USB_DEVICE_ID_MATCH_INT_INFO \
- | USB_DEVICE_ID_MATCH_VENDOR, \
- .idVendor = (id_vendor), \
- .bInterfaceClass = (cl), \
- .bInterfaceSubClass = (sc), \
- .bInterfaceProtocol = (pr), \
- .driver_info = (flags) \
-}
-
const struct usb_device_id usb_storage_usb_ids[] = {
# include "unusual_devs.h"
{ } /* Terminating entry */
@@ -49,7 +35,6 @@ MODULE_DEVICE_TABLE(usb, usb_storage_usb_ids);
#undef UNUSUAL_DEV
#undef COMPLIANT_DEV
#undef USUAL_DEV
-#undef UNUSUAL_VENDOR_INTF
/*
* The table of devices to ignore
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 2/7] usb-storage,uas: make internal quirks flags 64bit
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
2023-10-16 7:25 ` [PATCH 1/7] usb-storage: remove UNUSUAL_VENDOR_INTF macro Milan Broz
@ 2023-10-16 7:25 ` Milan Broz
2023-10-21 10:19 ` Greg KH
2023-10-16 7:26 ` [PATCH 3/7] usb-storage: use fflags index only in usb-storage driver Milan Broz
` (5 subsequent siblings)
7 siblings, 1 reply; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:25 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
Switch internal usb-storage quirk value to 64-bit as quirks currently
use all 32 bits.
(Following patches are needed to use driver_info with a 64-bit
value properly.)
Signed-off-by: Milan Broz <gmazyland@gmail.com>
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
---
drivers/usb/storage/uas-detect.h | 4 ++--
drivers/usb/storage/uas.c | 4 ++--
drivers/usb/storage/usb.c | 8 ++++----
drivers/usb/storage/usb.h | 4 ++--
drivers/usb/storage/usual-tables.c | 2 +-
5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index d73282c0ec50..4d3b49e5b87a 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -54,12 +54,12 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
static int uas_use_uas_driver(struct usb_interface *intf,
const struct usb_device_id *id,
- unsigned long *flags_ret)
+ u64 *flags_ret)
{
struct usb_host_endpoint *eps[4] = { };
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- unsigned long flags = id->driver_info;
+ u64 flags = 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 2583ee9815c5..696bb0b23599 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -37,7 +37,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
- unsigned long flags;
+ u64 flags;
int qdepth, resetting;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
unsigned use_streams:1;
@@ -988,7 +988,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
struct Scsi_Host *shost = NULL;
struct uas_dev_info *devinfo;
struct usb_device *udev = interface_to_usbdev(intf);
- unsigned long dev_flags;
+ u64 dev_flags;
if (!uas_use_uas_driver(intf, id, &dev_flags))
return -ENODEV;
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index bb1fbeddc5aa..d1ad6a2509ab 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -460,13 +460,13 @@ static int associate_dev(struct us_data *us, struct usb_interface *intf)
#define TOLOWER(x) ((x) | 0x20)
/* Adjust device flags based on the "quirks=" module parameter */
-void usb_stor_adjust_quirks(struct usb_device *udev, unsigned long *fflags)
+void usb_stor_adjust_quirks(struct usb_device *udev, u64 *fflags)
{
char *p;
u16 vid = le16_to_cpu(udev->descriptor.idVendor);
u16 pid = le16_to_cpu(udev->descriptor.idProduct);
- unsigned f = 0;
- unsigned int mask = (US_FL_SANE_SENSE | US_FL_BAD_SENSE |
+ u64 f = 0;
+ u64 mask = (US_FL_SANE_SENSE | US_FL_BAD_SENSE |
US_FL_FIX_CAPACITY | US_FL_IGNORE_UAS |
US_FL_CAPACITY_HEURISTICS | US_FL_IGNORE_DEVICE |
US_FL_NOT_LOCKABLE | US_FL_MAX_SECTORS_64 |
@@ -605,7 +605,7 @@ static int get_device_info(struct us_data *us, const struct usb_device_id *id,
us->fflags &= ~US_FL_GO_SLOW;
if (us->fflags)
- dev_info(pdev, "Quirks match for vid %04x pid %04x: %lx\n",
+ dev_info(pdev, "Quirks match for vid %04x pid %04x: %llx\n",
le16_to_cpu(dev->descriptor.idVendor),
le16_to_cpu(dev->descriptor.idProduct),
us->fflags);
diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index fd3f32670873..97c6196d639b 100644
--- a/drivers/usb/storage/usb.h
+++ b/drivers/usb/storage/usb.h
@@ -95,7 +95,7 @@ struct us_data {
struct usb_interface *pusb_intf; /* this interface */
const struct us_unusual_dev *unusual_dev;
/* device-filter entry */
- unsigned long fflags; /* fixed flags from filter */
+ u64 fflags; /* fixed flags from filter */
unsigned long dflags; /* dynamic atomic bitflags */
unsigned int send_bulk_pipe; /* cached pipe values */
unsigned int recv_bulk_pipe;
@@ -192,7 +192,7 @@ extern int usb_stor_probe2(struct us_data *us);
extern void usb_stor_disconnect(struct usb_interface *intf);
extern void usb_stor_adjust_quirks(struct usb_device *dev,
- unsigned long *fflags);
+ u64 *fflags);
#define module_usb_stor_driver(__driver, __sht, __name) \
static int __init __driver##_init(void) \
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index b3c3ea04c11c..a26029e43dfd 100644
--- a/drivers/usb/storage/usual-tables.c
+++ b/drivers/usb/storage/usual-tables.c
@@ -19,7 +19,7 @@
vendorName, productName, useProtocol, useTransport, \
initFunction, flags) \
{ USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax), \
- .driver_info = (flags) }
+ .driver_info = (kernel_ulong_t)(flags) }
#define COMPLIANT_DEV UNUSUAL_DEV
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 2/7] usb-storage,uas: make internal quirks flags 64bit
2023-10-16 7:25 ` [PATCH 2/7] usb-storage,uas: make internal quirks flags 64bit Milan Broz
@ 2023-10-21 10:19 ` Greg KH
0 siblings, 0 replies; 46+ messages in thread
From: Greg KH @ 2023-10-21 10:19 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
On Mon, Oct 16, 2023 at 09:25:59AM +0200, Milan Broz wrote:
> Switch internal usb-storage quirk value to 64-bit as quirks currently
> use all 32 bits.
>
> (Following patches are needed to use driver_info with a 64-bit
> value properly.)
Nit, this sentence isn't needed, I'll go delete it when I apply it, and
patch 1/7 to my tree now, thanks.
greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 3/7] usb-storage: use fflags index only in usb-storage driver
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
2023-10-16 7:25 ` [PATCH 1/7] usb-storage: remove UNUSUAL_VENDOR_INTF macro Milan Broz
2023-10-16 7:25 ` [PATCH 2/7] usb-storage,uas: make internal quirks flags 64bit Milan Broz
@ 2023-10-16 7:26 ` Milan Broz
2023-10-21 10:21 ` Greg KH
2023-10-16 7:26 ` [PATCH 4/7] usb-storage,uas: use host helper to generate driver info Milan Broz
` (4 subsequent siblings)
7 siblings, 1 reply; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:26 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
This patch adds a parameter to use driver_info translation function
(which will be defined in the following patch).
Only USB storage driver will use it, as other drivers do not need
more than 32-bit quirk flags.
Signed-off-by: Milan Broz <gmazyland@gmail.com>
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
---
drivers/usb/storage/alauda.c | 2 +-
drivers/usb/storage/cypress_atacb.c | 2 +-
drivers/usb/storage/datafab.c | 2 +-
drivers/usb/storage/ene_ub6250.c | 2 +-
drivers/usb/storage/freecom.c | 2 +-
drivers/usb/storage/isd200.c | 2 +-
drivers/usb/storage/jumpshot.c | 2 +-
drivers/usb/storage/karma.c | 2 +-
drivers/usb/storage/onetouch.c | 2 +-
drivers/usb/storage/realtek_cr.c | 2 +-
drivers/usb/storage/sddr09.c | 2 +-
drivers/usb/storage/sddr55.c | 2 +-
drivers/usb/storage/shuttle_usbat.c | 2 +-
drivers/usb/storage/usb.c | 9 +++++----
drivers/usb/storage/usb.h | 3 ++-
15 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
index 115f05a6201a..74e293981ab1 100644
--- a/drivers/usb/storage/alauda.c
+++ b/drivers/usb/storage/alauda.c
@@ -1241,7 +1241,7 @@ static int alauda_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - alauda_usb_ids) + alauda_unusual_dev_list,
- &alauda_host_template);
+ &alauda_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/cypress_atacb.c b/drivers/usb/storage/cypress_atacb.c
index 98b3ec352a13..2fc939f709b0 100644
--- a/drivers/usb/storage/cypress_atacb.c
+++ b/drivers/usb/storage/cypress_atacb.c
@@ -246,7 +246,7 @@ static int cypress_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - cypress_usb_ids) + cypress_unusual_dev_list,
- &cypress_host_template);
+ &cypress_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/datafab.c b/drivers/usb/storage/datafab.c
index bcc4a2fad863..fad9eca3cad9 100644
--- a/drivers/usb/storage/datafab.c
+++ b/drivers/usb/storage/datafab.c
@@ -727,7 +727,7 @@ static int datafab_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - datafab_usb_ids) + datafab_unusual_dev_list,
- &datafab_host_template);
+ &datafab_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index 97c66c0d91f4..6985d3419b3c 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -2331,7 +2331,7 @@ static int ene_ub6250_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - ene_ub6250_usb_ids) + ene_ub6250_unusual_dev_list,
- &ene_ub6250_host_template);
+ &ene_ub6250_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/freecom.c b/drivers/usb/storage/freecom.c
index 2b098b55c4cb..6d971bd711d8 100644
--- a/drivers/usb/storage/freecom.c
+++ b/drivers/usb/storage/freecom.c
@@ -548,7 +548,7 @@ static int freecom_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - freecom_usb_ids) + freecom_unusual_dev_list,
- &freecom_host_template);
+ &freecom_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 4e0eef1440b7..ecdc494756a2 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1545,7 +1545,7 @@ static int isd200_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - isd200_usb_ids) + isd200_unusual_dev_list,
- &isd200_host_template);
+ &isd200_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
index 229bf0c1afc9..1ade1e45c81d 100644
--- a/drivers/usb/storage/jumpshot.c
+++ b/drivers/usb/storage/jumpshot.c
@@ -653,7 +653,7 @@ static int jumpshot_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - jumpshot_usb_ids) + jumpshot_unusual_dev_list,
- &jumpshot_host_template);
+ &jumpshot_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/karma.c b/drivers/usb/storage/karma.c
index 38ddfedef629..60868be0e38c 100644
--- a/drivers/usb/storage/karma.c
+++ b/drivers/usb/storage/karma.c
@@ -205,7 +205,7 @@ static int karma_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - karma_usb_ids) + karma_unusual_dev_list,
- &karma_host_template);
+ &karma_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/onetouch.c b/drivers/usb/storage/onetouch.c
index 01f3c2779ccf..fe34f20cce03 100644
--- a/drivers/usb/storage/onetouch.c
+++ b/drivers/usb/storage/onetouch.c
@@ -280,7 +280,7 @@ static int onetouch_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - onetouch_usb_ids) + onetouch_unusual_dev_list,
- &onetouch_host_template);
+ &onetouch_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index 0c423916d7bf..892b26714b5f 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -1041,7 +1041,7 @@ static int realtek_cr_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - realtek_cr_ids) +
realtek_cr_unusual_dev_list,
- &realtek_cr_host_template);
+ &realtek_cr_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/sddr09.c b/drivers/usb/storage/sddr09.c
index 51bcd4a43690..107eeb7fda04 100644
--- a/drivers/usb/storage/sddr09.c
+++ b/drivers/usb/storage/sddr09.c
@@ -1753,7 +1753,7 @@ static int sddr09_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - sddr09_usb_ids) + sddr09_unusual_dev_list,
- &sddr09_host_template);
+ &sddr09_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index 15dc25801cdc..c64b72de453f 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -985,7 +985,7 @@ static int sddr55_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - sddr55_usb_ids) + sddr55_unusual_dev_list,
- &sddr55_host_template);
+ &sddr55_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/shuttle_usbat.c b/drivers/usb/storage/shuttle_usbat.c
index f0d0ca37163d..3ac82f49401c 100644
--- a/drivers/usb/storage/shuttle_usbat.c
+++ b/drivers/usb/storage/shuttle_usbat.c
@@ -1838,7 +1838,7 @@ static int usbat_probe(struct usb_interface *intf,
result = usb_stor_probe1(&us, intf, id,
(id - usbat_usb_ids) + usbat_unusual_dev_list,
- &usbat_host_template);
+ &usbat_host_template, 0);
if (result)
return result;
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index d1ad6a2509ab..db8c4d2c8d11 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -574,7 +574,7 @@ EXPORT_SYMBOL_GPL(usb_stor_adjust_quirks);
/* Get the unusual_devs entries and the string descriptors */
static int get_device_info(struct us_data *us, const struct usb_device_id *id,
- const struct us_unusual_dev *unusual_dev)
+ const struct us_unusual_dev *unusual_dev, int fflags_use_index)
{
struct usb_device *dev = us->pusb_dev;
struct usb_interface_descriptor *idesc =
@@ -925,7 +925,8 @@ int usb_stor_probe1(struct us_data **pus,
struct usb_interface *intf,
const struct usb_device_id *id,
const struct us_unusual_dev *unusual_dev,
- const struct scsi_host_template *sht)
+ const struct scsi_host_template *sht,
+ int fflags_use_index)
{
struct Scsi_Host *host;
struct us_data *us;
@@ -962,7 +963,7 @@ int usb_stor_probe1(struct us_data **pus,
goto BadDevice;
/* Get the unusual_devs entries and the descriptors */
- result = get_device_info(us, id, unusual_dev);
+ result = get_device_info(us, id, unusual_dev, fflags_use_index);
if (result)
goto BadDevice;
@@ -1120,7 +1121,7 @@ static int storage_probe(struct usb_interface *intf,
}
result = usb_stor_probe1(&us, intf, id, unusual_dev,
- &usb_stor_host_template);
+ &usb_stor_host_template, 1);
if (result)
return result;
diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index 97c6196d639b..975c47efcce7 100644
--- a/drivers/usb/storage/usb.h
+++ b/drivers/usb/storage/usb.h
@@ -187,7 +187,8 @@ extern int usb_stor_probe1(struct us_data **pus,
struct usb_interface *intf,
const struct usb_device_id *id,
const struct us_unusual_dev *unusual_dev,
- const struct scsi_host_template *sht);
+ const struct scsi_host_template *sht,
+ int fflags_use_index);
extern int usb_stor_probe2(struct us_data *us);
extern void usb_stor_disconnect(struct usb_interface *intf);
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 3/7] usb-storage: use fflags index only in usb-storage driver
2023-10-16 7:26 ` [PATCH 3/7] usb-storage: use fflags index only in usb-storage driver Milan Broz
@ 2023-10-21 10:21 ` Greg KH
2023-10-26 10:27 ` Milan Broz
0 siblings, 1 reply; 46+ messages in thread
From: Greg KH @ 2023-10-21 10:21 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
On Mon, Oct 16, 2023 at 09:26:00AM +0200, Milan Broz wrote:
> This patch adds a parameter to use driver_info translation function
> (which will be defined in the following patch).
>
> Only USB storage driver will use it, as other drivers do not need
> more than 32-bit quirk flags.
Then this really should be renamed to be something else.
Having a parameter be "0" means we have to go and look up the function
and see what it does and why everyone is passing 0 to it.
Make a "wrapper" function, and rename it to be something sane that does
not need the extra option, and then for the one place you do need it,
use a different function name and then both call the real function.
Does that makes sense?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 3/7] usb-storage: use fflags index only in usb-storage driver
2023-10-21 10:21 ` Greg KH
@ 2023-10-26 10:27 ` Milan Broz
0 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-26 10:27 UTC (permalink / raw)
To: Greg KH; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
On 10/21/23 12:21, Greg KH wrote:
> On Mon, Oct 16, 2023 at 09:26:00AM +0200, Milan Broz wrote:
>> This patch adds a parameter to use driver_info translation function
>> (which will be defined in the following patch).
>>
>> Only USB storage driver will use it, as other drivers do not need
>> more than 32-bit quirk flags.
>
> Then this really should be renamed to be something else.
>
> Having a parameter be "0" means we have to go and look up the function
> and see what it does and why everyone is passing 0 to it.
>
> Make a "wrapper" function, and rename it to be something sane that does
> not need the extra option, and then for the one place you do need it,
> use a different function name and then both call the real function.
>
> Does that makes sense?
Yes, fixed in v3 - and as it really simplified the patch to just few lines,
I merged in to one patch (as we touch these lines there anyway, it is
IMO more readable to have it in one patch).
Thanks!
Milan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 4/7] usb-storage,uas: use host helper to generate driver info
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
` (2 preceding siblings ...)
2023-10-16 7:26 ` [PATCH 3/7] usb-storage: use fflags index only in usb-storage driver Milan Broz
@ 2023-10-16 7:26 ` Milan Broz
2023-10-16 18:49 ` Alan Stern
2023-10-26 10:16 ` [PATCH v3] " Milan Broz
2023-10-16 7:26 ` [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms Milan Broz
` (3 subsequent siblings)
7 siblings, 2 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:26 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
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 device_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 integers.
This translation is unnecessary for a 64-bit system, but I keep it
in place for simplicity in this patch.
Signed-off-by: Milan Broz <gmazyland@gmail.com>
---
drivers/usb/storage/Makefile | 25 ++++
drivers/usb/storage/mkflags.c | 226 +++++++++++++++++++++++++++++
drivers/usb/storage/uas-detect.h | 4 +-
drivers/usb/storage/uas.c | 20 +--
drivers/usb/storage/usb-ids.h | 33 +++++
drivers/usb/storage/usb.c | 10 +-
drivers/usb/storage/usual-tables.c | 23 +--
7 files changed, 301 insertions(+), 40 deletions(-)
create mode 100644 drivers/usb/storage/mkflags.c
create mode 100644 drivers/usb/storage/usb-ids.h
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
index 46635fa4a340..612678f108d0 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
+
+# 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 flags as USB driver device_info
+# (where the value is stored) can be only 32-bit.
+# The most significant bit means it is index to 64-bit flags table pre-computed
+# by mkflags. 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
+clean-files := usb-ids.c usb-ids-uas.c
+HOSTCFLAGS_mkflags.o := -I $(srctree)/include/
+hostprogs += mkflags
+
+quiet_cmd_mkflag_storage = FLAGS $@
+cmd_mkflag_storage = $(obj)/mkflags storage > $@
+
+quiet_cmd_mkflag_uas = FLAGS $@
+cmd_mkflag_uas = $(obj)/mkflags uas > $@
+
+$(obj)/usb-ids.c: $(obj)/mkflags FORCE
+ $(call if_changed,mkflag_storage)
+
+$(obj)/usb-ids-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..2514ffef0154
--- /dev/null
+++ b/drivers/usb/storage/mkflags.c
@@ -0,0 +1,226 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+/*
+ * This is host-compiled generator for usb-ids.c (usb-storage)
+ * and usb-ids-uas.c (uas).
+ *
+ * Generated files contain pre-computed 32-bit flags as USB driver
+ * device_info (where the value is stored) can be only 32-bit.
+ * The most significant bit means that it is index to 64-bit flags table
+ * named usb_stor_di_to_u64 with size stored in usb_stor_di_idx_size
+ * (for sanity check).
+ *
+ * Note that usb-storage driver contains also UAS devices, while UAS
+ * driver contains only UAS entries (so there can be duplicities).
+ */
+
+#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
+
+/* USB-atached mass storage */
+#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"
+#undef UNUSUAL_DEV
+
+/* UAS */
+#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"
+#undef UNUSUAL_DEV
+
+/* Terminating entry */
+{ .flags = FLAGS_END }
+};
+#undef USUAL_DEV
+#undef COMPLIANT_DEV
+#undef IS_ENABLED
+
+/* Highest bit indicates it is index to usb_stor_di_to_u64 table */
+#define HI32 (uint32_t)(1UL << 31)
+
+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%04x, .idProduct =0x%04x, "
+ ".bcdDevice_lo = 0x%04x, .bcdDevice_hi = 0x%04x, .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(const char *type)
+{
+ int i, count;
+
+ printf("const u64 usb_%s_di_to_u64[] = {\n", type);
+ for (i = 0, count = 0; vals[i].flags != FLAGS_END; i++) {
+ if (vals[i].set == FLAGS_SET) {
+ printf("\t/* 0x%02x */ 0x%llx,\n", vals[i].idx, vals[i].flags);
+ count++;
+ }
+ }
+ printf("};\n\n");
+ printf("const unsigned long usb_%s_di_idx_size = %i;\n\n", type, count);
+}
+
+static void print_usb_storage(void)
+{
+ printf("#include <linux/usb.h>\n\n");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("stor");
+
+ /* 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{ } /* 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");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("uas");
+
+ /* 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{ } /* 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], "storage") && strcmp(argv[1], "uas"))) {
+ printf("Please specify output type: storage or uas.\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], "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..fbe068b138c4 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -54,12 +54,14 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
static int uas_use_uas_driver(struct usb_interface *intf,
const struct usb_device_id *id,
+ const u64 *di_to_u64,
+ unsigned long di_idx_size,
u64 *flags_ret)
{
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(di_to_u64, di_idx_size, 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..8a1c4449dcc9 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 "usb-ids.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 "usb-ids-uas.c"
static int uas_switch_interface(struct usb_device *udev,
struct usb_interface *intf)
@@ -990,7 +977,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
struct usb_device *udev = interface_to_usbdev(intf);
u64 dev_flags;
- if (!uas_use_uas_driver(intf, id, &dev_flags))
+ if (!uas_use_uas_driver(intf, id, usb_uas_di_to_u64, usb_uas_di_idx_size,
+ &dev_flags))
return -ENODEV;
if (uas_switch_interface(udev, intf))
diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h
new file mode 100644
index 000000000000..8bfd84e07f96
--- /dev/null
+++ b/drivers/usb/storage/usb-ids.h
@@ -0,0 +1,33 @@
+/* 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_di_idx_size;
+extern const unsigned long usb_uas_di_idx_size;
+extern const u64 usb_stor_di_to_u64[];
+extern const u64 usb_uas_di_to_u64[];
+
+static u64 usb_stor_di2flags(const u64 *di_to_u64,
+ unsigned long idx_size, unsigned long idx)
+{
+ u64 flags = 0;
+
+ if (idx < (1UL << 31))
+ return idx;
+
+ idx -= (1UL << 31);
+
+ if (idx < idx_size)
+ flags = di_to_u64[idx];
+ else
+ WARN_ONCE(true, "usb_stor_di_to_u64 table not updated");
+
+ return flags;
+}
+
+#endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index db8c4d2c8d11..bb48ab1bd461 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -56,6 +56,7 @@
#include "sierra_ms.h"
#include "option_ms.h"
+#include "usb-ids.h"
#if IS_ENABLED(CONFIG_USB_UAS)
#include "uas-detect.h"
#endif
@@ -589,7 +590,11 @@ 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(usb_stor_di_to_u64, usb_stor_di_idx_size,
+ id->driver_info);
+ else
+ us->fflags = id->driver_info;
usb_stor_adjust_quirks(us->pusb_dev, &us->fflags);
if (us->fflags & US_FL_IGNORE_DEVICE) {
@@ -1091,7 +1096,8 @@ static int storage_probe(struct usb_interface *intf,
/* If uas is enabled and this device can do uas then ignore it. */
#if IS_ENABLED(CONFIG_USB_UAS)
- if (uas_use_uas_driver(intf, id, NULL))
+ if (uas_use_uas_driver(intf, id, usb_stor_di_to_u64,
+ usb_stor_di_idx_size, NULL))
return -ENXIO;
#endif
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index a26029e43dfd..40ef861dbd08 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 usb-ids.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 "usb-ids.c"
/*
* The table of devices to ignore
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 4/7] usb-storage,uas: use host helper to generate driver info
2023-10-16 7:26 ` [PATCH 4/7] usb-storage,uas: use host helper to generate driver info Milan Broz
@ 2023-10-16 18:49 ` Alan Stern
2023-10-26 10:24 ` Milan Broz
2023-10-26 10:16 ` [PATCH v3] " Milan Broz
1 sibling, 1 reply; 46+ messages in thread
From: Alan Stern @ 2023-10-16 18:49 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On Mon, Oct 16, 2023 at 09:26:01AM +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 device_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 integers.
>
> This translation is unnecessary for a 64-bit system, but I keep it
> in place for simplicity in this patch.
>
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> ---
> drivers/usb/storage/Makefile | 25 ++++
> drivers/usb/storage/mkflags.c | 226 +++++++++++++++++++++++++++++
> drivers/usb/storage/uas-detect.h | 4 +-
> drivers/usb/storage/uas.c | 20 +--
> drivers/usb/storage/usb-ids.h | 33 +++++
> drivers/usb/storage/usb.c | 10 +-
> drivers/usb/storage/usual-tables.c | 23 +--
> 7 files changed, 301 insertions(+), 40 deletions(-)
> create mode 100644 drivers/usb/storage/mkflags.c
> create mode 100644 drivers/usb/storage/usb-ids.h
>
> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
> index 46635fa4a340..612678f108d0 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
> +
> +# 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 flags as USB driver device_info
s/flags as/flags, as/
Otherwise this seems to say that the 32-bit flags are converted to USB
driver device_info values -- an incorrect parsing that makes no sense
and will confuse readers. (It confused me at first.)
Also, don't you really mean "driver_info" rather than "driver
device_info"? That's the name of the field in struct usb_device_id.
Same comments apply to each place you used this text.
> +# (where the value is stored) can be only 32-bit.
> +# The most significant bit means it is index to 64-bit flags table pre-computed
> +# by mkflags. 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
> +clean-files := usb-ids.c usb-ids-uas.c
> +HOSTCFLAGS_mkflags.o := -I $(srctree)/include/
> +hostprogs += mkflags
> +
> +quiet_cmd_mkflag_storage = FLAGS $@
> +cmd_mkflag_storage = $(obj)/mkflags storage > $@
> +
> +quiet_cmd_mkflag_uas = FLAGS $@
> +cmd_mkflag_uas = $(obj)/mkflags uas > $@
> +
> +$(obj)/usb-ids.c: $(obj)/mkflags FORCE
> + $(call if_changed,mkflag_storage)
> +
> +$(obj)/usb-ids-uas.c: $(obj)/mkflags FORCE
> + $(call if_changed,mkflag_uas)
Hmmm... Where does this say that usb-ids.c and usb-ids-uas.c depend
on unusual_devs.h and unusual_uas.h? Is the kbuild system smart enough
to figure that out on its own (this seems unlikely)?
And do the FORCE things need to be there? After all, the .c files
don't need to be rebuilt if nothing has changed since the last build.
> diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c
> new file mode 100644
> index 000000000000..2514ffef0154
> --- /dev/null
> +++ b/drivers/usb/storage/mkflags.c
> @@ -0,0 +1,226 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +/*
> + * This is host-compiled generator for usb-ids.c (usb-storage)
> + * and usb-ids-uas.c (uas).
> + *
> + * Generated files contain pre-computed 32-bit flags as USB driver
> + * device_info (where the value is stored) can be only 32-bit.
> + * The most significant bit means that it is index to 64-bit flags table
> + * named usb_stor_di_to_u64 with size stored in usb_stor_di_idx_size
> + * (for sanity check).
> + *
> + * Note that usb-storage driver contains also UAS devices, while UAS
> + * driver contains only UAS entries (so there can be duplicities).
"duplicates", not "duplicities". ("duplicity" means something else --
basically it means "lying".)
> + */
> +
> +#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 {
"svals" is a rather generic and uninformative name. How about calling
it "unusual_dev_entry" or something like that, instead?
> + 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 };
If you give names to these enums and move their definitions up above
definition of svals, then you can declare the .type and .set fields to
be of the enumerated types instead of just integers.
> +#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
> +
> +/* USB-atached mass storage */
Spelling error. Also, considering that "UAS" stands for "USB-Attached
Storage", you might want to change this to "USB Mass Storage" to make
the distinction clearer. Or even change both comments to
"usb-storage" and "uas", since the driver names are more distinct than
the protocol names.
> +#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"
> +#undef UNUSUAL_DEV
> +
> +/* UAS */
> +#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"
> +#undef UNUSUAL_DEV
> +
> +/* Terminating entry */
> +{ .flags = FLAGS_END }
> +};
> +#undef USUAL_DEV
> +#undef COMPLIANT_DEV
> +#undef IS_ENABLED
> +
> +/* Highest bit indicates it is index to usb_stor_di_to_u64 table */
> +#define HI32 (uint32_t)(1UL << 31)
> +
> +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%04x, .idProduct =0x%04x, "
> + ".bcdDevice_lo = 0x%04x, .bcdDevice_hi = 0x%04x, .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(const char *type)
> +{
> + int i, count;
> +
> + printf("const u64 usb_%s_di_to_u64[] = {\n", type);
> + for (i = 0, count = 0; vals[i].flags != FLAGS_END; i++) {
> + if (vals[i].set == FLAGS_SET) {
> + printf("\t/* 0x%02x */ 0x%llx,\n", vals[i].idx, vals[i].flags);
> + count++;
> + }
> + }
> + printf("};\n\n");
> + printf("const unsigned long usb_%s_di_idx_size = %i;\n\n", type, count);
> +}
> +
> +static void print_usb_storage(void)
> +{
> + printf("#include <linux/usb.h>\n\n");
> +
> + /* conversion table from 32-bit device_flags to 64-bit flags */
> + print_usb_flags("stor");
> +
> + /* 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{ } /* 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");
> +
> + /* conversion table from 32-bit device_flags to 64-bit flags */
> + print_usb_flags("uas");
> +
> + /* 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{ } /* 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], "storage") && strcmp(argv[1], "uas"))) {
> + printf("Please specify output type: storage or uas.\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], "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..fbe068b138c4 100644
> --- a/drivers/usb/storage/uas-detect.h
> +++ b/drivers/usb/storage/uas-detect.h
> @@ -54,12 +54,14 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
>
> static int uas_use_uas_driver(struct usb_interface *intf,
> const struct usb_device_id *id,
> + const u64 *di_to_u64,
> + unsigned long di_idx_size,
> u64 *flags_ret)
> {
> 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(di_to_u64, di_idx_size, 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..8a1c4449dcc9 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 "usb-ids.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 "usb-ids-uas.c"
I think it would make more sense to put this #include immediately
after the "usb-ids.h" line. After all, the two are so similar.
>
> static int uas_switch_interface(struct usb_device *udev,
> struct usb_interface *intf)
> @@ -990,7 +977,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
> struct usb_device *udev = interface_to_usbdev(intf);
> u64 dev_flags;
>
> - if (!uas_use_uas_driver(intf, id, &dev_flags))
> + if (!uas_use_uas_driver(intf, id, usb_uas_di_to_u64, usb_uas_di_idx_size,
> + &dev_flags))
> return -ENODEV;
>
> if (uas_switch_interface(udev, intf))
> diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h
> new file mode 100644
> index 000000000000..8bfd84e07f96
> --- /dev/null
> +++ b/drivers/usb/storage/usb-ids.h
> @@ -0,0 +1,33 @@
> +/* 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_di_idx_size;
> +extern const unsigned long usb_uas_di_idx_size;
> +extern const u64 usb_stor_di_to_u64[];
> +extern const u64 usb_uas_di_to_u64[];
> +
> +static u64 usb_stor_di2flags(const u64 *di_to_u64,
> + unsigned long idx_size, unsigned long idx)
I really dislike all the places you've used "di", such as here in
"di2flags". Something a little more explicit would be much better,
such as "drv_info".
Also, idx_size refers to the size of the table, not the size of the
index. So a better name would be "table_size". Similarly,
"di_to_u64" sounds like a function name, not an array name. You could
call it "drv_info_u64_table".
> +{
> + u64 flags = 0;
> +
> + if (idx < (1UL << 31))
> + return idx;
> +
> + idx -= (1UL << 31);
> +
> + if (idx < idx_size)
> + flags = di_to_u64[idx];
> + else
> + WARN_ONCE(true, "usb_stor_di_to_u64 table not updated");
I think Greg KH is against introducing new usages of
WARN/WARN_ON/WARN_ONCE. You can change this to pr_warn().
> +
> + return flags;
> +}
> +
> +#endif
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index db8c4d2c8d11..bb48ab1bd461 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -56,6 +56,7 @@
> #include "sierra_ms.h"
> #include "option_ms.h"
>
> +#include "usb-ids.h"
> #if IS_ENABLED(CONFIG_USB_UAS)
> #include "uas-detect.h"
> #endif
> @@ -589,7 +590,11 @@ 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(usb_stor_di_to_u64, usb_stor_di_idx_size,
> + id->driver_info);
> + else
> + us->fflags = id->driver_info;
> usb_stor_adjust_quirks(us->pusb_dev, &us->fflags);
>
> if (us->fflags & US_FL_IGNORE_DEVICE) {
> @@ -1091,7 +1096,8 @@ static int storage_probe(struct usb_interface *intf,
>
> /* If uas is enabled and this device can do uas then ignore it. */
> #if IS_ENABLED(CONFIG_USB_UAS)
> - if (uas_use_uas_driver(intf, id, NULL))
> + if (uas_use_uas_driver(intf, id, usb_stor_di_to_u64,
> + usb_stor_di_idx_size, NULL))
> return -ENXIO;
> #endif
>
> diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
> index a26029e43dfd..40ef861dbd08 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 usb-ids.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 "usb-ids.c"
>
> /*
> * The table of devices to ignore
> --
> 2.42.0
Alan Stern
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/7] usb-storage,uas: use host helper to generate driver info
2023-10-16 18:49 ` Alan Stern
@ 2023-10-26 10:24 ` Milan Broz
0 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-26 10:24 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On 10/16/23 20:49, Alan Stern wrote:
> On Mon, Oct 16, 2023 at 09:26:01AM +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 device_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 integers.
>>
>> This translation is unnecessary for a 64-bit system, but I keep it
>> in place for simplicity in this patch.
>>
>> Signed-off-by: Milan Broz <gmazyland@gmail.com>
>> ---
>> drivers/usb/storage/Makefile | 25 ++++
>> drivers/usb/storage/mkflags.c | 226 +++++++++++++++++++++++++++++
>> drivers/usb/storage/uas-detect.h | 4 +-
>> drivers/usb/storage/uas.c | 20 +--
>> drivers/usb/storage/usb-ids.h | 33 +++++
>> drivers/usb/storage/usb.c | 10 +-
>> drivers/usb/storage/usual-tables.c | 23 +--
>> 7 files changed, 301 insertions(+), 40 deletions(-)
>> create mode 100644 drivers/usb/storage/mkflags.c
>> create mode 100644 drivers/usb/storage/usb-ids.h
>>
>> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
>> index 46635fa4a340..612678f108d0 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
>> +
>> +# 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 flags as USB driver device_info
>
> s/flags as/flags, as/
>
> Otherwise this seems to say that the 32-bit flags are converted to USB
> driver device_info values -- an incorrect parsing that makes no sense
> and will confuse readers. (It confused me at first.)
>
> Also, don't you really mean "driver_info" rather than "driver
> device_info"? That's the name of the field in struct usb_device_id.
Yes, not sure why I mixed these. Fixed in v3 patch (and now only one
patch is needed as 2 previous are merged in usb-next).
I hope I fixed all other comments too, thanks!
Milan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v3] usb-storage,uas: use host helper to generate driver info
2023-10-16 7:26 ` [PATCH 4/7] usb-storage,uas: use host helper to generate driver info Milan Broz
2023-10-16 18:49 ` Alan Stern
@ 2023-10-26 10:16 ` Milan Broz
2023-10-27 15:45 ` Alan Stern
2023-10-28 17:41 ` [PATCH v4] " Milan Broz
1 sibling, 2 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-26 10:16 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
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>
---
Changes from v2
- Rebased to usb-testing tree
- Include changes from Alan Stern and Greg KH reviews (thanks!)
- Remove FORCE from Makefile add explicit dependence on unusual*.h headers
- Avoid use of #ifdef (mkflags.c need -D CONFIG_64BIT=X flag now)
- Use drv_info in functions and variable names (instead of di)
- Use wrapper for usb_stor_probe1(), this simplifies the previous separate
patch (no need to touch other drivers) so it can be merged here directly
- Merge 64bit optimization to this patch too
Changes from v1
- Separate flags generation from OPAL command patchset
(it means there is currently no quirk flag that requires this patch yet)
drivers/usb/storage/Makefile | 32 ++++
drivers/usb/storage/mkflags.c | 233 +++++++++++++++++++++++++++++
drivers/usb/storage/uas-detect.h | 6 +-
drivers/usb/storage/uas.c | 23 +--
drivers/usb/storage/usb-ids.h | 37 +++++
drivers/usb/storage/usb.c | 32 +++-
drivers/usb/storage/usual-tables.c | 23 +--
7 files changed, 339 insertions(+), 47 deletions(-)
create mode 100644 drivers/usb/storage/mkflags.c
create mode 100644 drivers/usb/storage/usb-ids.h
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
+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)
diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c
new file mode 100644
index 000000000000..e9c7eb524999
--- /dev/null
+++ b/drivers/usb/storage/mkflags.c
@@ -0,0 +1,233 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+/*
+ * This is host-compiled generator for usb-ids.c (usb-storage)
+ * and usb-ids-uas.c (uas).
+ *
+ * Generated files contain pre-computed 32-bit values, as USB
+ * driver_info (where the value is stored) can be only 32-bit.
+ * The most significant bit means that it is index to 64-bit
+ * pre-computed table named usb_stor_drv_info_u64_table with size
+ * stored in usb_stor_drv_info_u64_table_size (for sanity check).
+ *
+ * Note that usb-storage driver contains also UAS devices, while UAS
+ * driver contains only UAS entries (so there can be duplicates).
+ */
+
+#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>
+
+typedef enum { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS } dev_type;
+typedef enum { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE } dev_flags_set;
+#define FLAGS_END (uint64_t)-1
+
+struct unusual_dev_entry {
+ dev_type 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;
+ dev_flags_set set;
+ unsigned int idx;
+};
+
+static struct unusual_dev_entry unusual_dev_entries[] = {
+#define USUAL_DEV(useProto, useTrans) \
+{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 }
+
+#define COMPLIANT_DEV UNUSUAL_DEV
+#define IS_ENABLED(x) 0
+
+/* usb-storage */
+#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"
+#undef UNUSUAL_DEV
+
+/* uas */
+#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"
+#undef UNUSUAL_DEV
+
+/* Terminating entry */
+{ .flags = FLAGS_END }
+};
+#undef USUAL_DEV
+#undef COMPLIANT_DEV
+#undef IS_ENABLED
+
+/* Highest bit indicates it is index to usb_stor_drv_info_u64_table */
+#define HI32 (uint32_t)(1UL << 31)
+
+static uint64_t get_driver_info(uint64_t flags, unsigned int idx)
+{
+ if (CONFIG_64BIT)
+ return flags;
+
+ if (flags < HI32)
+ return flags;
+
+ /* Use index that will be processed in usb_stor_drv_info_to_flags */
+ 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(dev_type type)
+{
+ for (int i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[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%04x, .idProduct =0x%04x, "
+ ".bcdDevice_lo = 0x%04x, .bcdDevice_hi = 0x%04x, .driver_info = 0x%llx },\n",
+ unusual_dev_entries[i].idVendor, unusual_dev_entries[i].idProduct,
+ unusual_dev_entries[i].bcdDevice_lo, unusual_dev_entries[i].bcdDevice_hi,
+ get_driver_info(unusual_dev_entries[i].flags, unusual_dev_entries[i].idx));
+ } else if (type == TYPE_CLASS)
+ print_class(unusual_dev_entries[i].bDeviceSubClass, unusual_dev_entries[i].bDeviceProtocol);
+ }
+}
+
+static void print_usb_flags(const char *type)
+{
+ int i, count;
+
+ if (CONFIG_64BIT) {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {};\n", type);
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = 0;\n\n", type);
+ } else {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {\n", type);
+ for (i = 0, count = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].set == FLAGS_SET) {
+ printf("\t/* 0x%02x */ 0x%llx,\n", unusual_dev_entries[i].idx, unusual_dev_entries[i].flags);
+ count++;
+ }
+ }
+ printf("};\n\n");
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = %i;\n\n", type, count);
+ }
+}
+
+static void print_usb_storage(void)
+{
+ printf("#include <linux/usb.h>\n\n");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("stor");
+
+ /* usb_storage_usb_ids */
+ printf("const struct usb_device_id usb_storage_usb_ids[] = {\n");
+
+ /* usb-storage driver devices */
+ print_type(TYPE_DEVICE_STORAGE);
+
+ /* uas driver 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{ } /* 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");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("uas");
+
+ /* uas_usb_ids */
+ printf("const struct usb_device_id uas_usb_ids[] = {\n");
+
+ /* uas driver 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{ } /* 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], "storage") && strcmp(argv[1], "uas"))) {
+ printf("Please specify output type: storage or uas.\n");
+ return 1;
+ }
+
+ for (i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].type == TYPE_CLASS)
+ continue;
+ skip = 0;
+ if (unusual_dev_entries[i].flags >= HI32) {
+ for (j = 0; j < i; j++) {
+ if (unusual_dev_entries[j].flags == unusual_dev_entries[i].flags &&
+ unusual_dev_entries[j].set == FLAGS_SET) {
+ skip = 1;
+ idx_old = unusual_dev_entries[j].idx;
+ break;
+ }
+ }
+ if (skip) {
+ unusual_dev_entries[i].idx = idx_old;
+ unusual_dev_entries[i].set = FLAGS_DUPLICATE;
+ } else {
+ unusual_dev_entries[i].idx = idx;
+ unusual_dev_entries[i].set = FLAGS_SET;
+ idx++;
+ }
+ }
+ }
+
+ 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..fe904d3072ec 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -54,12 +54,16 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
static int uas_use_uas_driver(struct usb_interface *intf,
const struct usb_device_id *id,
+ const u64 *drv_info_u64_table,
+ unsigned long drv_info_u64_table_size,
u64 *flags_ret)
{
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_drv_info_to_flags(drv_info_u64_table,
+ drv_info_u64_table_size,
+ 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..5b5dc8afda11 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -26,9 +26,13 @@
#include <scsi/scsi_host.h>
#include <scsi/scsi_tcq.h>
+#include "usb-ids.h"
#include "uas-detect.h"
#include "scsiglue.h"
+/* The table of devices is pre-generated in usb-ids-uas.c */
+#include "usb-ids-uas.c"
+
#define MAX_CMNDS 256
struct uas_dev_info {
@@ -909,22 +913,6 @@ 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
-
static int uas_switch_interface(struct usb_device *udev,
struct usb_interface *intf)
{
@@ -990,7 +978,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
struct usb_device *udev = interface_to_usbdev(intf);
u64 dev_flags;
- if (!uas_use_uas_driver(intf, id, &dev_flags))
+ if (!uas_use_uas_driver(intf, id, usb_uas_drv_info_u64_table,
+ usb_uas_drv_info_u64_table_size, &dev_flags))
return -ENODEV;
if (uas_switch_interface(udev, intf))
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
+}
+
+#endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index d1ad6a2509ab..1e564ea52fc5 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -56,6 +56,7 @@
#include "sierra_ms.h"
#include "option_ms.h"
+#include "usb-ids.h"
#if IS_ENABLED(CONFIG_USB_UAS)
#include "uas-detect.h"
#endif
@@ -574,7 +575,7 @@ EXPORT_SYMBOL_GPL(usb_stor_adjust_quirks);
/* Get the unusual_devs entries and the string descriptors */
static int get_device_info(struct us_data *us, const struct usb_device_id *id,
- const struct us_unusual_dev *unusual_dev)
+ const struct us_unusual_dev *unusual_dev, int fflags_use_index)
{
struct usb_device *dev = us->pusb_dev;
struct usb_interface_descriptor *idesc =
@@ -589,7 +590,11 @@ 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_drv_info_to_flags(usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, id->driver_info);
+ else
+ us->fflags = id->driver_info;
usb_stor_adjust_quirks(us->pusb_dev, &us->fflags);
if (us->fflags & US_FL_IGNORE_DEVICE) {
@@ -921,11 +926,12 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
}
/* First part of general USB mass-storage probing */
-int usb_stor_probe1(struct us_data **pus,
+static int usb_stor_probe1_fflags(struct us_data **pus,
struct usb_interface *intf,
const struct usb_device_id *id,
const struct us_unusual_dev *unusual_dev,
- const struct scsi_host_template *sht)
+ const struct scsi_host_template *sht,
+ int fflags_use_index)
{
struct Scsi_Host *host;
struct us_data *us;
@@ -962,7 +968,7 @@ int usb_stor_probe1(struct us_data **pus,
goto BadDevice;
/* Get the unusual_devs entries and the descriptors */
- result = get_device_info(us, id, unusual_dev);
+ result = get_device_info(us, id, unusual_dev, fflags_use_index);
if (result)
goto BadDevice;
@@ -981,6 +987,15 @@ int usb_stor_probe1(struct us_data **pus,
release_everything(us);
return result;
}
+
+int usb_stor_probe1(struct us_data **pus,
+ struct usb_interface *intf,
+ const struct usb_device_id *id,
+ const struct us_unusual_dev *unusual_dev,
+ const struct scsi_host_template *sht)
+{
+ return usb_stor_probe1_fflags(pus, intf, id, unusual_dev, sht, 0);
+}
EXPORT_SYMBOL_GPL(usb_stor_probe1);
/* Second part of general USB mass-storage probing */
@@ -1090,7 +1105,8 @@ static int storage_probe(struct usb_interface *intf,
/* If uas is enabled and this device can do uas then ignore it. */
#if IS_ENABLED(CONFIG_USB_UAS)
- if (uas_use_uas_driver(intf, id, NULL))
+ if (uas_use_uas_driver(intf, id, usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, NULL))
return -ENXIO;
#endif
@@ -1119,8 +1135,8 @@ static int storage_probe(struct usb_interface *intf,
id->idVendor, id->idProduct);
}
- result = usb_stor_probe1(&us, intf, id, unusual_dev,
- &usb_stor_host_template);
+ result = usb_stor_probe1_fflags(&us, intf, id, unusual_dev,
+ &usb_stor_host_template, 1);
if (result)
return result;
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index a26029e43dfd..40ef861dbd08 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 usb-ids.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 "usb-ids.c"
/*
* The table of devices to ignore
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v3] usb-storage,uas: use host helper to generate driver info
2023-10-26 10:16 ` [PATCH v3] " Milan Broz
@ 2023-10-27 15:45 ` Alan Stern
2023-10-28 17:41 ` [PATCH v4] " Milan Broz
1 sibling, 0 replies; 46+ messages in thread
From: Alan Stern @ 2023-10-27 15:45 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
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@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>
> ---
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
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH v4] usb-storage,uas: use host helper to generate driver info
2023-10-26 10:16 ` [PATCH v3] " Milan Broz
2023-10-27 15:45 ` Alan Stern
@ 2023-10-28 17:41 ` Milan Broz
2023-10-30 17:40 ` Alan Stern
2023-11-03 20:17 ` [PATCH v5] " Milan Broz
1 sibling, 2 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-28 17:41 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
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>
---
Changes from v3
- Include changes from Alan Stern review
- Make unusual*.h mkflags dependences implicit in Makefile
- Avoid conditional macros in function definition (usb-ids.h)
Changes from v2
- Rebased to usb-testing tree
- Include changes from Alan Stern and Greg KH reviews (thanks!)
- Remove FORCE from Makefile add explicit dependence on unusual*.h headers
- Avoid use of #ifdef (mkflags.c need -D CONFIG_64BIT=X flag now)
- Use drv_info in functions and variable names (instead of di)
- Use wrapper for usb_stor_probe1(), this simplifies the previous separate
patch (no need to touch other drivers) so it can be merged here directly
- Merge 64bit optimization to this patch too
Changes from v1
- Separate flags generation from OPAL command patchset
(it means there is currently no quirk flag that requires this patch yet)
drivers/usb/storage/Makefile | 31 ++++
drivers/usb/storage/mkflags.c | 233 +++++++++++++++++++++++++++++
drivers/usb/storage/uas-detect.h | 6 +-
drivers/usb/storage/uas.c | 23 +--
drivers/usb/storage/usb-ids.h | 43 ++++++
drivers/usb/storage/usb.c | 32 +++-
drivers/usb/storage/usual-tables.c | 23 +--
7 files changed, 344 insertions(+), 47 deletions(-)
create mode 100644 drivers/usb/storage/mkflags.c
create mode 100644 drivers/usb/storage/usb-ids.h
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
+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 > $@
+
+$(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/mkflags.c b/drivers/usb/storage/mkflags.c
new file mode 100644
index 000000000000..e9c7eb524999
--- /dev/null
+++ b/drivers/usb/storage/mkflags.c
@@ -0,0 +1,233 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+/*
+ * This is host-compiled generator for usb-ids.c (usb-storage)
+ * and usb-ids-uas.c (uas).
+ *
+ * Generated files contain pre-computed 32-bit values, as USB
+ * driver_info (where the value is stored) can be only 32-bit.
+ * The most significant bit means that it is index to 64-bit
+ * pre-computed table named usb_stor_drv_info_u64_table with size
+ * stored in usb_stor_drv_info_u64_table_size (for sanity check).
+ *
+ * Note that usb-storage driver contains also UAS devices, while UAS
+ * driver contains only UAS entries (so there can be duplicates).
+ */
+
+#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>
+
+typedef enum { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS } dev_type;
+typedef enum { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE } dev_flags_set;
+#define FLAGS_END (uint64_t)-1
+
+struct unusual_dev_entry {
+ dev_type 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;
+ dev_flags_set set;
+ unsigned int idx;
+};
+
+static struct unusual_dev_entry unusual_dev_entries[] = {
+#define USUAL_DEV(useProto, useTrans) \
+{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 }
+
+#define COMPLIANT_DEV UNUSUAL_DEV
+#define IS_ENABLED(x) 0
+
+/* usb-storage */
+#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"
+#undef UNUSUAL_DEV
+
+/* uas */
+#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"
+#undef UNUSUAL_DEV
+
+/* Terminating entry */
+{ .flags = FLAGS_END }
+};
+#undef USUAL_DEV
+#undef COMPLIANT_DEV
+#undef IS_ENABLED
+
+/* Highest bit indicates it is index to usb_stor_drv_info_u64_table */
+#define HI32 (uint32_t)(1UL << 31)
+
+static uint64_t get_driver_info(uint64_t flags, unsigned int idx)
+{
+ if (CONFIG_64BIT)
+ return flags;
+
+ if (flags < HI32)
+ return flags;
+
+ /* Use index that will be processed in usb_stor_drv_info_to_flags */
+ 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(dev_type type)
+{
+ for (int i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[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%04x, .idProduct =0x%04x, "
+ ".bcdDevice_lo = 0x%04x, .bcdDevice_hi = 0x%04x, .driver_info = 0x%llx },\n",
+ unusual_dev_entries[i].idVendor, unusual_dev_entries[i].idProduct,
+ unusual_dev_entries[i].bcdDevice_lo, unusual_dev_entries[i].bcdDevice_hi,
+ get_driver_info(unusual_dev_entries[i].flags, unusual_dev_entries[i].idx));
+ } else if (type == TYPE_CLASS)
+ print_class(unusual_dev_entries[i].bDeviceSubClass, unusual_dev_entries[i].bDeviceProtocol);
+ }
+}
+
+static void print_usb_flags(const char *type)
+{
+ int i, count;
+
+ if (CONFIG_64BIT) {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {};\n", type);
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = 0;\n\n", type);
+ } else {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {\n", type);
+ for (i = 0, count = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].set == FLAGS_SET) {
+ printf("\t/* 0x%02x */ 0x%llx,\n", unusual_dev_entries[i].idx, unusual_dev_entries[i].flags);
+ count++;
+ }
+ }
+ printf("};\n\n");
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = %i;\n\n", type, count);
+ }
+}
+
+static void print_usb_storage(void)
+{
+ printf("#include <linux/usb.h>\n\n");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("stor");
+
+ /* usb_storage_usb_ids */
+ printf("const struct usb_device_id usb_storage_usb_ids[] = {\n");
+
+ /* usb-storage driver devices */
+ print_type(TYPE_DEVICE_STORAGE);
+
+ /* uas driver 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{ } /* 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");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("uas");
+
+ /* uas_usb_ids */
+ printf("const struct usb_device_id uas_usb_ids[] = {\n");
+
+ /* uas driver 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{ } /* 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], "storage") && strcmp(argv[1], "uas"))) {
+ printf("Please specify output type: storage or uas.\n");
+ return 1;
+ }
+
+ for (i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].type == TYPE_CLASS)
+ continue;
+ skip = 0;
+ if (unusual_dev_entries[i].flags >= HI32) {
+ for (j = 0; j < i; j++) {
+ if (unusual_dev_entries[j].flags == unusual_dev_entries[i].flags &&
+ unusual_dev_entries[j].set == FLAGS_SET) {
+ skip = 1;
+ idx_old = unusual_dev_entries[j].idx;
+ break;
+ }
+ }
+ if (skip) {
+ unusual_dev_entries[i].idx = idx_old;
+ unusual_dev_entries[i].set = FLAGS_DUPLICATE;
+ } else {
+ unusual_dev_entries[i].idx = idx;
+ unusual_dev_entries[i].set = FLAGS_SET;
+ idx++;
+ }
+ }
+ }
+
+ 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..fe904d3072ec 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -54,12 +54,16 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
static int uas_use_uas_driver(struct usb_interface *intf,
const struct usb_device_id *id,
+ const u64 *drv_info_u64_table,
+ unsigned long drv_info_u64_table_size,
u64 *flags_ret)
{
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_drv_info_to_flags(drv_info_u64_table,
+ drv_info_u64_table_size,
+ 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..5b5dc8afda11 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -26,9 +26,13 @@
#include <scsi/scsi_host.h>
#include <scsi/scsi_tcq.h>
+#include "usb-ids.h"
#include "uas-detect.h"
#include "scsiglue.h"
+/* The table of devices is pre-generated in usb-ids-uas.c */
+#include "usb-ids-uas.c"
+
#define MAX_CMNDS 256
struct uas_dev_info {
@@ -909,22 +913,6 @@ 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
-
static int uas_switch_interface(struct usb_device *udev,
struct usb_interface *intf)
{
@@ -990,7 +978,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
struct usb_device *udev = interface_to_usbdev(intf);
u64 dev_flags;
- if (!uas_use_uas_driver(intf, id, &dev_flags))
+ if (!uas_use_uas_driver(intf, id, usb_uas_drv_info_u64_table,
+ usb_uas_drv_info_u64_table_size, &dev_flags))
return -ENODEV;
if (uas_switch_interface(udev, intf))
diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h
new file mode 100644
index 000000000000..286d620d18a4
--- /dev/null
+++ b/drivers/usb/storage/usb-ids.h
@@ -0,0 +1,43 @@
+/* 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[];
+
+#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)
+{
+ 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
+
+#endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index d1ad6a2509ab..1e564ea52fc5 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -56,6 +56,7 @@
#include "sierra_ms.h"
#include "option_ms.h"
+#include "usb-ids.h"
#if IS_ENABLED(CONFIG_USB_UAS)
#include "uas-detect.h"
#endif
@@ -574,7 +575,7 @@ EXPORT_SYMBOL_GPL(usb_stor_adjust_quirks);
/* Get the unusual_devs entries and the string descriptors */
static int get_device_info(struct us_data *us, const struct usb_device_id *id,
- const struct us_unusual_dev *unusual_dev)
+ const struct us_unusual_dev *unusual_dev, int fflags_use_index)
{
struct usb_device *dev = us->pusb_dev;
struct usb_interface_descriptor *idesc =
@@ -589,7 +590,11 @@ 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_drv_info_to_flags(usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, id->driver_info);
+ else
+ us->fflags = id->driver_info;
usb_stor_adjust_quirks(us->pusb_dev, &us->fflags);
if (us->fflags & US_FL_IGNORE_DEVICE) {
@@ -921,11 +926,12 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
}
/* First part of general USB mass-storage probing */
-int usb_stor_probe1(struct us_data **pus,
+static int usb_stor_probe1_fflags(struct us_data **pus,
struct usb_interface *intf,
const struct usb_device_id *id,
const struct us_unusual_dev *unusual_dev,
- const struct scsi_host_template *sht)
+ const struct scsi_host_template *sht,
+ int fflags_use_index)
{
struct Scsi_Host *host;
struct us_data *us;
@@ -962,7 +968,7 @@ int usb_stor_probe1(struct us_data **pus,
goto BadDevice;
/* Get the unusual_devs entries and the descriptors */
- result = get_device_info(us, id, unusual_dev);
+ result = get_device_info(us, id, unusual_dev, fflags_use_index);
if (result)
goto BadDevice;
@@ -981,6 +987,15 @@ int usb_stor_probe1(struct us_data **pus,
release_everything(us);
return result;
}
+
+int usb_stor_probe1(struct us_data **pus,
+ struct usb_interface *intf,
+ const struct usb_device_id *id,
+ const struct us_unusual_dev *unusual_dev,
+ const struct scsi_host_template *sht)
+{
+ return usb_stor_probe1_fflags(pus, intf, id, unusual_dev, sht, 0);
+}
EXPORT_SYMBOL_GPL(usb_stor_probe1);
/* Second part of general USB mass-storage probing */
@@ -1090,7 +1105,8 @@ static int storage_probe(struct usb_interface *intf,
/* If uas is enabled and this device can do uas then ignore it. */
#if IS_ENABLED(CONFIG_USB_UAS)
- if (uas_use_uas_driver(intf, id, NULL))
+ if (uas_use_uas_driver(intf, id, usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, NULL))
return -ENXIO;
#endif
@@ -1119,8 +1135,8 @@ static int storage_probe(struct usb_interface *intf,
id->idVendor, id->idProduct);
}
- result = usb_stor_probe1(&us, intf, id, unusual_dev,
- &usb_stor_host_template);
+ result = usb_stor_probe1_fflags(&us, intf, id, unusual_dev,
+ &usb_stor_host_template, 1);
if (result)
return result;
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index a26029e43dfd..40ef861dbd08 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 usb-ids.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 "usb-ids.c"
/*
* The table of devices to ignore
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4] usb-storage,uas: use host helper to generate driver info
2023-10-28 17:41 ` [PATCH v4] " Milan Broz
@ 2023-10-30 17:40 ` Alan Stern
2023-10-30 18:16 ` Milan Broz
2023-11-03 20:17 ` [PATCH v5] " Milan Broz
1 sibling, 1 reply; 46+ messages in thread
From: Alan Stern @ 2023-10-30 17:40 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4] usb-storage,uas: use host helper to generate driver info
2023-10-30 17:40 ` Alan Stern
@ 2023-10-30 18:16 ` Milan Broz
0 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-30 18:16 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On 10/30/23 18:40, Alan Stern wrote:
> 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.
ok, thx, this is perhaps relict of some previous try (I tried different
approaches) - will remove it.
> 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 /* ... */.
Perhaps just copied from header file, it is different format there.
(And my bad, I forget to run checkpatch after many last changes...)
>
> 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.
ok, I'll send next version once I have time to do it.
Thanks,
Milan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v5] usb-storage,uas: use host helper to generate driver info
2023-10-28 17:41 ` [PATCH v4] " Milan Broz
2023-10-30 17:40 ` Alan Stern
@ 2023-11-03 20:17 ` Milan Broz
2023-11-03 20:30 ` Alan Stern
2023-11-05 18:20 ` [PATCH v6] " Milan Broz
1 sibling, 2 replies; 46+ messages in thread
From: Milan Broz @ 2023-11-03 20:17 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
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>
---
Changes from v4
- Remove uneeded dependences from Makefile
- Fix checkpatch warnings
Changes from v3
- Include changes from Alan Stern review
- Make unusual*.h mkflags dependences implicit in Makefile
- Avoid conditional macros in function definition (usb-ids.h)
Changes from v2
- Rebased to usb-testing tree
- Include changes from Alan Stern and Greg KH reviews (thanks!)
- Remove FORCE from Makefile add explicit dependence on unusual*.h headers
- Avoid use of #ifdef (mkflags.c need -D CONFIG_64BIT=X flag now)
- Use drv_info in functions and variable names (instead of di)
- Use wrapper for usb_stor_probe1(), this simplifies the previous separate
patch (no need to touch other drivers) so it can be merged here directly
- Merge 64bit optimization to this patch too
Changes from v1
- Separate flags generation from OPAL command patchset
(it means there is currently no quirk flag that requires this patch yet)
drivers/usb/storage/Makefile | 29 ++++
drivers/usb/storage/mkflags.c | 242 +++++++++++++++++++++++++++++
drivers/usb/storage/uas-detect.h | 6 +-
drivers/usb/storage/uas.c | 23 +--
drivers/usb/storage/usb-ids.h | 43 +++++
drivers/usb/storage/usb.c | 32 +++-
drivers/usb/storage/usual-tables.c | 23 +--
7 files changed, 351 insertions(+), 47 deletions(-)
create mode 100644 drivers/usb/storage/mkflags.c
create mode 100644 drivers/usb/storage/usb-ids.h
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
index 46635fa4a340..e59bcfb4f777 100644
--- a/drivers/usb/storage/Makefile
+++ b/drivers/usb/storage/Makefile
@@ -45,3 +45,32 @@ 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.
+
+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 > $@
+
+$(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/mkflags.c b/drivers/usb/storage/mkflags.c
new file mode 100644
index 000000000000..1b35d08f43d1
--- /dev/null
+++ b/drivers/usb/storage/mkflags.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * This is host-compiled generator for usb-ids.c (usb-storage)
+ * and usb-ids-uas.c (uas).
+ *
+ * Generated files contain pre-computed 32-bit values, as USB
+ * driver_info (where the value is stored) can be only 32-bit.
+ * The most significant bit means that it is index to 64-bit
+ * pre-computed table named usb_stor_drv_info_u64_table with size
+ * stored in usb_stor_drv_info_u64_table_size (for sanity check).
+ *
+ * Note that usb-storage driver contains also UAS devices, while UAS
+ * driver contains only UAS entries (so there can be duplicates).
+ */
+
+#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>
+
+enum dev_type { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS };
+enum dev_flags_set { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE };
+#define FLAGS_END ((uint64_t)-1)
+
+struct unusual_dev_entry {
+ enum dev_type 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;
+ enum dev_flags_set set;
+ unsigned int idx;
+};
+
+static struct unusual_dev_entry unusual_dev_entries[] = {
+#define USUAL_DEV(useProto, useTrans) \
+{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 }
+
+#define COMPLIANT_DEV UNUSUAL_DEV
+#define IS_ENABLED(x) 0
+
+/* usb-storage */
+#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"
+#undef UNUSUAL_DEV
+
+/* uas */
+#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"
+#undef UNUSUAL_DEV
+
+/* Terminating entry */
+{ .flags = FLAGS_END }
+};
+#undef USUAL_DEV
+#undef COMPLIANT_DEV
+#undef IS_ENABLED
+
+/* Highest bit indicates it is index to usb_stor_drv_info_u64_table */
+#define HI32 ((uint32_t)(1UL << 31))
+
+static uint64_t get_driver_info(uint64_t flags, unsigned int idx)
+{
+ if (CONFIG_64BIT)
+ return flags;
+
+ if (flags < HI32)
+ return flags;
+
+ /* Use index that will be processed in usb_stor_drv_info_to_flags */
+ 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(enum dev_type type)
+{
+ for (int i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[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%04x, .idProduct =0x%04x, ",
+ unusual_dev_entries[i].idVendor,
+ unusual_dev_entries[i].idProduct);
+ printf(".bcdDevice_lo = 0x%04x, .bcdDevice_hi = 0x%04x, ",
+ unusual_dev_entries[i].bcdDevice_lo,
+ unusual_dev_entries[i].bcdDevice_hi);
+ printf(".driver_info = 0x%llx },\n",
+ get_driver_info(unusual_dev_entries[i].flags,
+ unusual_dev_entries[i].idx));
+ } else if (type == TYPE_CLASS)
+ print_class(unusual_dev_entries[i].bDeviceSubClass,
+ unusual_dev_entries[i].bDeviceProtocol);
+ }
+}
+
+static void print_usb_flags(const char *type)
+{
+ int i, count;
+
+ if (CONFIG_64BIT) {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {};\n", type);
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = 0;\n\n", type);
+ } else {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {\n", type);
+ for (i = 0, count = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].set == FLAGS_SET) {
+ printf("\t/* 0x%02x */ 0x%llx,\n",
+ unusual_dev_entries[i].idx,
+ unusual_dev_entries[i].flags);
+ count++;
+ }
+ }
+ printf("};\n\n");
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = %i;\n\n", type, count);
+ }
+}
+
+static void print_usb_storage(void)
+{
+ printf("#include <linux/usb.h>\n\n");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("stor");
+
+ /* usb_storage_usb_ids */
+ printf("const struct usb_device_id usb_storage_usb_ids[] = {\n");
+
+ /* usb-storage driver devices */
+ print_type(TYPE_DEVICE_STORAGE);
+
+ /* uas driver 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{ } /* 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");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("uas");
+
+ /* uas_usb_ids */
+ printf("const struct usb_device_id uas_usb_ids[] = {\n");
+
+ /* uas driver 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{ } /* 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], "storage") && strcmp(argv[1], "uas"))) {
+ printf("Please specify output type: storage or uas.\n");
+ return 1;
+ }
+
+ for (i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].type == TYPE_CLASS)
+ continue;
+ skip = 0;
+ if (unusual_dev_entries[i].flags >= HI32) {
+ for (j = 0; j < i; j++) {
+ if (unusual_dev_entries[j].flags == unusual_dev_entries[i].flags &&
+ unusual_dev_entries[j].set == FLAGS_SET) {
+ skip = 1;
+ idx_old = unusual_dev_entries[j].idx;
+ break;
+ }
+ }
+ if (skip) {
+ unusual_dev_entries[i].idx = idx_old;
+ unusual_dev_entries[i].set = FLAGS_DUPLICATE;
+ } else {
+ unusual_dev_entries[i].idx = idx;
+ unusual_dev_entries[i].set = FLAGS_SET;
+ idx++;
+ }
+ }
+ }
+
+ 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..fe904d3072ec 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -54,12 +54,16 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
static int uas_use_uas_driver(struct usb_interface *intf,
const struct usb_device_id *id,
+ const u64 *drv_info_u64_table,
+ unsigned long drv_info_u64_table_size,
u64 *flags_ret)
{
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_drv_info_to_flags(drv_info_u64_table,
+ drv_info_u64_table_size,
+ 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..5b5dc8afda11 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -26,9 +26,13 @@
#include <scsi/scsi_host.h>
#include <scsi/scsi_tcq.h>
+#include "usb-ids.h"
#include "uas-detect.h"
#include "scsiglue.h"
+/* The table of devices is pre-generated in usb-ids-uas.c */
+#include "usb-ids-uas.c"
+
#define MAX_CMNDS 256
struct uas_dev_info {
@@ -909,22 +913,6 @@ 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
-
static int uas_switch_interface(struct usb_device *udev,
struct usb_interface *intf)
{
@@ -990,7 +978,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
struct usb_device *udev = interface_to_usbdev(intf);
u64 dev_flags;
- if (!uas_use_uas_driver(intf, id, &dev_flags))
+ if (!uas_use_uas_driver(intf, id, usb_uas_drv_info_u64_table,
+ usb_uas_drv_info_u64_table_size, &dev_flags))
return -ENODEV;
if (uas_switch_interface(udev, intf))
diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h
new file mode 100644
index 000000000000..286d620d18a4
--- /dev/null
+++ b/drivers/usb/storage/usb-ids.h
@@ -0,0 +1,43 @@
+/* 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[];
+
+#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)
+{
+ 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
+
+#endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index d1ad6a2509ab..1e564ea52fc5 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -56,6 +56,7 @@
#include "sierra_ms.h"
#include "option_ms.h"
+#include "usb-ids.h"
#if IS_ENABLED(CONFIG_USB_UAS)
#include "uas-detect.h"
#endif
@@ -574,7 +575,7 @@ EXPORT_SYMBOL_GPL(usb_stor_adjust_quirks);
/* Get the unusual_devs entries and the string descriptors */
static int get_device_info(struct us_data *us, const struct usb_device_id *id,
- const struct us_unusual_dev *unusual_dev)
+ const struct us_unusual_dev *unusual_dev, int fflags_use_index)
{
struct usb_device *dev = us->pusb_dev;
struct usb_interface_descriptor *idesc =
@@ -589,7 +590,11 @@ 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_drv_info_to_flags(usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, id->driver_info);
+ else
+ us->fflags = id->driver_info;
usb_stor_adjust_quirks(us->pusb_dev, &us->fflags);
if (us->fflags & US_FL_IGNORE_DEVICE) {
@@ -921,11 +926,12 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
}
/* First part of general USB mass-storage probing */
-int usb_stor_probe1(struct us_data **pus,
+static int usb_stor_probe1_fflags(struct us_data **pus,
struct usb_interface *intf,
const struct usb_device_id *id,
const struct us_unusual_dev *unusual_dev,
- const struct scsi_host_template *sht)
+ const struct scsi_host_template *sht,
+ int fflags_use_index)
{
struct Scsi_Host *host;
struct us_data *us;
@@ -962,7 +968,7 @@ int usb_stor_probe1(struct us_data **pus,
goto BadDevice;
/* Get the unusual_devs entries and the descriptors */
- result = get_device_info(us, id, unusual_dev);
+ result = get_device_info(us, id, unusual_dev, fflags_use_index);
if (result)
goto BadDevice;
@@ -981,6 +987,15 @@ int usb_stor_probe1(struct us_data **pus,
release_everything(us);
return result;
}
+
+int usb_stor_probe1(struct us_data **pus,
+ struct usb_interface *intf,
+ const struct usb_device_id *id,
+ const struct us_unusual_dev *unusual_dev,
+ const struct scsi_host_template *sht)
+{
+ return usb_stor_probe1_fflags(pus, intf, id, unusual_dev, sht, 0);
+}
EXPORT_SYMBOL_GPL(usb_stor_probe1);
/* Second part of general USB mass-storage probing */
@@ -1090,7 +1105,8 @@ static int storage_probe(struct usb_interface *intf,
/* If uas is enabled and this device can do uas then ignore it. */
#if IS_ENABLED(CONFIG_USB_UAS)
- if (uas_use_uas_driver(intf, id, NULL))
+ if (uas_use_uas_driver(intf, id, usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, NULL))
return -ENXIO;
#endif
@@ -1119,8 +1135,8 @@ static int storage_probe(struct usb_interface *intf,
id->idVendor, id->idProduct);
}
- result = usb_stor_probe1(&us, intf, id, unusual_dev,
- &usb_stor_host_template);
+ result = usb_stor_probe1_fflags(&us, intf, id, unusual_dev,
+ &usb_stor_host_template, 1);
if (result)
return result;
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index a26029e43dfd..40ef861dbd08 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 usb-ids.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 "usb-ids.c"
/*
* The table of devices to ignore
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v5] usb-storage,uas: use host helper to generate driver info
2023-11-03 20:17 ` [PATCH v5] " Milan Broz
@ 2023-11-03 20:30 ` Alan Stern
2023-11-04 8:01 ` Milan Broz
2023-11-05 18:20 ` [PATCH v6] " Milan Broz
1 sibling, 1 reply; 46+ messages in thread
From: Alan Stern @ 2023-11-03 20:30 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On Fri, Nov 03, 2023 at 09:17:09PM +0100, 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>
> ---
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5] usb-storage,uas: use host helper to generate driver info
2023-11-03 20:30 ` Alan Stern
@ 2023-11-04 8:01 ` Milan Broz
2023-11-04 14:12 ` Alan Stern
0 siblings, 1 reply; 46+ messages in thread
From: Milan Broz @ 2023-11-04 8:01 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On 11/3/23 21:30, Alan Stern wrote:
> On Fri, Nov 03, 2023 at 09:17:09PM +0100, 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>
>> ---
>
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Thanks.
Unfortunately, the build rules, I removed in this version, are needed, see
https://lore.kernel.org/oe-kbuild-all/202311041507.AOYwj5tK-lkp@intel.com/
I'll keep fixed version in my branch on kernel.org for a day and once
there are no such bot reports, new version v6 appears in the list.
Thanks,
Milan
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5] usb-storage,uas: use host helper to generate driver info
2023-11-04 8:01 ` Milan Broz
@ 2023-11-04 14:12 ` Alan Stern
0 siblings, 0 replies; 46+ messages in thread
From: Alan Stern @ 2023-11-04 14:12 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On Sat, Nov 04, 2023 at 09:01:44AM +0100, Milan Broz wrote:
> Thanks.
>
> Unfortunately, the build rules, I removed in this version, are needed, see
> https://lore.kernel.org/oe-kbuild-all/202311041507.AOYwj5tK-lkp@intel.com/
Bizarre. I wonder why it worked on my system but not in the test build.
Maybe because I wasn't starting from a clean slate but instead from one
where old versions of the object files already existed.
> I'll keep fixed version in my branch on kernel.org for a day and once
> there are no such bot reports, new version v6 appears in the list.
Okay.
Alan Stern
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v6] usb-storage,uas: use host helper to generate driver info
2023-11-03 20:17 ` [PATCH v5] " Milan Broz
2023-11-03 20:30 ` Alan Stern
@ 2023-11-05 18:20 ` Milan Broz
2024-01-28 1:50 ` Greg KH
1 sibling, 1 reply; 46+ messages in thread
From: Milan Broz @ 2023-11-05 18:20 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
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>
---
Changes from v5
- Reintroduce Makefile dependences as per report
https://lore.kernel.org/oe-kbuild-all/202311041507.AOYwj5tK-lkp@intel.com/
Changes from v4
- Remove uneeded dependences from Makefile
- Fix checkpatch warnings
Changes from v3
- Include changes from Alan Stern review
- Make unusual*.h mkflags dependences implicit in Makefile
- Avoid conditional macros in function definition (usb-ids.h)
Changes from v2
- Rebased to usb-testing tree
- Include changes from Alan Stern and Greg KH reviews (thanks!)
- Remove FORCE from Makefile add explicit dependence on unusual*.h headers
- Avoid use of #ifdef (mkflags.c need -D CONFIG_64BIT=X flag now)
- Use drv_info in functions and variable names (instead of di)
- Use wrapper for usb_stor_probe1(), this simplifies the previous separate
patch (no need to touch other drivers) so it can be merged here directly
- Merge 64bit optimization to this patch too
Changes from v1
- Separate flags generation from OPAL command patchset
(it means there is currently no quirk flag that requires this patch yet)
drivers/usb/storage/Makefile | 31 ++++
drivers/usb/storage/mkflags.c | 242 +++++++++++++++++++++++++++++
drivers/usb/storage/uas-detect.h | 6 +-
drivers/usb/storage/uas.c | 23 +--
drivers/usb/storage/usb-ids.h | 43 +++++
drivers/usb/storage/usb.c | 32 +++-
drivers/usb/storage/usual-tables.c | 23 +--
7 files changed, 353 insertions(+), 47 deletions(-)
create mode 100644 drivers/usb/storage/mkflags.c
create mode 100644 drivers/usb/storage/usb-ids.h
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
+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 > $@
+
+$(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/mkflags.c b/drivers/usb/storage/mkflags.c
new file mode 100644
index 000000000000..1b35d08f43d1
--- /dev/null
+++ b/drivers/usb/storage/mkflags.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * This is host-compiled generator for usb-ids.c (usb-storage)
+ * and usb-ids-uas.c (uas).
+ *
+ * Generated files contain pre-computed 32-bit values, as USB
+ * driver_info (where the value is stored) can be only 32-bit.
+ * The most significant bit means that it is index to 64-bit
+ * pre-computed table named usb_stor_drv_info_u64_table with size
+ * stored in usb_stor_drv_info_u64_table_size (for sanity check).
+ *
+ * Note that usb-storage driver contains also UAS devices, while UAS
+ * driver contains only UAS entries (so there can be duplicates).
+ */
+
+#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>
+
+enum dev_type { TYPE_DEVICE_STORAGE, TYPE_DEVICE_UAS, TYPE_CLASS };
+enum dev_flags_set { FLAGS_NOT_SET, FLAGS_SET, FLAGS_DUPLICATE };
+#define FLAGS_END ((uint64_t)-1)
+
+struct unusual_dev_entry {
+ enum dev_type 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;
+ enum dev_flags_set set;
+ unsigned int idx;
+};
+
+static struct unusual_dev_entry unusual_dev_entries[] = {
+#define USUAL_DEV(useProto, useTrans) \
+{ TYPE_CLASS, useProto, useTrans, 0, 0, 0, 0, 0, FLAGS_NOT_SET, 0 }
+
+#define COMPLIANT_DEV UNUSUAL_DEV
+#define IS_ENABLED(x) 0
+
+/* usb-storage */
+#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"
+#undef UNUSUAL_DEV
+
+/* uas */
+#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"
+#undef UNUSUAL_DEV
+
+/* Terminating entry */
+{ .flags = FLAGS_END }
+};
+#undef USUAL_DEV
+#undef COMPLIANT_DEV
+#undef IS_ENABLED
+
+/* Highest bit indicates it is index to usb_stor_drv_info_u64_table */
+#define HI32 ((uint32_t)(1UL << 31))
+
+static uint64_t get_driver_info(uint64_t flags, unsigned int idx)
+{
+ if (CONFIG_64BIT)
+ return flags;
+
+ if (flags < HI32)
+ return flags;
+
+ /* Use index that will be processed in usb_stor_drv_info_to_flags */
+ 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(enum dev_type type)
+{
+ for (int i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[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%04x, .idProduct =0x%04x, ",
+ unusual_dev_entries[i].idVendor,
+ unusual_dev_entries[i].idProduct);
+ printf(".bcdDevice_lo = 0x%04x, .bcdDevice_hi = 0x%04x, ",
+ unusual_dev_entries[i].bcdDevice_lo,
+ unusual_dev_entries[i].bcdDevice_hi);
+ printf(".driver_info = 0x%llx },\n",
+ get_driver_info(unusual_dev_entries[i].flags,
+ unusual_dev_entries[i].idx));
+ } else if (type == TYPE_CLASS)
+ print_class(unusual_dev_entries[i].bDeviceSubClass,
+ unusual_dev_entries[i].bDeviceProtocol);
+ }
+}
+
+static void print_usb_flags(const char *type)
+{
+ int i, count;
+
+ if (CONFIG_64BIT) {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {};\n", type);
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = 0;\n\n", type);
+ } else {
+ printf("const u64 usb_%s_drv_info_u64_table[] = {\n", type);
+ for (i = 0, count = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].set == FLAGS_SET) {
+ printf("\t/* 0x%02x */ 0x%llx,\n",
+ unusual_dev_entries[i].idx,
+ unusual_dev_entries[i].flags);
+ count++;
+ }
+ }
+ printf("};\n\n");
+ printf("const unsigned long usb_%s_drv_info_u64_table_size = %i;\n\n", type, count);
+ }
+}
+
+static void print_usb_storage(void)
+{
+ printf("#include <linux/usb.h>\n\n");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("stor");
+
+ /* usb_storage_usb_ids */
+ printf("const struct usb_device_id usb_storage_usb_ids[] = {\n");
+
+ /* usb-storage driver devices */
+ print_type(TYPE_DEVICE_STORAGE);
+
+ /* uas driver 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{ } /* 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");
+
+ /* conversion table from 32-bit device_flags to 64-bit flags */
+ print_usb_flags("uas");
+
+ /* uas_usb_ids */
+ printf("const struct usb_device_id uas_usb_ids[] = {\n");
+
+ /* uas driver 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{ } /* 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], "storage") && strcmp(argv[1], "uas"))) {
+ printf("Please specify output type: storage or uas.\n");
+ return 1;
+ }
+
+ for (i = 0; unusual_dev_entries[i].flags != FLAGS_END; i++) {
+ if (unusual_dev_entries[i].type == TYPE_CLASS)
+ continue;
+ skip = 0;
+ if (unusual_dev_entries[i].flags >= HI32) {
+ for (j = 0; j < i; j++) {
+ if (unusual_dev_entries[j].flags == unusual_dev_entries[i].flags &&
+ unusual_dev_entries[j].set == FLAGS_SET) {
+ skip = 1;
+ idx_old = unusual_dev_entries[j].idx;
+ break;
+ }
+ }
+ if (skip) {
+ unusual_dev_entries[i].idx = idx_old;
+ unusual_dev_entries[i].set = FLAGS_DUPLICATE;
+ } else {
+ unusual_dev_entries[i].idx = idx;
+ unusual_dev_entries[i].set = FLAGS_SET;
+ idx++;
+ }
+ }
+ }
+
+ 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..fe904d3072ec 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -54,12 +54,16 @@ static int uas_find_endpoints(struct usb_host_interface *alt,
static int uas_use_uas_driver(struct usb_interface *intf,
const struct usb_device_id *id,
+ const u64 *drv_info_u64_table,
+ unsigned long drv_info_u64_table_size,
u64 *flags_ret)
{
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_drv_info_to_flags(drv_info_u64_table,
+ drv_info_u64_table_size,
+ 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..5b5dc8afda11 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -26,9 +26,13 @@
#include <scsi/scsi_host.h>
#include <scsi/scsi_tcq.h>
+#include "usb-ids.h"
#include "uas-detect.h"
#include "scsiglue.h"
+/* The table of devices is pre-generated in usb-ids-uas.c */
+#include "usb-ids-uas.c"
+
#define MAX_CMNDS 256
struct uas_dev_info {
@@ -909,22 +913,6 @@ 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
-
static int uas_switch_interface(struct usb_device *udev,
struct usb_interface *intf)
{
@@ -990,7 +978,8 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
struct usb_device *udev = interface_to_usbdev(intf);
u64 dev_flags;
- if (!uas_use_uas_driver(intf, id, &dev_flags))
+ if (!uas_use_uas_driver(intf, id, usb_uas_drv_info_u64_table,
+ usb_uas_drv_info_u64_table_size, &dev_flags))
return -ENODEV;
if (uas_switch_interface(udev, intf))
diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h
new file mode 100644
index 000000000000..286d620d18a4
--- /dev/null
+++ b/drivers/usb/storage/usb-ids.h
@@ -0,0 +1,43 @@
+/* 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[];
+
+#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)
+{
+ 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
+
+#endif
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index d1ad6a2509ab..1e564ea52fc5 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -56,6 +56,7 @@
#include "sierra_ms.h"
#include "option_ms.h"
+#include "usb-ids.h"
#if IS_ENABLED(CONFIG_USB_UAS)
#include "uas-detect.h"
#endif
@@ -574,7 +575,7 @@ EXPORT_SYMBOL_GPL(usb_stor_adjust_quirks);
/* Get the unusual_devs entries and the string descriptors */
static int get_device_info(struct us_data *us, const struct usb_device_id *id,
- const struct us_unusual_dev *unusual_dev)
+ const struct us_unusual_dev *unusual_dev, int fflags_use_index)
{
struct usb_device *dev = us->pusb_dev;
struct usb_interface_descriptor *idesc =
@@ -589,7 +590,11 @@ 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_drv_info_to_flags(usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, id->driver_info);
+ else
+ us->fflags = id->driver_info;
usb_stor_adjust_quirks(us->pusb_dev, &us->fflags);
if (us->fflags & US_FL_IGNORE_DEVICE) {
@@ -921,11 +926,12 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
}
/* First part of general USB mass-storage probing */
-int usb_stor_probe1(struct us_data **pus,
+static int usb_stor_probe1_fflags(struct us_data **pus,
struct usb_interface *intf,
const struct usb_device_id *id,
const struct us_unusual_dev *unusual_dev,
- const struct scsi_host_template *sht)
+ const struct scsi_host_template *sht,
+ int fflags_use_index)
{
struct Scsi_Host *host;
struct us_data *us;
@@ -962,7 +968,7 @@ int usb_stor_probe1(struct us_data **pus,
goto BadDevice;
/* Get the unusual_devs entries and the descriptors */
- result = get_device_info(us, id, unusual_dev);
+ result = get_device_info(us, id, unusual_dev, fflags_use_index);
if (result)
goto BadDevice;
@@ -981,6 +987,15 @@ int usb_stor_probe1(struct us_data **pus,
release_everything(us);
return result;
}
+
+int usb_stor_probe1(struct us_data **pus,
+ struct usb_interface *intf,
+ const struct usb_device_id *id,
+ const struct us_unusual_dev *unusual_dev,
+ const struct scsi_host_template *sht)
+{
+ return usb_stor_probe1_fflags(pus, intf, id, unusual_dev, sht, 0);
+}
EXPORT_SYMBOL_GPL(usb_stor_probe1);
/* Second part of general USB mass-storage probing */
@@ -1090,7 +1105,8 @@ static int storage_probe(struct usb_interface *intf,
/* If uas is enabled and this device can do uas then ignore it. */
#if IS_ENABLED(CONFIG_USB_UAS)
- if (uas_use_uas_driver(intf, id, NULL))
+ if (uas_use_uas_driver(intf, id, usb_stor_drv_info_u64_table,
+ usb_stor_drv_info_u64_table_size, NULL))
return -ENXIO;
#endif
@@ -1119,8 +1135,8 @@ static int storage_probe(struct usb_interface *intf,
id->idVendor, id->idProduct);
}
- result = usb_stor_probe1(&us, intf, id, unusual_dev,
- &usb_stor_host_template);
+ result = usb_stor_probe1_fflags(&us, intf, id, unusual_dev,
+ &usb_stor_host_template, 1);
if (result)
return result;
diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c
index a26029e43dfd..40ef861dbd08 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 usb-ids.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 "usb-ids.c"
/*
* The table of devices to ignore
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v6] usb-storage,uas: use host helper to generate driver info
2023-11-05 18:20 ` [PATCH v6] " Milan Broz
@ 2024-01-28 1:50 ` Greg KH
2024-01-29 12:15 ` Milan Broz
0 siblings, 1 reply; 46+ messages in thread
From: Greg KH @ 2024-01-28 1:50 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
On Sun, Nov 05, 2023 at 07:20:47PM +0100, 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>
I see the need for this, but why now? We haven't run out of ids yet
have we? Do we need to add another one?
Also, after building, I get the following files marked by git as needed
to be added to the tree, so perhaps you also need a .gitignore file:
$ git status
On branch work-testing
Untracked files:
(use "git add <file>..." to include in what will be committed)
drivers/usb/storage/mkflags
drivers/usb/storage/usb-ids-uas.c
drivers/usb/storage/usb-ids.c
thanks,
greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v6] usb-storage,uas: use host helper to generate driver info
2024-01-28 1:50 ` Greg KH
@ 2024-01-29 12:15 ` Milan Broz
0 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2024-01-29 12:15 UTC (permalink / raw)
To: Greg KH; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
Hi Greg,
On 1/28/24 02:50, Greg KH wrote:
> On Sun, Nov 05, 2023 at 07:20:47PM +0100, 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>
>
> I see the need for this, but why now? We haven't run out of ids yet
> have we? Do we need to add another one?
I had a patchset that needed a new flag; that was the motivation.
However, after a discussion with SCSI people, it needed to be done differently,
and I had no time to rewrite it yet.
In the meantime, the flag transition patch was split from the series.
If you want to merge it now, I can easily rebase & fix gitignore issue.
Or it can wait for the next first real user.
Just I have spent a lot of time with this, so no need to repeat this exercise, though :-)
Thanks,
Milan
> Also, after building, I get the following files marked by git as needed
> to be added to the tree, so perhaps you also need a .gitignore file:
>
> $ git status
> On branch work-testing
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
> drivers/usb/storage/mkflags
> drivers/usb/storage/usb-ids-uas.c
> drivers/usb/storage/usb-ids.c
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
` (3 preceding siblings ...)
2023-10-16 7:26 ` [PATCH 4/7] usb-storage,uas: use host helper to generate driver info Milan Broz
@ 2023-10-16 7:26 ` Milan Broz
2023-10-21 10:21 ` Greg KH
2023-10-21 10:22 ` Greg KH
2023-10-16 7:26 ` [PATCH 6/7] usb-storage,uas: enable security commands for USB-attached storage Milan Broz
` (2 subsequent siblings)
7 siblings, 2 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:26 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
This patch optimizes the previous one for 64-bit platforms, where
unsigned long is 64-bit, so we do not need to convert quirk flags
to 32-bit index.
Signed-off-by: Milan Broz <gmazyland@gmail.com>
---
drivers/usb/storage/Makefile | 3 +++
drivers/usb/storage/mkflags.c | 9 +++++++++
drivers/usb/storage/usb-ids.h | 4 ++++
3 files changed, 16 insertions(+)
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
index 612678f108d0..62ebaa76ef95 100644
--- a/drivers/usb/storage/Makefile
+++ b/drivers/usb/storage/Makefile
@@ -57,6 +57,9 @@ $(obj)/usual-tables.o: $(obj)/usb-ids.c
$(obj)/uas.o: $(obj)/usb-ids-uas.c
clean-files := usb-ids.c usb-ids-uas.c
HOSTCFLAGS_mkflags.o := -I $(srctree)/include/
+ifdef CONFIG_64BIT
+HOSTCFLAGS_mkflags.o += -D CONFIG_64BIT
+endif
hostprogs += mkflags
quiet_cmd_mkflag_storage = FLAGS $@
diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c
index 2514ffef0154..08c37d2e52d6 100644
--- a/drivers/usb/storage/mkflags.c
+++ b/drivers/usb/storage/mkflags.c
@@ -89,11 +89,15 @@ static struct svals vals[] = {
static unsigned long get_device_info(uint64_t flags, unsigned int idx)
{
+#ifndef CONFIG_64BIT
if (flags < HI32)
return (unsigned long)flags;
/* Use index that will be processed in usb_stor_di2flags */
return HI32 + idx;
+#else
+ return flags;
+#endif
}
static void print_class(uint8_t bDeviceSubClass, uint8_t bDeviceProtocol)
@@ -123,6 +127,10 @@ static void print_type(unsigned int type)
static void print_usb_flags(const char *type)
{
+#ifdef CONFIG_64BIT
+ printf("const u64 usb_%s_di_to_u64[] = {};\n", type);
+ printf("const unsigned long usb_%s_di_idx_size = 0;\n\n", type);
+#else
int i, count;
printf("const u64 usb_%s_di_to_u64[] = {\n", type);
@@ -134,6 +142,7 @@ static void print_usb_flags(const char *type)
}
printf("};\n\n");
printf("const unsigned long usb_%s_di_idx_size = %i;\n\n", type, count);
+#endif
}
static void print_usb_storage(void)
diff --git a/drivers/usb/storage/usb-ids.h b/drivers/usb/storage/usb-ids.h
index 8bfd84e07f96..4abe1a6d3a66 100644
--- a/drivers/usb/storage/usb-ids.h
+++ b/drivers/usb/storage/usb-ids.h
@@ -15,6 +15,9 @@ extern const u64 usb_uas_di_to_u64[];
static u64 usb_stor_di2flags(const u64 *di_to_u64,
unsigned long idx_size, unsigned long idx)
{
+#if IS_ENABLED(CONFIG_64BIT)
+ return idx;
+#else
u64 flags = 0;
if (idx < (1UL << 31))
@@ -28,6 +31,7 @@ static u64 usb_stor_di2flags(const u64 *di_to_u64,
WARN_ONCE(true, "usb_stor_di_to_u64 table not updated");
return flags;
+#endif
}
#endif
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms
2023-10-16 7:26 ` [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms Milan Broz
@ 2023-10-21 10:21 ` Greg KH
2023-10-21 10:22 ` Greg KH
1 sibling, 0 replies; 46+ messages in thread
From: Greg KH @ 2023-10-21 10:21 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
On Mon, Oct 16, 2023 at 09:26:02AM +0200, Milan Broz wrote:
> This patch optimizes the previous one for 64-bit platforms,
What is "previous one"? We don't know that when we go and look at the
changelog in the future.
> where
> unsigned long is 64-bit, so we do not need to convert quirk flags
> to 32-bit index.
>
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
Why not just do it properly the first time? You are fixing up a patch
that you added, which should not be needed.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms
2023-10-16 7:26 ` [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms Milan Broz
2023-10-21 10:21 ` Greg KH
@ 2023-10-21 10:22 ` Greg KH
1 sibling, 0 replies; 46+ messages in thread
From: Greg KH @ 2023-10-21 10:22 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, stern, oneukum
On Mon, Oct 16, 2023 at 09:26:02AM +0200, Milan Broz wrote:
> This patch optimizes the previous one for 64-bit platforms, where
> unsigned long is 64-bit, so we do not need to convert quirk flags
> to 32-bit index.
>
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> ---
> drivers/usb/storage/Makefile | 3 +++
> drivers/usb/storage/mkflags.c | 9 +++++++++
> drivers/usb/storage/usb-ids.h | 4 ++++
> 3 files changed, 16 insertions(+)
>
> diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
> index 612678f108d0..62ebaa76ef95 100644
> --- a/drivers/usb/storage/Makefile
> +++ b/drivers/usb/storage/Makefile
> @@ -57,6 +57,9 @@ $(obj)/usual-tables.o: $(obj)/usb-ids.c
> $(obj)/uas.o: $(obj)/usb-ids-uas.c
> clean-files := usb-ids.c usb-ids-uas.c
> HOSTCFLAGS_mkflags.o := -I $(srctree)/include/
> +ifdef CONFIG_64BIT
> +HOSTCFLAGS_mkflags.o += -D CONFIG_64BIT
> +endif
> hostprogs += mkflags
>
> quiet_cmd_mkflag_storage = FLAGS $@
> diff --git a/drivers/usb/storage/mkflags.c b/drivers/usb/storage/mkflags.c
> index 2514ffef0154..08c37d2e52d6 100644
> --- a/drivers/usb/storage/mkflags.c
> +++ b/drivers/usb/storage/mkflags.c
> @@ -89,11 +89,15 @@ static struct svals vals[] = {
>
> static unsigned long get_device_info(uint64_t flags, unsigned int idx)
> {
> +#ifndef CONFIG_64BIT
> if (flags < HI32)
> return (unsigned long)flags;
>
> /* Use index that will be processed in usb_stor_di2flags */
> return HI32 + idx;
> +#else
> + return flags;
> +#endif
Please try to keep #ifdef out of .c files, it makes maintenance a real
pain and is not the kernel coding style at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 6/7] usb-storage,uas: enable security commands for USB-attached storage
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
` (4 preceding siblings ...)
2023-10-16 7:26 ` [PATCH 5/7] usb-storage,uas: do not convert device_info for 64-bit platforms Milan Broz
@ 2023-10-16 7:26 ` Milan Broz
2023-10-16 7:26 ` [PATCH 7/7] usb-storage,uas: disable security commands (OPAL) for RT9210 chip family Milan Broz
2023-10-16 17:33 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Alan Stern
7 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:26 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
This patch enables security commands (currently mostly OPAL hardware
encryption) for UAS and usb-storage drivers.
The SCSI layer uses security commands for the initial OPAL support
check (discovery command) and for in-kernel sed-ioctl interface.
Some adapters support these commands, but firmware can be buggy or
implemented incorrectly; the patch also adds a new quirk IGNORE_OPAL
to disable security commands for particular devices.
If adapters do not implement needed commands (ATA-12 pass-thru),
the commands are rejected, and OPAL support remains disabled.
(This is how it already works if OPAL command is sent from userspace
directly, like in sedutils.)
Signed-off-by: Milan Broz <gmazyland@gmail.com>
---
Documentation/admin-guide/kernel-parameters.txt | 2 ++
drivers/usb/storage/scsiglue.c | 4 ++++
drivers/usb/storage/uas.c | 5 +++++
drivers/usb/storage/usb.c | 5 ++++-
include/linux/usb_usual.h | 2 ++
5 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..e3f072cbb833 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6885,6 +6885,8 @@
y = ALWAYS_SYNC (issue a SYNCHRONIZE_CACHE
even if the device claims no cache,
not on uas)
+ z = IGNORE_OPAL (the device security commands
+ (OPAL) are broken, do not enable them);
Example: quirks=0419:aaf5:rl,0421:0433:rc
user_debug= [KNL,ARM]
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index c54e9805da53..ef93813a2049 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -209,6 +209,10 @@ static int slave_configure(struct scsi_device *sdev)
/* Do not attempt to use WRITE SAME */
sdev->no_write_same = 1;
+ /* Allow security commands (OPAL) passthrough */
+ if (!(us->fflags & US_FL_IGNORE_OPAL))
+ sdev->security_supported = 1;
+
/*
* Some disks return the total number of blocks in response
* to READ CAPACITY rather than the highest block number.
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 8a1c4449dcc9..8967767d6753 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -865,6 +865,11 @@ static int uas_slave_configure(struct scsi_device *sdev)
/* Some disks cannot handle WRITE_SAME */
if (devinfo->flags & US_FL_NO_SAME)
sdev->no_write_same = 1;
+
+ /* Allow security commands (OPAL) passthrough */
+ if (!(devinfo->flags & US_FL_IGNORE_OPAL))
+ sdev->security_supported = 1;
+
/*
* Some disks return the total number of blocks in response
* to READ CAPACITY rather than the highest block number.
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index bb48ab1bd461..3facc80292d7 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -477,7 +477,7 @@ void usb_stor_adjust_quirks(struct usb_device *udev, u64 *fflags)
US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
US_FL_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
US_FL_MAX_SECTORS_240 | US_FL_NO_REPORT_LUNS |
- US_FL_ALWAYS_SYNC);
+ US_FL_ALWAYS_SYNC | US_FL_IGNORE_OPAL);
p = quirks;
while (*p) {
@@ -566,6 +566,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, u64 *fflags)
case 'y':
f |= US_FL_ALWAYS_SYNC;
break;
+ case 'z':
+ f |= US_FL_IGNORE_OPAL;
+ break;
/* Ignore unrecognized flag characters */
}
}
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index 712363c7a2e8..0181c94d7d91 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -88,6 +88,8 @@
/* Cannot handle WRITE_SAME */ \
US_FLAG(SENSE_AFTER_SYNC, 0x80000000) \
/* Do REQUEST_SENSE after SYNCHRONIZE_CACHE */ \
+ US_FLAG(IGNORE_OPAL, 0x100000000) \
+ /* Security commands (OPAL) are broken */ \
#define US_FLAG(name, value) US_FL_##name = value ,
enum { US_DO_ALL_FLAGS };
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 7/7] usb-storage,uas: disable security commands (OPAL) for RT9210 chip family
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
` (5 preceding siblings ...)
2023-10-16 7:26 ` [PATCH 6/7] usb-storage,uas: enable security commands for USB-attached storage Milan Broz
@ 2023-10-16 7:26 ` Milan Broz
2023-10-16 17:33 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Alan Stern
7 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-16 7:26 UTC (permalink / raw)
To: linux-usb; +Cc: usb-storage, linux-scsi, stern, gregkh, oneukum, Milan Broz
Realtek 9210 family (NVME to USB bridge) adapters always set
the write-protected bit for the whole drive if an OPAL locking range
is defined (even if the OPAL locking range just covers part of the disk).
The only way to recover is PSID reset and physical reconnection of the device.
This looks like a wrong implementation of OPAL standard (and I will try
to report it to Realtek as it happens for all firmware versions I have),
but for now, these adapters are unusable for OPAL.
Signed-off-by: Milan Broz <gmazyland@gmail.com>
---
drivers/usb/storage/unusual_devs.h | 11 +++++++++++
drivers/usb/storage/unusual_uas.h | 11 +++++++++++
2 files changed, 22 insertions(+)
diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 20dcbccb290b..b32afded85ae 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -1476,6 +1476,17 @@ UNUSUAL_DEV( 0x0bc2, 0x3332, 0x0000, 0x9999,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_WP_DETECT ),
+/*
+ * Realtek 9210 family set global write-protection flag
+ * for any OPAL locking range making device unusable
+ * Reported-by: Milan Broz <gmazyland@gmail.com>
+ */
+UNUSUAL_DEV( 0x0bda, 0x9210, 0x0000, 0xffff,
+ "Realtek",
+ "USB to NVMe/SATA bridge",
+ USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+ US_FL_IGNORE_OPAL),
+
UNUSUAL_DEV( 0x0d49, 0x7310, 0x0000, 0x9999,
"Maxtor",
"USB to SATA",
diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
index 1f8c9b16a0fb..8a32bb1654ed 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -83,6 +83,17 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_REPORT_LUNS),
+/*
+ * Realtek 9210 family set global write-protection flag
+ * for any OPAL locking range making device unusable
+ * Reported-by: Milan Broz <gmazyland@gmail.com>
+ */
+UNUSUAL_DEV(0x0bda, 0x9210, 0x0000, 0xffff,
+ "Realtek",
+ "USB to NVMe/SATA bridge",
+ USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+ US_FL_IGNORE_OPAL),
+
/* Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> */
UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999,
"Initio Corporation",
--
2.42.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices.
2023-10-16 7:25 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Milan Broz
` (6 preceding siblings ...)
2023-10-16 7:26 ` [PATCH 7/7] usb-storage,uas: disable security commands (OPAL) for RT9210 chip family Milan Broz
@ 2023-10-16 17:33 ` Alan Stern
2023-10-16 17:48 ` Milan Broz
7 siblings, 1 reply; 46+ messages in thread
From: Alan Stern @ 2023-10-16 17:33 UTC (permalink / raw)
To: Milan Broz; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On Mon, Oct 16, 2023 at 09:25:57AM +0200, Milan Broz wrote:
> This patchset adds support for OPAL commands (self-encrypted drives)
> through USB-attached storage (usb-storage and UAS drivers).
This is version 2 of the proposed patch set, but you didn't include the
version number in the email Subject: lines and you didn't include the
summary of differences from v1 below the "---" lines of the various
patches.
Patches 5, 6, and 7 look okay. You can add my Reviewed-by: to each of
them.
I've got some additional comments on patch 4 (in a separate email).
Alan Stern
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices.
2023-10-16 17:33 ` [PATCH 0/7] usb-storage,uas: Support OPAL commands on USB attached devices Alan Stern
@ 2023-10-16 17:48 ` Milan Broz
0 siblings, 0 replies; 46+ messages in thread
From: Milan Broz @ 2023-10-16 17:48 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, usb-storage, linux-scsi, gregkh, oneukum
On 10/16/23 19:33, Alan Stern wrote:
> On Mon, Oct 16, 2023 at 09:25:57AM +0200, Milan Broz wrote:
>> This patchset adds support for OPAL commands (self-encrypted drives)
>> through USB-attached storage (usb-storage and UAS drivers).
>
> This is version 2 of the proposed patch set, but you didn't include the
> version number in the email Subject: lines and you didn't include the
> summary of differences from v1 below the "---" lines of the various
> patches.
Hi,
well, the first patchset was RFC, so I sent is as "the first real version".
Perhaps not the correct way, sorry for that.
Anyway, if you see the discussion about OPAL change on SCSI list, another
solution (inside USB storage driver) is needed.
So, please ignore patch 6/7, these will be needed, but I have to rewrite
SCSI logic to USB glue/UAS driver.
But for the generic 64-bit flags (patch 1-5), if you see this useful, please review it.
Common requirement is that kernel patch need an user for merge
(and my flag is currently no going to be used without rewrite).
But that time will come one day, and if I can save people time to reinvent
the 64-bit quirks logic, it would be nice to merge it.
Thanks,
Milan
>
> Patches 5, 6, and 7 look okay. You can add my Reviewed-by: to each of
> them.
>
> I've got some additional comments on patch 4 (in a separate email).
>
> Alan Stern
^ permalink raw reply [flat|nested] 46+ messages in thread