[PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

[PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

[Hector Martin]

At least some JMicron controllers issue buggy oversized DMA reads when
fetching context descriptors, always fetching 0x20 bytes at once for
descriptors which are only 0x10 bytes long. This is often harmless, but
can cause page faults on modern systems with IOMMUs:

DMAR: [DMA Read] Request device [05:00.0] fault addr fff56000 [fault reason 06] PTE Read access is not set
firewire_ohci 0000:05:00.0: DMA context IT0 has stopped, error code: evt_descriptor_read

This works around the problem by always leaving 0x10 padding bytes at
the end of descriptor buffer pages, which should be harmless to do
unconditionally for controllers in case others have the same behavior.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/firewire/ohci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 8bf89267dc25..d731b413cb2c 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1130,7 +1130,13 @@ static int context_add_buffer(struct context *ctx)
 		return -ENOMEM;
 
 	offset = (void *)&desc->buffer - (void *)desc;
-	desc->buffer_size = PAGE_SIZE - offset;
+	/*
+	 * Some controllers, like JMicron ones, always issue 0x20-byte DMA reads
+	 * for descriptors, even 0x10-byte ones. This can cause page faults when
+	 * an IOMMU is in use and the oversized read crosses a page boundary.
+	 * Work around this by always leaving at least 0x10 bytes of padding.
+	 */
+	desc->buffer_size = PAGE_SIZE - offset - 0x10;
 	desc->buffer_bus = bus_addr + offset;
 	desc->used = 0;
 
-- 
2.14.3

Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

[Clemens Ladisch]

Hector Martin wrote:
> At least some JMicron controllers issue buggy oversized DMA reads when
> fetching context descriptors, always fetching 0x20 bytes at once for
> descriptors which are only 0x10 bytes long. This is often harmless, but
> can cause page faults on modern systems with IOMMUs:
>
> DMAR: [DMA Read] Request device [05:00.0] fault addr fff56000 [fault reason 06] PTE Read access is not set
> firewire_ohci 0000:05:00.0: DMA context IT0 has stopped, error code: evt_descriptor_read
>
> This works around the problem by always leaving 0x10 padding bytes at
> the end of descriptor buffer pages, which should be harmless to do
> unconditionally for controllers in case others have the same behavior.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>

Reviewed-by: Clemens Ladisch <clemens@ladisch.de>

> ---
>  drivers/firewire/ohci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 8bf89267dc25..d731b413cb2c 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -1130,7 +1130,13 @@ static int context_add_buffer(struct context *ctx)
>  		return -ENOMEM;
>
>  	offset = (void *)&desc->buffer - (void *)desc;
> -	desc->buffer_size = PAGE_SIZE - offset;
> +	/*
> +	 * Some controllers, like JMicron ones, always issue 0x20-byte DMA reads
> +	 * for descriptors, even 0x10-byte ones. This can cause page faults when
> +	 * an IOMMU is in use and the oversized read crosses a page boundary.
> +	 * Work around this by always leaving at least 0x10 bytes of padding.
> +	 */
> +	desc->buffer_size = PAGE_SIZE - offset - 0x10;
>  	desc->buffer_bus = bus_addr + offset;
>  	desc->used = 0;
>

Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

[Stefan Richter]

On Nov 03 Clemens Ladisch wrote:
> Hector Martin wrote:
> > At least some JMicron controllers issue buggy oversized DMA reads when
> > fetching context descriptors, always fetching 0x20 bytes at once for
> > descriptors which are only 0x10 bytes long. This is often harmless, but
> > can cause page faults on modern systems with IOMMUs:
> >
> > DMAR: [DMA Read] Request device [05:00.0] fault addr fff56000 [fault reason 06] PTE Read access is not set
> > firewire_ohci 0000:05:00.0: DMA context IT0 has stopped, error code: evt_descriptor_read
> >
> > This works around the problem by always leaving 0x10 padding bytes at
> > the end of descriptor buffer pages, which should be harmless to do
> > unconditionally for controllers in case others have the same behavior.
> >
> > Signed-off-by: Hector Martin <marcan@marcan.st>  
> 
> Reviewed-by: Clemens Ladisch <clemens@ladisch.de>

Thanks Hector for the troubleshooting and for the patch.
Thanks Clemens for the review.

It's been a while since I last reviewed and tested kernel patches, and
also my main FireWire equipped PC is currently tied up in work for which
reboots aren't desirable.  But I am updating a long unused secondary
FireWire'd PC right now and give the patch some testing this week.  (This
one even has a JMicron controller, but not an IOMMU.)

> > ---
> >  drivers/firewire/ohci.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> > index 8bf89267dc25..d731b413cb2c 100644
> > --- a/drivers/firewire/ohci.c
> > +++ b/drivers/firewire/ohci.c
> > @@ -1130,7 +1130,13 @@ static int context_add_buffer(struct context *ctx)
> >  		return -ENOMEM;
> >
> >  	offset = (void *)&desc->buffer - (void *)desc;
> > -	desc->buffer_size = PAGE_SIZE - offset;
> > +	/*
> > +	 * Some controllers, like JMicron ones, always issue 0x20-byte DMA reads
> > +	 * for descriptors, even 0x10-byte ones. This can cause page faults when
> > +	 * an IOMMU is in use and the oversized read crosses a page boundary.
> > +	 * Work around this by always leaving at least 0x10 bytes of padding.
> > +	 */
> > +	desc->buffer_size = PAGE_SIZE - offset - 0x10;
> >  	desc->buffer_bus = bus_addr + offset;
> >  	desc->used = 0;
> >  
-- 
Stefan Richter
-======----= =-== -==--
http://arcgraph.de/sr/

Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

[Hector Martin 'marcan']

On 2017-11-13 06:05, Stefan Richter wrote:
> Thanks Hector for the troubleshooting and for the patch.
> Thanks Clemens for the review.
> 
> It's been a while since I last reviewed and tested kernel patches, and
> also my main FireWire equipped PC is currently tied up in work for which
> reboots aren't desirable.  But I am updating a long unused secondary
> FireWire'd PC right now and give the patch some testing this week.  (This
> one even has a JMicron controller, but not an IOMMU.)
> 
Hi Stefan, did you ever get around to testing the patch? It doesn't seem
to have made it into any trees or percolated up to mainline yet.

-- 
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

[Stefan Richter]

On Jan 11 Hector Martin 'marcan' wrote:
> On 2017-11-13 06:05, Stefan Richter wrote:
> > Thanks Hector for the troubleshooting and for the patch.
> > Thanks Clemens for the review.
> > 
> > It's been a while since I last reviewed and tested kernel patches, and
> > also my main FireWire equipped PC is currently tied up in work for which
> > reboots aren't desirable.  But I am updating a long unused secondary
> > FireWire'd PC right now and give the patch some testing this week.  (This
> > one even has a JMicron controller, but not an IOMMU.)
> >   
> Hi Stefan, did you ever get around to testing the patch? It doesn't seem
> to have made it into any trees or percolated up to mainline yet.

Pushed to linux1394.git:for-next now.  Sorry that I took so long.
Thanks once more for your effort.

(Getting the test computer up to date was a PITA; no fun when there is
almost no spare time...)
-- 
Stefan Richter
-======---=- ---= -==-=
http://arcgraph.de/sr/