[PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Frederic Danis]

This reverts commit 9119fba0cfeda6d415c9f068df66838a104b87cb.

This commit prevents from sending "big" file using Bluetooth.
When sending a lot of data quickly through the Bluetooth interface, and
after a variable amount of data sent, transfer fails with error:
    kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00

Found on T100TA.

After reverting this commit, send works fine for any file size.

Signed-off-by: Frederic Danis <frederic.danis-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/tty/serial/8250/8250_dma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 21d01a4..e508939 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -80,10 +80,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 		return 0;
 
 	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
-	if (dma->tx_size < p->port.fifosize) {
-		ret = -EINVAL;
-		goto err;
-	}
 
 	desc = dmaengine_prep_slave_single(dma->txchan,
 					   dma->tx_addr + xmit->tail,
-- 
1.9.1

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Peter Hurley]

[ + Andy Shevchenko ]

On 09/30/2015 08:46 AM, Frederic Danis wrote:
> This reverts commit 9119fba0cfeda6d415c9f068df66838a104b87cb.

Hi Frederic,

It's customary to mail the commit author in patches which revert
their work.

> This commit prevents from sending "big" file using Bluetooth.
> When sending a lot of data quickly through the Bluetooth interface, and
> after a variable amount of data sent, transfer fails with error:
>     kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00

Hmmm, was there any other information regarding why this happened?
Eg., did the driver mistakenly re-order i/o (DMA occurred after PIO)?
Or, might there be some race condition in the dmaengine driver on this
platform?

Regards,
Peter Hurley

> Found on T100TA.
> 
> After reverting this commit, send works fine for any file size.
> 
> Signed-off-by: Frederic Danis <frederic.danis-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 21d01a4..e508939 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -80,10 +80,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>  		return 0;
>  
>  	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
> -	if (dma->tx_size < p->port.fifosize) {
> -		ret = -EINVAL;
> -		goto err;
> -	}
>  
>  	desc = dmaengine_prep_slave_single(dma->txchan,
>  					   dma->tx_addr + xmit->tail,
> 

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Andy Shevchenko]

On Wed, 2015-09-30 at 09:52 -0400, Peter Hurley wrote:
> [ + Andy Shevchenko ]
> 
> On 09/30/2015 08:46 AM, Frederic Danis wrote:
> > This reverts commit 9119fba0cfeda6d415c9f068df66838a104b87cb.
> 
> Hi Frederic,
> 
> It's customary to mail the commit author in patches which revert
> their work.
> 
> > This commit prevents from sending "big" file using Bluetooth.
> > When sending a lot of data quickly through the Bluetooth interface, 
> > and
> > after a variable amount of data sent, transfer fails with error:
> >     kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00
> 
> Hmmm, was there any other information regarding why this happened?

Frederic informed us privately about this issue. I tried to hunt the
root cause but mostly (there was one observation but apparently seems
it is not related to this issue) nothing looks wrong.

> Eg., did the driver mistakenly re-order i/o (DMA occurred after PIO)?

I have few tracing dumps which Frederic provides to me, but I couldn't
see such a problem. Maybe I missed it. My commit 9119fba0cfed
introduces interleaved IO (DMA/PIO) which makes it like DMA -> PIO ->
DMA -> ...


> Or, might there be some race condition in the dmaengine driver on 
> this
> platform?

There is [1] might be a clue, though I don't think it's directly
related.

[1] http://www.spinics.net/lists/linux-spi/msg01151.html
The change prevents interleaved DMA/PIO IO.

> 
> Regards,
> Peter Hurley
> 
> > Found on T100TA.
> > 
> > After reverting this commit, send works fine for any file size.
> > 
> > Signed-off-by: Frederic Danis <frederic.danis-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >  drivers/tty/serial/8250/8250_dma.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_dma.c 
> > b/drivers/tty/serial/8250/8250_dma.c
> > index 21d01a4..e508939 100644
> > --- a/drivers/tty/serial/8250/8250_dma.c
> > +++ b/drivers/tty/serial/8250/8250_dma.c
> > @@ -80,10 +80,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
> >  		return 0;
> >  
> >  	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, 
> > UART_XMIT_SIZE);
> > -	if (dma->tx_size < p->port.fifosize) {
> > -		ret = -EINVAL;
> > -		goto err;
> > -	}
> >  
> >  	desc = dmaengine_prep_slave_single(dma->txchan,
> >  					   dma->tx_addr + xmit
> > ->tail,
> > 
> 

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Peter Hurley]

[ + Heikko ]

On 09/30/2015 10:12 AM, Andy Shevchenko wrote:
> On Wed, 2015-09-30 at 09:52 -0400, Peter Hurley wrote:
>> [ + Andy Shevchenko ]
>>
>> On 09/30/2015 08:46 AM, Frederic Danis wrote:
>>> This reverts commit 9119fba0cfeda6d415c9f068df66838a104b87cb.
>>
>> Hi Frederic,
>>
>> It's customary to mail the commit author in patches which revert
>> their work.
>>
>>> This commit prevents from sending "big" file using Bluetooth.
>>> When sending a lot of data quickly through the Bluetooth interface, 
>>> and
>>> after a variable amount of data sent, transfer fails with error:
>>>     kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00
>>
>> Hmmm, was there any other information regarding why this happened?
> 
> Frederic informed us privately about this issue.

Ok, except no one else knows that.

Least of all Greg, since the revert wasn't even addressed to him either :/

> I tried to hunt the
> root cause but mostly (there was one observation but apparently seems
> it is not related to this issue) nothing looks wrong.
> 
>> Eg., did the driver mistakenly re-order i/o (DMA occurred after PIO)?
> 
> I have few tracing dumps which Frederic provides to me, but I couldn't
> see such a problem. Maybe I missed it.

Ok.

> My commit 9119fba0cfed introduces interleaved IO (DMA/PIO) which makes
> it like DMA -> PIO -> DMA -> ...

I realize that. However interleaving DMA and PIO _must_ be supported
by the platform to enable DMA for 8250 serial devices anway.

When Heikki added DMA support to the 8250 driver, he left out important
functionality which requires interleaved DMA and PIO.

When I fix that omission, platforms using DMA that shouldn't be are going
to get broken anyway.

Regards,
Peter Hurley

>> Or, might there be some race condition in the dmaengine driver on 
>> this
>> platform?
> 
> There is [1] might be a clue, though I don't think it's directly
> related.
> 
> [1] http://www.spinics.net/lists/linux-spi/msg01151.html
> The change prevents interleaved DMA/PIO IO.
> 
>>
>> Regards,
>> Peter Hurley
>>
>>> Found on T100TA.
>>>
>>> After reverting this commit, send works fine for any file size.
>>>
>>> Signed-off-by: Frederic Danis <frederic.danis-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>> ---
>>>  drivers/tty/serial/8250/8250_dma.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_dma.c 
>>> b/drivers/tty/serial/8250/8250_dma.c
>>> index 21d01a4..e508939 100644
>>> --- a/drivers/tty/serial/8250/8250_dma.c
>>> +++ b/drivers/tty/serial/8250/8250_dma.c
>>> @@ -80,10 +80,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>>>  		return 0;
>>>  
>>>  	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, 
>>> UART_XMIT_SIZE);
>>> -	if (dma->tx_size < p->port.fifosize) {
>>> -		ret = -EINVAL;
>>> -		goto err;
>>> -	}
>>>  
>>>  	desc = dmaengine_prep_slave_single(dma->txchan,
>>>  					   dma->tx_addr + xmit
>>> ->tail,
>>>
>>
> 

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Andy Shevchenko]

On Wed, 2015-09-30 at 16:11 -0400, Peter Hurley wrote:
> [ + Heikko ]
> 
> On 09/30/2015 10:12 AM, Andy Shevchenko wrote:
> > On Wed, 2015-09-30 at 09:52 -0400, Peter Hurley wrote:
> > > [ + Andy Shevchenko ]
> > > 
> > > On 09/30/2015 08:46 AM, Frederic Danis wrote:
> > > > This reverts commit 9119fba0cfeda6d415c9f068df66838a104b87cb.
> > > 
> > > Hi Frederic,
> > > 
> > > It's customary to mail the commit author in patches which revert
> > > their work.
> > > 
> > > > This commit prevents from sending "big" file using Bluetooth.
> > > > When sending a lot of data quickly through the Bluetooth 
> > > > interface, 
> > > > and
> > > > after a variable amount of data sent, transfer fails with 
> > > > error:
> > > >     kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00

Frederic, I forgot to ask how exactly the hw setup looks like?

The message above is not present in the kernel (the closest what I
found is 

drivers/bluetooth/btintel.c:98: BT_ERR("%s: Hardware error 0x%2.2x",
hdev->name, code);

which makes me think you have USB BT adapter attached to the same
machine. Am I wrong?

> > > 
> > > Hmmm, was there any other information regarding why this 
> > > happened?
> > 
> > Frederic informed us privately about this issue.
> 
> Ok, except no one else knows that.
> 
> Least of all Greg, since the revert wasn't even addressed to him 
> either :/

Agree, it's better to inform all parties about the problem.

> 
> > I tried to hunt the
> > root cause but mostly (there was one observation but apparently 
> > seems
> > it is not related to this issue) nothing looks wrong.
> > 
> > > Eg., did the driver mistakenly re-order i/o (DMA occurred after 
> > > PIO)?
> > 
> > I have few tracing dumps which Frederic provides to me, but I 
> > couldn't
> > see such a problem. Maybe I missed it.
> 
> Ok.
> 
> > My commit 9119fba0cfed introduces interleaved IO (DMA/PIO) which 
> > makes
> > it like DMA -> PIO -> DMA -> ...
> 
> I realize that. However interleaving DMA and PIO _must_ be supported
> by the platform to enable DMA for 8250 serial devices anway.
> 
> When Heikki added DMA support to the 8250 driver, he left out 
> important
> functionality which requires interleaved DMA and PIO.
> 
> When I fix that omission, platforms using DMA that shouldn't be are 
> going
> to get broken anyway.
> 
> Regards,
> Peter Hurley
> 
> > > Or, might there be some race condition in the dmaengine driver on 
> > > 
> > > this
> > > platform?
> > 
> > There is [1] might be a clue, though I don't think it's directly
> > related.
> > 
> > [1] http://www.spinics.net/lists/linux-spi/msg01151.html
> > The change prevents interleaved DMA/PIO IO.
> > 
> > > 
> > > Regards,
> > > Peter Hurley
> > > 
> > > > Found on T100TA.
> > > > 
> > > > After reverting this commit, send works fine for any file size.
> > > > 
> > > > Signed-off-by: Frederic Danis <frederic.danis-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > ---
> > > >  drivers/tty/serial/8250/8250_dma.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/8250/8250_dma.c 
> > > > b/drivers/tty/serial/8250/8250_dma.c
> > > > index 21d01a4..e508939 100644
> > > > --- a/drivers/tty/serial/8250/8250_dma.c
> > > > +++ b/drivers/tty/serial/8250/8250_dma.c
> > > > @@ -80,10 +80,6 @@ int serial8250_tx_dma(struct uart_8250_port 
> > > > *p)
> > > >  		return 0;
> > > >  
> > > >  	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, 
> > > > 
> > > > UART_XMIT_SIZE);
> > > > -	if (dma->tx_size < p->port.fifosize) {
> > > > -		ret = -EINVAL;
> > > > -		goto err;
> > > > -	}
> > > >  
> > > >  	desc = dmaengine_prep_slave_single(dma->txchan,
> > > >  					   dma->tx_addr + xmit
> > > > ->tail,
> > > > 
> > > 
> > 
> 

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Marcel Holtmann]

Hi Andy,

>>>> It's customary to mail the commit author in patches which revert
>>>> their work.
>>>> 
>>>>> This commit prevents from sending "big" file using Bluetooth.
>>>>> When sending a lot of data quickly through the Bluetooth 
>>>>> interface, 
>>>>> and
>>>>> after a variable amount of data sent, transfer fails with 
>>>>> error:
>>>>>    kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00
> 
> Frederic, I forgot to ask how exactly the hw setup looks like?
> 
> The message above is not present in the kernel (the closest what I
> found is 
> 
> drivers/bluetooth/btintel.c:98: BT_ERR("%s: Hardware error 0x%2.2x",
> hdev->name, code);
> 
> which makes me think you have USB BT adapter attached to the same
> machine. Am I wrong?

the Bluetooth core will generate that error in case the driver does not provide hdev->hw_error callback. We have not hocked that up for the UART driver yet. Look at net/bluetooth/hci_core.c to find the common error message.

Regards

Marcel

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Frederic Danis]

[ + Greg Kroah-Hartman ]

On 01/10/2015 10:55, Andy Shevchenko wrote:
> On Wed, 2015-09-30 at 16:11 -0400, Peter Hurley wrote:
>> [ + Heikko ]
>>
>> On 09/30/2015 10:12 AM, Andy Shevchenko wrote:
>>> On Wed, 2015-09-30 at 09:52 -0400, Peter Hurley wrote:
>>>> [ + Andy Shevchenko ]
>>>>
>>>> On 09/30/2015 08:46 AM, Frederic Danis wrote:
>>>>> This reverts commit 9119fba0cfeda6d415c9f068df66838a104b87cb.
>>>> Hi Frederic,
>>>>
>>>> It's customary to mail the commit author in patches which revert
>>>> their work.
>>>>
>>>>> This commit prevents from sending "big" file using Bluetooth.
>>>>> When sending a lot of data quickly through the Bluetooth
>>>>> interface,
>>>>> and
>>>>> after a variable amount of data sent, transfer fails with
>>>>> error:
>>>>>      kernel: [  415.247453] Bluetooth: hci0 hardware error 0x00
> Frederic, I forgot to ask how exactly the hw setup looks like?
>
> The message above is not present in the kernel (the closest what I
> found is
>
> drivers/bluetooth/btintel.c:98: BT_ERR("%s: Hardware error 0x%2.2x",
> hdev->name, code);
>
> which makes me think you have USB BT adapter attached to the same
> machine. Am I wrong?
>
>>>> Hmmm, was there any other information regarding why this
>>>> happened?
>>> Frederic informed us privately about this issue.
>> Ok, except no one else knows that.
>>
>> Least of all Greg, since the revert wasn't even addressed to him
>> either :/
> Agree, it's better to inform all parties about the problem.
>
>>> I tried to hunt the
>>> root cause but mostly (there was one observation but apparently
>>> seems
>>> it is not related to this issue) nothing looks wrong.
>>>
>>>> Eg., did the driver mistakenly re-order i/o (DMA occurred after
>>>> PIO)?
>>> I have few tracing dumps which Frederic provides to me, but I
>>> couldn't
>>> see such a problem. Maybe I missed it.
>> Ok.
>>
>>> My commit 9119fba0cfed introduces interleaved IO (DMA/PIO) which
>>> makes
>>> it like DMA -> PIO -> DMA -> ...
>> I realize that. However interleaving DMA and PIO _must_ be supported
>> by the platform to enable DMA for 8250 serial devices anway.
>>
>> When Heikki added DMA support to the 8250 driver, he left out
>> important
>> functionality which requires interleaved DMA and PIO.
>>
>> When I fix that omission, platforms using DMA that shouldn't be are
>> going
>> to get broken anyway.
>>
>> Regards,
>> Peter Hurley
>>
>>>> Or, might there be some race condition in the dmaengine driver on
>>>>
>>>> this
>>>> platform?
>>> There is [1] might be a clue, though I don't think it's directly
>>> related.
>>>
>>> [1] http://www.spinics.net/lists/linux-spi/msg01151.html
>>> The change prevents interleaved DMA/PIO IO.
>>>
>>>> Regards,
>>>> Peter Hurley
>>>>
>>>>> Found on T100TA.
>>>>>
>>>>> After reverting this commit, send works fine for any file size.
>>>>>
>>>>> Signed-off-by: Frederic Danis <frederic.danis-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>>> ---
>>>>>   drivers/tty/serial/8250/8250_dma.c | 4 ----
>>>>>   1 file changed, 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/8250/8250_dma.c
>>>>> b/drivers/tty/serial/8250/8250_dma.c
>>>>> index 21d01a4..e508939 100644
>>>>> --- a/drivers/tty/serial/8250/8250_dma.c
>>>>> +++ b/drivers/tty/serial/8250/8250_dma.c
>>>>> @@ -80,10 +80,6 @@ int serial8250_tx_dma(struct uart_8250_port
>>>>> *p)
>>>>>   		return 0;
>>>>>   
>>>>>   	dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail,
>>>>>
>>>>> UART_XMIT_SIZE);
>>>>> -	if (dma->tx_size < p->port.fifosize) {
>>>>> -		ret = -EINVAL;
>>>>> -		goto err;
>>>>> -	}
>>>>>   
>>>>>   	desc = dmaengine_prep_slave_single(dma->txchan,
>>>>>   					   dma->tx_addr + xmit
>>>>> ->tail,
>>>>>


-- 
Frederic Danis                            Open Source Technology Center
frederic.danis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Greg KH]

On Tue, Oct 06, 2015 at 11:17:32AM +0200, Frederic Danis wrote:
> [ + Greg Kroah-Hartman ]

Why?  What am I supposed to do with this?

greg k-h

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Frederic Danis]

Hello Greg,

On 06/10/2015 11:34, Greg KH wrote:
> On Tue, Oct 06, 2015 at 11:17:32AM +0200, Frederic Danis wrote:
> > [ + Greg Kroah-Hartman ]
>
> Why?  What am I supposed to do with this?

I add you to this thread to inform you of this problem and the revert 
patch I sent. (As suggested by Peter and Andy)

Regards

Fred

Re: [PATCH] Revert "serial: 8250_dma: don't bother DMA with small transfers"

[Greg KH]

On Tue, Oct 06, 2015 at 11:48:18AM +0200, Frederic Danis wrote:
> Hello Greg,
> 
> On 06/10/2015 11:34, Greg KH wrote:
> >On Tue, Oct 06, 2015 at 11:17:32AM +0200, Frederic Danis wrote:
> >> [ + Greg Kroah-Hartman ]
> >
> >Why?  What am I supposed to do with this?
> 
> I add you to this thread to inform you of this problem and the revert patch
> I sent. (As suggested by Peter and Andy)

What thread?  I don't see any patch here in your email that I can do
anything about, do you?

Come on, you know how to properly submit patches, do it correctly
please.

greg k-h