Re: [smartpqi updates PATCH V2 09/11] smartpqi: fix duplicate device nodes for tape changers

[Paul Menzel]

Dear Don,


Am 05.10.21 um 22:23 schrieb Don.Brace@xxxxxxxxxxxxx:

> > 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