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

[PATCH V2] 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 displaying the error message, only when an error occurs.

Finally, update the error message to make it clear that enabling the
FIFO failed and display the error code.

Fixes: 222dcdff3405 ("serial: tegra: check for FIFO mode enabled status")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
Changes since V1:
- Updated the error message to make it more meaningful.

drivers/tty/serial/serial-tegra.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 222032792d6c..eba5b9ecba34 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1045,9 +1045,11 @@ 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,
+				"Failed to enable FIFO mode: %d\n", ret);
 			return ret;
+		}
 	} else {
 		/*
 		 * For all tegra devices (up to t210), there is a hardware
-- 
2.25.1

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

[Thierry Reding]

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

On Wed, Jun 30, 2021 at 01:56:43PM +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 displaying the error message, only when an error occurs.
> 
> Finally, update the error message to make it clear that enabling the
> FIFO failed and display the error code.
> 
> Fixes: 222dcdff3405 ("serial: tegra: check for FIFO mode enabled status")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Updated the error message to make it more meaningful.
> 
> drivers/tty/serial/serial-tegra.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Looks good:

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

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

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

[Michał Mirosław]

On Wed, Jun 30, 2021 at 01:56:43PM +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 displaying the error message, only when an error occurs.
> 
> Finally, update the error message to make it clear that enabling the
> FIFO failed and display the error code.
[...]
> @@ -1045,9 +1045,11 @@ 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,
> +				"Failed to enable FIFO mode: %d\n", ret);

Could you change this to use %pe and ERR_PTR(ret)?

Best Regards
Michał Mirosław

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

[Jon Hunter]

On 08/07/2021 23:25, Michał Mirosław wrote:
> On Wed, Jun 30, 2021 at 01:56:43PM +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 displaying the error message, only when an error occurs.
>>
>> Finally, update the error message to make it clear that enabling the
>> FIFO failed and display the error code.
> [...]
>> @@ -1045,9 +1045,11 @@ 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,
>> +				"Failed to enable FIFO mode: %d\n", ret);
> 
> Could you change this to use %pe and ERR_PTR(ret)?

Sorry, but it is not clear to me why this would be necessary in this case.

Jon

-- 
nvpublic

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

[Jon Hunter]

On 09/07/2021 09:34, Jon Hunter wrote:
> 
> 
> On 08/07/2021 23:25, Michał Mirosław wrote:
>> On Wed, Jun 30, 2021 at 01:56:43PM +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 displaying the error message, only when an error occurs.
>>>
>>> Finally, update the error message to make it clear that enabling the
>>> FIFO failed and display the error code.
>> [...]
>>> @@ -1045,9 +1045,11 @@ 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,
>>> +				"Failed to enable FIFO mode: %d\n", ret);
>>
>> Could you change this to use %pe and ERR_PTR(ret)?
> 
> Sorry, but it is not clear to me why this would be necessary in this case.

I see so '%pe' prints the symbolic name of the error code. While that is
nice, it also looks a bit odd. Given that we simply print the error code
values in this driver, from looking at other prints, I prefer to keep it
as is for consistency.

Jon

-- 
nvpublic

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

[Michał Mirosław]

On Fri, Jul 09, 2021 at 12:38:07PM +0100, Jon Hunter wrote:
> 
> On 09/07/2021 09:34, Jon Hunter wrote:
> > 
> > 
> > On 08/07/2021 23:25, Michał Mirosław wrote:
> >> On Wed, Jun 30, 2021 at 01:56:43PM +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 displaying the error message, only when an error occurs.
> >>>
> >>> Finally, update the error message to make it clear that enabling the
> >>> FIFO failed and display the error code.
> >> [...]
> >>> @@ -1045,9 +1045,11 @@ 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,
> >>> +				"Failed to enable FIFO mode: %d\n", ret);
> >>
> >> Could you change this to use %pe and ERR_PTR(ret)?
> > 
> > Sorry, but it is not clear to me why this would be necessary in this case.
> 
> I see so '%pe' prints the symbolic name of the error code. While that is
> nice, it also looks a bit odd. Given that we simply print the error code
> values in this driver, from looking at other prints, I prefer to keep it
> as is for consistency.

It is a quite new facility of printk(), so I woudn't expect it to be
present in older code. It saves a bit of time when you occasionally
hit an error, so even incremental conversion seems beneficial for me.

Best Regards
Michał Mirosław

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

[Dmitry Osipenko]

09.07.2021 20:55, Michał Mirosław пишет:
> On Fri, Jul 09, 2021 at 12:38:07PM +0100, Jon Hunter wrote:
>>
>> On 09/07/2021 09:34, Jon Hunter wrote:
>>>
>>>
>>> On 08/07/2021 23:25, Michał Mirosław wrote:
>>>> On Wed, Jun 30, 2021 at 01:56:43PM +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 displaying the error message, only when an error occurs.
>>>>>
>>>>> Finally, update the error message to make it clear that enabling the
>>>>> FIFO failed and display the error code.
>>>> [...]
>>>>> @@ -1045,9 +1045,11 @@ 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,
>>>>> +				"Failed to enable FIFO mode: %d\n", ret);
>>>>
>>>> Could you change this to use %pe and ERR_PTR(ret)?
>>>
>>> Sorry, but it is not clear to me why this would be necessary in this case.
>>
>> I see so '%pe' prints the symbolic name of the error code. While that is
>> nice, it also looks a bit odd. Given that we simply print the error code
>> values in this driver, from looking at other prints, I prefer to keep it
>> as is for consistency.
> 
> It is a quite new facility of printk(), so I woudn't expect it to be
> present in older code. It saves a bit of time when you occasionally
> hit an error, so even incremental conversion seems beneficial for me.

It doesn't feel like a good approach to use ERR_PTR where it's not very
appropriate. I suppose printk could get a new specifier, like '%de' for
example, for the verbose integer error codes, couldn't it?