Re: [PATCH 30/51] DMA-API: dma: dw_dmac.c: convert to use dma_coerce_mask_and_coherent()

Re: [PATCH 30/51] DMA-API: dma: dw_dmac.c: convert to use dma_coerce_mask_and_coherent()

[Andy Shevchenko]

On Thu, 2013-09-19 at 22:55 +0100, Russell King wrote:
> This code sequence:
> 	if (!pdev->dev.dma_mask) {
> 		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> 		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> 	}
> bypasses the architectures check on the DMA mask.  It can be replaced
> with dma_coerce_mask_and_coherent(), avoiding the direct initialization
> of this mask.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/dma/dw/platform.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index e35d975..453822c 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -191,11 +191,9 @@ static int dw_probe(struct platform_device *pdev)
>  	if (IS_ERR(chip->regs))
>  		return PTR_ERR(chip->regs);
>  
> -	/* Apply default dma_mask if needed */
> -	if (!dev->dma_mask) {
> -		dev->dma_mask = &dev->coherent_dma_mask;
> -		dev->coherent_dma_mask = DMA_BIT_MASK(32);
> -	}
> +	err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	if (err)
> +		return err;

I have at least one question.

In case of new code you always assign dev->dma_mask.

static inline int dma_coerce_mask_and_coherent(struct device *dev, u64
mask)
{
       dev->dma_mask = &dev->coherent_dma_mask;
       return dma_set_mask_and_coherent(dev, mask);
}

So, the question is how keep the initialized dma_mask (and should we do
so by your opinion)?

Additional question, who will free memory allocated for dma_mask in case
if it was allocated, dma_mask initialized and then your code just
substitute the address to some new value?


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 30/51] DMA-API: dma: dw_dmac.c: convert to use dma_coerce_mask_and_coherent()

[Russell King - ARM Linux]

On Fri, Sep 20, 2013 at 05:26:46PM +0300, Andy Shevchenko wrote:
> On Thu, 2013-09-19 at 22:55 +0100, Russell King wrote:
> > This code sequence:
> > 	if (!pdev->dev.dma_mask) {
> > 		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> > 		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> > 	}
> > bypasses the architectures check on the DMA mask.  It can be replaced
> > with dma_coerce_mask_and_coherent(), avoiding the direct initialization
> > of this mask.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/dma/dw/platform.c |    8 +++-----
> >  1 files changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> > index e35d975..453822c 100644
> > --- a/drivers/dma/dw/platform.c
> > +++ b/drivers/dma/dw/platform.c
> > @@ -191,11 +191,9 @@ static int dw_probe(struct platform_device *pdev)
> >  	if (IS_ERR(chip->regs))
> >  		return PTR_ERR(chip->regs);
> >  
> > -	/* Apply default dma_mask if needed */
> > -	if (!dev->dma_mask) {
> > -		dev->dma_mask = &dev->coherent_dma_mask;
> > -		dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > -	}
> > +	err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > +	if (err)
> > +		return err;
> 
> I have at least one question.
> 
> In case of new code you always assign dev->dma_mask.
> 
> static inline int dma_coerce_mask_and_coherent(struct device *dev, u64
> mask)
> {
>        dev->dma_mask = &dev->coherent_dma_mask;
>        return dma_set_mask_and_coherent(dev, mask);
> }
> 
> So, the question is how keep the initialized dma_mask (and should we do
> so by your opinion)?

Well, the way the DMA mask stuff is supposed to operate is:

- The device creator initializes the DMA mask to some default value.
- The driver then uses dma_set_mask() / dma_set_coherent_mask() /
  dma_set_mask_and_coherent() to adjust the mask according to the
  capabilities of the device, *even* if the mask is the same as the
  default.

This is specified in the various DMA API documents.

So, in PCI land, it works like this:
- When a PCI device is created, it has its mask set to 32-bit.
- When a driver comes along
  - if the device is capable of 64-bit addressing, it tries to set a
    64-bit mask.  If this fails, it tries to set a 32-bit mask and
    turns off 64-bit DMA.
  - if a device is not capable of 32-bit addressing but of a smaller
    space (there are some PCI devices which can only do 31-bit) then
    it tries to set that mask.
  If the driver can't successfully set a mask, it should fail to
  initialise.

This is where we should be headed with all drivers, and I would welcome
a patch for this driver to make it conform wrt the DMA API and DMA masks
in place of this patch.

Think of the coerse stuff as a middle-step to bring these types of issues
up to the fore.

Re: [PATCH 30/51] DMA-API: dma: dw_dmac.c: convert to use dma_coerce_mask_and_coherent()

[Andy Shevchenko]

On Fri, Sep 20, 2013 at 5:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Sep 20, 2013 at 05:26:46PM +0300, Andy Shevchenko wrote:
>> On Thu, 2013-09-19 at 22:55 +0100, Russell King wrote:
>> > This code sequence:
>> >     if (!pdev->dev.dma_mask) {
>> >             pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>> >             pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>> >     }
>> > bypasses the architectures check on the DMA mask.  It can be replaced
>> > with dma_coerce_mask_and_coherent(), avoiding the direct initialization
>> > of this mask.
>> >
>> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>> > ---
>> >  drivers/dma/dw/platform.c |    8 +++-----
>> >  1 files changed, 3 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
>> > index e35d975..453822c 100644
>> > --- a/drivers/dma/dw/platform.c
>> > +++ b/drivers/dma/dw/platform.c
>> > @@ -191,11 +191,9 @@ static int dw_probe(struct platform_device *pdev)
>> >     if (IS_ERR(chip->regs))
>> >             return PTR_ERR(chip->regs);
>> >
>> > -   /* Apply default dma_mask if needed */
>> > -   if (!dev->dma_mask) {
>> > -           dev->dma_mask = &dev->coherent_dma_mask;
>> > -           dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> > -   }
>> > +   err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> > +   if (err)
>> > +           return err;
>>
>> I have at least one question.
>>
>> In case of new code you always assign dev->dma_mask.
>>
>> static inline int dma_coerce_mask_and_coherent(struct device *dev, u64
>> mask)
>> {
>>        dev->dma_mask = &dev->coherent_dma_mask;
>>        return dma_set_mask_and_coherent(dev, mask);
>> }
>>
>> So, the question is how keep the initialized dma_mask (and should we do
>> so by your opinion)?
>
> Well, the way the DMA mask stuff is supposed to operate is:
>
> - The device creator initializes the DMA mask to some default value.
> - The driver then uses dma_set_mask() / dma_set_coherent_mask() /
>   dma_set_mask_and_coherent() to adjust the mask according to the
>   capabilities of the device, *even* if the mask is the same as the
>   default.
>
> This is specified in the various DMA API documents.
>
> So, in PCI land, it works like this:
> - When a PCI device is created, it has its mask set to 32-bit.
> - When a driver comes along
>   - if the device is capable of 64-bit addressing, it tries to set a
>     64-bit mask.  If this fails, it tries to set a 32-bit mask and
>     turns off 64-bit DMA.
>   - if a device is not capable of 32-bit addressing but of a smaller
>     space (there are some PCI devices which can only do 31-bit) then
>     it tries to set that mask.
>   If the driver can't successfully set a mask, it should fail to
>   initialise.
>

Thanks for explanation.

> This is where we should be headed with all drivers, and I would welcome
> a patch for this driver to make it conform wrt the DMA API and DMA masks
> in place of this patch.

I see.

> Think of the coerse stuff as a middle-step to bring these types of issues
> up to the fore.

I will check your patches on next week, though I think it will neglect
dma_mask set by PCI part of the code in PCI case of the driver.

Well, there are a lot of drivers that are designed like:
- core part as library *or* separate driver
- platform driver
- pci driver
- acpi driver
- ... whatever bus driver for this device

Core part doesn't know what the upper layer (bus layer) sets, and
approaches to set that may differ from bus to bus (e.g. PCI vs. ACPI
).
So, ACPI could set the proper values from its own guts and in core
driver we have to respect those values.

Tioday is Friday, and I'm too lazy now to think a bit how this could
be solved in case of your helper involved.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 30/51] DMA-API: dma: dw_dmac.c: convert to use dma_coerce_mask_and_coherent()

[Vinod Koul]

On Fri, Sep 20, 2013 at 06:26:16PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 20, 2013 at 5:40 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Sep 20, 2013 at 05:26:46PM +0300, Andy Shevchenko wrote:
> >> On Thu, 2013-09-19 at 22:55 +0100, Russell King wrote:
> >> > This code sequence:
> >> >     if (!pdev->dev.dma_mask) {
> >> >             pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> >> >             pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> >> >     }
> >> > bypasses the architectures check on the DMA mask.  It can be replaced
> >> > with dma_coerce_mask_and_coherent(), avoiding the direct initialization
> >> > of this mask.
> >> >
> >> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> >> > ---
> >> >  drivers/dma/dw/platform.c |    8 +++-----
> >> >  1 files changed, 3 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> >> > index e35d975..453822c 100644
> >> > --- a/drivers/dma/dw/platform.c
> >> > +++ b/drivers/dma/dw/platform.c
> >> > @@ -191,11 +191,9 @@ static int dw_probe(struct platform_device *pdev)
> >> >     if (IS_ERR(chip->regs))
> >> >             return PTR_ERR(chip->regs);
> >> >
> >> > -   /* Apply default dma_mask if needed */
> >> > -   if (!dev->dma_mask) {
> >> > -           dev->dma_mask = &dev->coherent_dma_mask;
> >> > -           dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> > -   }
> >> > +   err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> >> > +   if (err)
> >> > +           return err;
> >>
> >> I have at least one question.
> >>
> >> In case of new code you always assign dev->dma_mask.
> >>
> >> static inline int dma_coerce_mask_and_coherent(struct device *dev, u64
> >> mask)
> >> {
> >>        dev->dma_mask = &dev->coherent_dma_mask;
> >>        return dma_set_mask_and_coherent(dev, mask);
> >> }
> >>
> >> So, the question is how keep the initialized dma_mask (and should we do
> >> so by your opinion)?
> >
> > Well, the way the DMA mask stuff is supposed to operate is:
> >
> > - The device creator initializes the DMA mask to some default value.
> > - The driver then uses dma_set_mask() / dma_set_coherent_mask() /
> >   dma_set_mask_and_coherent() to adjust the mask according to the
> >   capabilities of the device, *even* if the mask is the same as the
> >   default.
> >
> > This is specified in the various DMA API documents.
> >
> > So, in PCI land, it works like this:
> > - When a PCI device is created, it has its mask set to 32-bit.
> > - When a driver comes along
> >   - if the device is capable of 64-bit addressing, it tries to set a
> >     64-bit mask.  If this fails, it tries to set a 32-bit mask and
> >     turns off 64-bit DMA.
> >   - if a device is not capable of 32-bit addressing but of a smaller
> >     space (there are some PCI devices which can only do 31-bit) then
> >     it tries to set that mask.
> >   If the driver can't successfully set a mask, it should fail to
> >   initialise.
> >
> 
> Thanks for explanation.
> 
> > This is where we should be headed with all drivers, and I would welcome
> > a patch for this driver to make it conform wrt the DMA API and DMA masks
> > in place of this patch.
> 
> I see.
> 
> > Think of the coerse stuff as a middle-step to bring these types of issues
> > up to the fore.
> 
> I will check your patches on next week, though I think it will neglect
> dma_mask set by PCI part of the code in PCI case of the driver.
> 
> Well, there are a lot of drivers that are designed like:
> - core part as library *or* separate driver
> - platform driver
> - pci driver
> - acpi driver
> - ... whatever bus driver for this device
> 
> Core part doesn't know what the upper layer (bus layer) sets, and
> approaches to set that may differ from bus to bus (e.g. PCI vs. ACPI
> ).
> So, ACPI could set the proper values from its own guts and in core
> driver we have to respect those values.
> 
> Tioday is Friday, and I'm too lazy now to think a bit how this could
> be solved in case of your helper involved.
okay its monday now :)

for dw_dmac where you have core part and platform part, since the capablties of
the *device* are known to the core part, IMO it is the job of core to set the
correct value as Russell described.

The platform or device creator may have set somthing else but when driver loads
in the core part of you understand the device and should reset the values...

~Vinod

--