Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1351262 > unrolled thread

[PATCH v4 0/3] mmc: sdhci: Set DMA mask properly

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] mmc: sdhci: Set DMA mask properly Alexandre Courbot <acourbot@nvidia.com> - 2016-03-07 03:10 +0100
    [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma() Alexandre Courbot <acourbot@nvidia.com> - 2016-03-07 03:10 +0100
      Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in  enable_dma() Adrian Hunter <adrian.hunter@intel.com> - 2016-03-08 14:30 +0100
        Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma() Alexandre Courbot <gnurou@gmail.com> - 2016-03-14 04:20 +0100
          Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in  enable_dma() Adrian Hunter <adrian.hunter@intel.com> - 2016-03-14 14:10 +0100
            Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in  enable_dma() Bjorn Helgaas <helgaas@kernel.org> - 2016-03-14 17:00 +0100
    Re: [PATCH v4 0/3] mmc: sdhci: Set DMA mask properly Adrian Hunter <adrian.hunter@intel.com> - 2016-03-16 14:00 +0100
    Re: [PATCH v4 0/3] mmc: sdhci: Set DMA mask properly Ulf Hansson <ulf.hansson@linaro.org> - 2016-03-16 14:10 +0100

#1351262 — [PATCH v4 0/3] mmc: sdhci: Set DMA mask properly

64-bit capable devices are supposed to set their own DMA mask. Currently
this does not happen for sdhci devices excepted for the two (sdhci-acpi
and sdhci-pci) that define a enable_dma() hook and do it there. However
this hook is called from several places while DMA mask is supposed to
be set only once ; for instance the sdhci-acpi driver maintains a flag
just to make sure the DMA mask is set only upon the first call of this
hook.

For the vast majority of drivers that do not define a enable_dma() hook, the
default 32-bit DMA mask is used and there is a risk of using unneeded bounce
buffers on hosts capable of 64-bit addressing.

The first patch adds a default DMA mask setting function that is called when
a DMA-capable host is added. It tries to set sane DMA masks according to the
device's reported capabilities.

The addition of this function seems to make the same code in sdhci-acpi and
sdhci-pci redundant, so it is removed from these drivers. On top of making
this series a negative line count, it also removes one usage of the obsolete
pci_set_dma_mask() function.

Changes since v3:
- Unset the SDHCI_USE_64_BIT_DMA flag if setting of 64-bit mask failed
- Carry Acked-bys

Alexandre Courbot (3):
  mmc: sdhci: Set DMA mask when adding host
  mmc: sdhci-acpi: Remove enable_dma() hook
  mmc: sdhci-pci: Do not set DMA mask in enable_dma()

 drivers/mmc/host/sdhci-acpi.c     | 30 ------------------------
 drivers/mmc/host/sdhci-pci-core.c | 15 ------------
 drivers/mmc/host/sdhci.c          | 48 +++++++++++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 52 deletions(-)

-- 
2.7.2

[toc] | [next] | [standalone]

#1351263 — [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma()

DMA mask will already be set by sdhci_set_dma_mask(), which
is equivalent to the removed code since pci_set_dma_mask()
expands to its DMA-API counterpart.

There should also be no reason to set the DMA mask after probe.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mmc/host/sdhci-pci-core.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index df3b8eced8c4..62aa5d0efcee 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -1302,7 +1302,6 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
 {
 	struct sdhci_pci_slot *slot;
 	struct pci_dev *pdev;
-	int ret = -1;
 
 	slot = sdhci_priv(host);
 	pdev = slot->chip->pdev;
@@ -1314,20 +1313,6 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
 			"doesn't fully claim to support it.\n");
 	}
 
-	if (host->flags & SDHCI_USE_64_BIT_DMA) {
-		if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) {
-			host->flags &= ~SDHCI_USE_64_BIT_DMA;
-		} else {
-			ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
-			if (ret)
-				dev_warn(&pdev->dev, "Failed to set 64-bit DMA mask\n");
-		}
-	}
-	if (ret)
-		ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-
 	pci_set_master(pdev);
 
 	return 0;
-- 
2.7.2

[toc] | [prev] | [next] | [standalone]

#1353036 — Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma()

On 07/03/16 04:07, Alexandre Courbot wrote:
> DMA mask will already be set by sdhci_set_dma_mask(), which
> is equivalent to the removed code since pci_set_dma_mask()
> expands to its DMA-API counterpart.
> 
> There should also be no reason to set the DMA mask after probe.

Let's run that by the PCI mailing list just to be sure.  The patches for
reference:

	http://marc.info/?l=linux-mmc&m=145731654328126&w=2
	http://marc.info/?l=linux-mmc&m=145731654328128&w=2

change the sdhci-pci driver to set the DMA mask once during probe instead of
every time during resume.  Is there any reason a PCI device driver might
need to set the DMA mask every time during resume?


> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/mmc/host/sdhci-pci-core.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index df3b8eced8c4..62aa5d0efcee 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -1302,7 +1302,6 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
>  {
>  	struct sdhci_pci_slot *slot;
>  	struct pci_dev *pdev;
> -	int ret = -1;
>  
>  	slot = sdhci_priv(host);
>  	pdev = slot->chip->pdev;
> @@ -1314,20 +1313,6 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host)
>  			"doesn't fully claim to support it.\n");
>  	}
>  
> -	if (host->flags & SDHCI_USE_64_BIT_DMA) {
> -		if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) {
> -			host->flags &= ~SDHCI_USE_64_BIT_DMA;
> -		} else {
> -			ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> -			if (ret)
> -				dev_warn(&pdev->dev, "Failed to set 64-bit DMA mask\n");
> -		}
> -	}
> -	if (ret)
> -		ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> -	if (ret)
> -		return ret;
> -
>  	pci_set_master(pdev);
>  
>  	return 0;
> 

[toc] | [prev] | [next] | [standalone]

#1356869 — Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma()

On Tue, Mar 8, 2016 at 10:18 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 07/03/16 04:07, Alexandre Courbot wrote:
>> DMA mask will already be set by sdhci_set_dma_mask(), which
>> is equivalent to the removed code since pci_set_dma_mask()
>> expands to its DMA-API counterpart.
>>
>> There should also be no reason to set the DMA mask after probe.
>
> Let's run that by the PCI mailing list just to be sure.  The patches for
> reference:
>
>         http://marc.info/?l=linux-mmc&m=145731654328126&w=2
>         http://marc.info/?l=linux-mmc&m=145731654328128&w=2
>
> change the sdhci-pci driver to set the DMA mask once during probe instead of
> every time during resume.  Is there any reason a PCI device driver might
> need to set the DMA mask every time during resume?

Not seeing much reaction for this patchset. PCI being the only
possible point of contention, can we maybe roll it into -next and see
what happens?

[toc] | [prev] | [next] | [standalone]

#1357226 — Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma()

On 14/03/16 05:15, Alexandre Courbot wrote:
> On Tue, Mar 8, 2016 at 10:18 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 07/03/16 04:07, Alexandre Courbot wrote:
>>> DMA mask will already be set by sdhci_set_dma_mask(), which
>>> is equivalent to the removed code since pci_set_dma_mask()
>>> expands to its DMA-API counterpart.
>>>
>>> There should also be no reason to set the DMA mask after probe.
>>
>> Let's run that by the PCI mailing list just to be sure.  The patches for
>> reference:
>>
>>         http://marc.info/?l=linux-mmc&m=145731654328126&w=2
>>         http://marc.info/?l=linux-mmc&m=145731654328128&w=2
>>
>> change the sdhci-pci driver to set the DMA mask once during probe instead of
>> every time during resume.  Is there any reason a PCI device driver might
>> need to set the DMA mask every time during resume?
> 
> Not seeing much reaction for this patchset. PCI being the only
> possible point of contention, can we maybe roll it into -next and see
> what happens?

+Rafael

Rafael, can you offer any thoughts on this:

PCI drivers that want to use DMA might call pci_set_master() in the pm
resume callback.  Some drivers (like sdhci-pci) also, presumably out of
convenience, set the DMA mask at the same time.  The question is: is it OK
instead to set the DMA mask just once during probe, or is there some other
reason the DMA mask needs to be set every time during resume?

[toc] | [prev] | [next] | [standalone]

#1357391 — Re: [PATCH v4 3/3] mmc: sdhci-pci: Do not set DMA mask in enable_dma()

On Mon, Mar 14, 2016 at 03:00:36PM +0200, Adrian Hunter wrote:
> On 14/03/16 05:15, Alexandre Courbot wrote:
> > On Tue, Mar 8, 2016 at 10:18 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> On 07/03/16 04:07, Alexandre Courbot wrote:
> >>> DMA mask will already be set by sdhci_set_dma_mask(), which
> >>> is equivalent to the removed code since pci_set_dma_mask()
> >>> expands to its DMA-API counterpart.
> >>>
> >>> There should also be no reason to set the DMA mask after probe.
> >>
> >> Let's run that by the PCI mailing list just to be sure.  The patches for
> >> reference:
> >>
> >>         http://marc.info/?l=linux-mmc&m=145731654328126&w=2
> >>         http://marc.info/?l=linux-mmc&m=145731654328128&w=2
> >>
> >> change the sdhci-pci driver to set the DMA mask once during probe instead of
> >> every time during resume.  Is there any reason a PCI device driver might
> >> need to set the DMA mask every time during resume?
> > 
> > Not seeing much reaction for this patchset. PCI being the only
> > possible point of contention, can we maybe roll it into -next and see
> > what happens?
> 
> +Rafael
> 
> Rafael, can you offer any thoughts on this:
> 
> PCI drivers that want to use DMA might call pci_set_master() in the pm
> resume callback.  Some drivers (like sdhci-pci) also, presumably out of
> convenience, set the DMA mask at the same time.  The question is: is it OK
> instead to set the DMA mask just once during probe, or is there some other
> reason the DMA mask needs to be set every time during resume?

I don't see a reason why the DMA mask would need to be set during
resume.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1358964

On 07/03/16 04:07, Alexandre Courbot wrote:
> 64-bit capable devices are supposed to set their own DMA mask. Currently
> this does not happen for sdhci devices excepted for the two (sdhci-acpi
> and sdhci-pci) that define a enable_dma() hook and do it there. However
> this hook is called from several places while DMA mask is supposed to
> be set only once ; for instance the sdhci-acpi driver maintains a flag
> just to make sure the DMA mask is set only upon the first call of this
> hook.
> 
> For the vast majority of drivers that do not define a enable_dma() hook, the
> default 32-bit DMA mask is used and there is a risk of using unneeded bounce
> buffers on hosts capable of 64-bit addressing.
> 
> The first patch adds a default DMA mask setting function that is called when
> a DMA-capable host is added. It tries to set sane DMA masks according to the
> device's reported capabilities.
> 
> The addition of this function seems to make the same code in sdhci-acpi and
> sdhci-pci redundant, so it is removed from these drivers. On top of making
> this series a negative line count, it also removes one usage of the obsolete
> pci_set_dma_mask() function.
> 
> Changes since v3:
> - Unset the SDHCI_USE_64_BIT_DMA flag if setting of 64-bit mask failed
> - Carry Acked-bys
> 
> Alexandre Courbot (3):
>   mmc: sdhci: Set DMA mask when adding host
>   mmc: sdhci-acpi: Remove enable_dma() hook
>   mmc: sdhci-pci: Do not set DMA mask in enable_dma()
> 
>  drivers/mmc/host/sdhci-acpi.c     | 30 ------------------------
>  drivers/mmc/host/sdhci-pci-core.c | 15 ------------
>  drivers/mmc/host/sdhci.c          | 48 +++++++++++++++++++++++++++++++++------
>  3 files changed, 41 insertions(+), 52 deletions(-)
> 

I had a couple of questions which have been answered, so for all 3 patches:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

[toc] | [prev] | [next] | [standalone]

#1358972

On 7 March 2016 at 03:07, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 64-bit capable devices are supposed to set their own DMA mask. Currently
> this does not happen for sdhci devices excepted for the two (sdhci-acpi
> and sdhci-pci) that define a enable_dma() hook and do it there. However
> this hook is called from several places while DMA mask is supposed to
> be set only once ; for instance the sdhci-acpi driver maintains a flag
> just to make sure the DMA mask is set only upon the first call of this
> hook.
>
> For the vast majority of drivers that do not define a enable_dma() hook, the
> default 32-bit DMA mask is used and there is a risk of using unneeded bounce
> buffers on hosts capable of 64-bit addressing.
>
> The first patch adds a default DMA mask setting function that is called when
> a DMA-capable host is added. It tries to set sane DMA masks according to the
> device's reported capabilities.
>
> The addition of this function seems to make the same code in sdhci-acpi and
> sdhci-pci redundant, so it is removed from these drivers. On top of making
> this series a negative line count, it also removes one usage of the obsolete
> pci_set_dma_mask() function.
>
> Changes since v3:
> - Unset the SDHCI_USE_64_BIT_DMA flag if setting of 64-bit mask failed
> - Carry Acked-bys
>
> Alexandre Courbot (3):
>   mmc: sdhci: Set DMA mask when adding host
>   mmc: sdhci-acpi: Remove enable_dma() hook
>   mmc: sdhci-pci: Do not set DMA mask in enable_dma()
>
>  drivers/mmc/host/sdhci-acpi.c     | 30 ------------------------
>  drivers/mmc/host/sdhci-pci-core.c | 15 ------------
>  drivers/mmc/host/sdhci.c          | 48 +++++++++++++++++++++++++++++++++------
>  3 files changed, 41 insertions(+), 52 deletions(-)
>
> --
> 2.7.2
>

Thanks, applied for next!

Kind regards
Uffe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web