Dear Kevin, dear Don,
Am 29.09.21 um 01:54 schrieb Don Brace:
From: Kevin Barnett <kevin.barnett@xxxxxxxxxxxxx>
Stop the OS from re-discovering multiple LUNs for
tape drive and medium changer.
Duplicate device nodes for Ultrium tape drive and
medium changer are being created.
The Ultrium tape drive is a multi-LUN SCSI target.
It presents a LUN for the tape drive and a 2nd
LUN for the medium changer.
Our controller FW lists both LUNs in the RPL
results.
Please document the firmware version (and controller) you tested with in
the commit message.
As a result, the smartpqi driver exposes both
devices to the OS. Then the OS does its normal
device discovery via the SCSI REPORT LUNS command,
which causes it to re-discover both devices a 2nd time,
which results in the duplicate device nodes.
Shortly describing the implementation (new struct member ignore_device)
would be nice.
Reviewed-by: Scott Benesh <scott.benesh@xxxxxxxxxxxxx>
Reviewed-by: Scott Teel <scott.teel@xxxxxxxxxxxxx>
Reviewed-by: Mike McGowen <mike.mcgowen@xxxxxxxxxxxxx>
Signed-off-by: Kevin Barnett <kevin.barnett@xxxxxxxxxxxxx>
Signed-off-by: Don Brace <don.brace@xxxxxxxxxxxxx>
---
drivers/scsi/smartpqi/smartpqi.h | 1 +
drivers/scsi/smartpqi/smartpqi_init.c | 23 +++++++++++++++++++----
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
index c439583a4ca5..aac88ac0a0b7 100644
--- a/drivers/scsi/smartpqi/smartpqi.h
+++ b/drivers/scsi/smartpqi/smartpqi.h
@@ -1106,6 +1106,7 @@ struct pqi_scsi_dev {
u8 keep_device : 1;
u8 volume_offline : 1;
u8 rescan : 1;
+ u8 ignore_device : 1;
Why not type bool?
bool aio_enabled; /* only valid for physical disks */
bool in_remove;
bool device_offline;
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index c28eb7ea4a24..8be116992cb0 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -6297,9 +6297,13 @@ static int pqi_slave_alloc(struct scsi_device *sdev)
rphy = target_to_rphy(starget);
device = pqi_find_device_by_sas_rphy(ctrl_info, rphy);
if (device) {
- device->target = sdev_id(sdev);
- device->lun = sdev->lun;
- device->target_lun_valid = true;
Off topic, with `u8 target_lun_valid : 1`, why is `true` used.
+ if (device->target_lun_valid) {
+ device->ignore_device = true;
+ } else {
+ device->target = sdev_id(sdev);
+ device->lun = sdev->lun;
+ device->target_lun_valid = true;
+ }
If the LUN should be ignored, is it actually valid? Why not extend
target_lun_valid and add a third option (enums?) to ignore it?
}
} else {
device = pqi_find_scsi_dev(ctrl_info, sdev_channel(sdev),
@@ -6336,14 +6340,25 @@ static int pqi_map_queues(struct Scsi_Host *shost)
ctrl_info->pci_dev, 0);
}
+static inline bool pqi_is_tape_changer_device(struct pqi_scsi_dev *device)
+{
+ return device->devtype == TYPE_TAPE || device->devtype == TYPE_MEDIUM_CHANGER;
Why also check for TYPE_TAPE? The function name should be updated then?
+}
+
static int pqi_slave_configure(struct scsi_device *sdev)
{
+ int rc = 0;
struct pqi_scsi_dev *device;
device = sdev->hostdata;
device->devtype = sdev->type;
- return 0;
+ if (pqi_is_tape_changer_device(device) && device->ignore_device) {
+ rc = -ENXIO;
+ device->ignore_device = false;
I’d add a `return -ENXIO` here, and remove the variable.
+ }
+
+ return rc;
}
static int pqi_getpciinfo_ioctl(struct pqi_ctrl_info *ctrl_info, void __user *arg)
Kind regards,
Paul
