| Message ID | 20200527141400.58087-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: use xarray for devices and targets | expand |
Looks good (I think),
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Hi Hannes,
I love your patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200526]
[cannot apply to hch-configfs/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/scsi/scsi_sysfs.c: In function 'scsi_sysfs_device_initialize':
>> drivers/scsi/scsi_sysfs.c:1625:20: warning: comparison is always true due to limited range of data type [-Wtype-limits]
1625 | if (sdev->lun_idx != (unsigned long)-1)
| ^~
drivers/scsi/scsi_sysfs.c: In function 'scsi_device_dev_release_usercontext':
drivers/scsi/scsi_sysfs.c:437:22: warning: 'sdev' is used uninitialized in this function [-Wuninitialized]
437 | struct scsi_target *starget = sdev->sdev_target;
| ^~~~~~~
vim +1625 drivers/scsi/scsi_sysfs.c
1592
1593 void scsi_sysfs_device_initialize(struct scsi_device *sdev)
1594 {
1595 unsigned long flags;
1596 struct Scsi_Host *shost = sdev->host;
1597 struct scsi_target *starget = sdev->sdev_target;
1598
1599 device_initialize(&sdev->sdev_gendev);
1600 sdev->sdev_gendev.bus = &scsi_bus_type;
1601 sdev->sdev_gendev.type = &scsi_dev_type;
1602 dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
1603 sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
1604
1605 device_initialize(&sdev->sdev_dev);
1606 sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
1607 sdev->sdev_dev.class = &sdev_class;
1608 dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%llu",
1609 sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
1610 /*
1611 * Get a default scsi_level from the target (derived from sibling
1612 * devices). This is the best we can do for guessing how to set
1613 * sdev->lun_in_cdb for the initial INQUIRY command. For LUN 0 the
1614 * setting doesn't matter, because all the bits are zero anyway.
1615 * But it does matter for higher LUNs.
1616 */
1617 sdev->scsi_level = starget->scsi_level;
1618 if (sdev->scsi_level <= SCSI_2 &&
1619 sdev->scsi_level != SCSI_UNKNOWN &&
1620 !shost->no_scsi2_lun_in_cdb)
1621 sdev->lun_in_cdb = 1;
1622
1623 transport_setup_device(&sdev->sdev_gendev);
1624 spin_lock_irqsave(shost->host_lock, flags);
> 1625 if (sdev->lun_idx != (unsigned long)-1)
1626 WARN_ON(!xa_insert(&starget->devices, sdev->lun_idx,
1627 sdev, GFP_KERNEL));
1628 else {
1629 struct xa_limit scsi_lun_limit = {
1630 .min = 256,
1631 .max = UINT_MAX,
1632 };
1633 WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx,
1634 sdev, scsi_lun_limit, GFP_KERNEL));
1635 }
1636 list_add_tail(&sdev->siblings, &shost->__devices);
1637 spin_unlock_irqrestore(shost->host_lock, flags);
1638 /*
1639 * device can now only be removed via __scsi_remove_device() so hold
1640 * the target. Target will be held in CREATED state until something
1641 * beneath it becomes visible (in which case it moves to RUNNING)
1642 */
1643 kref_get(&starget->reap_ref);
1644 }
1645
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Hannes,
I love your patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-m001-20200529 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
New smatch warnings:
drivers/scsi/scsi_sysfs.c:1625 scsi_sysfs_device_initialize() warn: always true condition '(sdev->lun_idx != -1) => (0-u32max != u64max)'
Old smatch warnings:
drivers/scsi/scsi_sysfs.c:437 scsi_device_dev_release_usercontext() error: potentially dereferencing uninitialized 'sdev'.
vim +1625 drivers/scsi/scsi_sysfs.c
1592
1593 void scsi_sysfs_device_initialize(struct scsi_device *sdev)
1594 {
1595 unsigned long flags;
1596 struct Scsi_Host *shost = sdev->host;
1597 struct scsi_target *starget = sdev->sdev_target;
1598
1599 device_initialize(&sdev->sdev_gendev);
1600 sdev->sdev_gendev.bus = &scsi_bus_type;
1601 sdev->sdev_gendev.type = &scsi_dev_type;
1602 dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
1603 sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
1604
1605 device_initialize(&sdev->sdev_dev);
1606 sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
1607 sdev->sdev_dev.class = &sdev_class;
1608 dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%llu",
1609 sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
1610 /*
1611 * Get a default scsi_level from the target (derived from sibling
1612 * devices). This is the best we can do for guessing how to set
1613 * sdev->lun_in_cdb for the initial INQUIRY command. For LUN 0 the
1614 * setting doesn't matter, because all the bits are zero anyway.
1615 * But it does matter for higher LUNs.
1616 */
1617 sdev->scsi_level = starget->scsi_level;
1618 if (sdev->scsi_level <= SCSI_2 &&
1619 sdev->scsi_level != SCSI_UNKNOWN &&
1620 !shost->no_scsi2_lun_in_cdb)
1621 sdev->lun_in_cdb = 1;
1622
1623 transport_setup_device(&sdev->sdev_gendev);
1624 spin_lock_irqsave(shost->host_lock, flags);
> 1625 if (sdev->lun_idx != (unsigned long)-1)
1626 WARN_ON(!xa_insert(&starget->devices, sdev->lun_idx,
1627 sdev, GFP_KERNEL));
1628 else {
1629 struct xa_limit scsi_lun_limit = {
1630 .min = 256,
1631 .max = UINT_MAX,
1632 };
1633 WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx,
1634 sdev, scsi_lun_limit, GFP_KERNEL));
1635 }
1636 list_add_tail(&sdev->siblings, &shost->__devices);
1637 spin_unlock_irqrestore(shost->host_lock, flags);
1638 /*
1639 * device can now only be removed via __scsi_remove_device() so hold
1640 * the target. Target will be held in CREATED state until something
1641 * beneath it becomes visible (in which case it moves to RUNNING)
1642 */
1643 kref_get(&starget->reap_ref);
1644 }
1645
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 25c815355d1a..36aec2b37caa 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -601,13 +601,14 @@ static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost, void starget_for_each_device(struct scsi_target *starget, void *data, void (*fn)(struct scsi_device *, void *)) { - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); struct scsi_device *sdev; + unsigned long lun_id = 0; - shost_for_each_device(sdev, shost) { - if ((sdev->channel == starget->channel) && - (sdev->id == starget->id)) - fn(sdev, data); + xa_for_each(&starget->devices, lun_id, sdev) { + if (scsi_device_get(sdev)) + continue; + fn(sdev, data); + scsi_device_put(sdev); } } EXPORT_SYMBOL(starget_for_each_device); @@ -629,14 +630,11 @@ EXPORT_SYMBOL(starget_for_each_device); void __starget_for_each_device(struct scsi_target *starget, void *data, void (*fn)(struct scsi_device *, void *)) { - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); struct scsi_device *sdev; + unsigned long lun_id = 0; - __shost_for_each_device(sdev, shost) { - if ((sdev->channel == starget->channel) && - (sdev->id == starget->id)) - fn(sdev, data); - } + xa_for_each(&starget->devices, lun_id, sdev) + fn(sdev, data); } EXPORT_SYMBOL(__starget_for_each_device); @@ -659,11 +657,15 @@ struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget, u64 lun) { struct scsi_device *sdev; + unsigned long lun_idx = 0; + + if (lun < 256) + return xa_load(&starget->devices, lun); - list_for_each_entry(sdev, &starget->devices, same_target_siblings) { + xa_for_each(&starget->devices, lun_idx, sdev) { if (sdev->sdev_state == SDEV_DEL) continue; - if (sdev->lun ==lun) + if (sdev->lun == lun) return sdev; } diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index c163fa22267c..23c99fbe47d5 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -361,9 +361,9 @@ static void scsi_kick_queue(struct request_queue *q) static void scsi_single_lun_run(struct scsi_device *current_sdev) { struct Scsi_Host *shost = current_sdev->host; - struct scsi_device *sdev, *tmp; + struct scsi_device *sdev; struct scsi_target *starget = scsi_target(current_sdev); - unsigned long flags; + unsigned long flags, lun_id = 0; spin_lock_irqsave(shost->host_lock, flags); starget->starget_sdev_user = NULL; @@ -380,8 +380,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) spin_lock_irqsave(shost->host_lock, flags); if (starget->starget_sdev_user) goto out; - list_for_each_entry_safe(sdev, tmp, &starget->devices, - same_target_siblings) { + xa_for_each(&starget->devices, lun_id, sdev) { if (sdev == current_sdev) continue; if (scsi_device_get(sdev)) @@ -390,7 +389,6 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) spin_unlock_irqrestore(shost->host_lock, flags); scsi_kick_queue(sdev->request_queue); spin_lock_irqsave(shost->host_lock, flags); - scsi_device_put(sdev); } out: diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index c47cddef1839..6fce5fe6ef32 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -232,10 +232,13 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, sdev->id = starget->id; sdev->lun = lun; sdev->channel = starget->channel; + if (lun < 256) + sdev->lun_idx = lun; + else + sdev->lun_idx = (unsigned int)-1; mutex_init(&sdev->state_mutex); sdev->sdev_state = SDEV_CREATED; INIT_LIST_HEAD(&sdev->siblings); - INIT_LIST_HEAD(&sdev->same_target_siblings); INIT_LIST_HEAD(&sdev->starved_entry); INIT_LIST_HEAD(&sdev->event_list); spin_lock_init(&sdev->list_lock); @@ -417,7 +420,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent, starget->id = id; starget->channel = channel; starget->can_queue = 0; - INIT_LIST_HEAD(&starget->devices); + xa_init(&starget->devices); starget->state = STARGET_CREATED; starget->scsi_level = SCSI_2; starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED; diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 95aaa96ce03b..b9ed56d6bd95 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -434,6 +434,7 @@ static void scsi_device_cls_release(struct device *class_dev) static void scsi_device_dev_release_usercontext(struct work_struct *work) { struct scsi_device *sdev; + struct scsi_target *starget = sdev->sdev_target; struct device *parent; struct list_head *this, *tmp; struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL; @@ -448,7 +449,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) spin_lock_irqsave(sdev->host->host_lock, flags); list_del(&sdev->siblings); - list_del(&sdev->same_target_siblings); + xa_erase(&starget->devices, sdev->lun_idx); list_del(&sdev->starved_entry); spin_unlock_irqrestore(sdev->host->host_lock, flags); @@ -1621,7 +1622,17 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) transport_setup_device(&sdev->sdev_gendev); spin_lock_irqsave(shost->host_lock, flags); - list_add_tail(&sdev->same_target_siblings, &starget->devices); + if (sdev->lun_idx != (unsigned long)-1) + WARN_ON(!xa_insert(&starget->devices, sdev->lun_idx, + sdev, GFP_KERNEL)); + else { + struct xa_limit scsi_lun_limit = { + .min = 256, + .max = UINT_MAX, + }; + WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx, + sdev, scsi_lun_limit, GFP_KERNEL)); + } list_add_tail(&sdev->siblings, &shost->__devices); spin_unlock_irqrestore(shost->host_lock, flags); /* diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 28034cc0fce5..2c6b9d8bc40e 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -104,7 +104,6 @@ struct scsi_device { /* the next two are protected by the host->host_lock */ struct list_head siblings; /* list of all devices on this host */ - struct list_head same_target_siblings; /* just the devices sharing same target id */ atomic_t device_busy; /* commands actually active on LLDD */ atomic_t device_blocked; /* Device returned QUEUE_FULL. */ @@ -123,6 +122,7 @@ struct scsi_device { unsigned int id, channel; u64 lun; + unsigned int lun_idx; /* Index into target device xarray */ unsigned int manufacturer; /* Manufacturer of device, for using * vendor-specific cmd's */ unsigned sector_size; /* size in bytes */ @@ -285,7 +285,7 @@ enum scsi_target_state { struct scsi_target { struct scsi_device *starget_sdev_user; struct list_head siblings; - struct list_head devices; + struct xarray devices; struct device dev; struct kref reap_ref; /* last put renders target invisible */ unsigned int channel;
Use xarray for device lookup by target. LUNs below 256 are linear, and can be used directly as the index into the xarray. LUNs above 256 have a distinct LUN format, and are not necessarily linear. They'll be stored in indices above 256 in the xarray, with the next free index in the xarray. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/scsi.c | 28 +++++++++++++++------------- drivers/scsi/scsi_lib.c | 8 +++----- drivers/scsi/scsi_scan.c | 7 +++++-- drivers/scsi/scsi_sysfs.c | 15 +++++++++++++-- include/scsi/scsi_device.h | 4 ++-- 5 files changed, 38 insertions(+), 24 deletions(-)