[PATCH] scsi: ips: fix missing break in switch

[PATCH] scsi: ips: fix missing break in switch

[Gustavo A. R. Silva]

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(+)

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

Re: [PATCH] scsi: ips: fix missing break in switch

[Martin K. Petersen]

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!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: ips: fix missing break in switch

[Finn Thain]

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

-- 

Re: [PATCH] scsi: ips: fix missing break in switch

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: ips: fix missing break in switch

[Finn Thain]

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.

-- 

Re: [PATCH] scsi: ips: fix missing break in switch

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: ips: fix missing break in switch

[Finn Thain]

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.

--