RE: [Bug 9106] Sun Fire v100 dmfe driver bug

RE: [Bug 9106] Sun Fire v100 dmfe driver bug

[Richard Mortimer]

I don't think that the seprom is set on SPARC systems so there
is not MAC address in there to read.
You need to read it from the appropriate OBP properties.

Search for local-mac-address and CONFIG_SPARC in tulip_core.c

Regards

Richard 

> -----Original Message-----
> From: ultralinux-owner@vger.kernel.org 
> [mailto:ultralinux-owner@vger.kernel.org] On Behalf Of Grant Grundler
> Sent: 04 November 2007 19:03
> To: ultralinux@vger.kernel.org
> Cc: grundler@parisc-linux.org; kovedi@gmail.com
> Subject: fwd: [Bug 9106] Sun Fire v100 dmfe driver bug
> 
> http://bugzilla.kernel.org/show_bug.cgi?id‘06
> 
> I have two possible reasons why dmfe driver is reading zeros 
> from the seprom for the MAC address.
> 1) IO port space routing is fu-bar - ie we are only able to 
> talk to the chip's config space.
> 2) outl() is implemented as a postable MMIO write (semantics 
> demand outl be non-postable).
> 
> Pursueing (2) for a bit, I looked at 
> linux-2.6.22.9/include/asm-sparc64/io.h:
> ...
>  74 static __inline__ void _outl(u32 l, unsigned long addr)
>  75 {
>  76         __asm__ __volatile__("stwa\t%r0, [%1] %2\t/* pci_outl */"
>  77                              : /* no outputs */
>  78                              : "Jr" (l), "r" (addr), "i" 
> (ASI_PHYS_BYPASS_EC    _E_L));
>  79 }
> ...
>  86 #define outl(__l, __addr)       (_outl((u32)(__l), 
> (unsigned long)(__addr)))
> ...
> 
> 
> Is "stwa" a posted write?
> I'm also open to ideas of what else might be wrong in dmfe.c driver.
> 
> 
> The initial symptom seems related to 
> linux-2.6.22.9/drivers/net/tulip/dmfe.c:
> ...
> 1576 static u16 read_srom_word(long ioaddr, int offset)
> 1577 {
> 1578         int i;
> 1579         u16 srom_data = 0;
> 1580         long cr9_ioaddr = ioaddr + DCR9;
> 1581 
> 1582         outl(CR9_SROM_READ, cr9_ioaddr);
> 1583         outl(CR9_SROM_READ | CR9_SRCS, cr9_ioaddr);
> 1584 
> 1585         /* Send the Read Command 110b */
> 1586         SROM_CLK_WRITE(SROM_DATA_1, cr9_ioaddr);
> 1587         SROM_CLK_WRITE(SROM_DATA_1, cr9_ioaddr);
> 1588         SROM_CLK_WRITE(SROM_DATA_0, cr9_ioaddr);
> ...
> 
> where SROM_CLK_WRITE is defined as:
> ...
>  175 #define SROM_CLK_WRITE(data, ioaddr) \
>  176         outl(data|CR9_SROM_READ|CR9_SRCS,ioaddr); \
>  177         udelay(5); \
>  178         outl(data|CR9_SROM_READ|CR9_SRCS|CR9_SRCLK,ioaddr); \
>  179         udelay(5); \
>  180         outl(data|CR9_SROM_READ|CR9_SRCS,ioaddr); \
>  181         udelay(5);
> 
> obviously, if outl() is posted, the udelay() won't be respected
> and there are already two writes in flight when the CPU gets 
> to line 1586.
> 
> 
> thanks,
> grant
> 
> ----- Forwarded message from bugme-daemon@bugzilla.kernel.org -----
> 
> X-Spam-Checker-Version: SpamAssassin 3.1.4 (2006-07-26) on 
> colo.lackof.org
> X-Spam-Level: 
> X-Spam-Status: No, score=0.9 required=5.5 
> testsºYES_40,FORGED_RCVD_HELO,
> 	NO_REAL_NAME autolearn=no version=3.1.4
> Subject: [Bug 9106] Sun Fire v100 dmfe driver bug
> X-Bugzilla-Product: Drivers
> X-Bugzilla-Severity: high
> X-Bugzilla-Keywords: 
> X-Bugzilla-Reason: CC AssignedTo
> X-Bugzilla-Component: Network
> To: grundler@parisc-linux.org
> From: bugme-daemon@bugzilla.kernel.org
> X-MIMEDefang-Filter: lf$Revision: 1.188 $
> X-Scanned-By: MIMEDefang 2.53 on 207.189.120.14
> X-Virus-Scanned: by amavisd-new-20030616-p10 (Debian) at lackof.org
> 
> http://bugzilla.kernel.org/show_bug.cgi?id‘06
> 
> 
> 
> 
> 
> ------- Comment #4 from kovedi@gmail.com  2007-10-08 00:29 -------
> I try to use the udelay(5) after line 1583 and 1596 but the error
> coming again after reboot.
> 
> the devices not have mac adresses
> 
> [   69.450854] eth0: Davicom DM9102 at pci0000:00:0c.0,
> 00:00:00:00:00:00, irq 9.
> [   69.477577] eth1: Davicom DM9102 at pci0000:00:05.0,
> 00:00:00:00:00:00, irq 10.
> 
> and always resetting the network devices.
> 
> [   84.923730] eth0: Tx timeout - resetting
> [   86.923705] eth0: Tx timeout - resetting
> [   88.923650] eth0: Tx timeout - resetting
> [   90.923636] eth0: Tx timeout - resetting
> [   92.923587] eth0: Tx timeout - resetting
> 
> 
> -- 
> Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug, or are watching someone who is.
> You are the assignee for the bug, or are watching the assignee.
> 
> ----- End forwarded message -----
> -
> To unsubscribe from this list: send the line "unsubscribe 
> ultralinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[David Miller]

From: Grant Grundler <grundler@parisc-linux.org>
Date: Sun, 4 Nov 2007 12:02:30 -0700

> http://bugzilla.kernel.org/show_bug.cgi?id‘06
> 
> I have two possible reasons why dmfe driver is reading zeros from the seprom for the MAC address.
> 1) IO port space routing is fu-bar - ie we are only able to talk to the chip's config space.
> 2) outl() is implemented as a postable MMIO write (semantics demand outl be non-postable).

The SROM reads aren't failing, think out of the box, the more likely
problem is:

3) Sun doesn't initialize the SROM for the onboard network devices.
   You have to obtain the MAC address and other settings by fetching
   them from the openfirmware device properties.

   So use something like:

#ifdef CONFIG_OF
	struct device_node *dp = pci_device_to_OF_node(pdev);
	const char *addr;

	addr = of_get_property(dp, "local-mac-address", NULL);
	if (addr) {
		memcpy(dev->dev_addr, addr, ETH_ALEN);
		return 1;
	}
	return 0;
#endif

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[Grant Grundler]

On Sun, Nov 04, 2007 at 01:49:58PM -0800, David Miller wrote:
> From: Grant Grundler <grundler@parisc-linux.org>
> Date: Sun, 4 Nov 2007 12:02:30 -0700
> 
> > http://bugzilla.kernel.org/show_bug.cgi?id‘06
> > 
> > I have two possible reasons why dmfe driver is reading zeros from the seprom for the MAC address.
> > 1) IO port space routing is fu-bar - ie we are only able to talk to the chip's config space.
> > 2) outl() is implemented as a postable MMIO write (semantics demand outl be non-postable).
> 
> The SROM reads aren't failing, think out of the box, the more likely
> problem is:
>
> 3) Sun doesn't initialize the SROM for the onboard network devices.
>    You have to obtain the MAC address and other settings by fetching
>    them from the openfirmware device properties.

Dave,
Indeed - I wasn't thinking "outside the box". I didn't realize
firmware set up resources needed for bit-banging. I thought
the seprom was accessed via I/O Port space registers.


>    So use something like:
> 
> #ifdef CONFIG_OF
> 	struct device_node *dp = pci_device_to_OF_node(pdev);
> 	const char *addr;
> 
> 	addr = of_get_property(dp, "local-mac-address", NULL);
> 	if (addr) {
> 		memcpy(dev->dev_addr, addr, ETH_ALEN);
> 		return 1;
> 	}
> 	return 0;
> #endif

Ok - will hack something up and ask the user to try that.
I'll also look for CONFIG_OF/CONFIG_SPARC in tulip_core.c as
suggested by Richard Mortimer.

Thanks!
grant

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[David Miller]

From: Grant Grundler <grundler@parisc-linux.org>
Date: Mon, 5 Nov 2007 23:53:22 -0700

> On Sun, Nov 04, 2007 at 01:49:58PM -0800, David Miller wrote:
> > 3) Sun doesn't initialize the SROM for the onboard network devices.
> >    You have to obtain the MAC address and other settings by fetching
> >    them from the openfirmware device properties.
> 
> Dave,
> Indeed - I wasn't thinking "outside the box". I didn't realize
> firmware set up resources needed for bit-banging. I thought
> the seprom was accessed via I/O Port space registers.

It's not that the SROM isn't there and working, it's simply that Sun
doesn't bother filling the SROM with any contents as the standard way
to indicate device properties is via the firmware device tree.

> Ok - will hack something up and ask the user to try that.
> I'll also look for CONFIG_OF/CONFIG_SPARC in tulip_core.c as
> suggested by Richard Mortimer.

Thanks!

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[Grant Grundler]

On Sun, Nov 04, 2007 at 01:49:58PM -0800, David Miller wrote:
...
> The SROM reads aren't failing, think out of the box, the more likely
> problem is:
> 
> 3) Sun doesn't initialize the SROM for the onboard network devices.
>    You have to obtain the MAC address and other settings by fetching
>    them from the openfirmware device properties.

Dave,
Thanks! This was the case with the dmfe driver on SPARC.
I've attached a patch as comment #6 on:
	http://bugzilla.kernel.org/show_bug.cgi?id‘06

User confirmed dmfe driver (with patch) finds the MAC addresses.
I'll ask Kyle McMartin to push this.

Unfortunately, the device still doesn't work.
Any SPARC volunteers interested in working on this bug?

I don't have the HW or interest to work on this further.
It's possible recent changes to dmfe might have also fixed some
of the issues (IIRC, phy reset patch was pushed in Decemeber).
I'd be happy to advise/review changes for anyone who works on it.

thanks,
grant

> 
>    So use something like:
> 
> #ifdef CONFIG_OF
> 	struct device_node *dp = pci_device_to_OF_node(pdev);
> 	const char *addr;
> 
> 	addr = of_get_property(dp, "local-mac-address", NULL);
> 	if (addr) {
> 		memcpy(dev->dev_addr, addr, ETH_ALEN);
> 		return 1;
> 	}
> 	return 0;
> #endif

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[David Miller]

From: Grant Grundler <grundler@parisc-linux.org>
Date: Sun, 6 Jan 2008 01:35:32 -0700

[ Andrew, one of your warning fixes from yesteryear broke
  SROM parsing in the DMFE tulip driver, see below.  ]

> Thanks! This was the case with the dmfe driver on SPARC.
> I've attached a patch as comment #6 on:
> 	http://bugzilla.kernel.org/show_bug.cgi?id‘06

If there is no MAC address in the SROM you'll also have to handle the
fact that there's almost guarenteed to not be any media mode data
there either.  Actually, there might be such information there and
this could be a clue.  See below.

This driver did work on sparc64 to some extent at some point in the
past.  So something did change over time to subtly break this thing.

The users log with the TX timeouts probably indicates that the chip
cannot DMA properly or send to the network for whatever reason.  Maybe
the media type is wrong.

BTW, I noticed this while reading the DMFE code:

	db->buf_pool_ptr = pci_alloc_consistent(pdev, TX_BUF_ALLOC *
			TX_DESC_CNT + 4, &db->buf_pool_dma_ptr);

That size looks strange, is this supposed to be:

	(TX_BUF_ALLOC * TX_DESC_CNT) + 4

or:

	TX_BUF_ALLOC * (TX_DESC_CNT + 4)

I think the latter is the intention, but the former is what
is actually happening.

Looking through the driver history I definitely see some bugs
introduced into the SROM code, for example:

	commit 16b110c3fd760620b4a787db6ed512fe531ab1b5
	Author: Andrew Morton <akpm@osdl.org>
	Date:   Mon Jun 20 15:32:59 2005 -0700

	[PATCH] dmfe warning fix
 ...
	This is basically a guess:
    
	Cc: Jeff Garzik <jgarzik@pobox.com>
	Signed-off-by: Andrew Morton <akpm@osdl.org>

--- a/drivers/net/tulip/dmfe.c
+++ b/drivers/net/tulip/dmfe.c
@@ -1802,7 +1802,7 @@ static void dmfe_parse_srom(struct dmfe_board_info * db)
 	if ( ( (int) srom[18] & 0xff) = SROM_V41_CODE) {
 		/* SROM V4.01 */
 		/* Get NIC support media mode */
-		db->NIC_capability = le16_to_cpup(srom + 34);
+		db->NIC_capability = le16_to_cpup((__le16 *)srom + 34/2);
 		db->PHY_reg4 = 0;
 		for (tmp_reg = 1; tmp_reg < 0x10; tmp_reg <<= 1) {
 			switch( db->NIC_capability & tmp_reg ) {
@@ -1814,7 +1814,8 @@ static void dmfe_parse_srom(struct dmfe_board_info * db)
 		}
 
 		/* Media Mode Force or not check */
-		dmfe_mode = le32_to_cpup(srom + 34) & le32_to_cpup(srom + 36);
+		dmfe_mode = le32_to_cpup((__le32 *)srom + 34/4) &
+				le32_to_cpup((__le32 *)srom + 36/4);
 		switch(dmfe_mode) {
 		case 0x4: dmfe_media_mode = DMFE_100MHF; break;	/* 100MHF */
 		case 0x2: dmfe_media_mode = DMFE_10MFD; break;	/* 10MFD */

Basically he is trying to deal with the fact that "srom" is
a "char *" but the typing requires converting to 16-bit
and 32-bit pointers so the offsets have to be adjusted.

But it is totally wrong in the cases where the offsets were not
a multiple or 2 or 4, respectively.  (hint: (34 % 4) = 2)

It would have been better to use expressions like:

	(__le16 *) (srom + 34)
	(__le32 *) (srom + 34)
	(__le32 *) (srom + 36)

So that change definitely subtly changed how the SROM is accessed
and likely is a good explanation for certain bugs, maybe even this
one we are discussing.

Thinking about this some more, these accesses are extremely queer.
Why would it use 32-bit accesses here to two 32-bit datums which
overlap, and "and" them together to figure out the 'dmfe_mode'?

This driver is in a bit of a state of disrepair.  Who is Tobias
Ringstrom (from MAINTAINERS) and when was the last time he did any
maintainence on dmfe?  He definitely hasn't done anything in
the GIT era as I look through the changelog.

Anyways, here is a fix for the above regression, if that DMA
sizing error above is a real one that will need a fix too:

[TULIP] DMFE: Fix SROM parsing regression.

Changeset 16b110c3fd760620b4a787db6ed512fe531ab1b5 (dmfe warning fix)
bothed up the offsets read from the SROM so that it doesn't read the
same datums it used to.

The change made transformations like turning:

	"srom + 34"

into

	"(__le32 *)srom + 34/4"

which doesn't work because 4 does not divide evenly
into 34 so we're using a different pointer offset
than in the original code.

I've changed theses cases in dmfe_parse_srom() to
consistently use "(type *)(srom + offset)" preserving
the offsets from the original code.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/tulip/dmfe.c b/drivers/net/tulip/dmfe.c
index b4891ca..6562004 100644
--- a/drivers/net/tulip/dmfe.c
+++ b/drivers/net/tulip/dmfe.c
@@ -1909,7 +1909,7 @@ static void dmfe_parse_srom(struct dmfe_board_info * db)
 	if ( ( (int) srom[18] & 0xff) = SROM_V41_CODE) {
 		/* SROM V4.01 */
 		/* Get NIC support media mode */
-		db->NIC_capability = le16_to_cpup((__le16 *)srom + 34/2);
+		db->NIC_capability = le16_to_cpup((__le16 *) (srom + 34));
 		db->PHY_reg4 = 0;
 		for (tmp_reg = 1; tmp_reg < 0x10; tmp_reg <<= 1) {
 			switch( db->NIC_capability & tmp_reg ) {
@@ -1921,8 +1921,8 @@ static void dmfe_parse_srom(struct dmfe_board_info * db)
 		}
 
 		/* Media Mode Force or not check */
-		dmfe_mode = le32_to_cpup((__le32 *)srom + 34/4) &
-				le32_to_cpup((__le32 *)srom + 36/4);
+		dmfe_mode = (le32_to_cpup((__le32 *) (srom + 34)) &
+			     le32_to_cpup((__le32 *) (srom + 36)));
 		switch(dmfe_mode) {
 		case 0x4: dmfe_media_mode = DMFE_100MHF; break;	/* 100MHF */
 		case 0x2: dmfe_media_mode = DMFE_10MFD; break;	/* 10MFD */

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[Grant Grundler]

On Mon, Jan 21, 2008 at 11:27:56PM -0800, David Miller wrote:
> This driver did work on sparc64 to some extent at some point in the
> past.  So something did change over time to subtly break this thing.

Good - that gives me some hope it can be made to work again.

> The users log with the TX timeouts probably indicates that the chip
> cannot DMA properly or send to the network for whatever reason.

Yes - or the interrupts aren't getting generated/delivered.

> Maybe the media type is wrong.
> 
> BTW, I noticed this while reading the DMFE code:
> 
> 	db->buf_pool_ptr = pci_alloc_consistent(pdev, TX_BUF_ALLOC *
> 			TX_DESC_CNT + 4, &db->buf_pool_dma_ptr);
> 
> That size looks strange, is this supposed to be:
> 
> 	(TX_BUF_ALLOC * TX_DESC_CNT) + 4
> 
> or:
> 
> 	TX_BUF_ALLOC * (TX_DESC_CNT + 4)
> 
> I think the latter is the intention, but the former is what
> is actually happening.

Note the same thing is happening in the pci_free_consistent call.

After looking the code a bit, my guess would be the former was intended.
I suspect this driver once suffered from alignment issues:
        /* pointer for memory physical address */
        dma_addr_t buf_pool_dma_ptr;    /* Tx buffer pool memory */
        dma_addr_t buf_pool_dma_start;  /* Tx buffer pool align dword */
...

and I'm going a bit out on a limb here by guessing this line:
	db->buf_pool_dma_start = db->buf_pool_dma_ptr;

might used to read something like:
	db->buf_pool_dma_start = db->buf_pool_dma_ptr & ~3ULL;

or two lines:
        db->buf_pool_dma_start = db->buf_pool_dma_ptr;
        db->buf_pool_dma_ptr &= ~3ULL;

to guarantee the DMA pool had a 4-byte alignment. So I think the +4 can
just be dropped.


> Looking through the driver history I definitely see some bugs
> introduced into the SROM code, for example:
...
> But it is totally wrong in the cases where the offsets were not
> a multiple or 2 or 4, respectively.  (hint: (34 % 4) = 2)

Ouch. Good catch.

> It would have been better to use expressions like:
> 
> 	(__le16 *) (srom + 34)
> 	(__le32 *) (srom + 34)
> 	(__le32 *) (srom + 36)

Yes. This is in fact what the dmfe.c v1.43 driver from Davicom does:
#define DRV_NAME        "dmfe"
#define DRV_VERSION     "1.43"
#define DRV_RELDATE     "2005-07-14"
...
                /* Get NIC support media mode */
                db->NIC_capability = le16_to_cpup((u16 *)(srom + 34));
...
                /* Media Mode Force or not check */
                dmfe_mode = le32_to_cpup((u32 *)(srom + 34)) & le32_to_cpup((u32 *)(srom + 36));

Driver is available from:
http://www.davicom.com.tw/eng/download/Driver/driver_9102a.htm
http://www.davicom.com.tw/big5/download/Driver/dm9102a/dmfe_1.43.tar.gz

> Thinking about this some more, these accesses are extremely queer.
> Why would it use 32-bit accesses here to two 32-bit datums which
> overlap, and "and" them together to figure out the 'dmfe_mode'?

Yeah, this looks wierd. But probably happens to work IFF only one bit
is set of the 16 bits at offset 0x36 and NONE of the same bits are
set at the 16 bits starting at offset 0x38. Given we only care about
bits 1-3 (see "switch(dmfe_mode)"), this should be safe to convert
to 16 bit accesses.

> This driver is in a bit of a state of disrepair.  Who is Tobias
> Ringstrom (from MAINTAINERS) and when was the last time he did any
> maintainence on dmfe?  He definitely hasn't done anything in
> the GIT era as I look through the changelog.

http://ringstrom.mine.nu/

and seems to have started a family since 2003 or so :)
	http://ringstrom.mine.nu/photo/ 

Google hasn't seen any contributions since about 2003.
Mostly active in 2001 on the kernel.
Maybe time to orphan the driver or find a new maintainer?

> Anyways, here is a fix for the above regression, if that DMA
> sizing error above is a real one that will need a fix too:
> 
> [TULIP] DMFE: Fix SROM parsing regression.
> 
> Changeset 16b110c3fd760620b4a787db6ed512fe531ab1b5 (dmfe warning fix)
> bothed up the offsets read from the SROM so that it doesn't read the
> same datums it used to.

In case it matters, looks good to me.

thanks,
grant

Re: [Bug 9106] Sun Fire v100 dmfe driver bug

[David Miller]

From: Grant Grundler <grundler@parisc-linux.org>
Date: Tue, 22 Jan 2008 23:19:50 -0700

> On Mon, Jan 21, 2008 at 11:27:56PM -0800, David Miller wrote:
> > BTW, I noticed this while reading the DMFE code:
> > 
> > 	db->buf_pool_ptr = pci_alloc_consistent(pdev, TX_BUF_ALLOC *
> > 			TX_DESC_CNT + 4, &db->buf_pool_dma_ptr);
> > 
> > That size looks strange, is this supposed to be:
> > 
> > 	(TX_BUF_ALLOC * TX_DESC_CNT) + 4
> > 
> > or:
> > 
> > 	TX_BUF_ALLOC * (TX_DESC_CNT + 4)
> > 
> > I think the latter is the intention, but the former is what
> > is actually happening.
> 
> Note the same thing is happening in the pci_free_consistent call.
> 
> After looking the code a bit, my guess would be the former was intended.
> I suspect this driver once suffered from alignment issues:
>         /* pointer for memory physical address */
>         dma_addr_t buf_pool_dma_ptr;    /* Tx buffer pool memory */
>         dma_addr_t buf_pool_dma_start;  /* Tx buffer pool align dword */
> ...
> 
> and I'm going a bit out on a limb here by guessing this line:
> 	db->buf_pool_dma_start = db->buf_pool_dma_ptr;
> 
> might used to read something like:
> 	db->buf_pool_dma_start = db->buf_pool_dma_ptr & ~3ULL;
> 
> or two lines:
>         db->buf_pool_dma_start = db->buf_pool_dma_ptr;
>         db->buf_pool_dma_ptr &= ~3ULL;
> 
> to guarantee the DMA pool had a 4-byte alignment. So I think the +4 can
> just be dropped.

Great, since we are guarenteed at least PAGE_SIZE alignment
from these allocations.

> Maybe time to orphan the driver or find a new maintainer?

It's a simple enough driver that if I had some cards
I would be willing to keep an eye on it.

> > Anyways, here is a fix for the above regression, if that DMA
> > sizing error above is a real one that will need a fix too:
> > 
> > [TULIP] DMFE: Fix SROM parsing regression.
> > 
> > Changeset 16b110c3fd760620b4a787db6ed512fe531ab1b5 (dmfe warning fix)
> > bothed up the offsets read from the SROM so that it doesn't read the
> > same datums it used to.
> 
> In case it matters, looks good to me.

Thanks for reviewing.