[PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[AnilKumar Ch]

This patch fixes an issue with transmit routine, which causes
"can_put_echo_skb: BUG! echo_skb is occupied!" messgae when
using "cansequence -p" on D_CAN controller.

In c_can driver, while transmitting packets tx_echo flag holds
the no of can frames put for transmission into the hardware.

As the comment above c_can_do_tx() indicates, if we find any packet
which is not transmitted then we should stop looking for more.
In the current implementation this is not taken care of causing the
said message.

Also, fix the condition used to find if the packet is transmitted
or not. Current code skips the first tx message object and ends up
checking one extra invalid object.

While at it, fix the comment on top of c_can_do_tx() to use the
terminology "packet" instead of "package" since it is more
standard.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 drivers/net/can/c_can/c_can.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 536bda0..9ac28df 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -686,7 +686,7 @@ static int c_can_get_berr_counter(const struct net_device *dev,
  *
  * We iterate from priv->tx_echo to priv->tx_next and check if the
  * packet has been transmitted, echo it back to the CAN framework.
- * If we discover a not yet transmitted package, stop looking for more.
+ * If we discover a not yet transmitted packet, stop looking for more.
  */
 static void c_can_do_tx(struct net_device *dev)
 {
@@ -698,7 +698,7 @@ static void c_can_do_tx(struct net_device *dev)
 	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
 		msg_obj_no = get_tx_echo_msg_obj(priv);
 		val = c_can_read_reg32(priv, &priv->regs->txrqst1);
-		if (!(val & (1 << msg_obj_no))) {
+		if (!(val & (1 << (msg_obj_no - 1)))) {
 			can_get_echo_skb(dev,
 					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
 			stats->tx_bytes += priv->read_reg(priv,
@@ -706,6 +706,8 @@ static void c_can_do_tx(struct net_device *dev)
 					& IF_MCONT_DLC_MASK;
 			stats->tx_packets++;
 			c_can_inval_msg_object(dev, 0, msg_obj_no);
+		} else {
+			break;
 		}
 	}
 
-- 
1.7.0.4

Re: [PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[Marc Kleine-Budde]

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

On 04/20/2012 11:57 AM, AnilKumar Ch wrote:
> This patch fixes an issue with transmit routine, which causes
> "can_put_echo_skb: BUG! echo_skb is occupied!" messgae when
> using "cansequence -p" on D_CAN controller.
> 
> In c_can driver, while transmitting packets tx_echo flag holds
> the no of can frames put for transmission into the hardware.
> 
> As the comment above c_can_do_tx() indicates, if we find any packet
> which is not transmitted then we should stop looking for more.
> In the current implementation this is not taken care of causing the
> said message.
> 
> Also, fix the condition used to find if the packet is transmitted
> or not. Current code skips the first tx message object and ends up
> checking one extra invalid object.

The two above listed issues can occur with the c_can hardware, too?
Please split into two separate patches. These should go to the stable
kernel, too. Can someone with a c_can hardware test these patches.

> While at it, fix the comment on top of c_can_do_tx() to use the
> terminology "packet" instead of "package" since it is more
> standard.

Add this to one of the above patches.

> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 536bda0..9ac28df 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -686,7 +686,7 @@ static int c_can_get_berr_counter(const struct net_device *dev,
>   *
>   * We iterate from priv->tx_echo to priv->tx_next and check if the
>   * packet has been transmitted, echo it back to the CAN framework.
> - * If we discover a not yet transmitted package, stop looking for more.
> + * If we discover a not yet transmitted packet, stop looking for more.
>   */
>  static void c_can_do_tx(struct net_device *dev)
>  {
> @@ -698,7 +698,7 @@ static void c_can_do_tx(struct net_device *dev)
>  	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
>  		msg_obj_no = get_tx_echo_msg_obj(priv);
>  		val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> -		if (!(val & (1 << msg_obj_no))) {
> +		if (!(val & (1 << (msg_obj_no - 1)))) {
>  			can_get_echo_skb(dev,
>  					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>  			stats->tx_bytes += priv->read_reg(priv,
> @@ -706,6 +706,8 @@ static void c_can_do_tx(struct net_device *dev)
>  					& IF_MCONT_DLC_MASK;
>  			stats->tx_packets++;
>  			c_can_inval_msg_object(dev, 0, msg_obj_no);
> +		} else {
> +			break;
>  		}
>  	}
>  

regards, 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: 262 bytes --]

RE: [PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[AnilKumar, Chimata]

Hi Marc,

Thanks for the comments.

On Tue, Apr 24, 2012 at 13:25:25, Marc Kleine-Budde wrote:
> On 04/20/2012 11:57 AM, AnilKumar Ch wrote:
> > This patch fixes an issue with transmit routine, which causes
> > "can_put_echo_skb: BUG! echo_skb is occupied!" messgae when
> > using "cansequence -p" on D_CAN controller.
> > 
> > In c_can driver, while transmitting packets tx_echo flag holds
> > the no of can frames put for transmission into the hardware.
> > 
> > As the comment above c_can_do_tx() indicates, if we find any packet
> > which is not transmitted then we should stop looking for more.
> > In the current implementation this is not taken care of causing the
> > said message.
> > 
> > Also, fix the condition used to find if the packet is transmitted
> > or not. Current code skips the first tx message object and ends up
> > checking one extra invalid object.
> 
> The two above listed issues can occur with the c_can hardware, too?
> Please split into two separate patches. These should go to the stable
> kernel, too. Can someone with a c_can hardware test these patches.

I will split into two patches. I had clubbed both the changes in a single
patch because both the changes are required to get rid of the error mentioned
in the subject line.

I will add the stable tag when submitting next, but since I do not have c_can
hardware, someone needs to confirm this actually solves a c_can issue. I have
read through the c_can specification and see that this would indeed be a problem
on c_can.

> 
> > While at it, fix the comment on top of c_can_do_tx() to use the
> > terminology "packet" instead of "package" since it is more
> > standard.
> 
> Add this to one of the above patches.

Sure.

Regards,
AnilKumar

Re: [PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[Marc Kleine-Budde]

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

On 04/24/2012 01:24 PM, AnilKumar, Chimata wrote:
> On Tue, Apr 24, 2012 at 13:25:25, Marc Kleine-Budde wrote:
>> On 04/20/2012 11:57 AM, AnilKumar Ch wrote:
>>> This patch fixes an issue with transmit routine, which causes
>>> "can_put_echo_skb: BUG! echo_skb is occupied!" messgae when
>>> using "cansequence -p" on D_CAN controller.
>>>
>>> In c_can driver, while transmitting packets tx_echo flag holds
>>> the no of can frames put for transmission into the hardware.
>>>
>>> As the comment above c_can_do_tx() indicates, if we find any packet
>>> which is not transmitted then we should stop looking for more.
>>> In the current implementation this is not taken care of causing the
>>> said message.
>>>
>>> Also, fix the condition used to find if the packet is transmitted
>>> or not. Current code skips the first tx message object and ends up
>>> checking one extra invalid object.
>>
>> The two above listed issues can occur with the c_can hardware, too?
>> Please split into two separate patches. These should go to the stable
>> kernel, too. Can someone with a c_can hardware test these patches.
> 
> I will split into two patches. I had clubbed both the changes in a single
> patch because both the changes are required to get rid of the error mentioned
> in the subject line.

okay - then it's better to keep them together.

> I will add the stable tag when submitting next, but since I do not have c_can
> hardware, someone needs to confirm this actually solves a c_can issue. I have
> read through the c_can specification and see that this would indeed be a problem
> on c_can.

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: 262 bytes --]

RE: [PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[AnilKumar, Chimata]

On Tue, Apr 24, 2012 at 16:58:07, Marc Kleine-Budde wrote:
> On 04/24/2012 01:24 PM, AnilKumar, Chimata wrote:
> > On Tue, Apr 24, 2012 at 13:25:25, Marc Kleine-Budde wrote:
> >> On 04/20/2012 11:57 AM, AnilKumar Ch wrote:
> >>> This patch fixes an issue with transmit routine, which causes
> >>> "can_put_echo_skb: BUG! echo_skb is occupied!" messgae when
> >>> using "cansequence -p" on D_CAN controller.
> >>>
> >>> In c_can driver, while transmitting packets tx_echo flag holds
> >>> the no of can frames put for transmission into the hardware.
> >>>
> >>> As the comment above c_can_do_tx() indicates, if we find any packet
> >>> which is not transmitted then we should stop looking for more.
> >>> In the current implementation this is not taken care of causing the
> >>> said message.
> >>>
> >>> Also, fix the condition used to find if the packet is transmitted
> >>> or not. Current code skips the first tx message object and ends up
> >>> checking one extra invalid object.
> >>
> >> The two above listed issues can occur with the c_can hardware, too?
> >> Please split into two separate patches. These should go to the stable
> >> kernel, too. Can someone with a c_can hardware test these patches.
> > 
> > I will split into two patches. I had clubbed both the changes in a single
> > patch because both the changes are required to get rid of the error mentioned
> > in the subject line.
> 
> okay - then it's better to keep them together.

After we got the status, c_can driver testing with this patch, can I send one
more patch (v2) with stable tag added or will you take care? I am ok with any
of these.

Regards
AnilKumar

Re: [PATCH 1/3] can: c_can: fix "BUG! echo_skb is occupied!" during transmit

[Marc Kleine-Budde]

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

On 04/24/2012 01:53 PM, AnilKumar, Chimata wrote:
>>>>> Also, fix the condition used to find if the packet is transmitted
>>>>> or not. Current code skips the first tx message object and ends up
>>>>> checking one extra invalid object.
>>>>
>>>> The two above listed issues can occur with the c_can hardware, too?
>>>> Please split into two separate patches. These should go to the stable
>>>> kernel, too. Can someone with a c_can hardware test these patches.
>>>
>>> I will split into two patches. I had clubbed both the changes in a single
>>> patch because both the changes are required to get rid of the error mentioned
>>> in the subject line.
>>
>> okay - then it's better to keep them together.
> 
> After we got the status, c_can driver testing with this patch, can I send one
> more patch (v2) with stable tag added or will you take care? I am ok with any
> of these.

Poke me, if I forget to push these patches upstream :)

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: 262 bytes --]