Commit 3a878c430fd6 ("tty: serial: msm: Add TX DMA support") resulted
in dropped characters and invalid characters in pio mode. Fix the
problem and add an additional information message that was important
in diagnosing the problem (reporting that DMA mode was not enabled).
From: Frank Rowand <[email protected]>
Commit 3a878c430fd6 ("tty: serial: msm: Add TX DMA support") regression.
The calculation of tx_count was moved from the old msm_handle_tx(),
now renamed msm_handle_tx_pio(), to the new msm_handle_tx(). The
move left out one size test.
The regression seen on the qcom-apq8074-dragonboard is dropped
characters and corrupted characters (values greater than 0x7f)
when DMA is not enabled.
Signed-off-by: Frank Rowand <[email protected]>
Cc: [email protected]
---
drivers/tty/serial/msm_serial.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Index: b/drivers/tty/serial/msm_serial.c
===================================================================
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -727,6 +727,8 @@ static void msm_handle_tx(struct uart_po
}
pio_count = CIRC_CNT(xmit->head, xmit->tail, UART_XMIT_SIZE);
+ pio_count = min3(pio_count, (unsigned int)UART_XMIT_SIZE - xmit->tail,
+ port->fifosize);
dma_count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
dma_min = 1; /* Always DMA */
@@ -738,9 +740,6 @@ static void msm_handle_tx(struct uart_po
dma_count = UARTDM_TX_MAX;
}
- if (pio_count > port->fifosize)
- pio_count = port->fifosize;
-
if (!dma->chan || dma_count < dma_min)
msm_handle_tx_pio(port, pio_count);
else
From: Frank Rowand <[email protected]>
Failure to enable DMA by the msm_serial driver is silent.
Add a message to report the failure.
Signed-off-by: Frank Rowand <[email protected]>
---
drivers/tty/serial/msm_serial.c | 1 +
1 file changed, 1 insertion(+)
Index: b/drivers/tty/serial/msm_serial.c
===================================================================
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -170,6 +170,7 @@ rel_tx:
dma_release_channel(dma->chan);
no_tx:
memset(dma, 0, sizeof(*dma));
+ dev_info(dev, "msm_serial: DMA not enabled\n");
}
static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base)
On 04/23, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Commit 3a878c430fd6 ("tty: serial: msm: Add TX DMA support") regression.
> The calculation of tx_count was moved from the old msm_handle_tx(),
> now renamed msm_handle_tx_pio(), to the new msm_handle_tx(). The
> move left out one size test.
>
> The regression seen on the qcom-apq8074-dragonboard is dropped
> characters and corrupted characters (values greater than 0x7f)
> when DMA is not enabled.
>
> Signed-off-by: Frank Rowand <[email protected]>
> Cc: [email protected]
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Thanks for finding this!
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/23, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Failure to enable DMA by the msm_serial driver is silent.
> Add a message to report the failure.
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> drivers/tty/serial/msm_serial.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Index: b/drivers/tty/serial/msm_serial.c
> ===================================================================
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -170,6 +170,7 @@ rel_tx:
> dma_release_channel(dma->chan);
> no_tx:
> memset(dma, 0, sizeof(*dma));
> + dev_info(dev, "msm_serial: DMA not enabled\n");
> }
>
Wouldn't this print twice for TX and RX channels? I'd prefer we
not print anything when this driver probes, just because it's a
bunch of log spam that we don't really need.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 4/25/2016 1:48 PM, Stephen Boyd wrote:
> On 04/23, Frank Rowand wrote:
>> From: Frank Rowand <[email protected]>
>>
>> Failure to enable DMA by the msm_serial driver is silent.
>> Add a message to report the failure.
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> drivers/tty/serial/msm_serial.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> Index: b/drivers/tty/serial/msm_serial.c
>> ===================================================================
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -170,6 +170,7 @@ rel_tx:
>> dma_release_channel(dma->chan);
>> no_tx:
>> memset(dma, 0, sizeof(*dma));
>> + dev_info(dev, "msm_serial: DMA not enabled\n");
>> }
>>
>
> Wouldn't this print twice for TX and RX channels? I'd prefer we
> not print anything when this driver probes, just because it's a
> bunch of log spam that we don't really need.
This is in msm_request_tx_dma(). I should have made the message
"msm_serial: TX DMA not enabled\n" and added a similar message
to msm_request_rx_dma().
Then it could print twice, once for TX and once for RX. :-)
For my board it would print twice because both requests would
fail for the same reason.
Should I add it to msm_request_rx_dma() also, but make both
locations dev_debug() instead of dev_info()?
-Frank
On 04/25, Frank Rowand wrote:
>
> This is in msm_request_tx_dma(). I should have made the message
> "msm_serial: TX DMA not enabled\n" and added a similar message
> to msm_request_rx_dma().
>
> Then it could print twice, once for TX and once for RX. :-)
> For my board it would print twice because both requests would
> fail for the same reason.
Ah right, the 3 line diff window caught me here.
>
> Should I add it to msm_request_rx_dma() also, but make both
> locations dev_debug() instead of dev_info()?
Honestly I don't see much point in having this at all. Why does
the user care if DMA is used or not? Don't they just want the
hardware to work? Maybe dev_dbg(), but again, debug junk. I'll
leave it up to you and Greg.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Sat 23 Apr 10:14 PDT 2016, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Commit 3a878c430fd6 ("tty: serial: msm: Add TX DMA support") regression.
> The calculation of tx_count was moved from the old msm_handle_tx(),
> now renamed msm_handle_tx_pio(), to the new msm_handle_tx(). The
> move left out one size test.
>
> The regression seen on the qcom-apq8074-dragonboard is dropped
> characters and corrupted characters (values greater than 0x7f)
> when DMA is not enabled.
>
> Signed-off-by: Frank Rowand <[email protected]>
> Cc: [email protected]
> ---
> drivers/tty/serial/msm_serial.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> Index: b/drivers/tty/serial/msm_serial.c
> ===================================================================
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -727,6 +727,8 @@ static void msm_handle_tx(struct uart_po
> }
>
> pio_count = CIRC_CNT(xmit->head, xmit->tail, UART_XMIT_SIZE);
> + pio_count = min3(pio_count, (unsigned int)UART_XMIT_SIZE - xmit->tail,
These two lines essentially reimplements CIRC_CNT_TO_END()
> + port->fifosize);
And this looks equivalent to the removed part below.
So I think a smaller patch would be to change the calculation to:
pio_count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
> dma_count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
>
> dma_min = 1; /* Always DMA */
> @@ -738,9 +740,6 @@ static void msm_handle_tx(struct uart_po
> dma_count = UARTDM_TX_MAX;
> }
>
> - if (pio_count > port->fifosize)
> - pio_count = port->fifosize;
> -
> if (!dma->chan || dma_count < dma_min)
> msm_handle_tx_pio(port, pio_count);
> else
However, as you've concluded that the problem is that we don't handle
wrapping writes let's look at msm_handle_tx_pio():
int tf_pointer = 0;
while (tf_pointer < pio_count) {
char buf[4];
if (is_uartdm)
num_chars = min(pio_count - tf_pointer,
(unsigned int)sizeof(buf));
else
num_chars = 1;
for (i = 0; i < num_chars; i++)
buf[i] = xmit->buf[xmit->tail + i];
xmit->tail = (xmit->tail + num_chars) & (UART_XMIT_SIZE - 1);
tf_pointer += num_chars;
}
So the problem you seem to run into is that we copy num_chars bytes
sequentially from xmit->tail, running outside the buffer.
So the problem is that the num_chars calculation isn't limited, perhaps
something like this instead:
num_chars = min3(tx_count - tf_pointer,
sizeof(buf),
(unsigned int)CIRC_CNT_TO_END(xmit->head,
xmit->tail,
UART_XMIT_SIZE));
You should either make msm_handle_tx_pio() handle wrapping buffers or
make the pio_count calculation follow the dma case (with _TO_END).
Regards,
Bjorn
On 4/25/2016 2:35 PM, Stephen Boyd wrote:
> On 04/25, Frank Rowand wrote:
>>
>> This is in msm_request_tx_dma(). I should have made the message
>> "msm_serial: TX DMA not enabled\n" and added a similar message
>> to msm_request_rx_dma().
>>
>> Then it could print twice, once for TX and once for RX. :-)
>> For my board it would print twice because both requests would
>> fail for the same reason.
>
> Ah right, the 3 line diff window caught me here.
>
>>
>> Should I add it to msm_request_rx_dma() also, but make both
>> locations dev_debug() instead of dev_info()?
>
> Honestly I don't see much point in having this at all. Why does
> the user care if DMA is used or not? Don't they just want the
> hardware to work? Maybe dev_dbg(), but again, debug junk. I'll
> leave it up to you and Greg.
If the user doesn't care if DMA is used then why even bother
implementing it in the driver? :-)
I don't _need_ the messages, I just need the driver to quit
dropping bytes and writing corrupt bytes. So patch 1 of 2 is
sufficient for my needs.
-Frank
On Sat, Apr 23, 2016 at 10:17:05AM -0700, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Failure to enable DMA by the msm_serial driver is silent.
> Add a message to report the failure.
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> drivers/tty/serial/msm_serial.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Index: b/drivers/tty/serial/msm_serial.c
> ===================================================================
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -170,6 +170,7 @@ rel_tx:
> dma_release_channel(dma->chan);
> no_tx:
> memset(dma, 0, sizeof(*dma));
> + dev_info(dev, "msm_serial: DMA not enabled\n");
Don't print out messages that a user can't do something with :(
So I agree, this is not needed, sorry.
greg k-h
On 4/28/2016 1:51 PM, Greg Kroah-Hartman wrote:
> On Sat, Apr 23, 2016 at 10:17:05AM -0700, Frank Rowand wrote:
>> From: Frank Rowand <[email protected]>
>>
>> Failure to enable DMA by the msm_serial driver is silent.
>> Add a message to report the failure.
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>> drivers/tty/serial/msm_serial.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> Index: b/drivers/tty/serial/msm_serial.c
>> ===================================================================
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -170,6 +170,7 @@ rel_tx:
>> dma_release_channel(dma->chan);
>> no_tx:
>> memset(dma, 0, sizeof(*dma));
>> + dev_info(dev, "msm_serial: DMA not enabled\n");
>
> Don't print out messages that a user can't do something with :(
>
> So I agree, this is not needed, sorry.
>
> greg k-h
>
I am ok with dropping patch 2. No problem.
-Frank