| Message ID | 20181016091223.GA19765@embeddedor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: ips: fix missing break in switch | expand |
Gustavo, > Add missing break statement in order to prevent the code from falling > through to case TEST_UNIT_READY. Applied to 4.20/scsi-queue, thanks!
On Tue, 16 Oct 2018, Martin K. Petersen wrote: > > Gustavo, > > > Add missing break statement in order to prevent the code from falling > > through to case TEST_UNIT_READY. > > Applied to 4.20/scsi-queue, thanks! > > This looks wrong to me. I think you've just prevented all START STOP commands sent to logical volumes from reaching return ((*ha->func.issue) (ha, scb)); I think a better patch is to add a "fall though" comment not a "break" statement. (I no longer have access to a ServeRAID board so I can't test.) --
Finn, > This looks wrong to me. I think you've just prevented all START STOP > commands sent to logical volumes from reaching > > return ((*ha->func.issue) (ha, scb)); > > I think a better patch is to add a "fall though" comment not a "break" > statement. (I no longer have access to a ServeRAID board so I can't > test.) When I looked at this a few days ago, it seemed that the fallthrough to the TUR/INQUIRY case statement was accidental and that the intent was to quickly complete START_STOP unit (which probably doesn't make much sense for a RAID device anyway). See the case statements above for another fast exit scenario. Sadly I have no way to test this. It just stuck out like a false positive in Gustavo's fallthrough markup patch.
On Tue, 16 Oct 2018, Martin K. Petersen wrote: > > Finn, > > > This looks wrong to me. I think you've just prevented all START STOP > > commands sent to logical volumes from reaching > > > > return ((*ha->func.issue) (ha, scb)); > > > > I think a better patch is to add a "fall though" comment not a "break" > > statement. (I no longer have access to a ServeRAID board so I can't > > test.) > > When I looked at this a few days ago, it seemed that the fallthrough to > the TUR/INQUIRY case statement was accidental and that the intent was to > quickly complete START_STOP unit (which probably doesn't make much sense > for a RAID device anyway). > > See the case statements above for another fast exit scenario. > But that's an error path. Also note that START_STOP also appears in ips_chkstatus(), which suggests to me that this command may be submitted legitimately. > Sadly I have no way to test this. It just stuck out like a false > positive in Gustavo's fallthrough markup patch. > Without testers, and without another maintainer to review this, I'd advocate a more prudent approach. I wonder whether there is another maintainer to do a review. The MAINTAINERS file seems to contradict itself. $ grep -C5 drivers/scsi/ips MAINTAINERS ... IBM ServeRAID RAID DRIVER S: Orphan F: drivers/scsi/ips.* ... IPS SCSI RAID DRIVER M: Adaptec OEM Raid Solutions <aacraid@microsemi.com> L: linux-scsi@vger.kernel.org W: http://www.adaptec.com/ S: Maintained F: drivers/scsi/ips* ... Note that 'drivers/scsi/ips*' got assigned to Microsemi by a janitorial change, as part of commit 679655daffdd ("MAINTAINERS - Add file patterns") in 2009. But for ips.c, the last acked-by from the vendor was in 2008. --
Finn, >> See the case statements above for another fast exit scenario. >> > > But that's an error path. Look further down. Several other SCSI commands are completed as NOPs the same way. Also, I don't see how the case statement for TUR/INQUIRY would do anything meaningful in terms of preparing a START STOP UNIT for the firmware.
On Wed, 17 Oct 2018, Martin K. Petersen wrote: > > >> See the case statements above for another fast exit scenario. > >> > > > > But that's an error path. > > Look further down. Several other SCSI commands are completed as NOPs the > same way. > That's true, but it doesn't indicate a bug to me. On the contrary, the entire switch (scb->scsi_cmd->cmnd[0]) statement in ips_send_cmd() appears to be carefully constructed, just like the matching statement in ips_chkstatus(). But none of these indications can decide the question. We just don't have enough information. (I'm sure that someone somewhere does have the relevant technical information...) If this really is undecidable, then I think the right patch is the more prudent one. That is, add a "fall through" comment, not a "break" statement. Or perhaps a "fall through (TODO: check this)" comment. > Also, I don't see how the case statement for TUR/INQUIRY would do > anything meaningful in terms of preparing a START STOP UNIT for the > firmware. > If SSU case doesn't do anything meaningful, then neither does the TUR/INQUIRY case, and then you can just delete all of that code: } else { scb->cmd.logical_info.op_code = IPS_CMD_GET_LD_INFO; scb->cmd.logical_info.command_id = IPS_COMMAND_ID(ha, scb); scb->cmd.logical_info.reserved = 0; scb->cmd.logical_info.reserved2 = 0; scb->data_len = sizeof (IPS_LD_INFO); scb->data_busaddr = ha->logical_drive_info_dma_addr; scb->flags = 0; scb->cmd.logical_info.buffer_addr = scb->data_busaddr; ret = IPS_SUCCESS; } FWIW, I think this line of reasoning is mistaken. --
diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c index bd6ac6b..fe587ef 100644 --- a/drivers/scsi/ips.c +++ b/drivers/scsi/ips.c @@ -3485,6 +3485,7 @@ ips_send_cmd(ips_ha_t * ha, ips_scb_t * scb) case START_STOP: scb->scsi_cmd->result = DID_OK << 16; + break; case TEST_UNIT_READY: case INQUIRY:
Add missing break statement in order to prevent the code from falling through to case TEST_UNIT_READY. Addresses-Coverity-ID: 1357338 ("Missing break in switch") Suggested-by: Martin K. Petersen <martin.petersen@oracle.com> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/scsi/ips.c | 1 + 1 file changed, 1 insertion(+)