Re: [PATCH 11/21] e1000: disable CRC stripping workaround
Jeff Garzik wrote: Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Signed-off-by: Jesse Brandeburg [EMAIL PROTECTED] Signed-off-by: Auke Kok [EMAIL PROTECTED] --- drivers/net/e1000/e1000_main.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index c44ed6f..7787299 100644 --- a/drivers/net/e1000/e1000_main.c +++ b/drivers/net/e1000/e1000_main.c @@ -1628,8 +1628,11 @@ e1000_setup_rctl(struct e1000_adapter *a E1000_RCTL_LBM_NO | E1000_RCTL_RDMTS_HALF | (adapter-hw.mc_filter_type E1000_RCTL_MO_SHIFT); +/* disable hardware stripping of CRC because it breaks + * BMC firmware connected over SMBUS if (adapter-hw.mac_type e1000_82543) rctl |= E1000_RCTL_SECRC; + */ if (adapter-hw.tbi_compatibility_on == 1) rctl |= E1000_RCTL_SBP; @@ -1696,7 +1699,9 @@ e1000_setup_rctl(struct e1000_adapter *a rfctl |= E1000_RFCTL_IPV6_DIS; E1000_WRITE_REG(adapter-hw, RFCTL, rfctl); -rctl |= E1000_RCTL_DTYP_PS | E1000_RCTL_SECRC; +/* disable the stripping of CRC because it breaks + * BMC firmware connected over SMBUS */ +rctl |= E1000_RCTL_DTYP_PS /* | E1000_RCTL_SECRC */; This is quite ugly. You are basically bloating the code with historic, dead code, no different than filling the e1000/*.c with '#if 0' regions. Just delete it, and explain why in the patch description. Adjusted the patch on our git server to this: diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index c44ed6f..a9e55dc 100644 --- a/drivers/net/e1000/e1000_main.c +++ b/drivers/net/e1000/e1000_main.c @@ -1628,9 +1628,6 @@ e1000_setup_rctl(struct e1000_adapter *a E1000_RCTL_LBM_NO | E1000_RCTL_RDMTS_HALF | (adapter-hw.mc_filter_type E1000_RCTL_MO_SHIFT); - if (adapter-hw.mac_type e1000_82543) - rctl |= E1000_RCTL_SECRC; - if (adapter-hw.tbi_compatibility_on == 1) rctl |= E1000_RCTL_SBP; else @@ -1696,7 +1693,7 @@ e1000_setup_rctl(struct e1000_adapter *a rfctl |= E1000_RFCTL_IPV6_DIS; E1000_WRITE_REG(adapter-hw, RFCTL, rfctl); - rctl |= E1000_RCTL_DTYP_PS | E1000_RCTL_SECRC; + rctl |= E1000_RCTL_DTYP_PS; psrctl |= adapter-rx_ps_bsize0 E1000_PSRCTL_BSIZE0_SHIFT; Cheers, Auke - 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 11/21] e1000: disable CRC stripping workaround
Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Signed-off-by: Jesse Brandeburg [EMAIL PROTECTED] Signed-off-by: Auke Kok [EMAIL PROTECTED] --- drivers/net/e1000/e1000_main.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index c44ed6f..7787299 100644 --- a/drivers/net/e1000/e1000_main.c +++ b/drivers/net/e1000/e1000_main.c @@ -1628,8 +1628,11 @@ e1000_setup_rctl(struct e1000_adapter *a E1000_RCTL_LBM_NO | E1000_RCTL_RDMTS_HALF | (adapter-hw.mc_filter_type E1000_RCTL_MO_SHIFT); + /* disable hardware stripping of CRC because it breaks +* BMC firmware connected over SMBUS if (adapter-hw.mac_type e1000_82543) rctl |= E1000_RCTL_SECRC; +*/ if (adapter-hw.tbi_compatibility_on == 1) rctl |= E1000_RCTL_SBP; @@ -1696,7 +1699,9 @@ e1000_setup_rctl(struct e1000_adapter *a rfctl |= E1000_RFCTL_IPV6_DIS; E1000_WRITE_REG(adapter-hw, RFCTL, rfctl); - rctl |= E1000_RCTL_DTYP_PS | E1000_RCTL_SECRC; + /* disable the stripping of CRC because it breaks +* BMC firmware connected over SMBUS */ + rctl |= E1000_RCTL_DTYP_PS /* | E1000_RCTL_SECRC */; This is quite ugly. You are basically bloating the code with historic, dead code, no different than filling the e1000/*.c with '#if 0' regions. Just delete it, and explain why in the patch description. Jeff - 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 11/21] e1000: disable CRC stripping workaround
Ben Greear wrote: Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Shouldn't you also have to subtract 4 bytes when setting the skb len in the receive logic? Perhaps when setting the rx-bytes counter as well? the hardware corrects for the size properly when we disable CRC stripping. The end result is the same. Cheers, Auke - 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 11/21] e1000: disable CRC stripping workaround
On 6/21/06, Ben Greear [EMAIL PROTECTED] wrote: Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Shouldn't you also have to subtract 4 bytes when setting the skb len in the receive logic? Perhaps when setting the rx-bytes counter as well? we thought about this, but most drivers don't strip the CRC, and we couldn't find any tests including bridging that cared if the CRC was there in the indicated packet. If you can find me a failing case I'll fix it. It was much simpler to leave it out, especially when we add back in the multiple descriptor receive code in the future (think about the case when subtracting the CRC makes the last descriptor disappear) Once again, let me know if you have info I don't :-) Thanks for the review, Jesse - 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 11/21] e1000: disable CRC stripping workaround
Jesse Brandeburg wrote: On 6/21/06, Ben Greear [EMAIL PROTECTED] wrote: Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Shouldn't you also have to subtract 4 bytes when setting the skb len in the receive logic? Perhaps when setting the rx-bytes counter as well? we thought about this, but most drivers don't strip the CRC, and we couldn't find any tests including bridging that cared if the CRC was there in the indicated packet. If you can find me a failing case I'll fix it. It was much simpler to leave it out, especially when we add back in the multiple descriptor receive code in the future (think about the case when subtracting the CRC makes the last descriptor disappear) Once again, let me know if you have info I don't :-) It should only be a problem if skb-len includes the extra 4 bytes for the crc. Then, if I transmit that skb to another interface, I am afraid that the crc will be seen as data in the packet. In the 2.6.13 days, the e1000 did not strip the CRC, but it subtracted 4 before it did the skb_put. So, the crc was correctly stripped/ignored. The e100 functioned similarly I believe. If you skb_put the extra 4 bytes, I believe this will break my (proprietary) app because on transmit it will append the extra 4 crc bytes, but that isn't your problem..and I can work around it. If the receiving NIC can handle pkts 4 bytes bigger than normal, it will probably still receive the packet w/out problem, but in truth, the frame will not be exactly correct. When you did your bridging tests, did you sniff the packets on the far side of the bridge to see if they were the right size? Thanks, Ben Thanks for the review, Jesse - 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 11/21] e1000: disable CRC stripping workaround
On Thu, Jun 22, 2006 at 08:39:10AM -0700, Jesse Brandeburg wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Shouldn't you also have to subtract 4 bytes when setting the skb len in the receive logic? Perhaps when setting the rx-bytes counter as well? we thought about this, but most drivers don't strip the CRC, Really? and we couldn't find any tests including bridging that cared if the CRC was there in the indicated packet. Bridging definitely cares -- some years ago there was a case where 8139too NICs would pass packets up the stack with 4 bytes of FCS, and that causes frames received on 8139too interfaces not to be forwarded to other interfaces because on TX, the frame would be too long. Maybe e1000 is okay with sending oversized frames, but other NIC drivers might not be. (Did you test without bridge-netfilter enabled? bridge-nf might trim incoming IP packets even in the bridging case.) cheers, Lennert - 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 11/21] e1000: disable CRC stripping workaround
On 6/22/06, Ben Greear [EMAIL PROTECTED] wrote: Jesse Brandeburg wrote: On 6/21/06, Ben Greear [EMAIL PROTECTED] wrote: Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Shouldn't you also have to subtract 4 bytes when setting the skb len in the receive logic? Perhaps when setting the rx-bytes counter as well? we thought about this, but most drivers don't strip the CRC, and we couldn't find any tests including bridging that cared if the CRC was there in the indicated packet. If you can find me a failing case I'll fix it. It was much simpler to leave it out, especially when we add back in the multiple descriptor receive code in the future (think about the case when subtracting the CRC makes the last descriptor disappear) Once again, let me know if you have info I don't :-) It should only be a problem if skb-len includes the extra 4 bytes for the crc. Then, if I transmit that skb to another interface, I am afraid that the crc will be seen as data in the packet. In the 2.6.13 days, the e1000 did not strip the CRC, but it subtracted 4 before it did the skb_put. So, the crc was correctly stripped/ignored. The e100 functioned similarly I believe. currently the e100 driver in 2.6.X strips the CRC in hardware. If you skb_put the extra 4 bytes, I believe this will break my (proprietary) app because on transmit it will append the extra 4 crc bytes, but that isn't your problem..and I can work around it. If the receiving NIC can handle pkts 4 bytes bigger than normal, it will probably still receive the packet w/out problem, but in truth, the frame will not be exactly correct. When you did your bridging tests, did you sniff the packets on the far side of the bridge to see if they were the right size? hm, probably not, we touch tested bridging (probably with TCP), and have completed several internal testing passes, to make sure it worked but I don't think we went so far as to sniff the traffic at the other end of the bridge. I'll look into it. Jesse - 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
[PATCH 11/21] e1000: disable CRC stripping workaround
CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Signed-off-by: Jesse Brandeburg [EMAIL PROTECTED] Signed-off-by: Auke Kok [EMAIL PROTECTED] --- drivers/net/e1000/e1000_main.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index c44ed6f..7787299 100644 --- a/drivers/net/e1000/e1000_main.c +++ b/drivers/net/e1000/e1000_main.c @@ -1628,8 +1628,11 @@ e1000_setup_rctl(struct e1000_adapter *a E1000_RCTL_LBM_NO | E1000_RCTL_RDMTS_HALF | (adapter-hw.mc_filter_type E1000_RCTL_MO_SHIFT); + /* disable hardware stripping of CRC because it breaks +* BMC firmware connected over SMBUS if (adapter-hw.mac_type e1000_82543) rctl |= E1000_RCTL_SECRC; +*/ if (adapter-hw.tbi_compatibility_on == 1) rctl |= E1000_RCTL_SBP; @@ -1696,7 +1699,9 @@ e1000_setup_rctl(struct e1000_adapter *a rfctl |= E1000_RFCTL_IPV6_DIS; E1000_WRITE_REG(adapter-hw, RFCTL, rfctl); - rctl |= E1000_RCTL_DTYP_PS | E1000_RCTL_SECRC; + /* disable the stripping of CRC because it breaks +* BMC firmware connected over SMBUS */ + rctl |= E1000_RCTL_DTYP_PS /* | E1000_RCTL_SECRC */; psrctl |= adapter-rx_ps_bsize0 E1000_PSRCTL_BSIZE0_SHIFT; -- Auke Kok [EMAIL PROTECTED] - 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 11/21] e1000: disable CRC stripping workaround
Kok, Auke wrote: CRC stripping is breaking SMBUS-connected BMC's. We disable this feature to make it work. This fixes related bugs regarding SOL. Shouldn't you also have to subtract 4 bytes when setting the skb len in the receive logic? Perhaps when setting the rx-bytes counter as well? Ben - 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
