Xen devel - Nov 2013 - [PATCH 0/2] swiotlb-xen fixes

Hi all,
this patch series is just a couple of fixes for swiotlb-xen.

In particular the slow path (bouncing on the swiotlb buffer for dma
operations) on ARM has two problems, each of them fixed by a separate
patch:

- a xen_dma_map_page call is missing in xen_swiotlb_map_sg_attrs;

- dma_capable often fails on ARM because devices don''t set the dma_mask
  correctly. This causes swiotlb-xen to take the slow path, and that is
  bad enough, but in xen_swiotlb_map_page also causes the function to
  return an error an fail.



Stefano Stabellini (2):
      swiotlb-xen: add missing xen_dma_map_page call
      swiotlb-xen: xen_swiotlb_map_page: do not error out if dma_capable fails

 drivers/xen/swiotlb-xen.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)


git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git
for-linus-3.13-fixes

Cheers,

Stefano

swiotlb-xen is missing a xen_dma_map_page call in
xen_swiotlb_map_sg_attrs, in the slow path.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/swiotlb-xen.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a224bc7..1eac073 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -555,6 +555,11 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct
scatterlist *sgl,
 				sg_dma_len(sgl) = 0;
 				return 0;
 			}
+			xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT),
+						map & ~PAGE_MASK,
+						sg->length,
+						dir,
+						attrs);
 			sg->dma_address = xen_phys_to_bus(map);
 		} else {
 			/* we are not interested in the dma_addr returned by
-- 
1.7.2.5

Many ARM devices do not set the dma_mask correctly today.
As a consequence dma_capable fails for them regardless of the address
passed to it.
In xen_swiotlb_map_page we currently use dma_capable to check if the
address returned by the swiotlb is good for dma for the device.
However the check would often fail even if the address is correct.
Considering that we know that the swiotlb buffer has a low address,
skip the check.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/swiotlb-xen.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 1eac073..543e30b 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -402,13 +402,6 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct
page *page,
 					map & ~PAGE_MASK, size, dir, attrs);
 	dev_addr = xen_phys_to_bus(map);
 
-	/*
-	 * Ensure that the address returned is DMA''ble
-	 */
-	if (!dma_capable(dev, dev_addr, size)) {
-		swiotlb_tbl_unmap_single(dev, map, size, dir);
-		dev_addr = 0;
-	}
 	return dev_addr;
 }
 EXPORT_SYMBOL_GPL(xen_swiotlb_map_page);
-- 
1.7.2.5

On Tue, Nov 12, 2013 at 02:11:59PM +0000, Stefano Stabellini
wrote:
> swiotlb-xen is missing a xen_dma_map_page call in
> xen_swiotlb_map_sg_attrs, in the slow path.
s/slow/bounce buffer/ I believe?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/xen/swiotlb-xen.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index a224bc7..1eac073 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -555,6 +555,11 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct
scatterlist *sgl,
>  				sg_dma_len(sgl) = 0;
>  				return 0;
>  			}
> +			xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT),
> +						map & ~PAGE_MASK,
> +						sg->length,
> +						dir,
> +						attrs);
>  			sg->dma_address = xen_phys_to_bus(map);
>  		} else {
>  			/* we are not interested in the dma_addr returned by
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Tue, Nov 12, 2013 at 02:12:00PM +0000, Stefano Stabellini
wrote:
> Many ARM devices do not set the dma_mask correctly today.
> As a consequence dma_capable fails for them regardless of the address
> passed to it.
Wouldn''t the DMA API debug warn of bad usage.
> In xen_swiotlb_map_page we currently use dma_capable to check if the
> address returned by the swiotlb is good for dma for the device.
Right..
> However the check would often fail even if the address is correct.
 .. and they will fail b/c the device hasn''t set its dma mask so
we use the platform default right? What is the platform default?
> Considering that we know that the swiotlb buffer has a low address,
> skip the check.
I am not following that sentence. Could you please explain to me
how the SWIOTLB buffer low address guarantees that we don''t need
the check?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
What are the drivers that are busted. Does DMA API debug flag them?

Thanks!
> ---
>  drivers/xen/swiotlb-xen.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 1eac073..543e30b 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -402,13 +402,6 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
>  					map & ~PAGE_MASK, size, dir, attrs);
>  	dev_addr = xen_phys_to_bus(map);
>  
> -	/*
> -	 * Ensure that the address returned is DMA''ble
> -	 */
> -	if (!dma_capable(dev, dev_addr, size)) {
> -		swiotlb_tbl_unmap_single(dev, map, size, dir);
> -		dev_addr = 0;
> -	}
>  	return dev_addr;
>  }
>  EXPORT_SYMBOL_GPL(xen_swiotlb_map_page);
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Tue, Nov 12, 2013 at 09:48:32AM -0500, Konrad Rzeszutek Wilk
wrote:
> On Tue, Nov 12, 2013 at 02:12:00PM +0000, Stefano Stabellini wrote:
> > Many ARM devices do not set the dma_mask correctly today.
> > As a consequence dma_capable fails for them regardless of the address
> > passed to it.
> 
> Wouldn''t the DMA API debug warn of bad usage.
No.  The problem is that dev->dma_mask is NULL, and that''s generally
interpreted as "this device doesn''t do DMA".

ARM drivers have been particularly bad in respect of getting this right:
there''s an overwhelming problem here of "I don''t need to
do that so I''m
not going to do it" rather than taking the attitude of "it''s
good
practice to do X because it will save me hastles in the future".

I''ll admit that even I''m guilty of the former in this regard
to some
extent: I''ve not added the necessary
dma_set_mask/dma_set_coherent_mask()
calls to the various drivers because they haven''t been a concern.  That
was wrong, because the DMA API technically requires it.

However, not having a DMA mask set when the device is created is
something I''m not guilty of - I always ensured with the old board
files that a DMA capabable device had a DMA mask set and those which
weren''t capable didn''t: this causes the DMA API to behave
correctly
and report according to the way the device was declared whether it is
DMA capable or not.

Unfortunately, others decided (especially post DT) that the solution
to this was to have the driver code - which could be modular - do things
like this:

static u64 mask = blah;

foo_probe(dev)
{
	if (!dev->dma_mask)
		dev->dma_mask = &mask;
}

which works nicely until you reload the module.

I strongly suggest _not_ working around this problem in subsystem code
but raising bug reports against the buggy drivers.  All drivers without
exception using DMA should have something like this pattern:

foo_probe(dev)
{
	int rc;

	rc = dma_set_mask(dev, DMA_BIT_MASK(bits));
	if (rc)
		return rc;
	dma_set_coherent_mask(dev, DMA_BIT_MASK(bits));
	...
}

The first call is required if the driver uses dma_map_*, the second call
is required if it makes use of coherent allocations and can be of the
above form if it''s previously used dma_set_mask with the same mask.
Otherwise, the return value of dma_set_coherent_mask() must be checked.

Of course, dma_set_mask() will fail if dev->dma_mask is NULL - and
that''s
something which must be fixed where the device was created, not by the
driver.

On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 12, 2013 at 02:11:59PM +0000, Stefano Stabellini wrote:
> > swiotlb-xen is missing a xen_dma_map_page call in
> > xen_swiotlb_map_sg_attrs, in the slow path.
> 
> s/slow/bounce buffer/ I believe?
right

> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  drivers/xen/swiotlb-xen.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index a224bc7..1eac073 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -555,6 +555,11 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev,
struct scatterlist *sgl,
> >  				sg_dma_len(sgl) = 0;
> >  				return 0;
> >  			}
> > +			xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT),
> > +						map & ~PAGE_MASK,
> > +						sg->length,
> > +						dir,
> > +						attrs);
> >  			sg->dma_address = xen_phys_to_bus(map);
> >  		} else {
> >  			/* we are not interested in the dma_addr returned by
> > -- 
> > 1.7.2.5
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>

Russell gave a great explanation of the issue so I am just going to
limit myself to answering to:

On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > Considering that we know that the swiotlb buffer has a low address,
> > skip the check.
> 
> I am not following that sentence. Could you please explain to me
> how the SWIOTLB buffer low address guarantees that we don''t need
> the check?
xen_swiotlb_fixup makes sure that the swiotlb buffer is lower than 4GB,
probably lower than 3GB, by passing dma_bits to
xen_create_contiguous_region.
This meets the requirements of most devices out there.
In fact we are not even running this check under the same conditions in
swiotlb_map_sg_attrs.
I admit that it is possible to come up with a scenario where the check
would be useful, but it is far easier to come up with scenarios where
not only is unneeded but it is even harmful.

Alternatively (without Rob''s "of: set dma_mask to point to
coherent_dma_mask") Linux 3.13 is going to fail to get the network
running on Midway. It is going to avoid fs mounting failures just
because we don''t do the same check in swiotlb_map_sg_attrs.

FYI given that Rob''s patch is probably going upstream soon anyway, I
don''t feel so strongly about this.

On 11/12/2013 11:27 AM, Stefano Stabellini wrote:
> Russell gave a great explanation of the issue so I am just going to
> limit myself to answering to:
> 
> On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
>>> Considering that we know that the swiotlb buffer has a low address,
>>> skip the check.
>>
>> I am not following that sentence. Could you please explain to me
>> how the SWIOTLB buffer low address guarantees that we don''t
need
>> the check?
> 
> xen_swiotlb_fixup makes sure that the swiotlb buffer is lower than 4GB,
> probably lower than 3GB, by passing dma_bits to
> xen_create_contiguous_region.
> This meets the requirements of most devices out there.
> In fact we are not even running this check under the same conditions in
> swiotlb_map_sg_attrs.
> I admit that it is possible to come up with a scenario where the check
> would be useful, but it is far easier to come up with scenarios where
> not only is unneeded but it is even harmful.
> 
> Alternatively (without Rob''s "of: set dma_mask to point to
> coherent_dma_mask") Linux 3.13 is going to fail to get the network
> running on Midway. It is going to avoid fs mounting failures just
> because we don''t do the same check in swiotlb_map_sg_attrs.
> 
> FYI given that Rob''s patch is probably going upstream soon anyway,
I
> don''t feel so strongly about this.
It is in Linus'' tree now.

Rob

On Tue, 12 Nov 2013, Rob Herring wrote:
> On 11/12/2013 11:27 AM, Stefano Stabellini wrote:
> > Russell gave a great explanation of the issue so I am just going to
> > limit myself to answering to:
> > 
> > On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> >>> Considering that we know that the swiotlb buffer has a low
address,
> >>> skip the check.
> >>
> >> I am not following that sentence. Could you please explain to me
> >> how the SWIOTLB buffer low address guarantees that we
don''t need
> >> the check?
> > 
> > xen_swiotlb_fixup makes sure that the swiotlb buffer is lower than
4GB,
> > probably lower than 3GB, by passing dma_bits to
> > xen_create_contiguous_region.
> > This meets the requirements of most devices out there.
> > In fact we are not even running this check under the same conditions
in
> > swiotlb_map_sg_attrs.
> > I admit that it is possible to come up with a scenario where the check
> > would be useful, but it is far easier to come up with scenarios where
> > not only is unneeded but it is even harmful.
> > 
> > Alternatively (without Rob''s "of: set dma_mask to point
to
> > coherent_dma_mask") Linux 3.13 is going to fail to get the
network
> > running on Midway. It is going to avoid fs mounting failures just
> > because we don''t do the same check in swiotlb_map_sg_attrs.
> > 
> > FYI given that Rob''s patch is probably going upstream soon
anyway, I
> > don''t feel so strongly about this.
> 
> It is in Linus'' tree now.
 
OK,  I''ll drop this patch then.