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