[PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[Ewan D. Milne]

The change to use dma_set_mask_and_coherent() incorrectly made a second
call with the 32 bit DMA mask value when the call with the 64 bit DMA
mask value succeeded.  This resulted in NVMe/FC connections failing due
to corrupted data buffers, and various other SCSI/FCP I/O errors.

Fixes: f30e1bfd6154 ("scsi: lpfc: use dma_set_mask_and_coherent")
Cc: <stable@vger.kernel.org>
Suggested-by: Don Dutile <ddutile@redhat.com>
Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/lpfc/lpfc_init.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index bede11e..60c178d 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -7367,9 +7367,9 @@ struct lpfc_rpi_hdr *
 		return error;
 
 	/* Set the device DMA mask size */
-	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
-	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
-		return error;
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
+		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
+			return error;
 
 	/* Get the bus address of Bar0 and Bar2 and the number of bytes
 	 * required by each mapping.
@@ -9745,9 +9745,9 @@ struct lpfc_cq_event *
 		return error;
 
 	/* Set the device DMA mask size */
-	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
-	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
-		return error;
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
+		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
+			return error;
 
 	/*
 	 * The BARs and register set definitions and offset locations are
-- 
1.8.3.1

Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[James Smart]

On 2/11/2019 7:05 AM, Ewan D. Milne wrote:
> The change to use dma_set_mask_and_coherent() incorrectly made a second
> call with the 32 bit DMA mask value when the call with the 64 bit DMA
> mask value succeeded.  This resulted in NVMe/FC connections failing due
> to corrupted data buffers, and various other SCSI/FCP I/O errors.
>
> Fixes: f30e1bfd6154 ("scsi: lpfc: use dma_set_mask_and_coherent")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Don Dutile <ddutile@redhat.com>
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>   drivers/scsi/lpfc/lpfc_init.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index bede11e..60c178d 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -7367,9 +7367,9 @@ struct lpfc_rpi_hdr *
>   		return error;
>   
>   	/* Set the device DMA mask size */
> -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> -		return error;
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
> +			return error;
>   
>   	/* Get the bus address of Bar0 and Bar2 and the number of bytes
>   	 * required by each mapping.
> @@ -9745,9 +9745,9 @@ struct lpfc_cq_event *
>   		return error;
>   
>   	/* Set the device DMA mask size */
> -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> -		return error;
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
> +			return error;
>   
>   	/*
>   	 * The BARs and register set definitions and offset locations are

Thanks Ewan.


Signed-off-by:  James Smart  <james.smart@broadcom.com>

-- james

Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[Christoph Hellwig]

On Mon, Feb 11, 2019 at 10:05:02AM -0500, Ewan D. Milne wrote:
> The change to use dma_set_mask_and_coherent() incorrectly made a second
> call with the 32 bit DMA mask value when the call with the 64 bit DMA
> mask value succeeded.  This resulted in NVMe/FC connections failing due
> to corrupted data buffers, and various other SCSI/FCP I/O errors.

Ooops, sorry.

>  	/* Set the device DMA mask size */
> -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> -		return error;
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)

But this still looks obsfuctating, why not:

	error = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
	if (error)
		error = dma_set_mask_and_coherent(&pdev->dev,
				DMA_BIT_MASK(32)));
	if (error)
		return error;

Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[Ewan D. Milne]

On Tue, 2019-02-12 at 00:06 -0800, Christoph Hellwig wrote:
> On Mon, Feb 11, 2019 at 10:05:02AM -0500, Ewan D. Milne wrote:
> > The change to use dma_set_mask_and_coherent() incorrectly made a second
> > call with the 32 bit DMA mask value when the call with the 64 bit DMA
> > mask value succeeded.  This resulted in NVMe/FC connections failing due
> > to corrupted data buffers, and various other SCSI/FCP I/O errors.
> 
> Ooops, sorry.
> 
> >  	/* Set the device DMA mask size */
> > -	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) ||
> > -	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
> > -		return error;
> > +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)) != 0)
> > +		if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0)
> 
> But this still looks obsfuctating, why not:
> 
> 	error = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> 	if (error)
> 		error = dma_set_mask_and_coherent(&pdev->dev,
> 				DMA_BIT_MASK(32)));
> 	if (error)
> 		return error;

I agree that making the flow of control explicit would be good, but...

It looks like this would introduce a different problem.  "error" is set to
-ENODEV earlier in the two functions so that the various error paths will return
that value.  If it is overwritten with a successful result from the call to
dma_set_mask_and_coherent() but a later call to e.g. ioremap() fails, the
functions will incorrectly return a value indicating that they have succeeded.

It also appears as if the patches to hptiop, hisi_sas and bfa need to be fixed up.
I don't have a test environment for these, although I might be able to modify the
test environment for bfa.  Martin/James, what about the others?

-Ewan

Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[Christoph Hellwig]

On Tue, Feb 12, 2019 at 10:37:45AM -0500, Ewan D. Milne wrote:
> 
> It looks like this would introduce a different problem.  "error" is set to
> -ENODEV earlier in the two functions so that the various error paths will return
> that value.  If it is overwritten with a successful result from the call to
> dma_set_mask_and_coherent() but a later call to e.g. ioremap() fails, the
> functions will incorrectly return a value indicating that they have succeeded.
> 
> It also appears as if the patches to hptiop, hisi_sas and bfa need to be fixed up.
> I don't have a test environment for these, although I might be able to modify the
> test environment for bfa.  Martin/James, what about the others?

I don't have a test environment for them either, and I doubt many
people have.  If you don't feel comfortable I can send a fix for
them.

Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[Martin K. Petersen]

Ewan,

> It also appears as if the patches to hptiop, hisi_sas and bfa need to
> be fixed up.  I don't have a test environment for these, although I
> might be able to modify the test environment for bfa.  Martin/James,
> what about the others?

I'm afraid I don't have any of those. But I'm sure John would be happy
to check hisi_sas.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: lpfc: fix calls to dma_set_mask_and_coherent()

[John Garry]

On 13/02/2019 03:10, Martin K. Petersen wrote:
>
> Ewan,
>
>> It also appears as if the patches to hptiop, hisi_sas and bfa need to
>> be fixed up.  I don't have a test environment for these, although I
>> might be able to modify the test environment for bfa.  Martin/James,
>> what about the others?
>
> I'm afraid I don't have any of those. But I'm sure John would be happy
> to check hisi_sas.
>

Happy to test. Or I can just send the fix myself. So what is the 
preferred pattern for setting the DMA mask?

John