scsi: ips: fix missing break in switch
Subsystems (3)
PATCH 1/1 scsi: ips: fix missing break in switch
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")
--- drivers/scsi/ips.c | 1 + 1 file changed, 1 insertion(+)
drivers/scsi/ips.c +1 -0
@@ -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 :
Signature · 1 line
2.7.4
Martin K. Petersen
AppliedGustavo,
Quoted 2 lines
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!
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 9 lines · Martin K. Petersen
Gustavo,
Quoted 2 lines
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,
Quoted 8 lines
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.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 12 lines · Martin K. Petersen
Finn,
Quoted 8 lines
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.)
Quoted 7 lines
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_STOPunit (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.
Quoted 3 lines
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 <[email protected]> L: [email protected] 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,
Quoted 4 lines
Quoted 2 lines
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.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 9 lines · Martin K. Petersen
Quoted 5 lines
Quoted 3 lines
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.
Quoted 4 lines
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.