Re: [PATCH 12/14] Spidernet Avoid possible RX chain corruption
On Thu, 2006-12-14 at 11:15 -0600, Linas Vepstas wrote: > On Thu, Dec 14, 2006 at 11:22:43AM +1100, Michael Ellerman wrote: > > > spider_net_refill_rx_chain(card); > > > - spider_net_enable_rxchtails(card); > > > spider_net_enable_rxdmac(card); > > > return 0; > > > > Didn't you just add that line? > > Dagnabbit. The earlier pach was moving around existing code. > Or, more precisely, trying to maintain the general function > of the old code even while moving things around. > > Later on, when I started looking at what the danged function > actually did, and the context it was in, I realized that it > was a bad idea to call the thing. So then I removed it. :-/ > > How should I handle this proceedurally? Resend the patch sequence? > Let it slide? If it was my code I'd redo the series, it's confusing and it's going to look confused in the git history IMHO. Currently the driver calls spider_net_enable_rxchtails() from spider_net_enable_card() and spider_net_handle_rxram_full(). Your patch 3/14 removes spider_net_handle_rxram_full() entirely, leaving spider_net_enable_card() as the only caller of spider_net_enable_rxchtails(). Patch 10/14 adds a call to spider_net_enable_rxchtails() in spider_net_alloc_rx_skbs(), and nothing else (except comment changes). Patch 12/14 removes the call to spider_net_enable_rxchtails() in spider_net_alloc_rx_skbs(), and nothing else. So as far as I can tell you should just drop 10/14 and 12/14. My worry is that amongst all that rearranging of code, it's not clear what the semantic change is. Admittedly I don't know the driver that well, but that's kind of the point - if you and Jim get moved onto a new project, someone needs to be able to pick up the driver and maintain it. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part
Re: [PATCH 12/14] Spidernet Avoid possible RX chain corruption
On Thu, Dec 14, 2006 at 11:15:11AM -0600, Linas Vepstas wrote: > On Thu, Dec 14, 2006 at 11:22:43AM +1100, Michael Ellerman wrote: > > > spider_net_refill_rx_chain(card); > > > - spider_net_enable_rxchtails(card); > > > spider_net_enable_rxdmac(card); > > > return 0; > > > > Didn't you just add that line? > > Dagnabbit. The earlier pach was moving around existing code. > Or, more precisely, trying to maintain the general function > of the old code even while moving things around. > > Later on, when I started looking at what the danged function > actually did, and the context it was in, I realized that it > was a bad idea to call the thing. So then I removed it. :-/ > > How should I handle this proceedurally? Resend the patch sequence? > Let it slide? Just keep it as is in this case. In case you have to redo the patch series for some other reason or for similar cases in the future put the patch to remove things in front of the one that reorders the surrounding bits. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/14] Spidernet Avoid possible RX chain corruption
On Thu, Dec 14, 2006 at 11:22:43AM +1100, Michael Ellerman wrote: > > spider_net_refill_rx_chain(card); > > - spider_net_enable_rxchtails(card); > > spider_net_enable_rxdmac(card); > > return 0; > > Didn't you just add that line? Dagnabbit. The earlier pach was moving around existing code. Or, more precisely, trying to maintain the general function of the old code even while moving things around. Later on, when I started looking at what the danged function actually did, and the context it was in, I realized that it was a bad idea to call the thing. So then I removed it. :-/ How should I handle this proceedurally? Resend the patch sequence? Let it slide? --linas - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/14] Spidernet Avoid possible RX chain corruption
On Wed, 2006-12-13 at 15:23 -0600, Linas Vepstas wrote: > Delete possible source of chain corruption; the hardware > already knows the location of the tail, and writing it > again is likely to mess it up. > > Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]> > Cc: James K Lewis <[EMAIL PROTECTED]> > Cc: Arnd Bergmann <[EMAIL PROTECTED]> > > > > drivers/net/spider_net.c |1 - > 1 file changed, 1 deletion(-) > > Index: linux-2.6.19-git7/drivers/net/spider_net.c > === > --- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-13 > 14:28:23.0 -0600 > +++ linux-2.6.19-git7/drivers/net/spider_net.c2006-12-13 > 14:28:25.0 -0600 > @@ -513,7 +513,6 @@ spider_net_alloc_rx_skbs(struct spider_n > /* This will allocate the rest of the rx buffers; >* if not, it's business as usual later on. */ > spider_net_refill_rx_chain(card); > - spider_net_enable_rxchtails(card); > spider_net_enable_rxdmac(card); > return 0; Didn't you just add that line? From "[PATCH 10/14] Spidernet RX Chain tail": > @@ -501,17 +501,18 @@ spider_net_alloc_rx_skbs(struct spider_n > > > > - /* this will allocate the rest of the rx buffers; if not, it's > -* business as usual later on */ > + /* This will allocate the rest of the rx buffers; > +* if not, it's business as usual later on. */ > spider_net_refill_rx_chain(card); > + spider_net_enable_rxchtails(card); > spider_net_enable_rxdmac(card); > return 0; cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part
[PATCH 12/14] Spidernet Avoid possible RX chain corruption
Delete possible source of chain corruption; the hardware already knows the location of the tail, and writing it again is likely to mess it up. Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]> Cc: James K Lewis <[EMAIL PROTECTED]> Cc: Arnd Bergmann <[EMAIL PROTECTED]> drivers/net/spider_net.c |1 - 1 file changed, 1 deletion(-) Index: linux-2.6.19-git7/drivers/net/spider_net.c === --- linux-2.6.19-git7.orig/drivers/net/spider_net.c 2006-12-13 14:28:23.0 -0600 +++ linux-2.6.19-git7/drivers/net/spider_net.c 2006-12-13 14:28:25.0 -0600 @@ -513,7 +513,6 @@ spider_net_alloc_rx_skbs(struct spider_n /* This will allocate the rest of the rx buffers; * if not, it's business as usual later on. */ spider_net_refill_rx_chain(card); - spider_net_enable_rxchtails(card); spider_net_enable_rxdmac(card); return 0; - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
