diff mbox

qla2xxx UBSAN warning in 4.14-rc1

Message ID F9C1B65E-B91F-439A-8069-B6D1B060AEBC@cavium.com (mailing list archive)
State Superseded
Headers show

Commit Message

Madhani, Himanshu Jan. 25, 2018, 7:25 a.m. UTC 
Hi Meelis, 

> On Jan 24, 2018, at 2:18 PM, Meelis Roos <mroos@linux.ee> wrote:

> 

>>> Hello, I decided to widen the coverage of my kernel testbed and put some 

>>> FC cards into servers. This one is a PCI-X QLA2340 in HP Proliant DL 380 

>>> G4 (first 64-bit generation of Proliants). I got a UBSAN warning from 

>>> qla2xxx before probing for the firmware.

>> 

>> Would it be possible for you to test the (completely untested) patch below?

> 

> It compiles without warnings and the driver loads without warnings.

> 

> Meanwhile I tried the following patch, also successfully.

> 

> However, the same problem is present in qla24xx_mbx_completion (and can 

> also be trivially patched over).

> 

> I did not understand the logic of what's goind on with mailboxes - there 

> seem to be up to 32 of them and for some reason, a bitmask is used for 

> iterating over them, with mboxes = ha->mcp->in_mb filtering out some 

> mailboxes, and in_mb bitmap value comes from firmware?

> 


> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c

> index 2fd79129bb2a..7868930ae1c8 100644

> --- a/drivers/scsi/qla2xxx/qla_isr.c

> +++ b/drivers/scsi/qla2xxx/qla_isr.c

> @@ -272,7 +272,7 @@ qla2x00_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0)

> 	struct device_reg_2xxx __iomem *reg = &ha->iobase->isp;

> 

> 	/* Read all mbox registers? */

> -	mboxes = (1 << ha->mbx_count) - 1;

> +	mboxes = (1ULL << ha->mbx_count) - 1;

> 	if (!ha->mcp)

> 		ql_dbg(ql_dbg_async, vha, 0x5001, "MBX pointer ERROR.\n");

> 	else

> 

> -- 

> Meelis Roos (mroos@linux.ee)


Since I did could not get hold of 4G adapter for testing, i was not able to
get to this one fixed in time. 

Bart’s change looks good and with your testing should be good to include.

I also noticed qla24xx_mbx_completion() will need this fix. I was able to confirm it on my setup with 16/32G adapter. 

Would you care to send formal patch and add my ACK to it?

Thanks for all the effort on getting this tested on your setup. 

Thanks,
- Himanshu
diff mbox

Patch

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index d1e7fd905f16..b97b14a89ac3 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -272,7 +272,7 @@  qla2x00_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0)
        struct device_reg_2xxx __iomem *reg = &ha->iobase->isp;

        /* Read all mbox registers? */
-       mboxes = (1 << ha->mbx_count) - 1;
+       mboxes = (ha->mbx_count != 32 ? 1U << ha->mbx_count : 0) - 1U;
        if (!ha->mcp)
                ql_dbg(ql_dbg_async, vha, 0x5001, "MBX pointer ERROR.\n");
        else
@@ -2881,7 +2881,7 @@  qla24xx_mbx_completion(scsi_qla_host_t *vha, uint16_t mb0)
        struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;

        /* Read all mbox registers? */
-       mboxes = (1 << ha->mbx_count) - 1;
+       mboxes = (ha->mbx_count != 32 ? 1U << ha->mbx_count : 0) - 1U;
        if (!ha->mcp)
                ql_dbg(ql_dbg_async, vha, 0x504e, "MBX pointer ERROR.\n”);