[PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Jay Cliburn]

Based upon feedback from Stephen Hemminger and Francois Romieu, please
consider this resubmitted patchset that provides support for the Attansic 
L1 gigabit ethernet adapter.  This patchset is built against 2.6.19-rc6.  
The original patchset was submitted 20060927.

The monolithic version of this patchset may be found at:
ftp://hogchain.net/pub/linux/m2v/attansic/kernel_driver/atl1-2.0.2/atl1-2.0.2-kernel2.6.19.rc6.patch.bz2

As a reminder, this driver a modified version of the Attansic reference 
driver for the L1 ethernet adapter.  Attansic has granted permission for 
its inclusion in the mainline kernel.

The patch contains:

 Kconfig             |   12 
 Makefile            |    1 
 atl1/Makefile       |   30 
 atl1/atl1.h         |  251 +++++
 atl1/atl1_ethtool.c |  530 ++++++++++
 atl1/atl1_hw.c      |  840 +++++++++++++++++
 atl1/atl1_hw.h      |  991 ++++++++++++++++++++
 atl1/atl1_main.c    | 2551 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 atl1/atl1_osdep.h   |   78 +
 atl1/atl1_param.c   |  203 ++++
 10 files changed, 5487 insertions(+)

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Alan]

Would be nice if it used atl_ not at_ so its less likely to cause
namespace clashes.

You have various macros for swaps that are pretty ugly - we have
cpu_to and le/be_to_cpu functions for most swapping cases and these are
generally optimised assembler (eg bswap on x86)

AT_DESC_USED/UNUSED would be better as inline functions but thats not a
serious concern.

Be careful with :1 bitfields when working with hardware - the compiler
has more than one choice about how to pack them.

The irq enable/disable use for locking on vlan appears unsafe. PCI
interrupt delivery is asynchronous which means you can get this happen


	card sends PCI interrupt
	We call irq_disable
	We take lock
	We poke bits
	We drop lock

	PCI interrupt arrives


This really does happen, typically its nasty to debug as well because you
usually only get it on PIII boards on the one in n-zillion times a
message collides and is retransmitted on the APIC bus.

skb->len is unsigned so <= 0 can be == 0. More importantly the subtraction
before the test will wrap and is completely unsafe (see at_xmit_frame)


Alan

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Chris Snook]

Alan wrote:
> Would be nice if it used atl_ not at_ so its less likely to cause
> namespace clashes.

Some of this code looked like Attansic may have meant to share it 
between drivers for atl1/atl2/atf1/atf2, but seeing as I can't find any 
code for those, I'll convert it all to atl1_ and let Attansic generalize 
the code if they ever decide they want to submit drivers.

> You have various macros for swaps that are pretty ugly - we have
> cpu_to and le/be_to_cpu functions for most swapping cases and these are
> generally optimised assembler (eg bswap on x86)
> 
> AT_DESC_USED/UNUSED would be better as inline functions but thats not a
> serious concern.
> 
> Be careful with :1 bitfields when working with hardware - the compiler
> has more than one choice about how to pack them.

Lacking a spec, I'm not entirely sure what the original intent was, so 
we're stuck with testing.  Is there a specific disambiguation technique 
you recommend?

> The irq enable/disable use for locking on vlan appears unsafe. PCI
> interrupt delivery is asynchronous which means you can get this happen
> 
> 
> 	card sends PCI interrupt
> 	We call irq_disable
> 	We take lock
> 	We poke bits
> 	We drop lock
> 
> 	PCI interrupt arrives
> 
> 
> This really does happen, typically its nasty to debug as well because you
> usually only get it on PIII boards on the one in n-zillion times a
> message collides and is retransmitted on the APIC bus.

Nice catch.  I admit the VLAN code is not so well audited or tested. 
Fortunately, the chip only seems to be on Asus M2V motherboards, at 
least for now, but I want to audit all of the locking code at some point.

> skb->len is unsigned so <= 0 can be == 0. More importantly the subtraction
> before the test will wrap and is completely unsafe (see at_xmit_frame)

Thanks!

	-- Chris

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Alan]

> > Be careful with :1 bitfields when working with hardware - the compiler
> > has more than one choice about how to pack them.
> 
> Lacking a spec, I'm not entirely sure what the original intent was, so 
> we're stuck with testing.  Is there a specific disambiguation technique 
> you recommend?

Assuming the driver works on x86 then you know how the bits are laid out.
You may find later you need to put in ifdefs for bitfield endian-ness as
we do in things like include/linux/ip.h

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Chris Snook]

Jay Cliburn wrote:
> Based upon feedback from Stephen Hemminger and Francois Romieu, please
> consider this resubmitted patchset that provides support for the Attansic 
> L1 gigabit ethernet adapter.  This patchset is built against 2.6.19-rc6.  
> The original patchset was submitted 20060927.
> 
> The monolithic version of this patchset may be found at:
> ftp://hogchain.net/pub/linux/m2v/attansic/kernel_driver/atl1-2.0.2/atl1-2.0.2-kernel2.6.19.rc6.patch.bz2
> 
> As a reminder, this driver a modified version of the Attansic reference 
> driver for the L1 ethernet adapter.  Attansic has granted permission for 
> its inclusion in the mainline kernel.
> 
> The patch contains:
> 
>  Kconfig             |   12 
>  Makefile            |    1 
>  atl1/Makefile       |   30 
>  atl1/atl1.h         |  251 +++++
>  atl1/atl1_ethtool.c |  530 ++++++++++
>  atl1/atl1_hw.c      |  840 +++++++++++++++++
>  atl1/atl1_hw.h      |  991 ++++++++++++++++++++
>  atl1/atl1_main.c    | 2551 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  atl1/atl1_osdep.h   |   78 +
>  atl1/atl1_param.c   |  203 ++++
>  10 files changed, 5487 insertions(+)
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

I've been working on this with Jay since his initial submission.  Thanks 
to everyone who has provided feedback on the resubmit.  We're currently 
quite short on actual testers, since the chip only seems to be on Asus 
M2V motherboards at present.  Please let me and Jay know if you have one 
of these boards and would like to test and/or have encountered bugs.

	-- Chris

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Luca Tettamanti]

Chris Snook <csnook@redhat.com> ha scritto:
> Jay Cliburn wrote:
>> Based upon feedback from Stephen Hemminger and Francois Romieu, please
>> consider this resubmitted patchset that provides support for the Attansic 
>> L1 gigabit ethernet adapter.  This patchset is built against 2.6.19-rc6.  
>> The original patchset was submitted 20060927.
[...]
> 
> I've been working on this with Jay since his initial submission.  Thanks 
> to everyone who has provided feedback on the resubmit.  We're currently 
> quite short on actual testers, since the chip only seems to be on Asus 
> M2V motherboards at present.  Please let me and Jay know if you have one 
> of these boards and would like to test and/or have encountered bugs.

Asus P5B-E also has L1 chip. I'll get the board in a few days and I'll
test whatever patch you throw at me ;)

Luca
-- 
Not an editor command: Wq

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Luca Tettamanti]

Hello,

Luca Tettamanti <kronos.it@gmail.com> ha scritto:
> Chris Snook <csnook@redhat.com> ha scritto:
>> 
>> I've been working on this with Jay since his initial submission.  Thanks 
>> to everyone who has provided feedback on the resubmit.  We're currently 
>> quite short on actual testers, since the chip only seems to be on Asus 
>> M2V motherboards at present.  Please let me and Jay know if you have one 
>> of these boards and would like to test and/or have encountered bugs.
> 
> Asus P5B-E also has L1 chip. I'll get the board in a few days and I'll
> test whatever patch you throw at me ;)

Got the board, done some basic testing: so far so good :)

The controller also supports MSI and (at least with my chipset - G965)
it works fine:

218:      80649          0   PCI-MSI-edge      eth1

which is nice, otherwise it ends up sharing the IRQ with SATA and USB.

I also have a small patch:

On probe failure and on removal the device should be disabled:

Signed-Off-By: Luca Tettamanti <kronos.it@gmail.com> 

---
 drivers/net/atl1/atl1_main.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/atl1/atl1_main.c b/drivers/net/atl1/atl1_main.c
index 167e28c..4883111 100644
--- a/drivers/net/atl1/atl1_main.c
+++ b/drivers/net/atl1/atl1_main.c
@@ -2224,14 +2224,14 @@ static int __devinit at_probe(struct pci
 			printk(KERN_DEBUG 
 				"%s: no usable DMA configuration, aborting\n",
  				at_driver_name);
-			return err;
+			goto err_dma;
 		}
 		pci_using_64 = false;
 	}
 	/* Mark all PCI regions associated with PCI device 
 	 * pdev as being reserved by owner at_driver_name */
 	if ((err = pci_request_regions(pdev, at_driver_name)))
-		return err;
+		goto err_request_regions;
 	/* Enables bus-mastering on the device and calls 
 	 * pcibios_set_master to do the needed arch specific settings */
 	pci_set_master(pdev);
@@ -2384,6 +2384,9 @@ static int __devinit at_probe(struct pci
 	free_netdev(netdev);
       err_alloc_etherdev:
 	pci_release_regions(pdev);
+      err_request_regions:
+      err_dma:
+	pci_disable_device(pdev);
 	return err;
 }
 
@@ -2410,6 +2413,7 @@ static void __devexit at_remove(struct p
 	iounmap(adapter->hw.hw_addr);
 	pci_release_regions(pdev);
 	free_netdev(netdev);
+	pci_disable_device(pdev);
 }
 
 static int at_suspend(struct pci_dev *pdev, pm_message_t state)


Luca
-- 
Not an editor command: Wq

Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

[Jay Cliburn]

Luca Tettamanti wrote:

> Got the board, done some basic testing: so far so good :)
> 
> The controller also supports MSI and (at least with my chipset - G965)
> it works fine:
> 
> 218:      80649          0   PCI-MSI-edge      eth1
> 
> which is nice, otherwise it ends up sharing the IRQ with SATA and USB.
> 
> I also have a small patch:

Thanks for the patch.  We'll add it for the next version.

FYI, TSO performance is _really_ bad; your tx speed will drop dramatically with 
TSO on (and it's on by default).  I haven't yet been able to find the problem. 
If you want to improve tx performance, turn off TSO with ethtool.

Jay