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

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

[Adrian Hunter]

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;
> 

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

[Alexandre Courbot]

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?

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

[Adrian Hunter]

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?

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

[Bjorn Helgaas]

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