From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-21 16:57:29
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 [off-list ref]
AuthorDate: Thu May 31 18:19:39 2012 +0000
Committer: David S. Miller [off-list ref]
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 [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
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
From: Jeff Garzik <hidden> Date: 2012-11-21 18:12:09
On 11/21/2012 11:57 AM, David Woodhouse wrote:
On Sat, 2012-06-02 at 23:50 +0000, Linux Kernel Mailing List wrote:
quoted
Gitweb: http://git.kernel.org/linus/;a=commit;h=b01af4579ec41f48e9b9c774e70bd6474ad210db
Commit: b01af4579ec41f48e9b9c774e70bd6474ad210db
Parent: 20e2a86485967c385d7c7befc1646e4d1d39362e
Author: Jason Wang [off-list ref]
AuthorDate: Thu May 31 18:19:39 2012 +0000
Committer: David S. Miller [off-list ref]
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
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-21 19:51:27
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
From: Ben Hutchings <hidden> Date: 2012-11-21 20:18:59
On Wed, 2012-11-21 at 19:51 +0000, David Woodhouse wrote:
On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
quoted
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.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-21 20:27:27
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 <redacted>
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.
From: Jeff Garzik <hidden> Date: 2012-11-21 20:40:53
On 11/21/2012 03:18 PM, Ben Hutchings wrote:
On Wed, 2012-11-21 at 19:51 +0000, David Woodhouse wrote:
quoted
On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
quoted
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
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-21 21:00:51
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
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 <redacted>
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
From: Ben Hutchings <hidden> Date: 2012-11-21 21:11:02
On Wed, 2012-11-21 at 15:40 -0500, Jeff Garzik wrote:
On 11/21/2012 03:18 PM, Ben Hutchings wrote:
quoted
On Wed, 2012-11-21 at 19:51 +0000, David Woodhouse wrote:
quoted
On Wed, 2012-11-21 at 13:12 -0500, Jeff Garzik wrote:
quoted
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.
From: Jason Wang <hidden> Date: 2012-11-22 18:38:11
On 11/22/2012 12:53 PM, Jeff Garzik wrote:
On 11/21/2012 11:39 PM, David Miller wrote:
quoted
From: Jeff Garzik <redacted>
Date: Wed, 21 Nov 2012 22:47:39 -0500
quoted
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.
From: David Miller <davem@davemloft.net> Date: 2012-11-22 19:10:31
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:
quoted
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 :-)
From: Jeff Garzik <hidden> Date: 2012-11-22 19:14:08
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 <redacted>
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
From: David Miller <davem@davemloft.net> Date: 2012-11-22 19:15:31
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:
quoted
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.
From: Jeff Garzik <hidden> Date: 2012-11-22 20:30:25
On 11/21/2012 11:39 PM, David Miller wrote:
From: Jeff Garzik <redacted>
Date: Wed, 21 Nov 2012 22:47:39 -0500
quoted
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
From: Jeff Garzik <redacted>
Date: Wed, 21 Nov 2012 22:47:39 -0500
quoted
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.
quoted
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
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-22 23:07:58
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
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-22 23:13:07
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
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-23 00:07:24
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
From: Jason Wang <hidden> Date: 2012-11-23 03:51:01
On 11/22/2012 12:53 PM, Jeff Garzik wrote:
On 11/21/2012 11:39 PM, David Miller wrote:
quoted
From: Jeff Garzik <redacted>
Date: Wed, 21 Nov 2012 22:47:39 -0500
quoted
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.
From: David Miller <davem@davemloft.net> Date: 2012-11-25 20:55:08
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 <redacted>