| Message ID | 20190211150502.9999-1-emilne@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: lpfc: fix calls to dma_set_mask_and_coherent() | expand |
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
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;
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
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.
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.
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
On Tue, 2019-02-12 at 10:34 -0800, Christoph Hellwig wrote: > 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. [ removed cc: stable ] Thanks, I have the patches written but Hannes has posted already. I prefer to test before posting but in this case testing one driver with a similar pattern is better than not having a fix available, I think. As long as it gets enough review. -Ewan
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
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(-)