Patchwork [BUG] 2.6.33-rc1 gianfar error message

login
register
mail settings
Submitter Kumar Gopalpet-B05799
Date Dec. 21, 2009, 8:49 a.m.
Message ID <9F4C7D19E8361D4C94921B95BE08B81BA08F83@zin33exm22.fsl.freescale.net>
Download mbox | patch
Permalink /patch/41526/
State RFC
Delegated to: David Miller
Headers show

Comments

Kumar Gopalpet-B05799 - Dec. 21, 2009, 8:49 a.m.
>-----Original Message-----
>From: Michael Guntsche [mailto:mike@it-loops.com] 
>Sent: Monday, December 21, 2009 12:57 PM
>To: Kumar Gopalpet-B05799
>Cc: netdev
>Subject: Re: [BUG] 2.6.33-rc1 gianfar error message
>
>On 21 Dec 09 09:58, Kumar Gopalpet-B05799 wrote:
>> Hi Michael,
>> 
>> It looks like even we are enabling only single tx queue, some how it 
>> is trying to pick up tx queue 66.
>> 
>> Can you please let me know what is your test setup and, 
>please give me
>The hardware used is a Mikrotik Routerboard RB600a, which is 
>83xx based.
>It has three NICs two of them being gianfar devices.
>
>> the location from where you cloned this kernel. 
>The kernel used is vanilla 2.6.33-rc1 from git.kernel.org plus 
>the Board specific patches. The patches do not change anything 
>on the network or driver side, they just add ATA, nand and 
>board specific support to the kernel. If you want I can 
>provide a git repo for it.
>
>> Can you also share the dts file you were using ?
>Attached you will find the DTS file being used.
>
>Kind regards,
>Michael
>
>
Michael, 

Kindly try the following changes at your side and let me know the
results. I couldn't test this
as I don't have any TSEC device with me.

                gfar_rx_checksum(skb, fcb);
@@ -2555,6 +2556,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q
*rx_queue, int rx_work_limit)
                                skb_put(skb, pkt_len);
                                rx_queue->stats.rx_bytes += pkt_len;

+                               skb_set_queue_mapping(skb,
rx_queue->qindex);
                                gfar_process_frame(dev, skb,
amount_pull);

                        } else {
--

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Guntsche - Dec. 21, 2009, 4:38 p.m.
On 21 Dec 09 14:19, Kumar Gopalpet-B05799 wrote:
> Michael, 
> 
> Kindly try the following changes at your side and let me know the
> results. I couldn't test this
> as I don't have any TSEC device with me.
<snip>

Your patch did not apply to vanilla 2.6.32-rc1 at all. I patched the
code myself and up to now I do not see any error messages. Uptime is at
33 minutes, without the patch the error message appeared immediately
after booting.

Just for my understanding. Is the first diff part of the fix as well?
With this change the FCB is only removed if there are padded bytes, why
would this make a difference here?

Kind regards,
Michael
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kumar Gopalpet-B05799 - Dec. 22, 2009, 5:02 a.m.
>-----Original Message-----
>From: Michael Guntsche [mailto:mike@it-loops.com] 
>Sent: Monday, December 21, 2009 10:09 PM
>To: Kumar Gopalpet-B05799
>Cc: netdev
>Subject: Re: [BUG] 2.6.33-rc1 gianfar error message
>
>On 21 Dec 09 14:19, Kumar Gopalpet-B05799 wrote:
>> Michael,
>> 
>> Kindly try the following changes at your side and let me know the 
>> results. I couldn't test this as I don't have any TSEC 
>device with me.
><snip>
>
>Your patch did not apply to vanilla 2.6.32-rc1 at all. I patched the
>code myself and up to now I do not see any error messages. Uptime is at
>33 minutes, without the patch the error message appeared immediately
>after booting.
>
I am sorry about that, I forgot to mention that my patch was against
net-next-2.6 tree.

>Just for my understanding. Is the first diff part of the fix as well?
>With this change the FCB is only removed if there are padded bytes, why
>would this make a difference here?
>

Yes, the first diff portion is not required.

Also, if you confirm that the changes are working fine.
I will generate a clean patch and send it out.
Thanks for reporting it and testing it out.

--

Thanks
Sandeep
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Guntsche - Dec. 22, 2009, 11:52 a.m.
On 22 Dec 09 10:32, Kumar Gopalpet-B05799 wrote:
> >Just for my understanding. Is the first diff part of the fix as well?
> >With this change the FCB is only removed if there are padded bytes, why
> >would this make a difference here?
> >
> 
> Yes, the first diff portion is not required.
> 
> Also, if you confirm that the changes are working fine.
> I will generate a clean patch and send it out.
> Thanks for reporting it and testing it out.

Hi,

I have now an uptime of 20 hours with no error messages so far. I had
one ppp disconnect but I think this was ADSL related. 

I will continue running this kernel for now to test it some more but I
think the initial problem I was seeing is fixed now.

Kind regards,
Michael

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Guntsche - Dec. 22, 2009, 6:10 p.m.
On 2009.12.22 10:32:49 , Kumar Gopalpet-B05799 wrote:
> >Just for my understanding. Is the first diff part of the fix as well?
> >With this change the FCB is only removed if there are padded bytes, why
> >would this make a difference here?
> >
> 
> Yes, the first diff portion is not required.
> 
> Also, if you confirm that the changes are working fine.
> I will generate a clean patch and send it out.
> Thanks for reporting it and testing it out.

Hi,

Since you said that the first patch is not needed

@@ -2470,10 +2470,11 @@ static int gfar_process_frame(struct net_device
        fcb = (struct rxfcb *)skb->data;

        /* Remove the FCB from the skb */
-       skb_set_queue_mapping(skb, fcb->rq);
        /* Remove the padded bytes, if there are any */
-       if (amount_pull)
+       if (amount_pull) {
+               skb_set_queue_mapping(skb, fcb->rq);
                skb_pull(skb, amount_pull);
+       }

        if (priv->rx_csum_enable)
                gfar_rx_checksum(skb, fcb);

I only applied the second one and tested again. Right after the reboot I got an error so apparently this change IS required as well. 

Kind regards,
Michael 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Guntsche - Dec. 22, 2009, 6:27 p.m.
On 2009.12.22 19:10:00 , Michael Guntsche wrote:
> Since you said that the first patch is not needed
> 
> @@ -2470,10 +2470,11 @@ static int gfar_process_frame(struct net_device
>         fcb = (struct rxfcb *)skb->data;
> 
>         /* Remove the FCB from the skb */
> -       skb_set_queue_mapping(skb, fcb->rq);
>         /* Remove the padded bytes, if there are any */
> -       if (amount_pull)
> +       if (amount_pull) {
> +               skb_set_queue_mapping(skb, fcb->rq);
>                 skb_pull(skb, amount_pull);
> +       }
> 
>         if (priv->rx_csum_enable)
>                 gfar_rx_checksum(skb, fcb);
> 
> I only applied the second one and tested again. Right after the reboot
> I got an error so apparently this change IS required as well. 

I tested this now in the opposite direction and apparently ONLY this
patch is needed. I commented out the second diff and did not get any
errors so far. Sorry for not testing this before sending my previous
mail.

Kind regards,
Michael
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kumar Gopalpet-B05799 - Dec. 23, 2009, 2:15 p.m.
>-----Original Message-----
>From: Michael Guntsche [mailto:mike@it-loops.com] 
>Sent: Tuesday, December 22, 2009 11:58 PM
>To: Kumar Gopalpet-B05799
>Cc: netdev
>Subject: Re: [BUG] 2.6.33-rc1 gianfar error message
>
>On 2009.12.22 19:10:00 , Michael Guntsche wrote:
>> Since you said that the first patch is not needed
>> 
>> @@ -2470,10 +2470,11 @@ static int gfar_process_frame(struct 
>net_device
>>         fcb = (struct rxfcb *)skb->data;
>> 
>>         /* Remove the FCB from the skb */
>> -       skb_set_queue_mapping(skb, fcb->rq);
>>         /* Remove the padded bytes, if there are any */
>> -       if (amount_pull)
>> +       if (amount_pull) {
>> +               skb_set_queue_mapping(skb, fcb->rq);
>>                 skb_pull(skb, amount_pull);
>> +       }
>> 
>>         if (priv->rx_csum_enable)
>>                 gfar_rx_checksum(skb, fcb);
>> 
>> I only applied the second one and tested again. Right after 
>the reboot 
>> I got an error so apparently this change IS required as well.
>
>I tested this now in the opposite direction and apparently 
>ONLY this patch is needed. I commented out the second diff and 
>did not get any errors so far. Sorry for not testing this 
>before sending my previous mail.
>

Ok. Thanks !

>Kind regards,
>Michael
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index e0620d0..4577d90 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2470,10 +2470,11 @@  static int gfar_process_frame(struct net_device
*dev, struct sk_buff *skb,
        fcb = (struct rxfcb *)skb->data;

        /* Remove the FCB from the skb */
-       skb_set_queue_mapping(skb, fcb->rq);
        /* Remove the padded bytes, if there are any */
-       if (amount_pull)
+       if (amount_pull) {
+               skb_set_queue_mapping(skb, fcb->rq);
                skb_pull(skb, amount_pull);
+       }

        if (priv->rx_csum_enable)