| Message ID | 20210928235442.201875-10-don.brace@microchip.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | smartpqi updates | expand |
On 9/28/21 6:54 PM, Don Brace wrote: > From: Kevin Barnett <kevin.barnett@microchip.com> > > 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. > > 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. > > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> > Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> Acked-by: John Donnelly <john.p.donnelly@oracle.com> > --- > 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; > 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; > + if (device->target_lun_valid) { > + device->ignore_device = true; > + } else { > + device->target = sdev_id(sdev); > + device->lun = sdev->lun; > + device->target_lun_valid = true; > + } > } > } 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; > +} > + > 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; > + } > + > + return rc; > } > > static int pqi_getpciinfo_ioctl(struct pqi_ctrl_info *ctrl_info, void __user *arg) >
Dear Kevin, dear Don, Am 29.09.21 um 01:54 schrieb Don Brace: > From: Kevin Barnett <kevin.barnett@microchip.com> > > 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@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com> > Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > 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
From: Paul Menzel [mailto:pmenzel@molgen.mpg.de] Subject: Re: [smartpqi updates PATCH V2 09/11] smartpqi: fix duplicate device nodes for tape changers Dear Kevin, dear Don, > Our controller FW lists both LUNs in the RPL results. Please document the firmware version (and controller) you tested with in the commit message. DON: Done in V3, thanks for your review. Shortly describing the implementation (new struct member ignore_device) would be nice. DON: Don in V3, thanks for your review. > u8 rescan : 1; > + u8 ignore_device : 1; Why not type bool? Don: They both take the same amount of memory and since the other members are also u8, the new member was also u8 for consistency. > - device->lun = sdev->lun; > - device->target_lun_valid = true; Off topic, with `u8 target_lun_valid : 1`, why is `true` used. Don: Has the same behavior, and carried forward from other member fields. > + 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? Don: The reason is that it takes advantage of the order the devices are added and how slave_alloc and slave_configure fit into this order. > + return device->devtype == TYPE_TAPE || device->devtype == > +TYPE_MEDIUM_CHANGER; Why also check for TYPE_TAPE? The function name should be updated then? Don: Because out tape changer consisted of the changer and one or more tape units and both were duplicated. > static int pqi_slave_configure(struct scsi_device *sdev) > + 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. Don: This works in conjunction with slave_alloc and is needed. > Kind regards, Paul Thanks for your review. Appreciate the inspection. Don and Kevin
Don,
> DON: Done in V3, thanks for your review.
Just a heads up that I already staged this for zeroday testing
yesterday. If you post a v3 I'll drop what I have. But I would prefer an
incremental patch.
Thanks!
-----Original Message----- From: Martin K. Petersen [mailto:martin.petersen@oracle.com] Subject: Re: [smartpqi updates PATCH V2 09/11] smartpqi: fix duplicate device nodes for tape changers Don, > DON: Done in V3, thanks for your review. Just a heads up that I already staged this for zeroday testing yesterday. If you post a v3 I'll drop what I have. But I would prefer an incremental patch. Thanks! -- Martin K. Petersen Oracle Linux Engineering DON: Incremental patch it is. I appreciate this. Thanks, Don
Dear Don, Am 05.10.21 um 22:23 schrieb Don.Brace@microchip.com: >> Our controller FW lists both LUNs in the RPL results. > > Please document the firmware version (and controller) you tested with in the commit message. > > DON: Done in V3, thanks for your review. When I understood Martin correctly, he already pulled the patches in. It’d be great if you added it in an answer then. > Shortly describing the implementation (new struct member ignore_device) would be nice. > DON: Don in V3, thanks for your review. > >> u8 rescan : 1; >> + u8 ignore_device : 1; > > Why not type bool? > Don: They both take the same amount of memory and since the other members are also u8, the new member was also u8 for consistency. Well, the below struct members are declared as bool. u8 volume_offline : 1; u8 rescan : 1; bool aio_enabled; /* only valid for physical disks */ It’d be great, if you could clean that up in the future. >> - device->lun = sdev->lun; >> - device->target_lun_valid = true; > > Off topic, with `u8 target_lun_valid : 1`, why is `true` used. > Don: Has the same behavior, and carried forward from other member fields. In my opinion, if bool is used, true/false should be used too. >> + 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? > > Don: The reason is that it takes advantage of the order the devices are added and how slave_alloc and slave_configure fit into this order. Ok. My answer should have also been to use a bitfield. Sorry about that. It does not look nice to me to add new attributes to work around firmware isuses. >> + return device->devtype == TYPE_TAPE || device->devtype == >> +TYPE_MEDIUM_CHANGER; > > Why also check for TYPE_TAPE? The function name should be updated then? > Don: Because our tape changer consisted of the changer and one or more tape units and both were duplicated. Yes, I figured. But the function name is still incorrect/misleading then? >> static int pqi_slave_configure(struct scsi_device *sdev) >> + 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. > Don: This works in conjunction with slave_alloc and is needed. Instead of + if (pqi_is_tape_changer_device(device) && device->ignore_device) { + rc = -ENXIO; + device->ignore_device = false; + } + + return rc; I meant + if (pqi_is_tape_changer_device(device) && device->ignore_device) { + device->ignore_device = false; + return -ENXIO; + } + + return 0; Lastly, some (debug) log messages would always be helpful in my opinion, if stuff is worked around. Kind regards, Paul
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; 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; + if (device->target_lun_valid) { + device->ignore_device = true; + } else { + device->target = sdev_id(sdev); + device->lun = sdev->lun; + device->target_lun_valid = true; + } } } 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; +} + 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; + } + + return rc; } static int pqi_getpciinfo_ioctl(struct pqi_ctrl_info *ctrl_info, void __user *arg)