Re: [PATCH v10 13/33] scsi: rdac: Fix sshdr use

[John Garry <john.g.garry@oracle.com>]

On 22/07/2023 18:10, Mike Christie wrote:
>>>> As I see, err is initially set to SCSI_DH_OK when declared. Then if we need to retry and 2nd call to scsi_execute_cmd() passes, such that rc == 0, then err still holds the old value. This seems same as pre-existing behaviour - is this proper?
>>> I guess this has been working by accident.
>>>
>>> When that happens we end up returning one of the retryable error codes
>>> to dm-multipath. It will then recall us, and before we re-run this
>>> function we will run check_ownership and see that the last call worked.
>>>

Hi Mike,

>> I'd suggest that we fix this up as a prep patch, if you don't mind.
>>
> Do you mean just change the description of this patch to reflect it fixes the
> second bug? It already is a prep patch. The second rdac patch is built over it.

AFAICS, this patch does not fix the bug where @err may hold a stale 
value. However this broken code goes away later in the series.

> stable can take the sshdr fix patches without the API change ones if they want.
> 
> I just put the sshdr one next to the API change one, so reviewers wouldn't
> have to jump back and forth between the front and back of the patchset.
> 
> Do you mean move all the sshd hdr patches to a separate patchset?
No, I was just suggesting that we fix the broken code also in a separate 
patch in this series, like:

diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c
b/drivers/scsi/device_handler/scsi_dh_rdac.c
index c5538645057a..5d338f12e68b 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -530,7 +530,7 @@ static void send_mode_select(struct work_struct *work)
                container_of(work, struct rdac_controller, ms_work);
        struct scsi_device *sdev = ctlr->ms_sdev;
        struct rdac_dh_data *h = sdev->handler_data;
-       int err = SCSI_DH_OK, retry_cnt = RDAC_RETRY_COUNT;
+       int err, retry_cnt = RDAC_RETRY_COUNT;
        struct rdac_queue_data *tmp, *qdata;
        LIST_HEAD(list);
        unsigned char cdb[MAX_COMMAND_SIZE];
@@ -549,6 +549,7 @@ static void send_mode_select(struct work_struct *work)
        spin_unlock(&ctlr->ms_lock);

  retry:
+       err = SCSI_DH_OK;
        memset(cdb, 0, sizeof(cdb));

        data_size = rdac_failover_get(ctlr, &list, cdb);

Thanks,
John