[PATCH] serial: tegra: Only print FIFO error message when an error occurs

[PATCH] serial: tegra: Only print FIFO error message when an error occurs

[Jon Hunter]

The Tegra serial driver always prints an error message when enabling the
FIFO for devices that have support for checking the FIFO enable status.
Fix this by only display the error message, when an error occurs.

Fixes: 222dcdff3405 ("serial: tegra: check for FIFO mode enabled status")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 222032792d6c..cd481f7ba8eb 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1045,9 +1045,10 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
 
 	if (tup->cdata->fifo_mode_enable_status) {
 		ret = tegra_uart_wait_fifo_mode_enabled(tup);
-		dev_err(tup->uport.dev, "FIFO mode not enabled\n");
-		if (ret < 0)
+		if (ret < 0) {
+			dev_err(tup->uport.dev, "FIFO mode not enabled\n");
 			return ret;
+		}
 	} else {
 		/*
 		 * For all tegra devices (up to t210), there is a hardware
-- 
2.25.1

Re: [PATCH] serial: tegra: Only print FIFO error message when an error occurs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On Wed, Jun 30, 2021 at 10:46:01AM +0100, Jon Hunter wrote:
> The Tegra serial driver always prints an error message when enabling the
> FIFO for devices that have support for checking the FIFO enable status.
> Fix this by only display the error message, when an error occurs.
> 
> Fixes: 222dcdff3405 ("serial: tegra: check for FIFO mode enabled status")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/tty/serial/serial-tegra.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index 222032792d6c..cd481f7ba8eb 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -1045,9 +1045,10 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>  
>  	if (tup->cdata->fifo_mode_enable_status) {
>  		ret = tegra_uart_wait_fifo_mode_enabled(tup);
> -		dev_err(tup->uport.dev, "FIFO mode not enabled\n");
> -		if (ret < 0)
> +		if (ret < 0) {
> +			dev_err(tup->uport.dev, "FIFO mode not enabled\n");

The error message seems a bit confusing. I read this as meaning "FIFO
mode was expected to be enabled but wasn't" whereas this really seems to
mean that for some reason the FIFO enable timed out.

In the former case it sounds like a some configuration mismatch, while
it's really something that went wrong during the process of enabling the
FIFO mode.

So I wonder if this should perhaps be something like:

	dev_err(tup->uport.dev, "FIFO mode enable timed out\n");

or something along those lines.

That said, this is a pre-existing problem and it's a bit pedantic, so in
either case:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] serial: tegra: Only print FIFO error message when an error occurs

[Jon Hunter]

On 30/06/2021 13:11, Thierry Reding wrote:
> On Wed, Jun 30, 2021 at 10:46:01AM +0100, Jon Hunter wrote:
>> The Tegra serial driver always prints an error message when enabling the
>> FIFO for devices that have support for checking the FIFO enable status.
>> Fix this by only display the error message, when an error occurs.
>>
>> Fixes: 222dcdff3405 ("serial: tegra: check for FIFO mode enabled status")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/tty/serial/serial-tegra.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>> index 222032792d6c..cd481f7ba8eb 100644
>> --- a/drivers/tty/serial/serial-tegra.c
>> +++ b/drivers/tty/serial/serial-tegra.c
>> @@ -1045,9 +1045,10 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>  
>>  	if (tup->cdata->fifo_mode_enable_status) {
>>  		ret = tegra_uart_wait_fifo_mode_enabled(tup);
>> -		dev_err(tup->uport.dev, "FIFO mode not enabled\n");
>> -		if (ret < 0)
>> +		if (ret < 0) {
>> +			dev_err(tup->uport.dev, "FIFO mode not enabled\n");
> 
> The error message seems a bit confusing. I read this as meaning "FIFO
> mode was expected to be enabled but wasn't" whereas this really seems to
> mean that for some reason the FIFO enable timed out.
> 
> In the former case it sounds like a some configuration mismatch, while
> it's really something that went wrong during the process of enabling the
> FIFO mode.
> 
> So I wonder if this should perhaps be something like:
> 
> 	dev_err(tup->uport.dev, "FIFO mode enable timed out\n");
> 
> or something along those lines.

Yes good point. Maybe I should just ...

	dev_err(tup->uport.dev,
		"Failed to enable FIFO mode: %d\n", ret);

Then if it is updated to ever return anything other that -ETIMEDOUT we
are covered. I will send a V2.

Jon

-- 
nvpublic