Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Mason]

On 10/11/2015 20:25, Måns Rullgård wrote:

> Mason writes:
> 
>> On 10/11/2015 17:14, Mans Rullgard wrote:
>>
>>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>>> It is an almost complete rewrite of a driver originally found in
>>> a Sigma Designs 2.6.22 tree.
>>>
>>> Signed-off-by: Mans Rullgard 
>>> ---
>>> Changes:
>>> - Refactored mdio access functions
>>> - Refactored register access helpers
>>> - Improved error handling in rx buffer allocation
>>> - Optimised some fifo parameters
>>> - Overhauled tx dma. Multiple packets are now chained in a single dma
>>>   operation if xmit_more is set, improving performance.
>>> - Improved rx irq handling. It's not possible to disable interrupts
>>>   entirely for napi poll, but they can be slowed down a little.
>>> - Use readx_poll_timeout in various places
>>> - Improved error detection
>>> - Improved statistics
>>> - Report hardware statistics counters through ethtool
>>> - Improved tangox-specific setup
>>> - Support for flow control using pause frames
>>> - Explanatory comments added
>>> - Various minor stylistic changes
>>> ---
>>>  drivers/net/ethernet/Kconfig |1 +
>>>  drivers/net/ethernet/Makefile|1 +
>>>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>>>  drivers/net/ethernet/aurora/Makefile |1 +
>>>  drivers/net/ethernet/aurora/nb8800.c | 1530 
>>> ++
>>>  drivers/net/ethernet/aurora/nb8800.h |  314 +++
>>>  6 files changed, 1867 insertions(+)
>>
>> The code has grown much since the previous patch, despite some
>> refactoring. Is this mostly due to ethtool_ops support?
>>
>>  drivers/net/ethernet/aurora/nb8800.c | 1146 
>> ++
>>  drivers/net/ethernet/aurora/nb8800.h |  230 +++
> 
> Some of the increase is from new features, some from improvements, and
> then there are a bunch of new comments.

Sweet.

With this version, my kernel boots faster than before
(I had been using a 5 month-old version.)

Before:

[0.613623] tangox-enet 26000.ethernet: SMP86xx internal Ethernet at 0x26000
[0.623638] libphy: tangox-mii: probed
[0.686527] tangox-enet 26000.ethernet: PHY: found Atheros 8035 ethernet at 
0x4
[0.697169] tangox-enet 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
...
[1.306360] Sending DHCP requests ..
[4.699969] tangox-enet 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[8.899671] ., OK
[8.926343] IP-Config: Got DHCP answer from 172.27.200.1, my address is 
172.27.64.49
...
[8.987327] Freeing unused kernel memory: 168K (c039e000 - c03c8000)

After:

[0.623526] libphy: nb8800-mii: probed
[0.628092] nb8800 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
...
[4.732948] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[4.752655] Sending DHCP requests ., OK
[4.782644] IP-Config: Got DHCP answer from 172.27.200.1, my address is 
172.27.64.49
...
[4.849298] Freeing unused kernel memory: 164K (c039f000 - c03c8000)


The DHCP request is sent later, but the kernel doesn't twiddle its thumbs
for 4 seconds after the link comes up. Does this come from not probing the
PHY anymore?

BTW, you're not using the PHY IRQ, right? I think I remember you saying
it didn't work reliably?

Regards.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Mason  writes:

> On 10/11/2015 20:25, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>> On 10/11/2015 17:14, Mans Rullgard wrote:
>>>
 This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
 It is an almost complete rewrite of a driver originally found in
 a Sigma Designs 2.6.22 tree.

 Signed-off-by: Mans Rullgard 
 ---
 Changes:
 - Refactored mdio access functions
 - Refactored register access helpers
 - Improved error handling in rx buffer allocation
 - Optimised some fifo parameters
 - Overhauled tx dma. Multiple packets are now chained in a single dma
   operation if xmit_more is set, improving performance.
 - Improved rx irq handling. It's not possible to disable interrupts
   entirely for napi poll, but they can be slowed down a little.
 - Use readx_poll_timeout in various places
 - Improved error detection
 - Improved statistics
 - Report hardware statistics counters through ethtool
 - Improved tangox-specific setup
 - Support for flow control using pause frames
 - Explanatory comments added
 - Various minor stylistic changes
 ---
  drivers/net/ethernet/Kconfig |1 +
  drivers/net/ethernet/Makefile|1 +
  drivers/net/ethernet/aurora/Kconfig  |   20 +
  drivers/net/ethernet/aurora/Makefile |1 +
  drivers/net/ethernet/aurora/nb8800.c | 1530 
 ++
  drivers/net/ethernet/aurora/nb8800.h |  314 +++
  6 files changed, 1867 insertions(+)
>>>
>>> The code has grown much since the previous patch, despite some
>>> refactoring. Is this mostly due to ethtool_ops support?
>>>
>>>  drivers/net/ethernet/aurora/nb8800.c | 1146 
>>> ++
>>>  drivers/net/ethernet/aurora/nb8800.h |  230 +++
>> 
>> Some of the increase is from new features, some from improvements, and
>> then there are a bunch of new comments.
>
> Sweet.
>
> With this version, my kernel boots faster than before
> (I had been using a 5 month-old version.)
>
> Before:
>
> [0.613623] tangox-enet 26000.ethernet: SMP86xx internal Ethernet at 
> 0x26000
> [0.623638] libphy: tangox-mii: probed
> [0.686527] tangox-enet 26000.ethernet: PHY: found Atheros 8035 ethernet 
> at 0x4
> [0.697169] tangox-enet 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
> ...
> [1.306360] Sending DHCP requests ..
> [4.699969] tangox-enet 26000.ethernet eth0: Link is Up - 1Gbps/Full - 
> flow control rx/tx
> [8.899671] ., OK
> [8.926343] IP-Config: Got DHCP answer from 172.27.200.1, my address is 
> 172.27.64.49
> ...
> [8.987327] Freeing unused kernel memory: 168K (c039e000 - c03c8000)
>
> After:
>
> [0.623526] libphy: nb8800-mii: probed
> [0.628092] nb8800 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
> ...
> [4.732948] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
> control rx/tx
> [4.752655] Sending DHCP requests ., OK
> [4.782644] IP-Config: Got DHCP answer from 172.27.200.1, my address is 
> 172.27.64.49
> ...
> [4.849298] Freeing unused kernel memory: 164K (c039f000 - c03c8000)
>
> The DHCP request is sent later, but the kernel doesn't twiddle its thumbs
> for 4 seconds after the link comes up. Does this come from not probing the
> PHY anymore?

No, that's from properly setting the link state initially down.

> BTW, you're not using the PHY IRQ, right? I think I remember you saying
> it didn't work reliably?

It doesn't seem to be wired up on any of my boards, or there's some
magic required to activate it that I'm unaware of.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Mason  writes:

> [ CCing a few knowledgeable people ]
>
> Despite the subject, this is about an Atheros 8035 PHY :-)
>
> On 12/11/2015 15:04, Måns Rullgård wrote:
>
>> Mason wrote:
>> 
>>> BTW, you're not using the PHY IRQ, right? I think I remember you saying
>>> it didn't work reliably?
>> 
>> It doesn't seem to be wired up on any of my boards, or there's some
>> magic required to activate it that I'm unaware of.
>
> Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin
> properly connected to AR8035 INT pin (pin 20).

I have a different board.

> 
>
> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>
> INT pin 20
> I/O, D, PD
> Interrupt Signal to System; default OD-gate, needs an external
> 10Kohm pull-up, active low; can be configured to I/O by register,
> active high.
>
> 4.1.17 Interrupt Enable
> Offset: 0x12
> Mode: Read/Write
> Hardware Reset: 0
>
> Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to
> the same "Interrupt Enable" register?

Seems like someone missed that it was already defined.

> In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12:
> link success/fail, speed/duplex changed, autoneg error
>
> Looks like at803x_config_intr() is used for 8031, but not for 8035...
>
> Relevant commit:
> 77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode"
>
> If I add .config_intr and .ack_interrupt to the 8035 struct, then I get
> (also added some traces)

I tried that just now, and I get nothing.  What interrupt did you
specify in your device tree?

> Questions:
>
> Can't at803x_ack_interrupt() just return phy_read(phydev, AT803X_INSR);

No, that would return the actual value of the register.  The caller
doesn't care about the value, but should be notified if there was an
error.

> Can at803x_config_intr() be used with the 8035

Probably.  The person who sent the patch for 8031 probably happened to
have that model.

> What about AT803X_INER/AT803X_INTR_ENABLE and AT803X_INSR/AT803X_INTR_STATUS

Accidental duplicates.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Måns Rullgård  writes:

> Mason  writes:
>
>> [ CCing a few knowledgeable people ]
>>
>> Despite the subject, this is about an Atheros 8035 PHY :-)
>>
>> On 12/11/2015 15:04, Måns Rullgård wrote:
>>
>>> Mason wrote:
>>> 
 BTW, you're not using the PHY IRQ, right? I think I remember you saying
 it didn't work reliably?
>>> 
>>> It doesn't seem to be wired up on any of my boards, or there's some
>>> magic required to activate it that I'm unaware of.
>>
>> Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin
>> properly connected to AR8035 INT pin (pin 20).
>
> I have a different board.
>
>> 
>>
>> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>>
>> INT pin 20
>> I/O, D, PD
>> Interrupt Signal to System; default OD-gate, needs an external
>> 10Kohm pull-up, active low; can be configured to I/O by register,
>> active high.
>>
>> 4.1.17 Interrupt Enable
>> Offset: 0x12
>> Mode: Read/Write
>> Hardware Reset: 0
>>
>> Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to
>> the same "Interrupt Enable" register?
>
> Seems like someone missed that it was already defined.
>
>> In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12:
>> link success/fail, speed/duplex changed, autoneg error
>>
>> Looks like at803x_config_intr() is used for 8031, but not for 8035...
>>
>> Relevant commit:
>> 77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode"
>>
>> If I add .config_intr and .ack_interrupt to the 8035 struct, then I get
>> (also added some traces)
>
> I tried that just now, and I get nothing.  What interrupt did you
> specify in your device tree?

It works with the interrupt set to trigger on rising edge.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Mason]

[ CCing a few knowledgeable people ]

Despite the subject, this is about an Atheros 8035 PHY :-)

On 12/11/2015 15:04, Måns Rullgård wrote:

> Mason wrote:
> 
>> BTW, you're not using the PHY IRQ, right? I think I remember you saying
>> it didn't work reliably?
> 
> It doesn't seem to be wired up on any of my boards, or there's some
> magic required to activate it that I'm unaware of.

Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin
properly connected to AR8035 INT pin (pin 20).



http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf

INT pin 20
I/O, D, PD
Interrupt Signal to System; default OD-gate, needs an external
10Kohm pull-up, active low; can be configured to I/O by register,
active high.


4.1.17 Interrupt Enable
Offset: 0x12
Mode: Read/Write
Hardware Reset: 0


Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to
the same "Interrupt Enable" register?

In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12:
link success/fail, speed/duplex changed, autoneg error

Looks like at803x_config_intr() is used for 8031, but not for 8035...

Relevant commit:
77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode"

If I add .config_intr and .ack_interrupt to the 8035 struct, then I get
(also added some traces)

[0.883517] *** at803x_config_intr: ENABLE
[1.576108] *** at803x_config_intr: DISABLE
[1.580467] *** at803x_config_intr: ENABLE
[1.584959] *** at803x_config_intr: DISABLE
[1.589297] *** at803x_config_intr: ENABLE
[4.321722] *** at803x_config_intr: DISABLE
[4.326054] *** at803x_config_intr: ENABLE
[4.330489] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[4.338335] *** at803x_config_intr: ENABLE

(Are all the ENABLE/DISABLE events expected?)

And if I unplug/replug the Ethernet cable,

[   71.903051] *** at803x_config_intr: DISABLE
[   71.907410] *** at803x_config_intr: ENABLE
[   71.912232] nb8800 26000.ethernet eth0: Link is Down
[   71.917309] *** at803x_config_intr: ENABLE
[   78.008972] *** at803x_config_intr: DISABLE
[   78.013375] *** at803x_config_intr: ENABLE
[   78.017797] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[   78.025702] *** at803x_config_intr: ENABLE

(Are all the ENABLE/DISABLE events expected there too?)

# cat /proc/interrupts 
CPU0   CPU1   
 18:107  0  irq0   1 Level serial
 54:  5  0  irq0  37 Edge  phy_interrupt
 55:   4953  0  irq0  38 Level eth0
211:   1147254   GIC  29 Edge  twd


Questions:

Can't at803x_ack_interrupt() just return phy_read(phydev, AT803X_INSR);
Can at803x_config_intr() be used with the 8035
What about AT803X_INER/AT803X_INTR_ENABLE and AT803X_INSR/AT803X_INTR_STATUS

Regards.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Wed, 11 Nov 2015 13:04:07 +
>
>> Måns Rullgård  writes:
>> 
>>> David Miller  writes:
>>>
 From: Måns Rullgård 
 Date: Wed, 11 Nov 2015 00:40:09 +

> When the DMA complete interrupt arrives, the next chain should be
> kicked off as quickly as possible, and I don't see why that would
> benefit from being done in napi context.

 NAPI isn't about low latency, it's about fairness and interrupt
 mitigation.

 You probably don't even realize that all of the TX SKB freeing you do
 in the hardware interrupt handler end up being actually processed by a
 scheduled software interrupt anyways.

 So you are gaining almost nothing by not doing TX completion in NAPI
 context, whereas by doing so you would be gaining a lot including
 more simplified locking or even the ability to do no locking at all.
>>>
>>> TX completion is separate from restarting the DMA, and moving that to
>>> NAPI may well be a good idea.  Should I simply napi_schedule() if the
>>> hardware indicates TX is complete and do the cleanup in the NAPI poll
>>> function?
>> 
>> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> Maybe I did something wrong.
>
> Did you fix all the locking in that change?
>
> Since all of your TX handling runs in software interrupt context, you
> can stop using IRQ locking and use BH locking driver-wide instead.
>
> And actually, no locking is really needed for TX processing.  With
> proper memory barriers and properly crafter queue state tests, you
> can run completely lockless.
>
> Again, look at example drivers.  I know, for example, that
> drivers/net/ethernet/broadcom/tg3.c runs TX lockless.  You'll
> see that tg3_tx() takes no locks at all.

The way the hardware works, once a DMA operation has been started,
adding more packets to the active chain can't be done reliably.  For
that reason, if start_xmit is called (with xmit_more zero) while a DMA
operation is in progress, the new packet(s) must be queued until the
hardware raises the DMA complete interrupt.  At that time, the next
pending DMA chain, if any, can be kicked off.  If the TX DMA channel is
idle when start_xmit is called, it can be started immediately.  Checking
the DMA status and starting it if idle has to be done atomically
somehow.

There is a separate indication for actual TX completion, and the
interrupt for that can be set to only fire every 7 frames or when a
timeout expires.  When this happens, the TX cleanup needs to run, and
that can obviously be done from NAPI without using any locks.

Bear in mind that this hardware is quite primitive compared to modern
high-performance Ethernet controllers from the likes of Intel and
Broadcom.  The documentation I have is dated 2003.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Eric Dumazet]

On Wed, 2015-11-11 at 14:15 +, Måns Rullgård wrote:

> FWIW, with UDP I get 650 Mbps.

Have you tried pktgen ? It should give you raw numbers, without the
stack overhead, especially when using the clone operator.

(It also has xmit_more support)

If the NIC can not get more than 650 Mbps, not worth trying implementing
tso in your driver (using net/core/tso.c helpers)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 13:04:07 +

> Måns Rullgård  writes:
> 
>> David Miller  writes:
>>
>>> From: Måns Rullgård 
>>> Date: Wed, 11 Nov 2015 00:40:09 +
>>>
 When the DMA complete interrupt arrives, the next chain should be
 kicked off as quickly as possible, and I don't see why that would
 benefit from being done in napi context.
>>>
>>> NAPI isn't about low latency, it's about fairness and interrupt
>>> mitigation.
>>>
>>> You probably don't even realize that all of the TX SKB freeing you do
>>> in the hardware interrupt handler end up being actually processed by a
>>> scheduled software interrupt anyways.
>>>
>>> So you are gaining almost nothing by not doing TX completion in NAPI
>>> context, whereas by doing so you would be gaining a lot including
>>> more simplified locking or even the ability to do no locking at all.
>>
>> TX completion is separate from restarting the DMA, and moving that to
>> NAPI may well be a good idea.  Should I simply napi_schedule() if the
>> hardware indicates TX is complete and do the cleanup in the NAPI poll
>> function?
> 
> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> Maybe I did something wrong.

Did you fix all the locking in that change?

Since all of your TX handling runs in software interrupt context, you
can stop using IRQ locking and use BH locking driver-wide instead.

And actually, no locking is really needed for TX processing.  With
proper memory barriers and properly crafter queue state tests, you
can run completely lockless.

Again, look at example drivers.  I know, for example, that
drivers/net/ethernet/broadcom/tg3.c runs TX lockless.  You'll
see that tg3_tx() takes no locks at all.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Eric Dumazet]

On Wed, 2015-11-11 at 13:04 +, Måns Rullgård wrote:

> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> Maybe I did something wrong.

What link speed have you used, what was the throughput you got,
and is the receiver using the same NIC ?




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Mason  writes:

> On 10/11/2015 22:51, Eric Dumazet wrote:
>
>> On Tue, 2015-11-10 at 21:21 +, Måns Rullgård wrote:
>> 
>>> Even ixgbe uses napi_complete() while netdevice.h says one should
>>> "consider using napi_complete_done() instead."  Did the author consider
>>> it and decide not to, or has the driver simply not been updated?
>> 
>> napi_complete_done() is quite new, very few drivers use it.
>
> I was hoping to back-port this driver for my platform. Are the recent
> features used in the driver available in 4.1? What about 3.14?

xmit_more was added in 3.18, napi_complete_done() in 3.19.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Eric Dumazet]

On Wed, 2015-11-11 at 13:54 +, Måns Rullgård wrote:
> Mason  writes:
> 
> > On 10/11/2015 22:51, Eric Dumazet wrote:
> >
> >> On Tue, 2015-11-10 at 21:21 +, Måns Rullgård wrote:
> >> 
> >>> Even ixgbe uses napi_complete() while netdevice.h says one should
> >>> "consider using napi_complete_done() instead."  Did the author consider
> >>> it and decide not to, or has the driver simply not been updated?
> >> 
> >> napi_complete_done() is quite new, very few drivers use it.
> >
> > I was hoping to back-port this driver for my platform. Are the recent
> > features used in the driver available in 4.1? What about 3.14?
> 
> xmit_more was added in 3.18, napi_complete_done() in 3.19.

xmit_more is a hint, you can simply consider skb->xmit_more being
replaced by 0 on old kernels if you want to backport.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Eric Dumazet  writes:

> On Wed, 2015-11-11 at 13:48 +, Måns Rullgård wrote:
>> Eric Dumazet  writes:
>> 
>> > On Wed, 2015-11-11 at 13:04 +, Måns Rullgård wrote:
>> >
>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> >> Maybe I did something wrong.
>> >
>> > What link speed have you used, what was the throughput you got,
>> > and is the receiver using the same NIC ?
>> 
>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
>> Why does it matter what NIC the receiver has?
>
> Because at 1Gb line rate, you better get GRO properly implemented in the
> receiver, so that TCP stack does not send one ACK every 2 MSS.
>
> Send speed is also dependent on the number of ACK packets the sender has
> to process.
>
> This is why I suggested you use napi_gro_receive() in your driver.

FWIW, with UDP I get 650 Mbps.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Måns Rullgård  writes:

> Eric Dumazet  writes:
>
>> On Wed, 2015-11-11 at 13:48 +, Måns Rullgård wrote:
>>> Eric Dumazet  writes:
>>> 
>>> > On Wed, 2015-11-11 at 13:04 +, Måns Rullgård wrote:
>>> >
>>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>>> >> Maybe I did something wrong.
>>> >
>>> > What link speed have you used, what was the throughput you got,
>>> > and is the receiver using the same NIC ?
>>> 
>>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
>>> Why does it matter what NIC the receiver has?
>>
>> Because at 1Gb line rate, you better get GRO properly implemented in the
>> receiver, so that TCP stack does not send one ACK every 2 MSS.
>>
>> Send speed is also dependent on the number of ACK packets the sender has
>> to process.
>>
>> This is why I suggested you use napi_gro_receive() in your driver.
>
> FWIW, with UDP I get 650 Mbps.

It seems the on-chip interconnect is limiting memory bandwidth available
to the Ethernet DMA.  If I increase the limits, I get 800 Mbps over TCP
and 850 Mbps over UDP with the driver posted here, the TCP case being
CPU bound in csum_partial_copy_from_user.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 18:25:05 +

> If the TX DMA channel is idle when start_xmit is called, it can be
> started immediately.  Checking the DMA status and starting it if
> idle has to be done atomically somehow.

->ndo_start_xmit() is guaranteed to be invoked atomically, protected
by the TX queue spinlock.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 19:35:05 +

>> I don't think it's silly at all.
> 
> I'm sure I read somewhere that the time spent spinning on a lock should
> be kept as small as possible.
> 
>> And unless you can measure it making a difference, don't knock the idea.
> 
> I tried using netif_tx_lock() in the IRQ handler instead, and it locked
> up solid.  Clearly that was the wrong thing to do.

Oh that's right, it's a BH lock not an IRQ one.

Yet another argument for doing everything in ->poll(), thus making all
operations outside of NAPI scheduling run in software interrupt
context, and therefore being able to make use of the TXQ lock for
this.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Wed, 11 Nov 2015 18:25:05 +
>
>> If the TX DMA channel is idle when start_xmit is called, it can be
>> started immediately.  Checking the DMA status and starting it if
>> idle has to be done atomically somehow.
>
> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
> by the TX queue spinlock.

Yes, but the DMA needs to be restarted from some other context if it was
busy when start_xmit checked.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Wed, 11 Nov 2015 19:09:19 +
>
>> David Miller  writes:
>> 
>>> From: Måns Rullgård 
>>> Date: Wed, 11 Nov 2015 18:25:05 +
>>>
 If the TX DMA channel is idle when start_xmit is called, it can be
 started immediately.  Checking the DMA status and starting it if
 idle has to be done atomically somehow.
>>>
>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>> by the TX queue spinlock.
>> 
>> Yes, but the DMA needs to be restarted from some other context if it was
>> busy when start_xmit checked.
>
> Then you can probably use the TXQ lock in the interrupt handler just for
> that.

That seems a bit heavy-handed when the critical section for this is only
a tiny part of the start_xmit function.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 19:25:46 +

> David Miller  writes:
> 
>> From: Måns Rullgård 
>> Date: Wed, 11 Nov 2015 19:17:07 +
>>
>>> David Miller  writes:
>>> 
 From: Måns Rullgård 
 Date: Wed, 11 Nov 2015 19:09:19 +

> David Miller  writes:
> 
>> From: Måns Rullgård 
>> Date: Wed, 11 Nov 2015 18:25:05 +
>>
>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>> started immediately.  Checking the DMA status and starting it if
>>> idle has to be done atomically somehow.
>>
>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>> by the TX queue spinlock.
> 
> Yes, but the DMA needs to be restarted from some other context if it was
> busy when start_xmit checked.

 Then you can probably use the TXQ lock in the interrupt handler just for
 that.
>>> 
>>> That seems a bit heavy-handed when the critical section for this is only
>>> a tiny part of the start_xmit function.
>>
>> Then what synchornization primitive other than spin locks are you going
>> to use for this?
>>
>> My point is that there is a spinlock the core code is _already_ taking,
>> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
>> take advantage of that rather than using another lock of your own.
> 
> I get that.  But that remains locked for the duration of ndo_start_xmit()
> whereas the part that needs to be synchronised with the DMA completion
> IRQ handler is tiny.  Having the IRQ handler spin for the duration of
> ndo_start_xmit() seemed silly to me.

I don't think it's silly at all.

And unless you can measure it making a difference, don't knock the idea.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 19:09:19 +

> David Miller  writes:
> 
>> From: Måns Rullgård 
>> Date: Wed, 11 Nov 2015 18:25:05 +
>>
>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>> started immediately.  Checking the DMA status and starting it if
>>> idle has to be done atomically somehow.
>>
>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>> by the TX queue spinlock.
> 
> Yes, but the DMA needs to be restarted from some other context if it was
> busy when start_xmit checked.

Then you can probably use the TXQ lock in the interrupt handler just for
that.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 19:17:07 +

> David Miller  writes:
> 
>> From: Måns Rullgård 
>> Date: Wed, 11 Nov 2015 19:09:19 +
>>
>>> David Miller  writes:
>>> 
 From: Måns Rullgård 
 Date: Wed, 11 Nov 2015 18:25:05 +

> If the TX DMA channel is idle when start_xmit is called, it can be
> started immediately.  Checking the DMA status and starting it if
> idle has to be done atomically somehow.

 ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
 by the TX queue spinlock.
>>> 
>>> Yes, but the DMA needs to be restarted from some other context if it was
>>> busy when start_xmit checked.
>>
>> Then you can probably use the TXQ lock in the interrupt handler just for
>> that.
> 
> That seems a bit heavy-handed when the critical section for this is only
> a tiny part of the start_xmit function.

Then what synchornization primitive other than spin locks are you going
to use for this?

My point is that there is a spinlock the core code is _already_ taking,
unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
take advantage of that rather than using another lock of your own.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Wed, 11 Nov 2015 19:17:07 +
>
>> David Miller  writes:
>> 
>>> From: Måns Rullgård 
>>> Date: Wed, 11 Nov 2015 19:09:19 +
>>>
 David Miller  writes:
 
> From: Måns Rullgård 
> Date: Wed, 11 Nov 2015 18:25:05 +
>
>> If the TX DMA channel is idle when start_xmit is called, it can be
>> started immediately.  Checking the DMA status and starting it if
>> idle has to be done atomically somehow.
>
> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
> by the TX queue spinlock.
 
 Yes, but the DMA needs to be restarted from some other context if it was
 busy when start_xmit checked.
>>>
>>> Then you can probably use the TXQ lock in the interrupt handler just for
>>> that.
>> 
>> That seems a bit heavy-handed when the critical section for this is only
>> a tiny part of the start_xmit function.
>
> Then what synchornization primitive other than spin locks are you going
> to use for this?
>
> My point is that there is a spinlock the core code is _already_ taking,
> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
> take advantage of that rather than using another lock of your own.

I get that.  But that remains locked for the duration of ndo_start_xmit()
whereas the part that needs to be synchronised with the DMA completion
IRQ handler is tiny.  Having the IRQ handler spin for the duration of
ndo_start_xmit() seemed silly to me.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Wed, 11 Nov 2015 19:35:05 +
>
>>> I don't think it's silly at all.
>> 
>> I'm sure I read somewhere that the time spent spinning on a lock should
>> be kept as small as possible.
>> 
>>> And unless you can measure it making a difference, don't knock the idea.
>> 
>> I tried using netif_tx_lock() in the IRQ handler instead, and it locked
>> up solid.  Clearly that was the wrong thing to do.
>
> Oh that's right, it's a BH lock not an IRQ one.
>
> Yet another argument for doing everything in ->poll(), thus making all
> operations outside of NAPI scheduling run in software interrupt
> context, and therefore being able to make use of the TXQ lock for
> this.

Well, I tried calling the DMA restart function from NAPI poll under
netif_tx_lock().  Now it works only as long as there is incoming
traffic.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Mans Rullgard]

This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
It is an almost complete rewrite of a driver originally found in
a Sigma Designs 2.6.22 tree.

Signed-off-by: Mans Rullgard 
---
Changes:
- Refactored mdio access functions
- Refactored register access helpers
- Improved error handling in rx buffer allocation
- Optimised some fifo parameters
- Overhauled tx dma. Multiple packets are now chained in a single dma
  operation if xmit_more is set, improving performance.
- Improved rx irq handling. It's not possible to disable interrupts
  entirely for napi poll, but they can be slowed down a little.
- Use readx_poll_timeout in various places
- Improved error detection
- Improved statistics
- Report hardware statistics counters through ethtool
- Improved tangox-specific setup
- Support for flow control using pause frames
- Explanatory comments added
- Various minor stylistic changes
---
 drivers/net/ethernet/Kconfig |1 +
 drivers/net/ethernet/Makefile|1 +
 drivers/net/ethernet/aurora/Kconfig  |   20 +
 drivers/net/ethernet/aurora/Makefile |1 +
 drivers/net/ethernet/aurora/nb8800.c | 1530 ++
 drivers/net/ethernet/aurora/nb8800.h |  314 +++
 6 files changed, 1867 insertions(+)
 create mode 100644 drivers/net/ethernet/aurora/Kconfig
 create mode 100644 drivers/net/ethernet/aurora/Makefile
 create mode 100644 drivers/net/ethernet/aurora/nb8800.c
 create mode 100644 drivers/net/ethernet/aurora/nb8800.h

diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 05aa759..8310163 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -29,6 +29,7 @@ source "drivers/net/ethernet/apm/Kconfig"
 source "drivers/net/ethernet/apple/Kconfig"
 source "drivers/net/ethernet/arc/Kconfig"
 source "drivers/net/ethernet/atheros/Kconfig"
+source "drivers/net/ethernet/aurora/Kconfig"
 source "drivers/net/ethernet/cadence/Kconfig"
 source "drivers/net/ethernet/adi/Kconfig"
 source "drivers/net/ethernet/broadcom/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index ddfc808..b435fb0 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_NET_XGENE) += apm/
 obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
 obj-$(CONFIG_NET_VENDOR_ARC) += arc/
 obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
+obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
 obj-$(CONFIG_NET_CADENCE) += cadence/
 obj-$(CONFIG_NET_BFIN) += adi/
 obj-$(CONFIG_NET_VENDOR_BROADCOM) += broadcom/
diff --git a/drivers/net/ethernet/aurora/Kconfig 
b/drivers/net/ethernet/aurora/Kconfig
new file mode 100644
index 000..a3c7106
--- /dev/null
+++ b/drivers/net/ethernet/aurora/Kconfig
@@ -0,0 +1,20 @@
+config NET_VENDOR_AURORA
+   bool "Aurora VLSI devices"
+   help
+ If you have a network (Ethernet) device belonging to this class,
+ say Y.
+
+ Note that the answer to this question doesn't directly affect the
+ kernel: saying N will just cause the configurator to skip all
+ questions about Aurora devices. If you say Y, you will be asked
+ for your specific device in the following questions.
+
+if NET_VENDOR_AURORA
+
+config AURORA_NB8800
+   tristate "Aurora AU-NB8800 support"
+   select PHYLIB
+   help
+Support for the AU-NB8800 gigabit Ethernet controller.
+
+endif
diff --git a/drivers/net/ethernet/aurora/Makefile 
b/drivers/net/ethernet/aurora/Makefile
new file mode 100644
index 000..6cb528a
--- /dev/null
+++ b/drivers/net/ethernet/aurora/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_AURORA_NB8800) += nb8800.o
diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
new file mode 100644
index 000..11cd389
--- /dev/null
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -0,0 +1,1530 @@
+/*
+ * Copyright (C) 2015 Mans Rullgard 
+ *
+ * Mostly rewritten, based on driver from Sigma Designs.  Original
+ * copyright notice below.
+ *
+ *
+ * Driver for tangox SMP864x/SMP865x/SMP867x/SMP868x builtin Ethernet Mac.
+ *
+ * Copyright (C) 2005 Maxime Bizon 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "nb8800.h"
+
+static int nb8800_dma_stop(struct net_device *dev);
+
+static inline u8 

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Eric Dumazet  writes:

> On Tue, 2015-11-10 at 16:14 +, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> ...
>
>> +
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +struct nb8800_priv *priv = netdev_priv(dev);
>> +struct nb8800_tx_desc *txd;
>> +struct nb8800_tx_buf *txb;
>> +struct nb8800_dma_desc *dma;
>> +dma_addr_t dma_addr;
>> +unsigned int dma_len;
>> +unsigned long flags;
>> +int align;
>> +int next;
>> +
>> +if (atomic_read(>tx_free) <= NB8800_DESC_LOW) {
>> +netif_stop_queue(dev);
>> +return NETDEV_TX_BUSY;
>> +}
>> +
>> +align = (8 - (uintptr_t)skb->data) & 7;
>> +
>> +dma_len = skb->len - align;
>> +dma_addr = dma_map_single(>dev, skb->data + align,
>> +  dma_len, DMA_TO_DEVICE);
>> +
>> +if (dma_mapping_error(>dev, dma_addr)) {
>> +netdev_err(dev, "tx dma mapping error\n");
>> +kfree_skb(skb);
>> +dev->stats.tx_dropped++;
>> +return NETDEV_TX_OK;
>> +}
>> +
>> +if (atomic_dec_return(>tx_free) <= NB8800_DESC_LOW)
>> +netif_stop_queue(dev);
>
> You probably also want to clear skb->xmit_more or risk a stall

Very true, will fix.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Mason]

On 10/11/2015 17:14, Mans Rullgard wrote:

> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
> It is an almost complete rewrite of a driver originally found in
> a Sigma Designs 2.6.22 tree.
> 
> Signed-off-by: Mans Rullgard 
> ---
> Changes:
> - Refactored mdio access functions
> - Refactored register access helpers
> - Improved error handling in rx buffer allocation
> - Optimised some fifo parameters
> - Overhauled tx dma. Multiple packets are now chained in a single dma
>   operation if xmit_more is set, improving performance.
> - Improved rx irq handling. It's not possible to disable interrupts
>   entirely for napi poll, but they can be slowed down a little.
> - Use readx_poll_timeout in various places
> - Improved error detection
> - Improved statistics
> - Report hardware statistics counters through ethtool
> - Improved tangox-specific setup
> - Support for flow control using pause frames
> - Explanatory comments added
> - Various minor stylistic changes
> ---
>  drivers/net/ethernet/Kconfig |1 +
>  drivers/net/ethernet/Makefile|1 +
>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>  drivers/net/ethernet/aurora/Makefile |1 +
>  drivers/net/ethernet/aurora/nb8800.c | 1530 
> ++
>  drivers/net/ethernet/aurora/nb8800.h |  314 +++
>  6 files changed, 1867 insertions(+)

The code has grown much since the previous patch, despite some
refactoring. Is this mostly due to ethtool_ops support?

 drivers/net/ethernet/aurora/nb8800.c | 1146 ++
 drivers/net/ethernet/aurora/nb8800.h |  230 +++

Regards.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Eric Dumazet  writes:

> On Tue, 2015-11-10 at 16:14 +, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> ...
>
>> +
>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>> +{
>> +struct nb8800_priv *priv = netdev_priv(dev);
>> +struct nb8800_rx_desc *rxd = >rx_descs[i];
>> +struct page *page = priv->rx_bufs[i].page;
>> +int offset = priv->rx_bufs[i].offset;
>> +void *data = page_address(page) + offset;
>> +dma_addr_t dma = rxd->desc.s_addr;
>> +struct sk_buff *skb;
>> +int size;
>> +int err;
>> +
>> +size = len <= RX_COPYBREAK ? len : RX_COPYHDR;
>> +
>> +skb = napi_alloc_skb(>napi, size);
>> +if (!skb) {
>> +netdev_err(dev, "rx skb allocation failed\n");
>> +dev->stats.rx_dropped++;
>> +return;
>> +}
>> +
>> +if (len <= RX_COPYBREAK) {
>> +dma_sync_single_for_cpu(>dev, dma, len, DMA_FROM_DEVICE);
>> +memcpy(skb_put(skb, len), data, len);
>> +dma_sync_single_for_device(>dev, dma, len,
>> +   DMA_FROM_DEVICE);
>> +} else {
>> +err = nb8800_alloc_rx(dev, i, true);
>> +if (err) {
>> +netdev_err(dev, "rx buffer allocation failed\n");
>> +dev->stats.rx_dropped++;
>> +return;
>> +}
>> +
>> +dma_unmap_page(>dev, dma, RX_BUF_SIZE, DMA_FROM_DEVICE);
>> +memcpy(skb_put(skb, RX_COPYHDR), data, RX_COPYHDR);
>> +skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>> +offset + RX_COPYHDR, len - RX_COPYHDR,
>> +RX_BUF_SIZE);
>> +}
>> +
>> +skb->protocol = eth_type_trans(skb, dev);
>> +netif_receive_skb(skb);
>> +}
>> +
>
> Any reason you do not use napi_gro_receive(>napi, skb) instead of
> netif_receive_skb() ?

Because I haven't been following the netdev list closely for the last
five years, and no documentation I read mentioned this function.  I can
certainly change it.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Mason  writes:

> On 10/11/2015 17:14, Mans Rullgard wrote:
>
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>> 
>> Signed-off-by: Mans Rullgard 
>> ---
>> Changes:
>> - Refactored mdio access functions
>> - Refactored register access helpers
>> - Improved error handling in rx buffer allocation
>> - Optimised some fifo parameters
>> - Overhauled tx dma. Multiple packets are now chained in a single dma
>>   operation if xmit_more is set, improving performance.
>> - Improved rx irq handling. It's not possible to disable interrupts
>>   entirely for napi poll, but they can be slowed down a little.
>> - Use readx_poll_timeout in various places
>> - Improved error detection
>> - Improved statistics
>> - Report hardware statistics counters through ethtool
>> - Improved tangox-specific setup
>> - Support for flow control using pause frames
>> - Explanatory comments added
>> - Various minor stylistic changes
>> ---
>>  drivers/net/ethernet/Kconfig |1 +
>>  drivers/net/ethernet/Makefile|1 +
>>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>>  drivers/net/ethernet/aurora/Makefile |1 +
>>  drivers/net/ethernet/aurora/nb8800.c | 1530 
>> ++
>>  drivers/net/ethernet/aurora/nb8800.h |  314 +++
>>  6 files changed, 1867 insertions(+)
>
> The code has grown much since the previous patch, despite some
> refactoring. Is this mostly due to ethtool_ops support?
>
>  drivers/net/ethernet/aurora/nb8800.c | 1146 
> ++
>  drivers/net/ethernet/aurora/nb8800.h |  230 +++

Some of the increase is from new features, some from improvements, and
then there are a bunch of new comments.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Tue, 10 Nov 2015 18:05:15 +
>
>> Because I haven't been following the netdev list closely for the last
>> five years, and no documentation I read mentioned this function.  I can
>> certainly change it.
>
> It is always advisable to mimick what other drivers do and use them as
> a reference, rather than depend upon documentation which by definition
> is always going to be out of sync with the source tree.

Sure.  The trick is to pick the right driver(s) to use as reference.
Quite a few of them don't use that function.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Tue, 10 Nov 2015 20:53:19 +

> David Miller  writes:
> 
>> From: Måns Rullgård 
>> Date: Tue, 10 Nov 2015 18:05:15 +
>>
>>> Because I haven't been following the netdev list closely for the last
>>> five years, and no documentation I read mentioned this function.  I can
>>> certainly change it.
>>
>> It is always advisable to mimick what other drivers do and use them as
>> a reference, rather than depend upon documentation which by definition
>> is always going to be out of sync with the source tree.
> 
> Sure.  The trick is to pick the right driver(s) to use as reference.
> Quite a few of them don't use that function.

If you really are stumped on this matter, start at least with the
ixgbe driver.  In fact pretty much every Intel ethernet driver is
a reasonable reference.  Others to check out are bnx2x and mlx5.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Tue, 10 Nov 2015 18:05:15 +

> Because I haven't been following the netdev list closely for the last
> five years, and no documentation I read mentioned this function.  I can
> certainly change it.

It is always advisable to mimick what other drivers do and use them as
a reference, rather than depend upon documentation which by definition
is always going to be out of sync with the source tree.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

David Miller  writes:

> From: Måns Rullgård 
> Date: Tue, 10 Nov 2015 20:53:19 +
>
>> David Miller  writes:
>> 
>>> From: Måns Rullgård 
>>> Date: Tue, 10 Nov 2015 18:05:15 +
>>>
 Because I haven't been following the netdev list closely for the last
 five years, and no documentation I read mentioned this function.  I can
 certainly change it.
>>>
>>> It is always advisable to mimick what other drivers do and use them as
>>> a reference, rather than depend upon documentation which by definition
>>> is always going to be out of sync with the source tree.
>> 
>> Sure.  The trick is to pick the right driver(s) to use as reference.
>> Quite a few of them don't use that function.
>
> If you really are stumped on this matter, start at least with the
> ixgbe driver.  In fact pretty much every Intel ethernet driver is
> a reasonable reference.  Others to check out are bnx2x and mlx5.

Even ixgbe uses napi_complete() while netdevice.h says one should
"consider using napi_complete_done() instead."  Did the author consider
it and decide not to, or has the driver simply not been updated?

As for the napi_gro_receive() function, calling that instead of
netif_receive_skb() is easy enough, or are there other things I should
be doing in addition?

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Eric Dumazet]

On Tue, 2015-11-10 at 21:21 +, Måns Rullgård wrote:

> Even ixgbe uses napi_complete() while netdevice.h says one should
> "consider using napi_complete_done() instead."  Did the author consider
> it and decide not to, or has the driver simply not been updated?

napi_complete_done() is quite new, very few drivers use it.

It still requires a tuning (/sys/class/net/ethX/gro_flush_timeout)

> 
> As for the napi_gro_receive() function, calling that instead of
> netif_receive_skb() is easy enough, or are there other things I should
> be doing in addition?

Nothing comes to mind, if you already have a NAPI context,
napi_gro_receive() is the way to go...



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Andy Shevchenko  writes:

> On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård  wrote:
>> Andy Shevchenko  writes:
>>
 +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
 +   u32 mask, u32 val)
 +{
 +   u32 old = nb8800_readb(priv, reg);
 +   u32 new = (old & ~mask) | val;
>>>
>>> Shoudn't be "… | (val & mask);" ?
>>
>> No, it's meant to replace the bits in mask with the corresponding bits
>> from val.
>
> But you unconditionally use entire val value which might bring bits
> outside of mask.

Very well, I'll apply the mask to both then.

 +   nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
 +   wmb();  /* ensure desc addr is written before starting DMA 
 */
>>>
>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps 
>>> mmiowb() ?
>>
>> Possibly.
>
> Standalone wmb() doesn't make sense.

It does if you need to enforce ordering between normal and I/O memory.
In fact, since the descriptor is filled in using normal memory accesses,
my understanding is that mmiowb() would be insufficient here.  The
comment could be improved, however.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Andy Shevchenko]

On Tue, Nov 10, 2015 at 6:14 PM, Mans Rullgard  wrote:
> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
> It is an almost complete rewrite of a driver originally found in
> a Sigma Designs 2.6.22 tree.

Few nitpicks below.

>
> Signed-off-by: Mans Rullgard 
> ---
> Changes:
> - Refactored mdio access functions
> - Refactored register access helpers
> - Improved error handling in rx buffer allocation
> - Optimised some fifo parameters
> - Overhauled tx dma. Multiple packets are now chained in a single dma
>   operation if xmit_more is set, improving performance.
> - Improved rx irq handling. It's not possible to disable interrupts
>   entirely for napi poll, but they can be slowed down a little.
> - Use readx_poll_timeout in various places
> - Improved error detection
> - Improved statistics
> - Report hardware statistics counters through ethtool
> - Improved tangox-specific setup
> - Support for flow control using pause frames
> - Explanatory comments added
> - Various minor stylistic changes
> ---
>  drivers/net/ethernet/Kconfig |1 +
>  drivers/net/ethernet/Makefile|1 +
>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>  drivers/net/ethernet/aurora/Makefile |1 +
>  drivers/net/ethernet/aurora/nb8800.c | 1530 
> ++
>  drivers/net/ethernet/aurora/nb8800.h |  314 +++
>  6 files changed, 1867 insertions(+)
>  create mode 100644 drivers/net/ethernet/aurora/Kconfig
>  create mode 100644 drivers/net/ethernet/aurora/Makefile
>  create mode 100644 drivers/net/ethernet/aurora/nb8800.c
>  create mode 100644 drivers/net/ethernet/aurora/nb8800.h
>
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index 05aa759..8310163 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -29,6 +29,7 @@ source "drivers/net/ethernet/apm/Kconfig"
>  source "drivers/net/ethernet/apple/Kconfig"
>  source "drivers/net/ethernet/arc/Kconfig"
>  source "drivers/net/ethernet/atheros/Kconfig"
> +source "drivers/net/ethernet/aurora/Kconfig"
>  source "drivers/net/ethernet/cadence/Kconfig"
>  source "drivers/net/ethernet/adi/Kconfig"
>  source "drivers/net/ethernet/broadcom/Kconfig"
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index ddfc808..b435fb0 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_NET_XGENE) += apm/
>  obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
>  obj-$(CONFIG_NET_VENDOR_ARC) += arc/
>  obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
> +obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
>  obj-$(CONFIG_NET_CADENCE) += cadence/
>  obj-$(CONFIG_NET_BFIN) += adi/
>  obj-$(CONFIG_NET_VENDOR_BROADCOM) += broadcom/
> diff --git a/drivers/net/ethernet/aurora/Kconfig 
> b/drivers/net/ethernet/aurora/Kconfig
> new file mode 100644
> index 000..a3c7106
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/Kconfig
> @@ -0,0 +1,20 @@
> +config NET_VENDOR_AURORA
> +   bool "Aurora VLSI devices"
> +   help
> + If you have a network (Ethernet) device belonging to this class,
> + say Y.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + questions about Aurora devices. If you say Y, you will be asked
> + for your specific device in the following questions.
> +
> +if NET_VENDOR_AURORA
> +
> +config AURORA_NB8800
> +   tristate "Aurora AU-NB8800 support"
> +   select PHYLIB
> +   help
> +Support for the AU-NB8800 gigabit Ethernet controller.
> +
> +endif
> diff --git a/drivers/net/ethernet/aurora/Makefile 
> b/drivers/net/ethernet/aurora/Makefile
> new file mode 100644
> index 000..6cb528a
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_AURORA_NB8800) += nb8800.o
> diff --git a/drivers/net/ethernet/aurora/nb8800.c 
> b/drivers/net/ethernet/aurora/nb8800.c
> new file mode 100644
> index 000..11cd389
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -0,0 +1,1530 @@
> +/*
> + * Copyright (C) 2015 Mans Rullgard 
> + *
> + * Mostly rewritten, based on driver from Sigma Designs.  Original
> + * copyright notice below.
> + *
> + *
> + * Driver for tangox SMP864x/SMP865x/SMP867x/SMP868x builtin Ethernet Mac.
> + *
> + * Copyright (C) 2005 Maxime Bizon 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> 

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Andy Shevchenko  writes:

>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>> +   u32 mask, u32 val)
>> +{
>> +   u32 old = nb8800_readb(priv, reg);
>> +   u32 new = (old & ~mask) | val;
>
> Shoudn't be "… | (val & mask);" ?

No, it's meant to replace the bits in mask with the corresponding bits
from val.

> + empty line?
>
>> +   if (new != old)
>> +   nb8800_writeb(priv, reg, new);
>> +}
>> +

[...]

>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>
> unsigned int i ?
> len as well?

Does it matter?  The values are nowhere near overflowing signed int.


[...]

>> +   /* If a packet arrived after we last checked but
>> +* before writing RX_ITR, the interrupt will be
>> +* delayed, so we retrieve it now. */
>
> Block comments usually
> /*
>  * text
>  */

Documentation/CodingStyle says net/ and drivers/net/ are special, though
currently a mix of styles can be found.  Personally, I don't
particularly care.

> Can be longer lines?

Still won't fit on two lines.

>> +   if (priv->rx_descs[next].report)
>> +   goto again;
>> +
>> +   napi_complete_done(napi, work);
>> +   }
>> +
>> +   return work;
>> +}
>> +
>> +static void nb8800_tx_dma_start(struct net_device *dev)
>> +{
>> +   struct nb8800_priv *priv = netdev_priv(dev);
>> +   struct nb8800_tx_buf *txb;
>> +   u32 txc_cr;
>> +
>> +   txb = >tx_bufs[priv->tx_queue];
>> +   if (!txb->ready)
>> +   return;
>> +
>> +   txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
>> +   if (txc_cr & TCR_EN)
>> +   return;
>> +
>> +   nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> +   wmb();  /* ensure desc addr is written before starting DMA */
>
> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?

Possibly.

>> +   nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
>> +
>> +   priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
>> +}

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Andy Shevchenko]

On Wed, Nov 11, 2015 at 1:07 AM, Måns Rullgård  wrote:
> Andy Shevchenko  writes:

> +   nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> +   wmb();  /* ensure desc addr is written before starting 
> DMA */

 Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps 
 mmiowb() ?
>>>
>>> Possibly.
>>
>> Standalone wmb() doesn't make sense.
>
> It does if you need to enforce ordering between normal and I/O memory.
> In fact, since the descriptor is filled in using normal memory accesses,
> my understanding is that mmiowb() would be insufficient here.  The
> comment could be improved, however.

Can you then explain what exactly you are assured against in all cases
where you are using wmb()s? It seems I don't recognize this part in
some excerpts.


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[David Miller]

From: Måns Rullgård 
Date: Wed, 11 Nov 2015 00:40:09 +

> When the DMA complete interrupt arrives, the next chain should be
> kicked off as quickly as possible, and I don't see why that would
> benefit from being done in napi context.

NAPI isn't about low latency, it's about fairness and interrupt
mitigation.

You probably don't even realize that all of the TX SKB freeing you do
in the hardware interrupt handler end up being actually processed by a
scheduled software interrupt anyways.

So you are gaining almost nothing by not doing TX completion in NAPI
context, whereas by doing so you would be gaining a lot including
more simplified locking or even the ability to do no locking at all.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Måns Rullgård]

Francois Romieu  writes:

> Mans Rullgard  :
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c 
>> b/drivers/net/ethernet/aurora/nb8800.c
>> new file mode 100644
>> index 000..11cd389
>> --- /dev/null
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
> [...]
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
> [...]
>> +
>> +netdev_sent_queue(dev, skb->len);
>> +
>> +smp_mb__before_spinlock();
>> +spin_lock_irqsave(>tx_lock, flags);
>
> At some point you may consider performing both Tx and Rx from napi context
> and thus replacing priv->tx_lock with netif_tx_lock.

That lock is to synchronise the DMA start between nb8800_xmit() and the
interrupt handler.  When the DMA complete interrupt arrives, the next
chain should be kicked off as quickly as possible, and I don't see why
that would benefit from being done in napi context.

>> +
>> +if (!skb->xmit_more) {
>> +priv->tx_chain->ready = true;
>> +priv->tx_chain = NULL;
>> +nb8800_tx_dma_start(dev);
>> +}
>> +
>> +spin_unlock_irqrestore(>tx_lock, flags);
>> +
>> +priv->tx_next = next;
>
> Are there strong reasons why nb8800_tx_done could not kick between
> spin_unlock_irqrestore and the non-local update of priv->tx_next ?

Good catch.  priv->tx_next wasn't accessed elsewhere in an earlier
version, and I forgot to fix that.  nb8800_tx_done() makes sure the DMA
has actually finished, so priv->tx_next should be updated before
starting the DMA rather than after.  The check against tx_next in
nb8800_tx_done() is only to put some limit on the loop and to avoid
confusion when nb8800_dma_stop() does it's dance.  There should be no
need for more synchronisation here than what the already present memory
barriers provide.

> [...]
>> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
>> +{
>> +struct net_device *dev = dev_id;
>> +struct nb8800_priv *priv = netdev_priv(dev);
>> +u32 val;
>> +
>> +/* tx interrupt */
>> +val = nb8800_readl(priv, NB8800_TXC_SR);
>> +if (val) {
> [...]
>> +}
>> +
>> +/* rx interrupt */
>> +val = nb8800_readl(priv, NB8800_RXC_SR);
>> +if (val) {
> [...]
>> +}
>> +
>> +return IRQ_HANDLED;
>
> Returning IRQ_HANDLED is fine if one of those hold:
> 1. you're sure that at least one of the "if" branch is used
> 2. you'll be able to quickly figure out what's happening whenever 1. stops
>being true.

You're right, better to check that the device really did have something
to say.

-- 
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html