* [PATCH 1/2] Add scsi_dev_info_list_del_keyed() @ 2011-01-06 20:31 Peter Jones 2011-01-06 20:31 ` [PATCH 2/2] Use scsi_devinfo functions to do matching of scsi IDs device_handler tables Peter Jones 0 siblings, 1 reply; 4+ messages in thread From: Peter Jones @ 2011-01-06 20:31 UTC (permalink / raw) To: James Bottomley; +Cc: Tejun Heo, Linux SCSI, Chandra Seetharaman, Peter Jones For scsi_dh.c to use devinfo lists, we have to be able to remove entries before rmmod. --- drivers/scsi/scsi_devinfo.c | 85 +++++++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_priv.h | 1 + 2 files changed, 86 insertions(+), 0 deletions(-) diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c index 43fad4c..82e9e5c 100644 --- a/drivers/scsi/scsi_devinfo.c +++ b/drivers/scsi/scsi_devinfo.c @@ -382,6 +382,91 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model, EXPORT_SYMBOL(scsi_dev_info_list_add_keyed); /** + * 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 *devinfo, *found = NULL; + struct scsi_dev_info_list_table *devinfo_table = + scsi_devinfo_lookup_by_key(key); + + 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; + found = devinfo; + } else { + if (!memcmp(devinfo->vendor, vendor, + sizeof(devinfo->vendor)) && + !memcmp(devinfo->model, model, + sizeof(devinfo->model))) + found = devinfo; + } + if (found) + break; + } + + if (found) { + list_del(&found->dev_info_list); + kfree(found); + return 0; + } + + return -ENOENT; +} +EXPORT_SYMBOL(scsi_dev_info_list_del_keyed); + +/** * scsi_dev_info_list_add_str - parse dev_list and add to the scsi_dev_info_list. * @dev_list: string of device flags to add * diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h index b4056d1..ce9e0ad 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -56,6 +56,7 @@ extern int scsi_get_device_flags_keyed(struct scsi_device *sdev, extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model, char *strflags, int flags, int key); +extern int scsi_dev_info_list_del_keyed(char *vendor, char *model, int key); extern int scsi_dev_info_add_list(int key, const char *name); extern int scsi_dev_info_remove_list(int key); -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] Use scsi_devinfo functions to do matching of scsi IDs device_handler tables. 2011-01-06 20:31 [PATCH 1/2] Add scsi_dev_info_list_del_keyed() Peter Jones @ 2011-01-06 20:31 ` Peter Jones 2011-01-06 20:36 ` Peter Jones 0 siblings, 1 reply; 4+ messages in thread From: Peter Jones @ 2011-01-06 20:31 UTC (permalink / raw) To: James Bottomley; +Cc: Tejun Heo, Linux SCSI, Chandra Seetharaman, Peter Jones Previously we were using strncmp in order to avoid having to include whitespace in the devlist, but this means "HSV1000" matches a device list entry that says "HSV100", which is wrong. This patch changes scsi_dh.c to use scsi_devinfo's matching functions instead, since they handle these cases correctly. --- drivers/scsi/device_handler/scsi_dh.c | 112 +++++++++++---------------------- drivers/scsi/scsi_priv.h | 1 + include/scsi/scsi_device.h | 1 + 3 files changed, 39 insertions(+), 75 deletions(-) diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c index 6fae3d2..9561e50 100644 --- a/drivers/scsi/device_handler/scsi_dh.c +++ b/drivers/scsi/device_handler/scsi_dh.c @@ -25,16 +25,9 @@ #include <scsi/scsi_dh.h> #include "../scsi_priv.h" -struct scsi_dh_devinfo_list { - struct list_head node; - char vendor[9]; - char model[17]; - struct scsi_device_handler *handler; -}; - static DEFINE_SPINLOCK(list_lock); static LIST_HEAD(scsi_dh_list); -static LIST_HEAD(scsi_dh_dev_list); +static int scsi_dh_list_idx = 1; static struct scsi_device_handler *get_device_handler(const char *name) { @@ -51,40 +44,18 @@ static struct scsi_device_handler *get_device_handler(const char *name) return found; } - -static struct scsi_device_handler * -scsi_dh_cache_lookup(struct scsi_device *sdev) +static struct scsi_device_handler *get_device_handler_by_idx(int idx) { - struct scsi_dh_devinfo_list *tmp; - struct scsi_device_handler *found_dh = NULL; + struct scsi_device_handler *tmp, *found = NULL; spin_lock(&list_lock); - list_for_each_entry(tmp, &scsi_dh_dev_list, node) { - if (!strncmp(sdev->vendor, tmp->vendor, strlen(tmp->vendor)) && - !strncmp(sdev->model, tmp->model, strlen(tmp->model))) { - found_dh = tmp->handler; + list_for_each_entry(tmp, &scsi_dh_list, list) { + if (tmp->idx == idx) { + found = tmp; break; } } spin_unlock(&list_lock); - - return found_dh; -} - -static int scsi_dh_handler_lookup(struct scsi_device_handler *scsi_dh, - struct scsi_device *sdev) -{ - int i, found = 0; - - for(i = 0; scsi_dh->devlist[i].vendor; i++) { - if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor, - strlen(scsi_dh->devlist[i].vendor)) && - !strncmp(sdev->model, scsi_dh->devlist[i].model, - strlen(scsi_dh->devlist[i].model))) { - found = 1; - break; - } - } return found; } @@ -102,41 +73,14 @@ device_handler_match(struct scsi_device_handler *scsi_dh, struct scsi_device *sdev) { struct scsi_device_handler *found_dh = NULL; - struct scsi_dh_devinfo_list *tmp; + int idx; - found_dh = scsi_dh_cache_lookup(sdev); - if (found_dh) - return found_dh; + idx = scsi_get_device_flags_keyed(sdev, sdev->vendor, sdev->model, + SCSI_DEVINFO_DH); + found_dh = get_device_handler_by_idx(idx); - if (scsi_dh) { - if (scsi_dh_handler_lookup(scsi_dh, sdev)) - found_dh = scsi_dh; - } else { - struct scsi_device_handler *tmp_dh; - - spin_lock(&list_lock); - list_for_each_entry(tmp_dh, &scsi_dh_list, list) { - if (scsi_dh_handler_lookup(tmp_dh, sdev)) - found_dh = tmp_dh; - } - spin_unlock(&list_lock); - } - - if (found_dh) { /* If device is found, add it to the cache */ - tmp = kmalloc(sizeof(*tmp), GFP_KERNEL); - if (tmp) { - strncpy(tmp->vendor, sdev->vendor, 8); - strncpy(tmp->model, sdev->model, 16); - tmp->vendor[8] = '\0'; - tmp->model[16] = '\0'; - tmp->handler = found_dh; - spin_lock(&list_lock); - list_add(&tmp->node, &scsi_dh_dev_list); - spin_unlock(&list_lock); - } else { - found_dh = NULL; - } - } + if (scsi_dh && found_dh != scsi_dh) + found_dh = NULL; return found_dh; } @@ -373,12 +317,25 @@ static int scsi_dh_notifier_remove(struct device *dev, void *data) */ int scsi_register_device_handler(struct scsi_device_handler *scsi_dh) { + int i; + if (get_device_handler(scsi_dh->name)) return -EBUSY; spin_lock(&list_lock); + scsi_dh->idx = scsi_dh_list_idx++; list_add(&scsi_dh->list, &scsi_dh_list); spin_unlock(&list_lock); + + for (i = 0; scsi_dh->devlist[i].vendor; i++) { + scsi_dev_info_list_add_keyed(1, /* compatible */ + scsi_dh->devlist[i].vendor, + scsi_dh->devlist[i].model, + NULL, + scsi_dh->idx, + SCSI_DEVINFO_DH); + } + bus_for_each_dev(&scsi_bus_type, NULL, scsi_dh, scsi_dh_notifier_add); printk(KERN_INFO "%s: device handler registered\n", scsi_dh->name); @@ -395,7 +352,7 @@ EXPORT_SYMBOL_GPL(scsi_register_device_handler); */ int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh) { - struct scsi_dh_devinfo_list *tmp, *pos; + int i; if (!get_device_handler(scsi_dh->name)) return -ENODEV; @@ -403,14 +360,14 @@ int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh) bus_for_each_dev(&scsi_bus_type, NULL, scsi_dh, scsi_dh_notifier_remove); + for (i = 0; scsi_dh->devlist[i].vendor; i++) { + scsi_dev_info_list_del_keyed(scsi_dh->devlist[i].vendor, + scsi_dh->devlist[i].model, + SCSI_DEVINFO_DH); + } + spin_lock(&list_lock); list_del(&scsi_dh->list); - list_for_each_entry_safe(pos, tmp, &scsi_dh_dev_list, node) { - if (pos->handler == scsi_dh) { - list_del(&pos->node); - kfree(pos); - } - } spin_unlock(&list_lock); printk(KERN_INFO "%s: device handler unregistered\n", scsi_dh->name); @@ -569,6 +526,10 @@ static int __init scsi_dh_init(void) { int r; + r = scsi_dev_info_add_list(SCSI_DEVINFO_DH, "SCSI Device Handler"); + if (r) + return r; + r = bus_register_notifier(&scsi_bus_type, &scsi_dh_nb); if (!r) @@ -583,6 +544,7 @@ static void __exit scsi_dh_exit(void) bus_for_each_dev(&scsi_bus_type, NULL, NULL, scsi_dh_sysfs_attr_remove); bus_unregister_notifier(&scsi_bus_type, &scsi_dh_nb); + scsi_dev_info_remove_list(SCSI_DEVINFO_DH); } module_init(scsi_dh_init); diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h index ce9e0ad..9868afd 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -45,6 +45,7 @@ static inline void scsi_log_completion(struct scsi_cmnd *cmd, int disposition) enum { SCSI_DEVINFO_GLOBAL = 0, SCSI_DEVINFO_SPI, + SCSI_DEVINFO_DH, }; extern int scsi_get_device_flags(struct scsi_device *sdev, diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 85867dc..f171c65 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -184,6 +184,7 @@ typedef void (*activate_complete)(void *, int); struct scsi_device_handler { /* Used by the infrastructure */ struct list_head list; /* list of scsi_device_handlers */ + int idx; /* Filled by the hardware handler */ struct module *module; -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] Use scsi_devinfo functions to do matching of scsi IDs device_handler tables. 2011-01-06 20:31 ` [PATCH 2/2] Use scsi_devinfo functions to do matching of scsi IDs device_handler tables Peter Jones @ 2011-01-06 20:36 ` Peter Jones 2011-01-06 20:38 ` [PATCH] " Peter Jones 0 siblings, 1 reply; 4+ messages in thread From: Peter Jones @ 2011-01-06 20:36 UTC (permalink / raw) To: Peter Jones; +Cc: James Bottomley, Tejun Heo, Linux SCSI, Chandra Seetharaman > int scsi_register_device_handler(struct scsi_device_handler *scsi_dh) > { > + int i; > + > if (get_device_handler(scsi_dh->name)) > return -EBUSY; > > spin_lock(&list_lock); > + scsi_dh->idx = scsi_dh_list_idx++; > list_add(&scsi_dh->list, &scsi_dh_list); > spin_unlock(&list_lock); > + > + for (i = 0; scsi_dh->devlist[i].vendor; i++) { > + scsi_dev_info_list_add_keyed(1, /* compatible */ This actually needs to be 0, or else this has the exact same bug I originally set out to fix. > + scsi_dh->devlist[i].vendor, > + scsi_dh->devlist[i].model, > + NULL, > + scsi_dh->idx, > + SCSI_DEVINFO_DH); > + } ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Use scsi_devinfo functions to do matching of scsi IDs device_handler tables. 2011-01-06 20:36 ` Peter Jones @ 2011-01-06 20:38 ` Peter Jones 0 siblings, 0 replies; 4+ messages in thread From: Peter Jones @ 2011-01-06 20:38 UTC (permalink / raw) To: James Bottomley; +Cc: Tejun Heo, Linux SCSI, Chandra Seetharaman, Peter Jones Previously we were using strncmp in order to avoid having to include whitespace in the devlist, but this means "HSV1000" matches a device list entry that says "HSV100", which is wrong. This patch changes scsi_dh.c to use scsi_devinfo's matching functions instead, since they handle these cases correctly. --- drivers/scsi/device_handler/scsi_dh.c | 112 +++++++++++---------------------- drivers/scsi/scsi_priv.h | 1 + include/scsi/scsi_device.h | 1 + 3 files changed, 39 insertions(+), 75 deletions(-) diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c index 6fae3d2..a21136a 100644 --- a/drivers/scsi/device_handler/scsi_dh.c +++ b/drivers/scsi/device_handler/scsi_dh.c @@ -25,16 +25,9 @@ #include <scsi/scsi_dh.h> #include "../scsi_priv.h" -struct scsi_dh_devinfo_list { - struct list_head node; - char vendor[9]; - char model[17]; - struct scsi_device_handler *handler; -}; - static DEFINE_SPINLOCK(list_lock); static LIST_HEAD(scsi_dh_list); -static LIST_HEAD(scsi_dh_dev_list); +static int scsi_dh_list_idx = 1; static struct scsi_device_handler *get_device_handler(const char *name) { @@ -51,40 +44,18 @@ static struct scsi_device_handler *get_device_handler(const char *name) return found; } - -static struct scsi_device_handler * -scsi_dh_cache_lookup(struct scsi_device *sdev) +static struct scsi_device_handler *get_device_handler_by_idx(int idx) { - struct scsi_dh_devinfo_list *tmp; - struct scsi_device_handler *found_dh = NULL; + struct scsi_device_handler *tmp, *found = NULL; spin_lock(&list_lock); - list_for_each_entry(tmp, &scsi_dh_dev_list, node) { - if (!strncmp(sdev->vendor, tmp->vendor, strlen(tmp->vendor)) && - !strncmp(sdev->model, tmp->model, strlen(tmp->model))) { - found_dh = tmp->handler; + list_for_each_entry(tmp, &scsi_dh_list, list) { + if (tmp->idx == idx) { + found = tmp; break; } } spin_unlock(&list_lock); - - return found_dh; -} - -static int scsi_dh_handler_lookup(struct scsi_device_handler *scsi_dh, - struct scsi_device *sdev) -{ - int i, found = 0; - - for(i = 0; scsi_dh->devlist[i].vendor; i++) { - if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor, - strlen(scsi_dh->devlist[i].vendor)) && - !strncmp(sdev->model, scsi_dh->devlist[i].model, - strlen(scsi_dh->devlist[i].model))) { - found = 1; - break; - } - } return found; } @@ -102,41 +73,14 @@ device_handler_match(struct scsi_device_handler *scsi_dh, struct scsi_device *sdev) { struct scsi_device_handler *found_dh = NULL; - struct scsi_dh_devinfo_list *tmp; + int idx; - found_dh = scsi_dh_cache_lookup(sdev); - if (found_dh) - return found_dh; + idx = scsi_get_device_flags_keyed(sdev, sdev->vendor, sdev->model, + SCSI_DEVINFO_DH); + found_dh = get_device_handler_by_idx(idx); - if (scsi_dh) { - if (scsi_dh_handler_lookup(scsi_dh, sdev)) - found_dh = scsi_dh; - } else { - struct scsi_device_handler *tmp_dh; - - spin_lock(&list_lock); - list_for_each_entry(tmp_dh, &scsi_dh_list, list) { - if (scsi_dh_handler_lookup(tmp_dh, sdev)) - found_dh = tmp_dh; - } - spin_unlock(&list_lock); - } - - if (found_dh) { /* If device is found, add it to the cache */ - tmp = kmalloc(sizeof(*tmp), GFP_KERNEL); - if (tmp) { - strncpy(tmp->vendor, sdev->vendor, 8); - strncpy(tmp->model, sdev->model, 16); - tmp->vendor[8] = '\0'; - tmp->model[16] = '\0'; - tmp->handler = found_dh; - spin_lock(&list_lock); - list_add(&tmp->node, &scsi_dh_dev_list); - spin_unlock(&list_lock); - } else { - found_dh = NULL; - } - } + if (scsi_dh && found_dh != scsi_dh) + found_dh = NULL; return found_dh; } @@ -373,12 +317,25 @@ static int scsi_dh_notifier_remove(struct device *dev, void *data) */ int scsi_register_device_handler(struct scsi_device_handler *scsi_dh) { + int i; + if (get_device_handler(scsi_dh->name)) return -EBUSY; spin_lock(&list_lock); + scsi_dh->idx = scsi_dh_list_idx++; list_add(&scsi_dh->list, &scsi_dh_list); spin_unlock(&list_lock); + + for (i = 0; scsi_dh->devlist[i].vendor; i++) { + scsi_dev_info_list_add_keyed(0, + scsi_dh->devlist[i].vendor, + scsi_dh->devlist[i].model, + NULL, + scsi_dh->idx, + SCSI_DEVINFO_DH); + } + bus_for_each_dev(&scsi_bus_type, NULL, scsi_dh, scsi_dh_notifier_add); printk(KERN_INFO "%s: device handler registered\n", scsi_dh->name); @@ -395,7 +352,7 @@ EXPORT_SYMBOL_GPL(scsi_register_device_handler); */ int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh) { - struct scsi_dh_devinfo_list *tmp, *pos; + int i; if (!get_device_handler(scsi_dh->name)) return -ENODEV; @@ -403,14 +360,14 @@ int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh) bus_for_each_dev(&scsi_bus_type, NULL, scsi_dh, scsi_dh_notifier_remove); + for (i = 0; scsi_dh->devlist[i].vendor; i++) { + scsi_dev_info_list_del_keyed(scsi_dh->devlist[i].vendor, + scsi_dh->devlist[i].model, + SCSI_DEVINFO_DH); + } + spin_lock(&list_lock); list_del(&scsi_dh->list); - list_for_each_entry_safe(pos, tmp, &scsi_dh_dev_list, node) { - if (pos->handler == scsi_dh) { - list_del(&pos->node); - kfree(pos); - } - } spin_unlock(&list_lock); printk(KERN_INFO "%s: device handler unregistered\n", scsi_dh->name); @@ -569,6 +526,10 @@ static int __init scsi_dh_init(void) { int r; + r = scsi_dev_info_add_list(SCSI_DEVINFO_DH, "SCSI Device Handler"); + if (r) + return r; + r = bus_register_notifier(&scsi_bus_type, &scsi_dh_nb); if (!r) @@ -583,6 +544,7 @@ static void __exit scsi_dh_exit(void) bus_for_each_dev(&scsi_bus_type, NULL, NULL, scsi_dh_sysfs_attr_remove); bus_unregister_notifier(&scsi_bus_type, &scsi_dh_nb); + scsi_dev_info_remove_list(SCSI_DEVINFO_DH); } module_init(scsi_dh_init); diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h index ce9e0ad..9868afd 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -45,6 +45,7 @@ static inline void scsi_log_completion(struct scsi_cmnd *cmd, int disposition) enum { SCSI_DEVINFO_GLOBAL = 0, SCSI_DEVINFO_SPI, + SCSI_DEVINFO_DH, }; extern int scsi_get_device_flags(struct scsi_device *sdev, diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 85867dc..f171c65 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -184,6 +184,7 @@ typedef void (*activate_complete)(void *, int); struct scsi_device_handler { /* Used by the infrastructure */ struct list_head list; /* list of scsi_device_handlers */ + int idx; /* Filled by the hardware handler */ struct module *module; -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-01-06 20:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-06 20:31 [PATCH 1/2] Add scsi_dev_info_list_del_keyed() Peter Jones 2011-01-06 20:31 ` [PATCH 2/2] Use scsi_devinfo functions to do matching of scsi IDs device_handler tables Peter Jones 2011-01-06 20:36 ` Peter Jones 2011-01-06 20:38 ` [PATCH] " Peter Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).