On Wed, Jan 10, 2024 at 10:15:34AM +0100, Greg Kroah-Hartman wrote:
On Tue, Jan 09, 2024 at 08:55:52AM -0500, Martin K. Petersen wrote:
Pavel,
This is bad idea. This changes return value, but without fixing callers; there will be subtle bugs somewhere.
I'm not sure why this particular change was backported since it was part of a larger cleanup of explicitly distinguishing between block layer sectors and device-specific LBAs. This was done to fix devices using PI with 4 KB blocks which would otherwise end up getting programmed with the wrong reference tag value.
At minimum, we need this:
87662a472a9d8980b26ba5803447df2c4981d467 scsi: iser: Use scsi_get_sector() instead of scsi_get_lba()
I agree this would be appropriate. Otherwise we'll print the error being at the wrong sector in case of an error on a PI device with 4 KB blocks. However, the message is purely informative.
That will fix iser, but there's also:
drivers/s390/scsi/zfcp_fsf.c: io->ref_tag_value = scsi_get_lba(scsi_cmnd) & 0xFFFFFFFF; drivers/scsi/isci/request.c: tc->ref_tag_seed_gen = scsi_get_lba(scmd) & 0xffffffff; drivers/scsi/isci/request.c: tc->ref_tag_seed_verify = scsi_get_lba(scmd) & 0xffffffff; drivers/scsi/lpfc/lpfc_scsi.c: lba = scsi_get_lba(sc); drivers/scsi/lpfc/lpfc_scsi.c: reftag = (uint32_t)scsi_get_lba(sc); /* Truncate LBA */ drivers/scsi/lpfc/lpfc_scsi.c: reftag = (uint32_t)scsi_get_lba(sc); /* Truncate LBA */ drivers/scsi/lpfc/lpfc_scsi.c: reftag = (uint32_t)scsi_get_lba(sc); /* Truncate LBA */ drivers/scsi/lpfc/lpfc_scsi.c: reftag = (uint32_t)scsi_get_lba(sc); /* Truncate LBA */ drivers/scsi/lpfc/lpfc_scsi.c: start_ref_tag = (uint32_t)scsi_get_lba(cmd); /* Truncate LBA */ drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: failing_sector = scsi_get_lba(cmd); drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmnd), drivers/scsi/lpfc/lpfc_scsi.c: (unsigned long long)scsi_get_lba(cmnd), drivers/scsi/qla2xxx/qla_iocb.c: (0xffffffff & scsi_get_lba(cmd))); drivers/scsi/qla2xxx/qla_iocb.c: (0xffffffff & scsi_get_lba(cmd))); drivers/scsi/qla2xxx/qla_iocb.c: (0xffffffff & scsi_get_lba(cmd))); drivers/scsi/qla2xxx/qla_isr.c: cmd->cmnd[0], (u64)scsi_get_lba(cmd), a_ref_tag, e_ref_tag, drivers/scsi/qla2xxx/qla_isr.c: sector_t lba_s = scsi_get_lba(cmd);
Save for two cases in lpfc_queuecommand (which like the iser case will print the wrong sector number on error) all these look OK to me. Note that almost all callers of scsi_get_lba() actually intended to get the protocol LBA as the name indicates and not the block layer sector number.
Ick, this is going to get complex fast. How about I revert the whole series, and then just add the one bugfix at the end, in a "fixed by hand" version like I have here below. Would that be better overall?
Thanks,
greg k-h
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 0c4bc42b55c2..3d3d139127ee 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1069,6 +1069,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, scsi_log_send(scmd); scmd->scsi_done = scsi_eh_done;
- scmd->flags |= SCMD_LAST;
/* * Lock sdev->state_mutex to avoid that scsi_device_quiesce() can @@ -2361,6 +2362,7 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg) scsi_init_command(dev, scmd); scmd->request = rq; scmd->cmnd = scsi_req(rq)->cmd;
- scmd->flags |= SCMD_LAST;
scmd->scsi_done = scsi_reset_provider_done_command; memset(&scmd->sdb, 0, sizeof(scmd->sdb));
Ok, I have now done this and will push out a -rc1 with these changes in it.
greg k-h