Re: [PATCH] can: xilinx: fix RX FIFO overflow error handling

Re: [PATCH] can: xilinx: fix RX FIFO overflow error handling

[Michal Simek]

+ Kedar

On 07/23/2015 11:13 PM, Andrea Scian wrote:
> Simply resetting the peripheral on RX FIFO overflow in not enough,
> because we also need to re-initialize the whole device.
> Also always enable RX FIFO overflow interrupt otherwise we may hang
> until another interrupt arrives (this happens if FIFO overrun and just
> read from CAN bus with candump)
> 
> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> ---
> 
> You can reproduce the problem with the following test-bed
> * connect a Zynq based board to a PC with a CAN bus adapter (e.g. PCAN USB)
> * start sending lots of CAN messages to Zynq system
> * configure and start xilinx CAN driver
> ** canconfig can0 bitrate 1000000
> ** canconfig can0 start
> * try to send/receive messages (cansend/candump)
> * if you send a CAN message you'll get the RX FIFO error and additional messages
> will not be received
> * if you try to get messages you simple don't receive them (no interrupt triggered)
> * with canconfig stop/start the hang goes away (if the others peers stop sending
> send lots of messages ;-) )
> 
> I tested the patch over xilinx-2014.04 kernel, but the patch applies cleanly to
> mainline too.
> 
> If someone has a better understanding of Xilinx CAN driver, please let me know if
> it's better to handle the error in a different manner.
> 
> Maybe the bus off handler need the same threadment too.
> 
> TIA,
> 
> Andrea Scian
> 
> ---
>  drivers/net/can/xilinx_can.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 5e8b560..c278177 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -100,6 +100,7 @@ enum xcan_reg {
>  #define XCAN_INTR_ALL		(XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |\
>  				 XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK | \
>  				 XCAN_IXR_RXNEMP_MASK | XCAN_IXR_ERROR_MASK | \
> +				 XCAN_IXR_RXOFLW_MASK | \
>  				 XCAN_IXR_ARBLST_MASK | XCAN_IXR_RXOK_MASK)
>  
>  /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
> @@ -526,6 +527,7 @@ static int xcan_rx(struct net_device *ndev)
>  	return 1;
>  }
>  
> +static void xcan_chip_stop(struct net_device *ndev);
>  /**
>   * xcan_err_interrupt - error frame Isr
>   * @ndev:	net_device pointer
> @@ -597,7 +599,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>  	if (isr & XCAN_IXR_RXOFLW_MASK) {
>  		stats->rx_over_errors++;
>  		stats->rx_errors++;
> -		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> +		xcan_chip_stop(ndev);
> +		xcan_chip_start(ndev);
>  		if (skb) {
>  			cf->can_id |= CAN_ERR_CRTL;
>  			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> 

Re: [PATCH] can: xilinx: fix RX FIFO overflow error handling

[Marc Kleine-Budde]

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

On 07/24/2015 06:15 AM, Michal Simek wrote:
> + Kedar
> 
> On 07/23/2015 11:13 PM, Andrea Scian wrote:
>> Simply resetting the peripheral on RX FIFO overflow in not enough,
>> because we also need to re-initialize the whole device.
>> Also always enable RX FIFO overflow interrupt otherwise we may hang
>> until another interrupt arrives (this happens if FIFO overrun and just
>> read from CAN bus with candump)
>>
>> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>

Sorry, I missed the original patch. Can you repost it to:
linux-can@vger.kernel.org

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] can: xilinx: fix RX FIFO overflow error handling

[Andrea Scian - DAVE Embedded Systems]

Michal,
Kedar,

Any feedback regarding my patch?
Have you ever experienced such a problem?

Andrea SCIAN

*DAVE Embedded Systems*
via Talponedo 29/A 33080 Porcia (PN) - Italy
Telephone: +39.0434.921215
Telefax: +39.0434.1994030
web: www.dave.eu <http://www.dave.eu>


Il 24/07/2015 06:15, Michal Simek ha scritto:
> + Kedar
>
> On 07/23/2015 11:13 PM, Andrea Scian wrote:
>> Simply resetting the peripheral on RX FIFO overflow in not enough,
>> because we also need to re-initialize the whole device.
>> Also always enable RX FIFO overflow interrupt otherwise we may hang
>> until another interrupt arrives (this happens if FIFO overrun and just
>> read from CAN bus with candump)
>>
>> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
>> ---
>>
>> You can reproduce the problem with the following test-bed
>> * connect a Zynq based board to a PC with a CAN bus adapter (e.g. PCAN USB)
>> * start sending lots of CAN messages to Zynq system
>> * configure and start xilinx CAN driver
>> ** canconfig can0 bitrate 1000000
>> ** canconfig can0 start
>> * try to send/receive messages (cansend/candump)
>> * if you send a CAN message you'll get the RX FIFO error and additional messages
>> will not be received
>> * if you try to get messages you simple don't receive them (no interrupt triggered)
>> * with canconfig stop/start the hang goes away (if the others peers stop sending
>> send lots of messages ;-) )
>>
>> I tested the patch over xilinx-2014.04 kernel, but the patch applies cleanly to
>> mainline too.
>>
>> If someone has a better understanding of Xilinx CAN driver, please let me know if
>> it's better to handle the error in a different manner.
>>
>> Maybe the bus off handler need the same threadment too.
>>
>> TIA,
>>
>> Andrea Scian
>>
>> ---
>>   drivers/net/can/xilinx_can.c |    5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>> index 5e8b560..c278177 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -100,6 +100,7 @@ enum xcan_reg {
>>   #define XCAN_INTR_ALL		(XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |\
>>   				 XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK | \
>>   				 XCAN_IXR_RXNEMP_MASK | XCAN_IXR_ERROR_MASK | \
>> +				 XCAN_IXR_RXOFLW_MASK | \
>>   				 XCAN_IXR_ARBLST_MASK | XCAN_IXR_RXOK_MASK)
>>
>>   /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
>> @@ -526,6 +527,7 @@ static int xcan_rx(struct net_device *ndev)
>>   	return 1;
>>   }
>>
>> +static void xcan_chip_stop(struct net_device *ndev);
>>   /**
>>    * xcan_err_interrupt - error frame Isr
>>    * @ndev:	net_device pointer
>> @@ -597,7 +599,8 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>>   	if (isr & XCAN_IXR_RXOFLW_MASK) {
>>   		stats->rx_over_errors++;
>>   		stats->rx_errors++;
>> -		priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
>> +		xcan_chip_stop(ndev);
>> +		xcan_chip_start(ndev);
>>   		if (skb) {
>>   			cf->can_id |= CAN_ERR_CRTL;
>>   			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
>>
>

RE: [PATCH] can: xilinx: fix RX FIFO overflow error handling

[Appana Durga Kedareswara Rao]

Hi,

	Thanks for the patch. Sorry for the delay in the reply.
Patch looks good to me 

Reviewed-by: Kedareswara rao Appana <appanad@xilinx.com>   

The bus off condition in the error interrupt handler also needs the same treatment sent the patch for the same.
Please resend your patch on top of my patch series.

Regards,
Kedar


> -----Original Message-----
> From: Andrea Scian - DAVE Embedded Systems
> [mailto:andrea.scian@dave.eu]
> Sent: Thursday, August 06, 2015 1:31 AM
> To: Michal Simek; Appana Durga Kedareswara Rao
> Cc: mkl@pengutronix.de; wg@grandegger.com; linux-can@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] can: xilinx: fix RX FIFO overflow error handling
> 
> 
> Michal,
> Kedar,
> 
> Any feedback regarding my patch?
> Have you ever experienced such a problem?
> 
> Andrea SCIAN
> 
> *DAVE Embedded Systems*
> via Talponedo 29/A 33080 Porcia (PN) - Italy
> Telephone: +39.0434.921215
> Telefax: +39.0434.1994030
> web: www.dave.eu <http://www.dave.eu>
> 
> 
> Il 24/07/2015 06:15, Michal Simek ha scritto:
> > + Kedar
> >
> > On 07/23/2015 11:13 PM, Andrea Scian wrote:
> >> Simply resetting the peripheral on RX FIFO overflow in not enough,
> >> because we also need to re-initialize the whole device.
> >> Also always enable RX FIFO overflow interrupt otherwise we may hang
> >> until another interrupt arrives (this happens if FIFO overrun and
> >> just read from CAN bus with candump)
> >>
> >> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> >> ---
> >>
> >> You can reproduce the problem with the following test-bed
> >> * connect a Zynq based board to a PC with a CAN bus adapter (e.g.
> >> PCAN USB)
> >> * start sending lots of CAN messages to Zynq system
> >> * configure and start xilinx CAN driver
> >> ** canconfig can0 bitrate 1000000
> >> ** canconfig can0 start
> >> * try to send/receive messages (cansend/candump)
> >> * if you send a CAN message you'll get the RX FIFO error and
> >> additional messages will not be received
> >> * if you try to get messages you simple don't receive them (no
> >> interrupt triggered)
> >> * with canconfig stop/start the hang goes away (if the others peers
> >> stop sending send lots of messages ;-) )
> >>
> >> I tested the patch over xilinx-2014.04 kernel, but the patch applies
> >> cleanly to mainline too.
> >>
> >> If someone has a better understanding of Xilinx CAN driver, please
> >> let me know if it's better to handle the error in a different manner.
> >>
> >> Maybe the bus off handler need the same threadment too.
> >>
> >> TIA,
> >>
> >> Andrea Scian
> >>
> >> ---
> >>   drivers/net/can/xilinx_can.c |    5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/can/xilinx_can.c
> >> b/drivers/net/can/xilinx_can.c index 5e8b560..c278177 100644
> >> --- a/drivers/net/can/xilinx_can.c
> >> +++ b/drivers/net/can/xilinx_can.c
> >> @@ -100,6 +100,7 @@ enum xcan_reg {
> >>   #define XCAN_INTR_ALL		(XCAN_IXR_TXOK_MASK |
> XCAN_IXR_BSOFF_MASK |\
> >>   				 XCAN_IXR_WKUP_MASK |
> XCAN_IXR_SLP_MASK | \
> >>   				 XCAN_IXR_RXNEMP_MASK |
> XCAN_IXR_ERROR_MASK | \
> >> +				 XCAN_IXR_RXOFLW_MASK | \
> >>   				 XCAN_IXR_ARBLST_MASK |
> XCAN_IXR_RXOK_MASK)
> >>
> >>   /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ @@ -526,6
> >> +527,7 @@ static int xcan_rx(struct net_device *ndev)
> >>   	return 1;
> >>   }
> >>
> >> +static void xcan_chip_stop(struct net_device *ndev);
> >>   /**
> >>    * xcan_err_interrupt - error frame Isr
> >>    * @ndev:	net_device pointer
> >> @@ -597,7 +599,8 @@ static void xcan_err_interrupt(struct net_device
> *ndev, u32 isr)
> >>   	if (isr & XCAN_IXR_RXOFLW_MASK) {
> >>   		stats->rx_over_errors++;
> >>   		stats->rx_errors++;
> >> -		priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_RESET_MASK);
> >> +		xcan_chip_stop(ndev);
> >> +		xcan_chip_start(ndev);
> >>   		if (skb) {
> >>   			cf->can_id |= CAN_ERR_CRTL;
> >>   			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> >>
> >