diff mbox series

[net,21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages

Message ID 20201103220636.972106-22-mkl@pengutronix.de
State New
Headers show
Series None | expand

Commit Message

Marc Kleine-Budde Nov. 3, 2020, 10:06 p.m. UTC
During debugging it turned out that some people have setups where the SPI
communication is more prone to CRC errors.

Increase the severity of both the transfer retry and transfer failure message
to give users feedback without the need to recompile the driver with debug
enabled.

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Thomas Kopp <thomas.kopp@microchip.com>
Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Manivannan Sadhasivam Nov. 5, 2020, 4:24 p.m. UTC | #1
Hi Marc,

On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:
> During debugging it turned out that some people have setups where the SPI

> communication is more prone to CRC errors.

> 

> Increase the severity of both the transfer retry and transfer failure message

> to give users feedback without the need to recompile the driver with debug

> enabled.

> 

> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> Cc: Thomas Kopp <thomas.kopp@microchip.com>

> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de

> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

> ---

>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------

>  1 file changed, 8 insertions(+), 8 deletions(-)

> 

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

> index ba25902dd78c..c9ffc5ea2b25 100644

> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,

>  			goto out;

>  		}

>  

> -		netdev_dbg(priv->ndev,

> -			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",

> -			   reg, val_len, (int)val_len, buf_rx->data,

> -			   get_unaligned_be16(buf_rx->data + val_len));

> -	}

> -

> -	if (err) {

>  		netdev_info(priv->ndev,

> -			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",

> +			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",

>  			    reg, val_len, (int)val_len, buf_rx->data,

>  			    get_unaligned_be16(buf_rx->data + val_len));


I'm not finding this inner debug log useful. Does the user really care
about the iterations it took to read a register? Just throwing the error
after max try seems better to me.

Thanks,
Mani

> +	}

> +

> +	if (err) {

> +		netdev_err(priv->ndev,

> +			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",

> +			   reg, val_len, (int)val_len, buf_rx->data,

> +			   get_unaligned_be16(buf_rx->data + val_len));

>  

>  		return err;

>  	}

> -- 

> 2.28.0

>
Marc Kleine-Budde Nov. 5, 2020, 4:39 p.m. UTC | #2
On 11/5/20 5:24 PM, Manivannan Sadhasivam wrote:
> Hi Marc,

> 

> On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:

>> During debugging it turned out that some people have setups where the SPI

>> communication is more prone to CRC errors.

>>

>> Increase the severity of both the transfer retry and transfer failure message

>> to give users feedback without the need to recompile the driver with debug

>> enabled.

>>

>> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

>> Cc: Thomas Kopp <thomas.kopp@microchip.com>

>> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de

>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

>> ---

>>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------

>>  1 file changed, 8 insertions(+), 8 deletions(-)

>>

>> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

>> index ba25902dd78c..c9ffc5ea2b25 100644

>> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

>> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

>> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,

>>  			goto out;

>>  		}

>>  

>> -		netdev_dbg(priv->ndev,

>> -			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",

>> -			   reg, val_len, (int)val_len, buf_rx->data,

>> -			   get_unaligned_be16(buf_rx->data + val_len));

>> -	}

>> -

>> -	if (err) {

>>  		netdev_info(priv->ndev,

>> -			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",

>> +			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",

>>  			    reg, val_len, (int)val_len, buf_rx->data,

>>  			    get_unaligned_be16(buf_rx->data + val_len));

> 

> I'm not finding this inner debug log useful. Does the user really care

> about the iterations it took to read a register? Just throwing the error

> after max try seems better to me.


Bitflips on the SPI should not happen, at least not regularly. Even with my
breadboard setup and max SPI frequency, I don't see any SPI CRC errors, unless
there is bad cabling. Then a retry most of the times helps and the user doesn't
notice. This drops performance...Yes logging drop performance, too, but at least
someone will notice.

My rationale was to give the user feedback and not cover a potential problem.
The driver is new and probably not widely used and I have no idea how often it
runs into read-CRC problems in production use.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Manivannan Sadhasivam Nov. 5, 2020, 6:14 p.m. UTC | #3
On Thu, Nov 05, 2020 at 05:39:31PM +0100, Marc Kleine-Budde wrote:
> On 11/5/20 5:24 PM, Manivannan Sadhasivam wrote:

> > Hi Marc,

> > 

> > On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:

> >> During debugging it turned out that some people have setups where the SPI

> >> communication is more prone to CRC errors.

> >>

> >> Increase the severity of both the transfer retry and transfer failure message

> >> to give users feedback without the need to recompile the driver with debug

> >> enabled.

> >>

> >> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> >> Cc: Thomas Kopp <thomas.kopp@microchip.com>

> >> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de

> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

> >> ---

> >>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------

> >>  1 file changed, 8 insertions(+), 8 deletions(-)

> >>

> >> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

> >> index ba25902dd78c..c9ffc5ea2b25 100644

> >> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

> >> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

> >> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,

> >>  			goto out;

> >>  		}

> >>  

> >> -		netdev_dbg(priv->ndev,

> >> -			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",

> >> -			   reg, val_len, (int)val_len, buf_rx->data,

> >> -			   get_unaligned_be16(buf_rx->data + val_len));

> >> -	}

> >> -

> >> -	if (err) {

> >>  		netdev_info(priv->ndev,

> >> -			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",

> >> +			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",

> >>  			    reg, val_len, (int)val_len, buf_rx->data,

> >>  			    get_unaligned_be16(buf_rx->data + val_len));

> > 

> > I'm not finding this inner debug log useful. Does the user really care

> > about the iterations it took to read a register? Just throwing the error

> > after max try seems better to me.

> 

> Bitflips on the SPI should not happen, at least not regularly. Even with my

> breadboard setup and max SPI frequency, I don't see any SPI CRC errors, unless

> there is bad cabling. Then a retry most of the times helps and the user doesn't

> notice. This drops performance...Yes logging drop performance, too, but at least

> someone will notice.

> 

> My rationale was to give the user feedback and not cover a potential problem.

> The driver is new and probably not widely used and I have no idea how often it

> runs into read-CRC problems in production use.

> 


Okay. Let's see if anyone complain ;)

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>


Thanks,
Mani

> Marc

> 

> -- 

> Pengutronix e.K.                 | Marc Kleine-Budde           |

> Embedded Linux                   | https://www.pengutronix.de  |

> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

>
Marc Kleine-Budde Nov. 6, 2020, 8:35 a.m. UTC | #4
On 11/5/20 7:14 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 05, 2020 at 05:39:31PM +0100, Marc Kleine-Budde wrote:
>> On 11/5/20 5:24 PM, Manivannan Sadhasivam wrote:
>>> Hi Marc,
>>>
>>> On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:
>>>> During debugging it turned out that some people have setups where the SPI
>>>> communication is more prone to CRC errors.
>>>>
>>>> Increase the severity of both the transfer retry and transfer failure message
>>>> to give users feedback without the need to recompile the driver with debug
>>>> enabled.
>>>>
>>>> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>> Cc: Thomas Kopp <thomas.kopp@microchip.com>
>>>> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> ---
>>>>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
>>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>>>> index ba25902dd78c..c9ffc5ea2b25 100644
>>>> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>>>> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>>>> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,
>>>>  			goto out;
>>>>  		}
>>>>  
>>>> -		netdev_dbg(priv->ndev,
>>>> -			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
>>>> -			   reg, val_len, (int)val_len, buf_rx->data,
>>>> -			   get_unaligned_be16(buf_rx->data + val_len));
>>>> -	}
>>>> -
>>>> -	if (err) {
>>>>  		netdev_info(priv->ndev,
>>>> -			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
>>>> +			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
>>>>  			    reg, val_len, (int)val_len, buf_rx->data,
>>>>  			    get_unaligned_be16(buf_rx->data + val_len));
>>>
>>> I'm not finding this inner debug log useful. Does the user really care
>>> about the iterations it took to read a register? Just throwing the error
>>> after max try seems better to me.
>>
>> Bitflips on the SPI should not happen, at least not regularly. Even with my
>> breadboard setup and max SPI frequency, I don't see any SPI CRC errors, unless
>> there is bad cabling. Then a retry most of the times helps and the user doesn't
>> notice. This drops performance...Yes logging drop performance, too, but at least
>> someone will notice.
>>
>> My rationale was to give the user feedback and not cover a potential problem.
>> The driver is new and probably not widely used and I have no idea how often it
>> runs into read-CRC problems in production use.
>>
> 
> Okay. Let's see if anyone complain ;)
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

The patch is part of my pull request and has already been merged by Jakub to
net/master.

Marc
diff mbox series

Patch

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index ba25902dd78c..c9ffc5ea2b25 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -330,17 +330,17 @@  mcp251xfd_regmap_crc_read(void *context,
 			goto out;
 		}
 
-		netdev_dbg(priv->ndev,
-			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
-			   reg, val_len, (int)val_len, buf_rx->data,
-			   get_unaligned_be16(buf_rx->data + val_len));
-	}
-
-	if (err) {
 		netdev_info(priv->ndev,
-			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
+			    "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
 			    reg, val_len, (int)val_len, buf_rx->data,
 			    get_unaligned_be16(buf_rx->data + val_len));
+	}
+
+	if (err) {
+		netdev_err(priv->ndev,
+			   "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
+			   reg, val_len, (int)val_len, buf_rx->data,
+			   get_unaligned_be16(buf_rx->data + val_len));
 
 		return err;
 	}