From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-04 05:24:32
Hi Stephen !
To try to validate some PCIe code we have around, I got myself a
SysKonnect SK-9Exx card (one x1 lane one port and one x4 lanes 2 ports
actually, though I've only tested the first one at this point, based on
a 88E8061 Marvell chip).
I've used the sky2 driver as of a random last week's git snapshot. I'll
double check tomorrow if there have been any update.
So far, I had no success on a couple of different machines, and I was
wondering if you had an idea of what's wrong. Here's what I've seen far:
- Card is probed fine
sky2 v1.5 addr 0xa0000000 irq 63 Yukon-XL (0xb3) rev 1
sky2 eth2: addr 00:00:5a:72:4c:2a
- As soon as I ifconfig it up, I get a steady stream of
sky2 eth2: receive descriptor error (hardware problem)
sky2 eth2: Link is down.
mesages in my kernel log, which makes me think that it's unhappy about
the format of the descriptors or something like that (endian issue ? see
below my notes about that). I'm also wondering why it seems to be
flooding me which makes me think the error condition is never cleared on
the chip.
Some of the things I've noticed and tweaked in the driver, without luck
so far:
- There is that bit when initializing the chip:
#ifdef __BIG_ENDIAN
/* byte swap descriptors in hardware */
{
u32 reg;
.../...
However, if you look at some of the routines filling up descriptors,
they still do swap the values put into some of the fields like lenght,
address. (Except an incosistency with rx_set_checksum which doesn't swap
whatever it writes to le->addr, though I've tried fixing that without
much success).
Thus I don't quite understand what the above is supposed to do. I've
tried disabling it but it didn't fix the problem. However, that leads me
to wondering wether there might be a deeper problem with descriptor
swapping. What kind of access size and what kind of swapping the chip is
supposedly doing ? 32 bits accesses ? 64 bits accesses ?
Because if you look at the definition for:
struct sky2_rx_le {
__le32 addr;
__le16 length;
u8 ctrl;
u8 opcode;
} __attribute((packed));
(As an example)
If the chips does LE accesses, then marking "lenght" as being an LE
value isn't enough. It also needs to be swapped with ctrl and opcode.
The layout then becomes:
struct sky2_rx_le {
__le32 addr;
u8 opcode;
u8 ctrl;
__le16 length;
} __attribute((packed));
Unless the chip does 2x16 bits accesses here but I very much doubt it.
Now, it's possible that this bit about "byteswapping descriptor" is
about doing that address munging, but would it do it without also
byteswapping the content of the individual fields ? That looks
unlikely... Thus I wonder wether we should either keep this HW byteswap
option _and_ remove all the byteswapping of fields when accessing addr
and lenght, or leave the byteswapping in, remove the HW byteswap option
_and_ fixup the structure definitions of the descriptor in the big
endian case ....
I haven't had time to try all those options yet, and I don't have access
to any documentation for that chip, but I wondered if, in the meantime,
you had any other idea about what may be going wrong or see any flaw in
my reasoning above.
Cheers,
Ben.
--
VGER BF report: U 0.505334
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-04 07:21:20
Because if you look at the definition for:
struct sky2_rx_le {
__le32 addr;
__le16 length;
u8 ctrl;
u8 opcode;
} __attribute((packed));
(As an example)
If the chips does LE accesses, then marking "lenght" as being an LE
value isn't enough. It also needs to be swapped with ctrl and opcode.
The layout then becomes:
struct sky2_rx_le {
__le32 addr;
u8 opcode;
u8 ctrl;
__le16 length;
} __attribute((packed));
And of course, I'm stupid... the structure layout in memory is well
defined and the chip is accessing it just fine... The problem are
actually related to
- That special "reverse" descriptor bit one can set in the chip....
That will indeed cause my scenario above to happen by causing the chip
to do a 32 bits byteswap it seems. But if we set this bit, we must also
remove all of the cpu_to_le/le_to_cpu conversions used to access
descriptors.
- I've decided not to set that bit for now, thus disabled the code
doing so (and thus requiring be/le conversions). We have cases where
they are missing, and thus it's not working.
I'm trying to get it right now, will send a patch if I can get something
working. At one point, I suppose I'll have the choice of either letting
the HW do the 32 bits flip, which involves playing #ifdef __BIG_ENDIAN
with the structures in sky2.h or just ignore that bit and do cpu_to_le*
when needed. I'm tempted to do the later...
Ben
--
VGER BF report: U 0.526041
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-04 07:42:39
This fixes sky2 driver on big endian machines. I choose not to use the
hardware byteswap facility as it would have required to have a different
definition of the various ring data structures and it looks ugly :) On
powerpc, there is pretty much no overhead at doing byteswap.
The patch has a couple of places where I reversed 2 assignments, they
are harmless, it was before I figured out that the chip will
(apparently) not access a descriptor before it's been told to do so via
MMIO, and thus the order of the writes to the descriptors is irrelevant
(I was also adding wmb's though I removed them).
There is a couple of places where we were doing a BE and not LE
conversion of a descriptor field (typically in the VLAN code). I'm not
sure what's up there but BE "felt" wrong. I have turned them into LE
conversions but then I haven't tested VLAN, and I might just
misudnerstand what's happening there so I'll let you decide what to do
about those.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Index: linux-work/drivers/net/sky2.c
===================================================================
@@ -1001,7 +1002,6 @@ static int sky2_rx_start(struct sky2_porsky2_rx_add(sky2,re->mapaddr);}-/**Thereceiverhangsifitreceivesframeslargerthanthe*packetbuffer.Asaworkaround,truncateoversizeframes,but
@@ -1250,8 +1250,8 @@ static int sky2_xmit_frame(struct sk_bufle=get_tx_le(sky2);le->tx.tso.size=cpu_to_le16(mss);le->tx.tso.rsvd=0;-le->opcode=OP_LRGLEN|HW_OWNER;le->ctrl=0;+le->opcode=OP_LRGLEN|HW_OWNER;sky2->tx_last_mss=mss;}
@@ -1262,11 +1262,11 @@ static int sky2_xmit_frame(struct sk_bufif(!le){le=get_tx_le(sky2);le->tx.addr=0;-le->opcode=OP_VLAN|HW_OWNER;le->ctrl=0;+le->opcode=OP_VLAN|HW_OWNER;}elsele->opcode|=OP_VLAN;-le->length=cpu_to_be16(vlan_tx_tag_get(skb));+le->length=cpu_to_le16(vlan_tx_tag_get(skb));ctrl|=INS_VLAN;}#endif
@@ -1955,8 +1955,8 @@ static int sky2_status_intr(struct sky2_dev=hw->dev[le->link];sky2=netdev_priv(dev);-length=le->length;-status=le->status;+length=le16_to_cpu(le->length);+status=le32_to_cpu(le->status);switch(le->opcode&~HW_OWNER){caseOP_RXSTAT:
@@ -1972,7 +1972,7 @@ static int sky2_status_intr(struct sky2_if(sky2->vlgrp&&(status&GMR_FS_VLAN)){vlan_hwaccel_receive_skb(skb,sky2->vlgrp,-be16_to_cpu(sky2->rx_tag));+le16_to_cpu(sky2->rx_tag));}else#endifnetif_receive_skb(skb);
@@ -2001,7 +2001,7 @@ static int sky2_status_intr(struct sky2_caseOP_RXCHKS:skb=sky2->rx_ring[sky2->rx_next].skb;skb->ip_summed=CHECKSUM_HW;-skb->csum=le16_to_cpu(status);+skb->csum=status&0xffffu;break;caseOP_TXINDEXLE:
@@ -3240,6 +3240,7 @@ static int __devinit sky2_probe(struct pstructnet_device*dev,*dev1=NULL;structsky2_hw*hw;interr,pm_cap,using_dac=0;+u32reg;err=pci_enable_device(pdev);if(err){
@@ -3303,16 +3304,12 @@ static int __devinit sky2_probe(struct p}hw->pm_cap=pm_cap;-#ifdef __BIG_ENDIAN-/* byte swap descriptors in hardware */-{-u32reg;--reg=sky2_pci_read32(hw,PCI_DEV_REG2);-reg|=PCI_REV_DESC;-sky2_pci_write32(hw,PCI_DEV_REG2,reg);-}-#endif+/* Don't let it byte swap descriptors in hardware. Clear the bit+*incaseapreviousdriverhavesetit+*/+reg=sky2_pci_read32(hw,PCI_DEV_REG2);+reg&=~PCI_REV_DESC;+sky2_pci_write32(hw,PCI_DEV_REG2,reg);/* ring for status responses */hw->st_le=pci_alloc_consistent(hw->pdev,STATUS_LE_BYTES,
The patch has a couple of places where I reversed 2 assignments, they
are harmless, it was before I figured out that the chip will
(apparently) not access a descriptor before it's been told to do so
via
MMIO, and thus the order of the writes to the descriptors is
irrelevant
(I was also adding wmb's though I removed them).
wmb() between writing the descriptors to main memory and writing
an MMIO to the device to tell it to go fetch them? That's still
required. The 970 you tested on won't care though ;-)
There is a couple of places where we were doing a BE and not LE
conversion of a descriptor field (typically in the VLAN code). I'm not
sure what's up there but BE "felt" wrong. I have turned them into LE
conversions but then I haven't tested VLAN, and I might just
misudnerstand what's happening there so I'll let you decide what to do
about those.
VLAN tags are big-endian on the wire, it might well be that these
chips never swizzle that?
Is this le->length thing a separate bugfix? Or just overly
protective coding (you write the field later again).
+ /* Don't let it byte swap descriptors in hardware. Clear the bit
+ * in case a previous driver have set it
+ */
+ reg = sky2_pci_read32(hw, PCI_DEV_REG2);
+ reg &= ~PCI_REV_DESC;
+ sky2_pci_write32(hw, PCI_DEV_REG2, reg);
Probably not needed, the chip gets a full reset at startup (or so
I hope anyway, heh).
Segher
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-04 21:33:36
On Mon, 2006-09-04 at 23:05 +0200, Segher Boessenkool wrote:
quoted
The patch has a couple of places where I reversed 2 assignments, they
are harmless, it was before I figured out that the chip will
(apparently) not access a descriptor before it's been told to do so
via
MMIO, and thus the order of the writes to the descriptors is
irrelevant
(I was also adding wmb's though I removed them).
wmb() between writing the descriptors to main memory and writing
an MMIO to the device to tell it to go fetch them? That's still
required. The 970 you tested on won't care though ;-)
Won't it ? Anyway, you misunderstood, there is a wmb() already in the
right place
in sky2_put_idx() if I understand things correctly (but then I don't
know the chip hardware well) and the ones I originally added weren't
necessary.
quoted
There is a couple of places where we were doing a BE and not LE
conversion of a descriptor field (typically in the VLAN code). I'm not
sure what's up there but BE "felt" wrong. I have turned them into LE
conversions but then I haven't tested VLAN, and I might just
misudnerstand what's happening there so I'll let you decide what to do
about those.
VLAN tags are big-endian on the wire, it might well be that these
chips never swizzle that?
Is this le->length thing a separate bugfix? Or just overly
protective coding (you write the field later again).
Overly protective coding when I was still looking for what the problem
was (wanted to have nice descriptors in memory without crap in them :)
I'll try without
quoted
+ /* Don't let it byte swap descriptors in hardware. Clear the bit
+ * in case a previous driver have set it
+ */
+ reg = sky2_pci_read32(hw, PCI_DEV_REG2);
+ reg &= ~PCI_REV_DESC;
+ sky2_pci_write32(hw, PCI_DEV_REG2, reg);
Probably not needed, the chip gets a full reset at startup (or so
I hope anyway, heh).
It is needed. I've verified that this bit resists to anything we do in
the driver. So if you load the old driver that sets it, unload it, then
load the fixed driver, you are toast... I'm also taking no risk in case
one has a firmware that sets this bit.
Ben.
From: Stephen Hemminger <hidden> Date: 2006-09-05 03:41:55
On Mon, 04 Sep 2006 17:42:27 +1000
Benjamin Herrenschmidt [off-list ref] wrote:
This fixes sky2 driver on big endian machines. I choose not to use the
hardware byteswap facility as it would have required to have a different
definition of the various ring data structures and it looks ugly :) On
powerpc, there is pretty much no overhead at doing byteswap.
The patch has a couple of places where I reversed 2 assignments, they
are harmless, it was before I figured out that the chip will
(apparently) not access a descriptor before it's been told to do so via
MMIO, and thus the order of the writes to the descriptors is irrelevant
(I was also adding wmb's though I removed them).
I'll put a minimized version of this in the next patch set.
There is no need to re order assignments.
There is a couple of places where we were doing a BE and not LE
conversion of a descriptor field (typically in the VLAN code). I'm not
sure what's up there but BE "felt" wrong. I have turned them into LE
conversions but then I haven't tested VLAN, and I might just
misudnerstand what's happening there so I'll let you decide what to do
about those.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
It may not need any swapping, it is hard to tell what the hardware
will do without experimentation.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-05 03:48:02
It may not need any swapping, it is hard to tell what the hardware
will do without experimentation.
Yes... did you have a chance to test the vlan stuff on LE machines
(x86) ? did it work with the BE swapping you were doing ? I've
purposedly removed in my patches the hardware side swapping of the
descriptors, as I explained, thus making the hardware react the same on
ppc and x86. Thus we need the exact same swapping macros on both
platforms).
I know pretty much nothing about vlan so I'm not too much about trying
to check that right now :)
Also, there is still the hw checksum issue .... I need to verify what's
up there, it might be a swapping problem as well... or not. Can you send
me your latest patch set so I can work from there ?
Cheers,
Ben
From: Stephen Hemminger <hidden> Date: 2006-09-05 16:31:26
On Tue, 05 Sep 2006 13:47:52 +1000
Benjamin Herrenschmidt [off-list ref] wrote:
quoted
It may not need any swapping, it is hard to tell what the hardware
will do without experimentation.
Yes... did you have a chance to test the vlan stuff on LE machines
(x86) ? did it work with the BE swapping you were doing ? I've
purposedly removed in my patches the hardware side swapping of the
descriptors, as I explained, thus making the hardware react the same on
ppc and x86. Thus we need the exact same swapping macros on both
platforms).
Last time I checked it worked. Private cable simulating VLAN
from other Linux card.
I know pretty much nothing about vlan so I'm not too much about trying
to check that right now :)
Also, there is still the hw checksum issue .... I need to verify what's
up there, it might be a swapping problem as well... or not. Can you send
me your latest patch set so I can work from there ?
Cheers,
Ben
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-05 21:11:16
On Mon, 2006-09-04 at 21:15 -0700, Stephen Hemminger wrote:
On Tue, 05 Sep 2006 13:47:52 +1000
Benjamin Herrenschmidt [off-list ref] wrote:
quoted
quoted
It may not need any swapping, it is hard to tell what the hardware
will do without experimentation.
Yes... did you have a chance to test the vlan stuff on LE machines
(x86) ? did it work with the BE swapping you were doing ? I've
purposedly removed in my patches the hardware side swapping of the
descriptors, as I explained, thus making the hardware react the same on
ppc and x86. Thus we need the exact same swapping macros on both
platforms).
Last time I checked it worked. Private cable simulating VLAN
from other Linux card.
Ok, so we should probably switch back the vlan bits to BE swapping
macros...
However, we then have an inconsistency with that bit:
#ifdef SKY2_VLAN_TAG_USED
case OP_RXVLAN:
sky2->rx_tag = length;
break;
case OP_RXCHKSVLAN:
sky2->rx_tag = length;
/* fall through */
#endif
in sky2_status_intr()
Where we read the lenght field directly without swapping (on the non
patched driver, on the patched driver, lenght will have gone through an
LE swap). That is, if you take the standpoint of a LE machine, you will
read that value as a little endian value while elsewhere, we manipulate
sky2->rx_tag as a BE value... (this is even without my patch)
Ben.
@@ -1227,7 +1227,7 @@/* Send high bits if changed or crosses boundary */if(addr64!=sky2->tx_addr64||high32(mapping+len)!=sky2->tx_addr64){le=get_tx_le(sky2);-le->tx.addr=cpu_to_le32(addr64);+le->addr=cpu_to_le32(addr64);le->ctrl=0;le->opcode=OP_ADDR64|HW_OWNER;sky2->tx_addr64=high32(mapping+len);
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-09-05 22:00:20
On Tue, 2006-09-05 at 14:36 -0700, Stephen Hemminger wrote:
This is the reduced version of your patch, plus I got rid of the union
in tx_le, it is a nuisance.
Thanks. I'll give it a go later today. The remaining nit is the
inconsitent swapping of the vlan tag which is manipulated at BE at times
and LE at others (later hapens in status_intr).
Ben.
@@ -1227,7 +1227,7 @@/* Send high bits if changed or crosses boundary */if(addr64!=sky2->tx_addr64||high32(mapping+len)!=sky2->tx_addr64){le=get_tx_le(sky2);-le->tx.addr=cpu_to_le32(addr64);+le->addr=cpu_to_le32(addr64);le->ctrl=0;le->opcode=OP_ADDR64|HW_OWNER;sky2->tx_addr64=high32(mapping+len);