On Fri, 31 Jul 2015, Giulio Bernardi wrote:
> P.S: it looks like scsi_dev_info_list_del_keyed() shares the same code, so maybe
> it should be modified too?
You're right. I'll split this into two patches: one to remove the
duplicate code, and one to fix the matching. The patches are attached.
Please check that I haven't made any errors.
Alan Stern
drivers/scsi/scsi_devinfo.c | 114 ++++++++++++++++----------------------------
1 file changed, 42 insertions(+), 72 deletions(-)
Index: usb-4.1/drivers/scsi/scsi_devinfo.c
===================================================================
--- usb-4.1.orig/drivers/scsi/scsi_devinfo.c
+++ usb-4.1/drivers/scsi/scsi_devinfo.c
@@ -390,25 +390,26 @@ int scsi_dev_info_list_add_keyed(int com
EXPORT_SYMBOL(scsi_dev_info_list_add_keyed);
/**
- * scsi_dev_info_list_del_keyed - remove one dev_info list entry.
+ * scsi_dev_info_list_find - find a matching dev_info list entry.
* @vendor: vendor string
* @model: model (product) string
* @key: specify list to use
*
* Description:
- * Remove and destroy one dev_info entry for @vendor, @model
+ * Finds the first dev_info entry matching @vendor, @model
* in list specified by @key.
*
- * Returns: 0 OK, -error on failure.
+ * Returns: pointer to matching entry, or ERR_PTR on failure.
**/
-int scsi_dev_info_list_del_keyed(char *vendor, char *model, int key)
+static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
+ const char *model, int key)
{
- struct scsi_dev_info_list *devinfo, *found = NULL;
+ struct scsi_dev_info_list *devinfo;
struct scsi_dev_info_list_table *devinfo_table =
scsi_devinfo_lookup_by_key(key);
if (IS_ERR(devinfo_table))
- return PTR_ERR(devinfo_table);
+ return (struct scsi_dev_info_list *) devinfo_table;
list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
dev_info_list) {
@@ -452,25 +453,42 @@ int scsi_dev_info_list_del_keyed(char *v
if (memcmp(devinfo->model, model,
min(max, strlen(devinfo->model))))
continue;
- found = devinfo;
+ return devinfo;
} else {
if (!memcmp(devinfo->vendor, vendor,
sizeof(devinfo->vendor)) &&
!memcmp(devinfo->model, model,
sizeof(devinfo->model)))
- found = devinfo;
+ return devinfo;
}
- if (found)
- break;
}
- if (found) {
- list_del(&found->dev_info_list);
- kfree(found);
- return 0;
- }
+ return ERR_PTR(-ENOENT);
+}
+
+/**
+ * scsi_dev_info_list_del_keyed - remove one dev_info list entry.
+ * @vendor: vendor string
+ * @model: model (product) string
+ * @key: specify list to use
+ *
+ * Description:
+ * Remove and destroy one dev_info entry for @vendor, @model
+ * in list specified by @key.
+ *
+ * Returns: 0 OK, -error on failure.
+ **/
+int scsi_dev_info_list_del_keyed(char *vendor, char *model, int key)
+{
+ struct scsi_dev_info_list *found;
- return -ENOENT;
+ found = scsi_dev_info_list_find(vendor, model, key);
+ if (IS_ERR(found))
+ return PTR_ERR(found);
+
+ list_del(&found->dev_info_list);
+ kfree(found);
+ return 0;
}
EXPORT_SYMBOL(scsi_dev_info_list_del_keyed);
@@ -565,64 +583,16 @@ int scsi_get_device_flags_keyed(struct s
int key)
{
struct scsi_dev_info_list *devinfo;
- struct scsi_dev_info_list_table *devinfo_table;
+ int err;
- devinfo_table = scsi_devinfo_lookup_by_key(key);
+ devinfo = scsi_dev_info_list_find(vendor, model, key);
+ if (!IS_ERR(devinfo))
+ return devinfo->flags;
+
+ err = PTR_ERR(devinfo);
+ if (err != -ENOENT)
+ return err;
- if (IS_ERR(devinfo_table))
- return PTR_ERR(devinfo_table);
-
- list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
- dev_info_list) {
- if (devinfo->compatible) {
- /*
- * Behave like the older version of get_device_flags.
- */
- size_t max;
- /*
- * XXX why skip leading spaces? If an odd INQUIRY
- * value, that should have been part of the
- * scsi_static_device_list[] entry, such as " FOO"
- * rather than "FOO". Since this code is already
- * here, and we don't know what device it is
- * trying to work with, leave it as-is.
- */
- max = 8; /* max length of vendor */
- while ((max > 0) && *vendor == ' ') {
- max--;
- vendor++;
- }
- /*
- * XXX removing the following strlen() would be
- * good, using it means that for a an entry not in
- * the list, we scan every byte of every vendor
- * listed in scsi_static_device_list[], and never match
- * a single one (and still have to compare at
- * least the first byte of each vendor).
- */
- if (memcmp(devinfo->vendor, vendor,
- min(max, strlen(devinfo->vendor))))
- continue;
- /*
- * Skip spaces again.
- */
- max = 16; /* max length of model */
- while ((max > 0) && *model == ' ') {
- max--;
- model++;
- }
- if (memcmp(devinfo->model, model,
- min(max, strlen(devinfo->model))))
- continue;
- return devinfo->flags;
- } else {
- if (!memcmp(devinfo->vendor, vendor,
- sizeof(devinfo->vendor)) &&
- !memcmp(devinfo->model, model,
- sizeof(devinfo->model)))
- return devinfo->flags;
- }
- }
/* nothing found, return nothing */
if (key != SCSI_DEVINFO_GLOBAL)
return 0;
drivers/scsi/scsi_devinfo.c | 70 ++++++++++++++++++++++----------------------
1 file changed, 36 insertions(+), 34 deletions(-)
Index: usb-4.1/drivers/scsi/scsi_devinfo.c
===================================================================
--- usb-4.1.orig/drivers/scsi/scsi_devinfo.c
+++ usb-4.1/drivers/scsi/scsi_devinfo.c
@@ -407,51 +407,53 @@ static struct scsi_dev_info_list *scsi_d
struct scsi_dev_info_list *devinfo;
struct scsi_dev_info_list_table *devinfo_table =
scsi_devinfo_lookup_by_key(key);
+ size_t vmax, mmax;
+ const char *vskip;
+ const char *mskip = model;
if (IS_ERR(devinfo_table))
return (struct scsi_dev_info_list *) devinfo_table;
+ /* Prepare for "compatible" matches */
+
+ /*
+ * XXX why skip leading spaces? If an odd INQUIRY
+ * value, that should have been part of the
+ * scsi_static_device_list[] entry, such as " FOO"
+ * rather than "FOO". Since this code is already
+ * here, and we don't know what device it is
+ * trying to work with, leave it as-is.
+ */
+ vmax = 8; /* max length of vendor */
+ vskip = vendor;
+ while (vmax > 0 && *vskip == ' ') {
+ vmax--;
+ vskip++;
+ }
+ /* Also skip trailing spaces */
+ while (vmax > 0 && vskip[vmax - 1] == ' ')
+ --vmax;
+
+ mmax = 16; /* max length of model */
+ mskip = model;
+ while (mmax > 0 && *mskip == ' ') {
+ mmax--;
+ mskip++;
+ }
+ while (mmax > 0 && mskip[mmax - 1] == ' ')
+ --mmax;
+
list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
dev_info_list) {
if (devinfo->compatible) {
/*
* Behave like the older version of get_device_flags.
*/
- size_t max;
- /*
- * XXX why skip leading spaces? If an odd INQUIRY
- * value, that should have been part of the
- * scsi_static_device_list[] entry, such as " FOO"
- * rather than "FOO". Since this code is already
- * here, and we don't know what device it is
- * trying to work with, leave it as-is.
- */
- max = 8; /* max length of vendor */
- while ((max > 0) && *vendor == ' ') {
- max--;
- vendor++;
- }
- /*
- * XXX removing the following strlen() would be
- * good, using it means that for a an entry not in
- * the list, we scan every byte of every vendor
- * listed in scsi_static_device_list[], and never match
- * a single one (and still have to compare at
- * least the first byte of each vendor).
- */
- if (memcmp(devinfo->vendor, vendor,
- min(max, strlen(devinfo->vendor))))
+ if (memcmp(devinfo->vendor, vskip, vmax) ||
+ devinfo->vendor[vmax])
continue;
- /*
- * Skip spaces again.
- */
- max = 16; /* max length of model */
- while ((max > 0) && *model == ' ') {
- max--;
- model++;
- }
- if (memcmp(devinfo->model, model,
- min(max, strlen(devinfo->model))))
+ if (memcmp(devinfo->model, mskip, mmax) ||
+ devinfo->model[mmax])
continue;
return devinfo;
} else {
