On Sun, 2021-01-03 at 19:49 +0100, Arnd Bergmann wrote:

On Sun, Jan 3, 2021 at 6:00 PM James Bottomley [off-list ref]
wrote:

quoted

On Sun, 2021-01-03 at 17:26 +0100, Arnd Bergmann wrote:
[...]

quoted

@@ -8209,7 +8208,7 @@ megasas_mgmt_fw_ioctl(struct

megasas_instance
*instance,
                if (instance->consistent_mask_64bit)
                        put_unaligned_le64(sense_handle,
sense_ptr);
                else
-                       put_unaligned_le32(sense_handle,
sense_ptr);
+                       put_unaligned_le64(sense_handle,
sense_ptr);
        }

This hunk can't be right.  It effectively means removing the if.

I'm just trying to restore the state before the regression introduced
in my 381d34e376e3 ("scsi: megaraid_sas: Check user-provided
offsets").

The old code always stored 'sizeof(long)' bytes into sense_ptr,
regardless of instance->consistent_mask_64bit, but it would truncate
the address to 32 bit if that was cleared. This was clearly bogus
and I tried to make it do something more meaningful, only storing
8 bytes into the structure if it was configured for 64-bit DMA,
regardless of the capabilities of the kernel.

Heh, well, all this depends on how the firmware interprets the pointer,
for which we don't seem to have a manual.  Instinct tells me the flag
MFI_FRAME_SENSE64 is what does this and that's conditioned on the same
if clause 100 lines above this, so the fix your proposing would still
seem to be wrong, because I think when that flag is not set, the device
expects the sense pointer to be 32 bit.

quoted

However, the if is needed because sense_handle is a dma_addr_t
which can be either 32 or 64 bit.  What about changing the if to

if (sizeof(dma_addr_t) == 8)

instead?

That would not be useful either, the device surely does not care
if the kernel supports 64-bit DMA. What we'd really need here is
someone with access to the interface specifications to see how
many bytes should be stored in the structure. I suspect always
storing 64 bits (as my patch does) is correct, and would send a
proper patch to remove the if() if Phil confirms that my test
patch fixes the regression.

Well, as I said above, I'm speculating the device does what we tell it,
and whether to use 32 or 64 bits for the sense pointer definitely seems
to be a flag the driver controls ... we really need someone with access
to the programming manual to tell us if this speculation is accurate,
though.

James