Re: 8139cp: set ring address before enabling receiver

Re: 8139cp: set ring address before enabling receiver

[David Woodhouse]

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

On Sat, 2012-06-02 at 23:50 +0000, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/linus/;a=commit;h=b01af4579ec41f48e9b9c774e70bd6474ad210db
> Commit:     b01af4579ec41f48e9b9c774e70bd6474ad210db
> Parent:     20e2a86485967c385d7c7befc1646e4d1d39362e
> Author:     Jason Wang <jasowang@redhat.com>
> AuthorDate: Thu May 31 18:19:39 2012 +0000
> Committer:  David S. Miller <davem@davemloft.net>
> CommitDate: Fri Jun 1 14:22:11 2012 -0400
> 
>     8139cp: set ring address before enabling receiver
>     
>     Currently, we enable the receiver before setting the ring address which could
>     lead the card DMA into unexpected areas. Solving this by set the ring address
>     before enabling the receiver.
>     
>     btw. I find and test this in qemu as I didn't have a 8139cp card in hand. please
>     review it carefully.
>     
>     Signed-off-by: Jason Wang <jasowang@redhat.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>

This commit appears to break Ethernet on my Traverse Geos router. With
OpenWRT and 3.6.6 I get:

[  124.068359] NETDEV WATCHDOG: eth1 (8139cp): transmit queue 0 timed out  
 ...     
[  124.260614] 8139cp 0000:00:0b.0: eth1: Transmit timeout, status  c   2b    1 80ac                                                                            


If I add code to *read* the *RingAddr registers, at the later point in
cp_init_hw() that they *used* to be set, I get the following:

[ 1126.909193] HiTxRingAddr 0000000000000000 (should be 0)                             
[ 1126.913880] RxRingAddr   000000000f1e5000 (sb f1e5000)                       
[ 1126.919018] TxRingAddr   000000000f344400 (sb f1e5400) 

Adding further debugging indicates that it's being changed in
cp_start_hw(), at the line which writes the CpCmd register. These two
outputs are from the surrounding lines...

[ 1331.650579] at line 960 TxRingAddr   000000000f3c6400 (sb f3c6400)           
[ 1331.656820] at line 962 TxRingAddr   000000000f3e4400 (sb f3c6400)  

The devices are:
00:0a.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL-8139/8139C/8139C+ [10ec:8139] (rev 20)                                                  
00:0b.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL-8139/8139C/8139C+ [10ec:8139] (rev 20)      

The other one (eth0) isn't connected, which is why I only see the errors
from eth1.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: 8139cp: set ring address before enabling receiver

[Jeff Garzik]

On 11/21/2012 11:57 AM, David Woodhouse wrote:
> On Sat, 2012-06-02 at 23:50 +0000, Linux Kernel Mailing List wrote:
>> Gitweb:     http://git.kernel.org/linus/;a=commit;h=b01af4579ec41f48e9b9c774e70bd6474ad210db
>> Commit:     b01af4579ec41f48e9b9c774e70bd6474ad210db
>> Parent:     20e2a86485967c385d7c7befc1646e4d1d39362e
>> Author:     Jason Wang <jasowang@redhat.com>
>> AuthorDate: Thu May 31 18:19:39 2012 +0000
>> Committer:  David S. Miller <davem@davemloft.net>
>> CommitDate: Fri Jun 1 14:22:11 2012 -0400
>>
>>      8139cp: set ring address before enabling receiver
>>
>>      Currently, we enable the receiver before setting the ring address which could
>>      lead the card DMA into unexpected areas. Solving this by set the ring address
>>      before enabling the receiver.
>>
>>      btw. I find and test this in qemu as I didn't have a 8139cp card in hand. please
>>      review it carefully.

What sticks out at me from the commit message?

It was not tested on the famously quirky 8139 hardware at all.

While I have not looked at the 8139C+ data sheet in a while, sometimes 
the hardware _did_ have a strange init order.

As this works in a simulator but fails on real hardware, it seems like 
an obvious regression caused by an untested [on read hardware] patch.

	Jeff

Re: 8139cp: set ring address before enabling receiver

[David Woodhouse]

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

On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
> 
> What sticks out at me from the commit message?
> 
> It was not tested on the famously quirky 8139 hardware at all.
> 
> While I have not looked at the 8139C+ data sheet in a while, sometimes 
> the hardware _did_ have a strange init order.
> 
> As this works in a simulator but fails on real hardware, it seems like 
> an obvious regression caused by an untested [on read hardware] patch.

The data sheet (v1.6, from http://realtek.info/pdf/rtl8139cp.pdf ) says
in §6.33 (C+ Command Register):
 "Enable C+ mode functions in C+CR register first,
 => Enable transmit/receive in Command register (offset 37h),
 => Configure other related registers (ex. Descriptor start address,
    TCR, RCR, ...)."

I understand the concern expressed in the offending commit message about
DMA happening to invalid addresses, and I'll look at the data sheet
harder to see when the DMA actually starts happening. But it definitely
seems that our current code isn't doing what the data sheet says.

I wonder if I can find one of these lying around and stick it in a
machine with an IOMMU...

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: 8139cp: set ring address before enabling receiver

[Ben Hutchings]

On Wed, 2012-11-21 at 19:51 +0000, David Woodhouse wrote:
> On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
> > 
> > What sticks out at me from the commit message?
> > 
> > It was not tested on the famously quirky 8139 hardware at all.
> > 
> > While I have not looked at the 8139C+ data sheet in a while, sometimes 
> > the hardware _did_ have a strange init order.
> > 
> > As this works in a simulator but fails on real hardware, it seems like 
> > an obvious regression caused by an untested [on read hardware] patch.
> 
> The data sheet (v1.6, from http://realtek.info/pdf/rtl8139cp.pdf ) says
> in §6.33 (C+ Command Register):
>  "Enable C+ mode functions in C+CR register first,
>  => Enable transmit/receive in Command register (offset 37h),
>  => Configure other related registers (ex. Descriptor start address,
>     TCR, RCR, ...)."
>
> I understand the concern expressed in the offending commit message about
> DMA happening to invalid addresses, and I'll look at the data sheet
> harder to see when the DMA actually starts happening. But it definitely
> seems that our current code isn't doing what the data sheet says.
> 
> I wonder if I can find one of these lying around and stick it in a
> machine with an IOMMU...

You might be able to avoid disaster by doing:

1. Set MAC filter to drop everything
2. Enable RX DMA
3. Set RX DMA ring address
4. Set MAC filter according to current flags & multicast list

I'm assuming, knowing nothing about this particular hardware, that the
MAC filter register(s) will accept writes before RX DMA is enabled.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH] 8139cp: set ring address after enabling C+ mode

[David Woodhouse]

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

This fixes (for me) a regression introduced by commit b01af457 ("8139cp:
set ring address before enabling receiver"). That commit configured the
descriptor ring addresses earlier in the initialisation sequence, in
order to avoid the possibility of triggering stray DMA before the
correct address had been set up.

Unfortunately, it seems that the hardware will scribble garbage into the
TxRingAddr registers when we enable "plus mode" Tx in the CpCmd
register. Observed on a Traverse Geos router board.

To deal with this, while not reintroducing the problem which led to the
original commit, we augment cp_start_hw() to write to the CpCmd register
*first*, then set the descriptor ring addresses, and then finally to
enable Rx and Tx in the original 8139 Cmd register. The datasheet
actually indicates that we should enable Tx/Rx in the Cmd register
*before* configuring the descriptor addresses, but that would appear to
re-introduce the problem that the offending commit b01af457 was trying
to solve. And this variant appears to work fine on real hardware.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Cc: stable@kernel.org [3.5+]

---
How about this? I'm still somewhat confused about when it actually
*does* start doing DMA, given what the datasheet says.

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 1c81825..5166d94 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -957,7 +957,35 @@ static void cp_reset_hw (struct cp_private *cp)
 
 static inline void cp_start_hw (struct cp_private *cp)
 {
+	dma_addr_t ring_dma;
+
 	cpw16(CpCmd, cp->cpcmd);
+
+	/*
+	 * These (at least TxRingAddr) need to be configured after the
+	 * corresponding bits in CpCmd are enabled. Datasheet v1.6 §6.33
+	 * (C+ Command Register) recommends that these and more be configured
+	 * *after* the [RT]xEnable bits in CpCmd are set. And on some hardware
+	 * it's been observed that the TxRingAddr is actually reset to garbage
+	 * when C+ mode Tx is enabled in CpCmd.
+	 */
+	cpw32_f(HiTxRingAddr, 0);
+	cpw32_f(HiTxRingAddr + 4, 0);
+
+	ring_dma = cp->ring_dma;
+	cpw32_f(RxRingAddr, ring_dma & 0xffffffff);
+	cpw32_f(RxRingAddr + 4, (ring_dma >> 16) >> 16);
+
+	ring_dma += sizeof(struct cp_desc) * CP_RX_RING_SIZE;
+	cpw32_f(TxRingAddr, ring_dma & 0xffffffff);
+	cpw32_f(TxRingAddr + 4, (ring_dma >> 16) >> 16);
+
+	/*
+	 * Strictly speaking, the datasheet says this should be enabled
+	 * *before* setting the descriptor addresses. But what, then, would
+	 * prevent it from doing DMA to random unconfigured addresses?
+	 * This variant appears to work fine.
+	 */
 	cpw8(Cmd, RxOn | TxOn);
 }
 
@@ -969,7 +997,6 @@ static void cp_enable_irq(struct cp_private *cp)
 static void cp_init_hw (struct cp_private *cp)
 {
 	struct net_device *dev = cp->dev;
-	dma_addr_t ring_dma;
 
 	cp_reset_hw(cp);
 
@@ -979,17 +1006,6 @@ static void cp_init_hw (struct cp_private *cp)
 	cpw32_f (MAC0 + 0, le32_to_cpu (*(__le32 *) (dev->dev_addr + 0)));
 	cpw32_f (MAC0 + 4, le32_to_cpu (*(__le32 *) (dev->dev_addr + 4)));
 
-	cpw32_f(HiTxRingAddr, 0);
-	cpw32_f(HiTxRingAddr + 4, 0);
-
-	ring_dma = cp->ring_dma;
-	cpw32_f(RxRingAddr, ring_dma & 0xffffffff);
-	cpw32_f(RxRingAddr + 4, (ring_dma >> 16) >> 16);
-
-	ring_dma += sizeof(struct cp_desc) * CP_RX_RING_SIZE;
-	cpw32_f(TxRingAddr, ring_dma & 0xffffffff);
-	cpw32_f(TxRingAddr + 4, (ring_dma >> 16) >> 16);
-
 	cp_start_hw(cp);
 	cpw8(TxThresh, 0x06); /* XXX convert magic num to a constant */
 


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
> This fixes (for me) a regression introduced by commit b01af457 ("8139cp:
> set ring address before enabling receiver"). That commit configured the
> descriptor ring addresses earlier in the initialisation sequence, in
> order to avoid the possibility of triggering stray DMA before the
> correct address had been set up.
> 
> Unfortunately, it seems that the hardware will scribble garbage into the
> TxRingAddr registers when we enable "plus mode" Tx in the CpCmd
> register. Observed on a Traverse Geos router board.
> 
> To deal with this, while not reintroducing the problem which led to the
> original commit, we augment cp_start_hw() to write to the CpCmd register
> *first*, then set the descriptor ring addresses, and then finally to
> enable Rx and Tx in the original 8139 Cmd register. The datasheet
> actually indicates that we should enable Tx/Rx in the Cmd register
> *before* configuring the descriptor addresses, but that would appear to
> re-introduce the problem that the offending commit b01af457 was trying
> to solve. And this variant appears to work fine on real hardware.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Cc: stable@kernel.org [3.5+]
> 
> ---
> How about this? I'm still somewhat confused about when it actually
> *does* start doing DMA, given what the datasheet says.

Straight to -stable ?

Afaik nobody complained from the original (pre b01af457) problem on
real hardware.

May be someone @realtek (hi Hayes) can give an explanation regarding
the CpCmd, RingAddr, Cmd init sequence and the start of DMA.

-- 
Ueimor

Re: 8139cp: set ring address before enabling receiver

[Jeff Garzik]

On 11/21/2012 03:18 PM, Ben Hutchings wrote:
> On Wed, 2012-11-21 at 19:51 +0000, David Woodhouse wrote:
>> On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
>>>
>>> What sticks out at me from the commit message?
>>>
>>> It was not tested on the famously quirky 8139 hardware at all.
>>>
>>> While I have not looked at the 8139C+ data sheet in a while, sometimes
>>> the hardware _did_ have a strange init order.
>>>
>>> As this works in a simulator but fails on real hardware, it seems like
>>> an obvious regression caused by an untested [on read hardware] patch.
>>
>> The data sheet (v1.6, from http://realtek.info/pdf/rtl8139cp.pdf ) says
>> in §6.33 (C+ Command Register):
>>   "Enable C+ mode functions in C+CR register first,
>>   => Enable transmit/receive in Command register (offset 37h),
>>   => Configure other related registers (ex. Descriptor start address,
>>      TCR, RCR, ...)."
>>
>> I understand the concern expressed in the offending commit message about
>> DMA happening to invalid addresses, and I'll look at the data sheet
>> harder to see when the DMA actually starts happening. But it definitely
>> seems that our current code isn't doing what the data sheet says.
>>
>> I wonder if I can find one of these lying around and stick it in a
>> machine with an IOMMU...
>
> You might be able to avoid disaster by doing:
>
> 1. Set MAC filter to drop everything
> 2. Enable RX DMA
> 3. Set RX DMA ring address
> 4. Set MAC filter according to current flags & multicast list
>
> I'm assuming, knowing nothing about this particular hardware, that the
> MAC filter register(s) will accept writes before RX DMA is enabled.

A larger point is that the commit was created to avoid imagined disaster 
on simulated hardware...

  ...and wound up creating behavior that is (a) contra to the data sheet 
and (b) breaks real hardware.

	Jeff

Re: 8139cp: set ring address before enabling receiver

[David Woodhouse]

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

On Wed, 2012-11-21 at 15:40 -0500, Jeff Garzik wrote:
> A larger point is that the commit was created to avoid imagined
> disaster on simulated hardware...

In their defence, I suspect that qemu/kvm is probably now the most
common type of RTL8139 on Linux deployments :)

And since KVM is now capable of supporting an IOMMU, which most *real*
boxes with RTL8139 won't have, it was probably a *real* problem rather
than just an imagined one.

>   ...and wound up creating behavior that is (a) contra to the data
> sheet and (b) breaks real hardware.

And again in their defence... the data sheet does appear to be
suggesting something completely stupid. The patch I just submitted
doesn't do what the data sheet says *either*, although I did at least
test it on real hardware. How many versions of the 8139C+ are there?
Should I be looking for more testing on different revisions?

I had a quick play with the Cfg9346 register. I note that when you set
it to 0x80 it *does* disable both network and bus mastering... and we
set it to the 'Write Enable' value 0xC0 while we're configuring
everything. I wondered if that might perhaps be the thing that made the
original behaviour, and the recommendation in the data sheet, sane.

But it doesn't disable operation when it's in the "Unlock" mode. I tried
setting the driver's value of Cfg9346_Lock to 0xC0 (i.e. leave it
write-enabled at all times), hoping that it would then fail to do any
DMA and prove that the original code was actually safe after all. But
the driver is working fine.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: 8139cp: set ring address before enabling receiver

[Ben Hutchings]

On Wed, 2012-11-21 at 15:40 -0500, Jeff Garzik wrote:
> On 11/21/2012 03:18 PM, Ben Hutchings wrote:
> > On Wed, 2012-11-21 at 19:51 +0000, David Woodhouse wrote:
> >> On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
> >>>
> >>> What sticks out at me from the commit message?
> >>>
> >>> It was not tested on the famously quirky 8139 hardware at all.
> >>>
> >>> While I have not looked at the 8139C+ data sheet in a while, sometimes
> >>> the hardware _did_ have a strange init order.
> >>>
> >>> As this works in a simulator but fails on real hardware, it seems like
> >>> an obvious regression caused by an untested [on read hardware] patch.
> >>
> >> The data sheet (v1.6, from http://realtek.info/pdf/rtl8139cp.pdf ) says
> >> in §6.33 (C+ Command Register):
> >>   "Enable C+ mode functions in C+CR register first,
> >>   => Enable transmit/receive in Command register (offset 37h),
> >>   => Configure other related registers (ex. Descriptor start address,
> >>      TCR, RCR, ...)."
> >>
> >> I understand the concern expressed in the offending commit message about
> >> DMA happening to invalid addresses, and I'll look at the data sheet
> >> harder to see when the DMA actually starts happening. But it definitely
> >> seems that our current code isn't doing what the data sheet says.
> >>
> >> I wonder if I can find one of these lying around and stick it in a
> >> machine with an IOMMU...
> >
> > You might be able to avoid disaster by doing:
> >
> > 1. Set MAC filter to drop everything
> > 2. Enable RX DMA
> > 3. Set RX DMA ring address
> > 4. Set MAC filter according to current flags & multicast list
> >
> > I'm assuming, knowing nothing about this particular hardware, that the
> > MAC filter register(s) will accept writes before RX DMA is enabled.
> 
> A larger point is that the commit was created to avoid imagined disaster 
> on simulated hardware...
>
>   ...and wound up creating behavior that is (a) contra to the data sheet 
> and (b) breaks real hardware.

I wasn't suggesting anyone should change this again without testing on
real hardware.  But the 'imagined disaster' seems to be an obvious and
real race condition, which the driver is just more likely to win when
racing real hardware than when racing virtual hardware.

(It could be that the hardware pre-fetches DMA descriptors, in which
case this is a 'how did that ever work?' bug.  Alternately, there could
be a hidden enable bit that doesn't get set until the RX DMA ring
address is written, in which case the driver may need a quirk for
emulations that lack that.  An IOMMU should be able to answer these
questions.)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Woodhouse]

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

On Wed, 2012-11-21 at 21:40 +0100, Francois Romieu wrote:
> Straight to -stable ?

That's the way it works. You put the Cc: stable on the *original* commit
that goes upstream. There's no sane way to retroactively add that tag
after it's already been merged and tested.

Yes, you can bug Greg manually to 'please add this upstream commit which
we forgot to mark as Cc: stable' but that isn't the way it's usually
done.

> Afaik nobody complained from the original (pre b01af457) problem on
> real hardware.
>
> May be someone @realtek (hi Hayes) can give an explanation regarding
> the CpCmd, RingAddr, Cmd init sequence and the start of DMA.

That would be really useful; thanks. To recap for Hayes' benefit: the
concern is that if we follow the instructions in §6.33 of the data
sheet:

Recommendation to C+ mode programming: Enable C+ mode functions in C+CR
register first, => Enable transmit/receive in Command register (offset
37h), => Configure other related registers (ex. Descriptor start
address, TCR, RCR, ...).

... then we appear to be starting up the DMA before we actually tell it
the descriptor ring addresses, which will cause stray DMA to random
unconfigured addresses!

Is there some detail of the hardware which prevents this from actually
happening? Or if not, is my proposed workaround (enabling Tx/Rx in the
C+ Command Register *first*, then setting the descriptor addresses, and
enabling Tx/Rx in the old-style Command register last) OK?

It was observed that when setting the descriptor addresses *first*, the
Transmit Descriptor Start Address Register was getting overwritten with
garbage when we enabled Tx in the C+ Command Register.

I note that we're also setting a bunch of other things in the Rx and Tx
config registers *after* operation all seems to have started up... is
that OK too?

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 21 Nov 2012 22:32:11 +0000

> On Wed, 2012-11-21 at 21:40 +0100, Francois Romieu wrote:
>> Straight to -stable ?
> 
> That's the way it works. You put the Cc: stable on the *original* commit
> that goes upstream. There's no sane way to retroactively add that tag
> after it's already been merged and tested.
> 
> Yes, you can bug Greg manually to 'please add this upstream commit which
> we forgot to mark as Cc: stable' but that isn't the way it's usually
> done.

On the contrary, for networking I submit everything manually and I
remove the CC: tags.

I have a queue on patchwork that I add such patches to, so that they
do not get lost.

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Woodhouse]

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

On Wed, 2012-11-21 at 17:40 -0500, David Miller wrote:
> On the contrary, for networking I submit everything manually and I
> remove the CC: tags.
> 
> I have a queue on patchwork that I add such patches to, so that they
> do not get lost.

Ah, right. Thanks for the correction. Is it even worth giving the hint
that this should be for the stable tree (from v3.5 onwards), or should I
leave you to work that all out for yourself? And if it *is* worth giving
that hint, is it better to do it in a comment after --- at the end of
the commit comment, rather than the "normal" 'Cc: stable' tag?

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 21 Nov 2012 22:52:54 +0000

> On Wed, 2012-11-21 at 17:40 -0500, David Miller wrote:
>> On the contrary, for networking I submit everything manually and I
>> remove the CC: tags.
>> 
>> I have a queue on patchwork that I add such patches to, so that they
>> do not get lost.
> 
> Ah, right. Thanks for the correction. Is it even worth giving the hint
> that this should be for the stable tree (from v3.5 onwards), or should I
> leave you to work that all out for yourself? And if it *is* worth giving
> that hint, is it better to do it in a comment after --- at the end of
> the commit comment, rather than the "normal" 'Cc: stable' tag?

The more information you give in the commit message the better, that
way I don't have to guess :-)

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Jeff Garzik]

On 11/21/2012 03:27 PM, David Woodhouse wrote:
> This fixes (for me) a regression introduced by commit b01af457 ("8139cp:
> set ring address before enabling receiver"). That commit configured the
> descriptor ring addresses earlier in the initialisation sequence, in
> order to avoid the possibility of triggering stray DMA before the
> correct address had been set up.
>
> Unfortunately, it seems that the hardware will scribble garbage into the
> TxRingAddr registers when we enable "plus mode" Tx in the CpCmd
> register. Observed on a Traverse Geos router board.
>
> To deal with this, while not reintroducing the problem which led to the
> original commit, we augment cp_start_hw() to write to the CpCmd register
> *first*, then set the descriptor ring addresses, and then finally to
> enable Rx and Tx in the original 8139 Cmd register. The datasheet
> actually indicates that we should enable Tx/Rx in the Cmd register
> *before* configuring the descriptor addresses, but that would appear to
> re-introduce the problem that the offending commit b01af457 was trying
> to solve. And this variant appears to work fine on real hardware.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Cc: stable@kernel.org [3.5+]
>
> ---
> How about this? I'm still somewhat confused about when it actually
> *does* start doing DMA, given what the datasheet says.

Well, we have three logical code states:

State A:  pre-b01af457, known working
State B:  b01af457, known broken
State C:  dwmw2 proposed fix, tested on 1 hardware, new technique, query 
open with Realtek

State A seems safer for late -rc, which is where we are.  Fix the 
regression by reverting to well-tested, widely deployed state.

Then apply your patch here as an immediate candidate for net-next.

	Jeff

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Wed, 21 Nov 2012 22:47:39 -0500

> State A:  pre-b01af457, known working
> State B:  b01af457, known broken

State A is also known buggy on the largest consumer of this driver,
the emulated hardware.

Please evaluate this realistically.

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Jeff Garzik]

On 11/21/2012 11:39 PM, David Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Wed, 21 Nov 2012 22:47:39 -0500
>
>> State A:  pre-b01af457, known working
>> State B:  b01af457, known broken
>
> State A is also known buggy on the largest consumer of this driver,
> the emulated hardware.
>
> Please evaluate this realistically.

If the simulator fails to match the hardware, that is a simulator bug.

It is disappointing to work around someone else's software bug in the 
kernel.

	Jeff

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Jason Wang]

On 11/22/2012 12:53 PM, Jeff Garzik wrote:
> On 11/21/2012 11:39 PM, David Miller wrote:
>> From: Jeff Garzik <jgarzik@pobox.com>
>> Date: Wed, 21 Nov 2012 22:47:39 -0500
>>
>>> State A:  pre-b01af457, known working
>>> State B:  b01af457, known broken
>>
>> State A is also known buggy on the largest consumer of this driver,
>> the emulated hardware.
>>
>> Please evaluate this realistically.
>
> If the simulator fails to match the hardware, that is a simulator bug.
CC realtek linux driver mainter (nic_swsd@realtek.com)

The problem the behaviour of the hardware is subtle, and we could not 
just infer it from the datasheet. Another issue is in some situation, 
the datasheet is conflict with what real hardware does, one example is  
the cfg9364 issue mentioned by David ( I also meet it during qemu 
development).

If the hardware always fit garbage into the TxRingAddr register when 
"plus mode" were enabled, it may send something from memory to the wire 
unexpectedly which looks really strange. If it does not change the 
RxRingAddr when enabling C+, another method is to keep setting the rx 
address before C+ enabling but does the tx after.
>
> It is disappointing to work around someone else's software bug in the 
> kernel.

Qemu also has some workarounds for the legacy kernels and even in this 
case: it initialize RxRingAddr to 0 and check it during receiving, it 
the addr is still zero ( which may mean the rx ring addr were set after 
the c+ is enabled), it won't do the receiving to prevent the corruption. 
So reverting is safe for rx now.
>
>     Jeff
>
>
>

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Francois Romieu]

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

Jeff Garzik <jgarzik@pobox.com> :
> On 11/21/2012 11:39 PM, David Miller wrote:
> >From: Jeff Garzik <jgarzik@pobox.com>
> >Date: Wed, 21 Nov 2012 22:47:39 -0500
> >
> >>State A:  pre-b01af457, known working
> >>State B:  b01af457, known broken
> >
> >State A is also known buggy on the largest consumer of this driver,
> >the emulated hardware.
>
> >Please evaluate this realistically.
> 
> If the simulator fails to match the hardware, that is a simulator bug.

Yes.

> It is disappointing to work around someone else's software bug in
> the kernel.

Yes. :o/

I like David Woodhouse's C (attached patch) since 1) Realtek does
not seem to care about oldies 2) the emulation will not be fixed in a
decent timeframe 3) real 8139cp users care.

It would be nice if gilboad could give it a try (users Cced).

Btw David W., could consider adding artificial delays between the writes
and see if / when things start to fail (CpCmd write in cp_start_hw is an
unflushed posted write for instance).

-- 
Ueimor

[-- Attachment #2: 8139cp.patch.gz --]
[-- Type: application/x-gzip, Size: 934 bytes --]

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Woodhouse]

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

On Thu, 2012-11-22 at 22:39 +0100, Francois Romieu wrote:
> Btw David W., could consider adding artificial delays between the
> writes and see if / when things start to fail (CpCmd write in
> cp_start_hw is an unflushed posted write for instance).

That's how I tracked it down to the CpCmd write. I littered the whole of
the init path with
 printk("at line %d TxRingAddr %08x%08 (sb %08x)\n", __LINE__,
         cpr32(TxRingAddr+4), cpr32(TxRingAddr), cp->ring_dma + whatever);
... until the output looked something like this:

root@geos:~# insmod ./8139cp.ko 
[ 1331.492486] 8139cp: 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
[ 1331.500388] 8139cp 0000:00:0a.0: eth0: RTL-8139Cx at 0xd10a6000, 00:0a:fa:22:
00:96, IRQ 10
[ 1331.509608] 8139cp 0000:00:0b.0: eth1: RTL-8139Cx at 0xd10a8100, 00:0a:fa:22:
00:97, IRQ 11
root@geos:~# [ 1331.644393] at line 995 TxRingAddr   000000000f3c6400 (sb f3c6400)
[ 1331.650579] at line 960 TxRingAddr   000000000f3c6400 (sb f3c6400)
[ 1331.656820] at line 962 TxRingAddr   000000000f3e4400 (sb f3c6400)
[ 1331.663020] at line 964 TxRingAddr   000000000f3e4400 (sb f3c6400)
[ 1331.669205] at line 998 TxRingAddr   000000000f3e4400 (sb f3c6400)
[ 1331.675412] at line 1001 TxRingAddr   000000000f3e4400 (sb f3c6400)
[ 1331.681706] at line 1003 TxRingAddr   000000000f3e4400 (sb f3c6400)
[ 1331.687977] at line 1005 TxRingAddr   000000000f3e4400 (sb f3c6400)

Each of those printks will have effectively flushed any prior posted
writes... not that this AMD Geode platform actually *does* post writes,
to my knowledge. And at 115200 baud, each one was about a 6ms delay.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Jason Wang]

On 11/22/2012 12:53 PM, Jeff Garzik wrote:
> On 11/21/2012 11:39 PM, David Miller wrote:
>> From: Jeff Garzik <jgarzik@pobox.com>
>> Date: Wed, 21 Nov 2012 22:47:39 -0500
>>
>>> State A:  pre-b01af457, known working
>>> State B:  b01af457, known broken
>>
>> State A is also known buggy on the largest consumer of this driver,
>> the emulated hardware.
>>
>> Please evaluate this realistically.
>
> If the simulator fails to match the hardware, that is a simulator bug.
Resend the mail because it's fail to post to the list yesterday.

CC realtek linux driver mainter (nic_swsd@realtek.com)

The problem the behaviour of the hardware is subtle, and we could not 
just infer it from the datasheet. Another issue is in some situation, 
the datasheet is conflict with what real hardware does, one example is 
the cfg9364 issue mentioned by David ( I also meet it during qemu 
development).

If the hardware always fit garbage into the TxRingAddr register when 
"plus mode" were enabled, it may send something from memory to the wire 
unexpectedly which looks really strange. If it does not change the 
RxRingAddr when enabling C+, another method is to keep setting the rx 
address before C+ enabling but does the tx after.
>
> It is disappointing to work around someone else's software bug in the 
> kernel.
>

Qemu also has some workarounds for the buggy kernels and even in this 
case: it initialize RxRingAddr to 0 and check it during receiving, it  
check whether the addr is still zero ( which may mean the rx ring addr 
were set after the c+ is enabled), it won't do the receiving to prevent 
the corruption. So reverting is safe for rx now.
>     Jeff
>
>
>

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[Gilboa Davara]

On Thu, Nov 22, 2012 at 11:39 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> It would be nice if gilboad could give it a try (users Cced).
>

Applied it against 3.6.6.
Seems to be working just fine.

> --
> Ueimor

- Gilboa

Re: [PATCH] 8139cp: set ring address after enabling C+ mode

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 21 Nov 2012 20:27:19 +0000

> This fixes (for me) a regression introduced by commit b01af457 ("8139cp:
> set ring address before enabling receiver"). That commit configured the
> descriptor ring addresses earlier in the initialisation sequence, in
> order to avoid the possibility of triggering stray DMA before the
> correct address had been set up.
> 
> Unfortunately, it seems that the hardware will scribble garbage into the
> TxRingAddr registers when we enable "plus mode" Tx in the CpCmd
> register. Observed on a Traverse Geos router board.
> 
> To deal with this, while not reintroducing the problem which led to the
> original commit, we augment cp_start_hw() to write to the CpCmd register
> *first*, then set the descriptor ring addresses, and then finally to
> enable Rx and Tx in the original 8139 Cmd register. The datasheet
> actually indicates that we should enable Tx/Rx in the Cmd register
> *before* configuring the descriptor addresses, but that would appear to
> re-introduce the problem that the offending commit b01af457 was trying
> to solve. And this variant appears to work fine on real hardware.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Applied to net-next.