diff mbox series

[v1,1/5] mpt3sas: Don't change the dma coherent mask after allocations

Message ID 1586957125-19460-2-git-send-email-suganath-prabu.subramani@broadcom.com (mailing list archive)
State Superseded
Headers show
Series mpt3sas: Fix changing coherent mask after allocation | expand

Commit Message

Suganath Prabu S April 15, 2020, 1:25 p.m. UTC 
From: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>

Currently driver is initially setting the dma coherent mask to 32 bit
and then after allocating the Reply Descriptor Post Queues(RDPQ) pools
it changes the dma coherent mask to 64/63 according to HBA generation.

But the DMA layer does not allow changing the DMA coherent mask after
there are outstanding allocations.

So, updating the driver to stop changing the dma coherent mask after
allocations.

Rename ioc variable "dma_mask" to "is_dma_32bit" and use it to set 32
bit DMA.
---
v1 Change log:
1) Incorporated the review comments from Christoph Hellwig

Signed-off-by: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 83 ++++++++++++++-----------------------
 drivers/scsi/mpt3sas/mpt3sas_base.h |  4 +-
 2 files changed, 32 insertions(+), 55 deletions(-)

Comments

Christoph Hellwig April 22, 2020, 6:34 a.m. UTC | #1 
On Wed, Apr 15, 2020 at 09:25:21AM -0400, Suganath Prabu wrote:
> From: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
> 
> Currently driver is initially setting the dma coherent mask to 32 bit
> and then after allocating the Reply Descriptor Post Queues(RDPQ) pools
> it changes the dma coherent mask to 64/63 according to HBA generation.
> 
> But the DMA layer does not allow changing the DMA coherent mask after
> there are outstanding allocations.
> 
> So, updating the driver to stop changing the dma coherent mask after
> allocations.
> 
> Rename ioc variable "dma_mask" to "is_dma_32bit" and use it to set 32
> bit DMA.
> ---
> v1 Change log:
> 1) Incorporated the review comments from Christoph Hellwig
> 
> Signed-off-by: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>

I still don't see why you don't simply take my original patch instead.
Suganath Prabu S April 22, 2020, 9:19 a.m. UTC | #2 
Hi Christoph,

Your original patch always set the dma coherent mask to 32bit.
But in this patch set, we first set the dma coherent to 64bit, if RDPQ
pools crosses
the 4gb boundary then we change it to 32 bit.
Like your original patch we will simplify.

Thanks,
Suganath


On Wed, Apr 22, 2020 at 12:04 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Apr 15, 2020 at 09:25:21AM -0400, Suganath Prabu wrote:
> > From: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
> >
> > Currently driver is initially setting the dma coherent mask to 32 bit
> > and then after allocating the Reply Descriptor Post Queues(RDPQ) pools
> > it changes the dma coherent mask to 64/63 according to HBA generation.
> >
> > But the DMA layer does not allow changing the DMA coherent mask after
> > there are outstanding allocations.
> >
> > So, updating the driver to stop changing the dma coherent mask after
> > allocations.
> >
> > Rename ioc variable "dma_mask" to "is_dma_32bit" and use it to set 32
> > bit DMA.
> > ---
> > v1 Change log:
> > 1) Incorporated the review comments from Christoph Hellwig
> >
> > Signed-off-by: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
>
> I still don't see why you don't simply take my original patch instead.
Christoph Hellwig April 22, 2020, 5:18 p.m. UTC | #3 
On Wed, Apr 22, 2020 at 02:49:42PM +0530, Suganath Prabu Subramani wrote:
> Hi Christoph,
> 
> Your original patch always set the dma coherent mask to 32bit.
> But in this patch set, we first set the dma coherent to 64bit, if RDPQ
> pools crosses
> the 4gb boundary then we change it to 32 bit.
> Like your original patch we will simplify.

This is however missing most of the cleanups.  Also the is_dma_32bit
flag is unused in this patch.  So I don't think it should be added
here, but only when used.  It should then also use a bool type
and be named use_32bit_dma.

When reworking your patch using all criteria I still end up with
something looking a lot like my original one, with the only big
difference that is also forces a 32-bit DMA streaming mask for
all the limited devices, which actually looks wrong to me:

---
From 5253b88eba38dbf50cbbe5f34c8f5b4c345cca57 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 22 Apr 2020 19:18:00 +0200
Subject: mpt3sas: don't change the dma coherent mask after allocations

The DMA layer does not allow changing the DMA coherent mask after
there are outstanding allocations.  Stop doing that and always
use a 32-bit coherent DMA mask in mpt3sas.

Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 71 ++++++++---------------------
 drivers/scsi/mpt3sas/mpt3sas_base.h |  2 -
 2 files changed, 20 insertions(+), 53 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 663782bb790d..3072599a187a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2806,55 +2806,33 @@ _base_build_sg_ieee(struct MPT3SAS_ADAPTER *ioc, void *psge,
 static int
 _base_config_dma_addressing(struct MPT3SAS_ADAPTER *ioc, struct pci_dev *pdev)
 {
-	u64 required_mask, coherent_mask;
 	struct sysinfo s;
-	/* Set 63 bit DMA mask for all SAS3 and SAS35 controllers */
-	int dma_mask = (ioc->hba_mpi_version_belonged > MPI2_VERSION) ? 63 : 64;
-
-	if (ioc->is_mcpu_endpoint)
-		goto try_32bit;
+	int dma_bits;
 
-	required_mask = dma_get_required_mask(&pdev->dev);
-	if (sizeof(dma_addr_t) == 4 || required_mask == 32)
-		goto try_32bit;
-
-	if (ioc->dma_mask)
-		coherent_mask = DMA_BIT_MASK(dma_mask);
+	if (ioc->is_mcpu_endpoint || sizeof(dma_addr_t) == 4 ||
+	    dma_get_required_mask(&pdev->dev) <= DMA_BIT_MASK(32))
+		dma_bits = 32;
+	/* Set 63 bit DMA mask for all SAS3 and SAS35 controllers */
+	else if (ioc->hba_mpi_version_belonged > MPI2_VERSION)
+		dma_bits = 63;
 	else
-		coherent_mask = DMA_BIT_MASK(32);
-
-	if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(dma_mask)) ||
-	    dma_set_coherent_mask(&pdev->dev, coherent_mask))
-		goto try_32bit;
+		dma_bits = 64;
 
-	ioc->base_add_sg_single = &_base_add_sg_single_64;
-	ioc->sge_size = sizeof(Mpi2SGESimple64_t);
-	ioc->dma_mask = dma_mask;
-	goto out;
-
- try_32bit:
-	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
+	if (dma_set_mask_and_coherent(&pdev->dev, dma_bits))
 		return -ENODEV;
 
-	ioc->base_add_sg_single = &_base_add_sg_single_32;
-	ioc->sge_size = sizeof(Mpi2SGESimple32_t);
-	ioc->dma_mask = 32;
- out:
-	si_meminfo(&s);
-	ioc_info(ioc, "%d BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (%ld kB)\n",
-		 ioc->dma_mask, convert_to_kb(s.totalram));
-
-	return 0;
-}
-
-static int
-_base_change_consistent_dma_mask(struct MPT3SAS_ADAPTER *ioc,
-				      struct pci_dev *pdev)
-{
-	if (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(ioc->dma_mask))) {
-		if (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)))
-			return -ENODEV;
+	if (dma_bits > 32) {
+		ioc->base_add_sg_single = &_base_add_sg_single_64;
+		ioc->sge_size = sizeof(Mpi2SGESimple64_t);
+	} else {
+		ioc->base_add_sg_single = &_base_add_sg_single_32;
+		ioc->sge_size = sizeof(Mpi2SGESimple32_t);
 	}
+
+	si_meminfo(&s);
+	ioc_info(ioc,
+		"%d BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (%ld kB)\n",
+		dma_bits, convert_to_kb(s.totalram));
 	return 0;
 }
 
@@ -5169,14 +5147,6 @@ _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
 		total_sz += sz;
 	} while (ioc->rdpq_array_enable && (++i < ioc->reply_queue_count));
 
-	if (ioc->dma_mask > 32) {
-		if (_base_change_consistent_dma_mask(ioc, ioc->pdev) != 0) {
-			ioc_warn(ioc, "no suitable consistent DMA mask for %s\n",
-				 pci_name(ioc->pdev));
-			goto out;
-		}
-	}
-
 	ioc->scsiio_depth = ioc->hba_queue_depth -
 	    ioc->hi_priority_depth - ioc->internal_depth;
 
@@ -7158,7 +7128,6 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
 	ioc->smp_affinity_enable = smp_affinity_enable;
 
 	ioc->rdpq_array_enable_assigned = 0;
-	ioc->dma_mask = 0;
 	if (ioc->is_aero_ioc)
 		ioc->base_readl = &_base_readl_aero;
 	else
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index e7197150721f..caae04086539 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1026,7 +1026,6 @@ typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
  * @ir_firmware: IR firmware present
  * @bars: bitmask of BAR's that must be configured
  * @mask_interrupts: ignore interrupt
- * @dma_mask: used to set the consistent dma mask
  * @pci_access_mutex: Mutex to synchronize ioctl, sysfs show path and
  *			pci resource handling
  * @fault_reset_work_q_name: fw fault work queue
@@ -1205,7 +1204,6 @@ struct MPT3SAS_ADAPTER {
 	u8		ir_firmware;
 	int		bars;
 	u8		mask_interrupts;
-	int		dma_mask;
 
 	/* fw fault handler */
 	char		fault_reset_work_q_name[20];
diff mbox series

Patch

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 663782b..8e937c8 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2806,55 +2806,40 @@  _base_build_sg_ieee(struct MPT3SAS_ADAPTER *ioc, void *psge,
 static int
 _base_config_dma_addressing(struct MPT3SAS_ADAPTER *ioc, struct pci_dev *pdev)
 {
-	u64 required_mask, coherent_mask;
 	struct sysinfo s;
-	/* Set 63 bit DMA mask for all SAS3 and SAS35 controllers */
-	int dma_mask = (ioc->hba_mpi_version_belonged > MPI2_VERSION) ? 63 : 64;
+	char *desc = "64";
+	u64 consistent_dma_mask = DMA_BIT_MASK(64);
+	u64 required_mask = dma_get_required_mask(&pdev->dev);
 
-	if (ioc->is_mcpu_endpoint)
-		goto try_32bit;
-
-	required_mask = dma_get_required_mask(&pdev->dev);
-	if (sizeof(dma_addr_t) == 4 || required_mask == 32)
-		goto try_32bit;
-
-	if (ioc->dma_mask)
-		coherent_mask = DMA_BIT_MASK(dma_mask);
-	else
-		coherent_mask = DMA_BIT_MASK(32);
-
-	if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(dma_mask)) ||
-	    dma_set_coherent_mask(&pdev->dev, coherent_mask))
+	if (ioc->is_mcpu_endpoint || ioc->is_dma_32bit ||
+	    sizeof(dma_addr_t) == 4 || required_mask == DMA_BIT_MASK(32))
 		goto try_32bit;
-
-	ioc->base_add_sg_single = &_base_add_sg_single_64;
-	ioc->sge_size = sizeof(Mpi2SGESimple64_t);
-	ioc->dma_mask = dma_mask;
-	goto out;
-
- try_32bit:
-	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)))
+	/*
+	 * Set 63 bit DMA mask for all SAS3 and SAS35 controllers
+	 */
+	if (ioc->hba_mpi_version_belonged > MPI2_VERSION) {
+		consistent_dma_mask = DMA_BIT_MASK(63);
+		desc = "63";
+	}
+	if (!dma_set_mask(&pdev->dev, consistent_dma_mask) &&
+	    !dma_set_coherent_mask(&pdev->dev, consistent_dma_mask)) {
+		ioc->base_add_sg_single = &_base_add_sg_single_64;
+		ioc->sge_size = sizeof(Mpi2SGESimple64_t);
+		goto out;
+	}
+try_32bit:
+	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))
+	    && !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) {
+		ioc->base_add_sg_single = &_base_add_sg_single_32;
+		ioc->sge_size = sizeof(Mpi2SGESimple32_t);
+		desc = "32";
+	} else
 		return -ENODEV;
-
-	ioc->base_add_sg_single = &_base_add_sg_single_32;
-	ioc->sge_size = sizeof(Mpi2SGESimple32_t);
-	ioc->dma_mask = 32;
- out:
+out:
 	si_meminfo(&s);
-	ioc_info(ioc, "%d BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (%ld kB)\n",
-		 ioc->dma_mask, convert_to_kb(s.totalram));
-
-	return 0;
-}
-
-static int
-_base_change_consistent_dma_mask(struct MPT3SAS_ADAPTER *ioc,
-				      struct pci_dev *pdev)
-{
-	if (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(ioc->dma_mask))) {
-		if (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)))
-			return -ENODEV;
-	}
+	ioc_info(ioc,
+		"%s BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (%ld kB)\n",
+		desc, convert_to_kb(s.totalram));
 	return 0;
 }
 
@@ -5169,14 +5154,6 @@  _base_allocate_memory_pools(struct MPT3SAS_ADAPTER *ioc)
 		total_sz += sz;
 	} while (ioc->rdpq_array_enable && (++i < ioc->reply_queue_count));
 
-	if (ioc->dma_mask > 32) {
-		if (_base_change_consistent_dma_mask(ioc, ioc->pdev) != 0) {
-			ioc_warn(ioc, "no suitable consistent DMA mask for %s\n",
-				 pci_name(ioc->pdev));
-			goto out;
-		}
-	}
-
 	ioc->scsiio_depth = ioc->hba_queue_depth -
 	    ioc->hi_priority_depth - ioc->internal_depth;
 
@@ -7158,7 +7135,7 @@  mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
 	ioc->smp_affinity_enable = smp_affinity_enable;
 
 	ioc->rdpq_array_enable_assigned = 0;
-	ioc->dma_mask = 0;
+	ioc->is_dma_32bit = 0;
 	if (ioc->is_aero_ioc)
 		ioc->base_readl = &_base_readl_aero;
 	else
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index e719715..396ac96 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1026,7 +1026,7 @@  typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
  * @ir_firmware: IR firmware present
  * @bars: bitmask of BAR's that must be configured
  * @mask_interrupts: ignore interrupt
- * @dma_mask: used to set the consistent dma mask
+ * @is_dma_32bit: used to set the consistent dma mask
  * @pci_access_mutex: Mutex to synchronize ioctl, sysfs show path and
  *			pci resource handling
  * @fault_reset_work_q_name: fw fault work queue
@@ -1205,7 +1205,7 @@  struct MPT3SAS_ADAPTER {
 	u8		ir_firmware;
 	int		bars;
 	u8		mask_interrupts;
-	int		dma_mask;
+	int		is_dma_32bit;
 
 	/* fw fault handler */
 	char		fault_reset_work_q_name[20];