Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

On Mon, Nov 13, 2017 at 11:54:14PM +0900, David Miller wrote:
> From: Simon Guinot <simon.gui...@sequanux.org>
> Date: Mon, 13 Nov 2017 15:51:15 +0100
> 
> > IIUC the driver stops the queue if a threshold of 316 Tx descriptors is
> > reached (default and worst value).
> 
> That's a lot of latency.

OK, then I'll keep the "tx_pending > 255" flushing condition. But note
there is no other software mechanism to limit the Tx latency inside the
mvneta driver. Should we add something ? And is that not rather the job
of the network stack to keep track of the latency and to limit the txq
size ?


signature.asc
Description: PGP signature

[PATCH v2] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

The mvneta controller provides a 8-bit register to update the pending
Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
added at once. In the current code the mvneta_txq_pend_desc_add function
assumes the caller takes care of this limit. But it is not the case. In
some situations (xmit_more flag), more than 255 descriptors are added.
When this happens, the Tx descriptor counter register is updated with a
wrong value, which breaks the whole Tx queue management.

This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
function to process more than 255 Tx descriptors.

Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
Cc: sta...@vger.kernel.org # 4.11+
Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

Changes for v2:

- Use do {} while instead of while {}.
- Keep "txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK" as a flushing
  condition for the Tx queue.

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 64a04975bcf8..bc93b69cfd1e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port 
*pp,
 {
u32 val;
 
-   /* Only 255 descriptors can be added at once ; Assume caller
-* process TX desriptors in quanta less than 256
-*/
-   val = pend_desc + txq->pending;
-   mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+   pend_desc += txq->pending;
+
+   /* Only 255 Tx descriptors can be added at once */
+   do {
+   val = min(pend_desc, 255);
+   mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+   pend_desc -= val;
+   } while (pend_desc > 0);
txq->pending = 0;
 }
 
-- 
2.15.0

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

On Sat, Nov 11, 2017 at 06:45:04PM +0900, David Miller wrote:
> From: Simon Guinot <simon.gui...@sequanux.org>
> Date: Wed,  8 Nov 2017 17:58:35 +0100
> 
> > @@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct 
> > net_device *dev)
> > if (txq->count >= txq->tx_stop_threshold)
> > netif_tx_stop_queue(nq);
> >  
> > -   if (!skb->xmit_more || netif_xmit_stopped(nq) ||
> > -   txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
> > +   if (!skb->xmit_more || netif_xmit_stopped(nq))
> > mvneta_txq_pend_desc_add(pp, txq, frags);
> > else
> > txq->pending += frags;
> 
> As David Laight said, you should not allow unlimited amounts of
> ->xmit_more frames to be queued without a TX doorbell update.
> 
> Therefore, please keep some kind of limit here otherwise latency
> will spike in some circumstances.

Hi David,

IIUC the driver stops the queue if a threshold of 316 Tx descriptors is
reached (default and worst value).

Is that a "good enough" kind of limit ? Or do you want me to keep the
condition above ?

Simon


signature.asc
Description: PGP signature

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

Hi Sven and Andreas,

Please, can you try with this patch ?

Thanks.

Simon

On Wed, Nov 08, 2017 at 05:58:35PM +0100, Simon Guinot wrote:
> The mvneta controller provides a 8-bit register to update the pending
> Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
> added at once. In the current code the mvneta_txq_pend_desc_add function
> assumes the caller takes care of this limit. But it is not the case. In
> some situations (xmit_more flag), more than 255 descriptors are added.
> When this happens, the Tx descriptor counter register is updated with a
> wrong value, which breaks the whole Tx queue management.
> 
> This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
> function to process more than 255 Tx descriptors.
> 
> Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
> Cc: sta...@vger.kernel.org # 4.11+
> Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 64a04975bcf8..027c08ce4e5d 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port 
> *pp,
>  {
>   u32 val;
>  
> - /* Only 255 descriptors can be added at once ; Assume caller
> -  * process TX desriptors in quanta less than 256
> -  */
> - val = pend_desc + txq->pending;
> - mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
> + pend_desc += txq->pending;
> +
> + /* Only 255 Tx descriptors can be added at once */
> + while (pend_desc > 0) {
> + val = min(pend_desc, 255);
> + mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
> + pend_desc -= val;
> + }
>   txq->pending = 0;
>  }
>  
> @@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct 
> net_device *dev)
>   if (txq->count >= txq->tx_stop_threshold)
>   netif_tx_stop_queue(nq);
>  
> - if (!skb->xmit_more || netif_xmit_stopped(nq) ||
> - txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
> + if (!skb->xmit_more || netif_xmit_stopped(nq))
>   mvneta_txq_pend_desc_add(pp, txq, frags);
>   else
>   txq->pending += frags;
> -- 
> 2.9.3


signature.asc
Description: PGP signature

[PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

The mvneta controller provides a 8-bit register to update the pending
Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
added at once. In the current code the mvneta_txq_pend_desc_add function
assumes the caller takes care of this limit. But it is not the case. In
some situations (xmit_more flag), more than 255 descriptors are added.
When this happens, the Tx descriptor counter register is updated with a
wrong value, which breaks the whole Tx queue management.

This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
function to process more than 255 Tx descriptors.

Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
Cc: sta...@vger.kernel.org # 4.11+
Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 64a04975bcf8..027c08ce4e5d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port 
*pp,
 {
u32 val;
 
-   /* Only 255 descriptors can be added at once ; Assume caller
-* process TX desriptors in quanta less than 256
-*/
-   val = pend_desc + txq->pending;
-   mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+   pend_desc += txq->pending;
+
+   /* Only 255 Tx descriptors can be added at once */
+   while (pend_desc > 0) {
+   val = min(pend_desc, 255);
+   mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+   pend_desc -= val;
+   }
txq->pending = 0;
 }
 
@@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct 
net_device *dev)
if (txq->count >= txq->tx_stop_threshold)
netif_tx_stop_queue(nq);
 
-   if (!skb->xmit_more || netif_xmit_stopped(nq) ||
-   txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
+   if (!skb->xmit_more || netif_xmit_stopped(nq))
mvneta_txq_pend_desc_add(pp, txq, frags);
else
txq->pending += frags;
-- 
2.9.3

Re: Problems with mvneta

[Simon Guinot]

On Tue, Oct 31, 2017 at 03:27:40PM +0100, Thomas Petazzoni wrote:
> Hello,

Hi Thomas,

> 
> Let's add Simon Guinot in the loop.
> 
> On Tue, 31 Oct 2017 15:23:22 +0100, Sven Müller wrote:
> > After quite a long time of trying to reproduce the issue without any 
> > success I got 3 network crashes today. And all errors occurred with a 
> > kernel including the patch: 
> > 
> > 2a90f7e1d5d04e4f1060268e0b55a2c702bbd67a
> > 
> > At least according to Andreas' and my problems we can exclude the 6ad2 
> > patch as the source of the errors. 
> 
> Simon, 2a90f7e1d5d04e4f1060268e0b55a2c702bbd67a is your commit, adding
> xmit_more support, and a number of people are reporting stability
> issues with this patch applied.

I wrote an earlier version of this patch. But I think this commit has
been modified by the submitter Marcin Wojtas because I don't remember
anything about the maximum number of descriptors allowed to be flush.

> 
> Do you think you will have some time to look into this ?

No I don't have time to look into that.

But after a quick look, I wonder what is happening if
"txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK" ? Because IIUC
mvneta_txq_pend_desc_add() is called anyway. And according to the
comment inside the function, it assumes there is less than 255
descriptors to send... It looks suspect.

Simon


signature.asc
Description: PGP signature

Re: [PATCH v9 0/8] thunderbolt: Introducing Thunderbolt(TM) Networking

[Simon Guinot]

On Fri, Nov 18, 2016 at 12:20:07PM +0100, Simon Guinot wrote:
> On Fri, Nov 18, 2016 at 08:48:36AM +, Levy, Amir (Jer) wrote:
> > On Tue, Nov 15 2016, 12:59 PM, Simon Guinot wrote:
> > > On Wed, Nov 09, 2016 at 03:42:53PM +, Levy, Amir (Jer) wrote:
> > > > On Wed, Nov 9 2016, 04:36 PM, Simon Guinot wrote:
> > > > > Hi Amir,
> > > > >
> > > > > I have an ASUS "All Series/Z87-DELUXE/QUAD" motherboard with a 
> > > > > Thunderbolt 2 "Falcon Ridge" chipset (device ID 156d).
> > > > >
> > > > > Is the thunderbolt-icm driver supposed to work with this chipset ?
> > > > >
> > > >
> > > > Yes, the thunderbolt-icm supports Falcon Ridge, device ID 156c.
> > > > 156d is the bridge -
> > > > http://lxr.free-electrons.com/source/include/linux/pci_ids.h#L2619
> > > >
> > > > > I have installed both a 4.8.6 Linux kernel (patched with your v9
> > > > > series) and the thunderbolt-software-daemon (27 october release) 
> > > > > inside a Debian system (Jessie).
> > > > >
> > > > > If I connect the ASUS motherboard with a MacBook Pro (Thunderbolt 
> > > > > 2, device ID 156c), I can see that the thunderbolt-icm driver is 
> > > > > loaded and that the thunderbolt-software-daemon is well started. 
> > > > > But the Ethernet interface is not created.
> > > > >
> > > > > I have attached to this email the syslog file. There is the logs 
> > > > > from both the kernel and the daemon inside. Note that the daemon 
> > > > > logs are everything but clear about what could be the issue. Maybe 
> > > > > I missed some kind of configuration ? But I failed to find any 
> > > > > valuable information about configuring the driver and/or the 
> > > > > daemon in
> > > the various documentation files.
> > > > >
> > > > > Please, can you provide some guidance ? I'd really like to test 
> > > > > your patch series.
> > > >
> > > > First, thank you very much for willing to test it.
> > > > Thunderbolt Networking support was added during Falcon Ridge, in the
> > > latest FR images.
> > > > Do you know which Thunderbolt image version you have on your system?
> > > > Currently I submitted only Thunderbolt Networking feature in Linux, 
> > > > and we plan to add more features like reading the image version and
> > > updating the image.
> > > > If you don't know the image version, the only thing I can suggest is 
> > > > to load windows, install thunderbolt SW and check in the Thunderbolt
> > > application the image version.
> > > > To know if image update is needed, you can check - 
> > > > https://thunderbolttechnology.net/updates
> > > 
> > > Hi Amir,
> > > 
> > > From the Windows Thunderbolt software, I can read 13.00 for the 
> > > firmware version. And from https://thunderbolttechnology.net/updates, 
> > > I can see that there is no update available for my ASUS motherboard.
> > > 
> > > Am I good to go ?
> > > 
> > 
> > Thunderbolt Networking is supported on both Thunderbolt(tm) 2 and 
> > Thunderbolt(tm) 3 systems.  
> > Thunderbolt 2 systems must have updated NVM (version 25 or later) in order 
> > for the functionality to work properly.  
> > If the system does not have the update, please contact the OEM directly for 
> > an updated NVM.  
> > For best functionality and support, Intel recommends using Thunderbolt 3 
> > systems for all validation and testing.
> 
> Maybe it is worth mentioning in the documentation and/or in the Kconfig
> help message that a minimal firmware version is needed for Thunderbolt 2
> controllers.
> 
> It would have saved some time for me :)
> 
> > 
> > > BTW, it is quite a shame that the Thunderbolt firmware version can't 
> > > be read from Linux.
> > > 
> > 
> > This is WIP, once this patch will be upstream, we will be able to focus more
> > on aligning Linux with the Thunderbolt features that we have for windows.
> 
> Well, I rather see the firmware identification and update as basic
> features on the top of which ones you can build a driver. For example in
> this case this would allow the ICM driver and/or the userland daemon to
> exit with a useful error message rather than just not working without any
> explanation.
> 
> Next week I'll t

Re: [PATCH v9 0/8] thunderbolt: Introducing Thunderbolt(TM) Networking

[Simon Guinot]

On Fri, Nov 18, 2016 at 08:48:36AM +, Levy, Amir (Jer) wrote:
> On Tue, Nov 15 2016, 12:59 PM, Simon Guinot wrote:
> > On Wed, Nov 09, 2016 at 03:42:53PM +, Levy, Amir (Jer) wrote:
> > > On Wed, Nov 9 2016, 04:36 PM, Simon Guinot wrote:
> > > > Hi Amir,
> > > >
> > > > I have an ASUS "All Series/Z87-DELUXE/QUAD" motherboard with a 
> > > > Thunderbolt 2 "Falcon Ridge" chipset (device ID 156d).
> > > >
> > > > Is the thunderbolt-icm driver supposed to work with this chipset ?
> > > >
> > >
> > > Yes, the thunderbolt-icm supports Falcon Ridge, device ID 156c.
> > > 156d is the bridge -
> > > http://lxr.free-electrons.com/source/include/linux/pci_ids.h#L2619
> > >
> > > > I have installed both a 4.8.6 Linux kernel (patched with your v9
> > > > series) and the thunderbolt-software-daemon (27 october release) 
> > > > inside a Debian system (Jessie).
> > > >
> > > > If I connect the ASUS motherboard with a MacBook Pro (Thunderbolt 
> > > > 2, device ID 156c), I can see that the thunderbolt-icm driver is 
> > > > loaded and that the thunderbolt-software-daemon is well started. 
> > > > But the Ethernet interface is not created.
> > > >
> > > > I have attached to this email the syslog file. There is the logs 
> > > > from both the kernel and the daemon inside. Note that the daemon 
> > > > logs are everything but clear about what could be the issue. Maybe 
> > > > I missed some kind of configuration ? But I failed to find any 
> > > > valuable information about configuring the driver and/or the 
> > > > daemon in
> > the various documentation files.
> > > >
> > > > Please, can you provide some guidance ? I'd really like to test 
> > > > your patch series.
> > >
> > > First, thank you very much for willing to test it.
> > > Thunderbolt Networking support was added during Falcon Ridge, in the
> > latest FR images.
> > > Do you know which Thunderbolt image version you have on your system?
> > > Currently I submitted only Thunderbolt Networking feature in Linux, 
> > > and we plan to add more features like reading the image version and
> > updating the image.
> > > If you don't know the image version, the only thing I can suggest is 
> > > to load windows, install thunderbolt SW and check in the Thunderbolt
> > application the image version.
> > > To know if image update is needed, you can check - 
> > > https://thunderbolttechnology.net/updates
> > 
> > Hi Amir,
> > 
> > From the Windows Thunderbolt software, I can read 13.00 for the 
> > firmware version. And from https://thunderbolttechnology.net/updates, 
> > I can see that there is no update available for my ASUS motherboard.
> > 
> > Am I good to go ?
> > 
> 
> Thunderbolt Networking is supported on both Thunderbolt(tm) 2 and 
> Thunderbolt(tm) 3 systems.  
> Thunderbolt 2 systems must have updated NVM (version 25 or later) in order 
> for the functionality to work properly.  
> If the system does not have the update, please contact the OEM directly for 
> an updated NVM.  
> For best functionality and support, Intel recommends using Thunderbolt 3 
> systems for all validation and testing.

Maybe it is worth mentioning in the documentation and/or in the Kconfig
help message that a minimal firmware version is needed for Thunderbolt 2
controllers.

It would have saved some time for me :)

> 
> > BTW, it is quite a shame that the Thunderbolt firmware version can't 
> > be read from Linux.
> > 
> 
> This is WIP, once this patch will be upstream, we will be able to focus more
> on aligning Linux with the Thunderbolt features that we have for windows.

Well, I rather see the firmware identification and update as basic
features on the top of which ones you can build a driver. For example in
this case this would allow the ICM driver and/or the userland daemon to
exit with a useful error message rather than just not working without any
explanation.

Next week I'll try the driver with a Thunderbolt 3 controller.

Thanks for your help!

Simon


signature.asc
Description: PGP signature

Re: [PATCH v9 0/8] thunderbolt: Introducing Thunderbolt(TM) Networking

[Simon Guinot]

On Wed, Nov 09, 2016 at 03:42:53PM +, Levy, Amir (Jer) wrote:
> On Wed, Nov 9 2016, 04:36 PM, Simon Guinot wrote:
> > On Wed, Nov 09, 2016 at 04:20:00PM +0200, Amir Levy wrote:
> > > This driver enables Thunderbolt Networking on non-Apple platforms 
> > > running Linux.
> > >
> > > Thunderbolt Networking provides peer-to-peer connections to transfer 
> > > files between computers, perform PC migrations, and/or set up small 
> > > workgroups with shared storage.
> > >
> > > This is a virtual connection that emulates an Ethernet adapter that 
> > > enables Ethernet networking with the benefit of Thunderbolt 
> > > superfast medium capability.
> > >
> > > Thunderbolt Networking enables two hosts and several devices that 
> > > have a Thunderbolt controller to be connected together in a linear 
> > > (Daisy
> > > chain) series from a single port.
> > >
> > > Thunderbolt Networking for Linux is compatible with Thunderbolt 
> > > Networking on systems running macOS or Windows and also supports 
> > > Thunderbolt generation 2 and 3 controllers.
> > >
> > > Note that all pre-existing Thunderbolt generation 3 features, such 
> > > as USB, Display and other Thunderbolt device connectivity will 
> > > continue to function exactly as they did prior to enabling Thunderbolt 
> > > Networking.
> > >
> > > Code and Software Specifications:
> > > This kernel code creates a virtual ethernet device for computer to 
> > > computer communication over a Thunderbolt cable.
> > > The new driver is a separate driver to the existing Thunderbolt driver.
> > > It is designed to work on systems running Linux that interface with 
> > > Intel Connection Manager (ICM) firmware based Thunderbolt 
> > > controllers that support Thunderbolt Networking.
> > > The kernel code operates in coordination with the Thunderbolt user- 
> > > space daemon to implement full Thunderbolt networking functionality.
> > >
> > > Hardware Specifications:
> > > Thunderbolt Hardware specs have not yet been published but are used 
> > > where necessary for register definitions.
> > 
> > Hi Amir,
> > 
> > I have an ASUS "All Series/Z87-DELUXE/QUAD" motherboard with a 
> > Thunderbolt 2 "Falcon Ridge" chipset (device ID 156d).
> > 
> > Is the thunderbolt-icm driver supposed to work with this chipset ?
> > 
> 
> Yes, the thunderbolt-icm supports Falcon Ridge, device ID 156c.
> 156d is the bridge - 
> http://lxr.free-electrons.com/source/include/linux/pci_ids.h#L2619
> 
> > I have installed both a 4.8.6 Linux kernel (patched with your v9
> > series) and the thunderbolt-software-daemon (27 october release) 
> > inside a Debian system (Jessie).
> > 
> > If I connect the ASUS motherboard with a MacBook Pro (Thunderbolt 2, 
> > device ID 156c), I can see that the thunderbolt-icm driver is loaded 
> > and that the thunderbolt-software-daemon is well started. But the 
> > Ethernet interface is not created.
> > 
> > I have attached to this email the syslog file. There is the logs from 
> > both the kernel and the daemon inside. Note that the daemon logs are 
> > everything but clear about what could be the issue. Maybe I missed 
> > some kind of configuration ? But I failed to find any valuable 
> > information about configuring the driver and/or the daemon in the various 
> > documentation files.
> > 
> > Please, can you provide some guidance ? I'd really like to test your 
> > patch series.
> 
> First, thank you very much for willing to test it.
> Thunderbolt Networking support was added during Falcon Ridge, in the latest 
> FR images.
> Do you know which Thunderbolt image version you have on your system?
> Currently I submitted only Thunderbolt Networking feature in Linux, and we 
> plan to add
> more features like reading the image version and updating the image.
> If you don't know the image version, the only thing I can suggest is to load 
> windows, install thunderbolt SW
> and check in the Thunderbolt application the image version.
> To know if image update is needed, you can check - 
> https://thunderbolttechnology.net/updates

Hi Amir,

From the Windows Thunderbolt software, I can read 13.00 for the firmware
version. And from https://thunderbolttechnology.net/updates, I can see
that there is no update available for my ASUS motherboard.

Am I good to go ?

BTW, it is quite a shame that the Thunderbolt firmware version can't be
read from Linux.

Simon


signature.asc
Description: PGP signature

Re: [PATCH 06/13] net: mvneta: enable mixed egress processing using HR timer

[Simon Guinot]

Hi Marcin,

On Sun, Nov 22, 2015 at 08:53:52AM +0100, Marcin Wojtas wrote:
> Mixed approach allows using higher interrupt threshold (increased back to
> 15 packets), useful in high throughput. In case of small amount of data
> or very short TX queues HR timer ensures releasing buffers with small
> latency.
> 
> Along with existing tx_done processing by coalescing interrupts this
> commit enables triggering HR timer each time the packets are sent.
> Time threshold can also be configured, using ethtool.
> 
> Signed-off-by: Marcin Wojtas <m...@semihalf.com>
> Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 89 
> +--
>  1 file changed, 85 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 9c9e858..f5acaf6 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 

ktime.h is already included by hrtimer.h.

>  #include 
>  #include 
>  #include 
> @@ -226,7 +228,8 @@
>  /* Various constants */
>  
>  /* Coalescing */
> -#define MVNETA_TXDONE_COAL_PKTS  1
> +#define MVNETA_TXDONE_COAL_PKTS  15
> +#define MVNETA_TXDONE_COAL_USEC  100

Maybe we should keep the default configuration and let the user choose
to enable (or not) this feature ?

>  #define MVNETA_RX_COAL_PKTS  32
>  #define MVNETA_RX_COAL_USEC  100
>  
> @@ -356,6 +359,11 @@ struct mvneta_port {
>   struct net_device *dev;
>   struct notifier_block cpu_notifier;
>  
> + /* Egress finalization */
> + struct tasklet_struct tx_done_tasklet;
> + struct hrtimer tx_done_timer;
> + bool timer_scheduled;

I think we could use hrtimer_is_queued() instead of introducing a new
variable.

[...]

Simon


signature.asc
Description: Digital signature

Re: [PATCH v2 net 4/6] net: mvneta: fix error path for building skb

[Simon Guinot]

On Thu, Nov 26, 2015 at 07:08:11PM +0100, Marcin Wojtas wrote:
> In the actual RX processing, there is same error path for both descriptor
> ring refilling and building skb fails. This is not correct, because after
> successful refill, the ring is already updated with newly allocated
> buffer. Then, in case of build_skb() fail, hitherto code left the original
> buffer unmapped.
> 
> This patch fixes above situation by swapping error check of skb build with
> DMA-unmap of original buffer.
> 
> Signed-off-by: Marcin Wojtas <m...@semihalf.com>
> Cc: <sta...@vger.kernel.org> # v4.2+
> Fixes a84e32894191 ("net: mvneta: fix refilling for Rx DMA buffers")
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

Acked-by: Simon Guinot <simon.gui...@sequanux.org>

Thanks,

Simon

> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 0c3d923..62cf971 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1580,12 +1580,16 @@ static int mvneta_rx(struct mvneta_port *pp, int 
> rx_todo,
>   }
>  
>   skb = build_skb(data, pp->frag_size > PAGE_SIZE ? 0 : 
> pp->frag_size);
> - if (!skb)
> - goto err_drop_frame;
>  
> + /* After refill old buffer has to be unmapped regardless
> +  * the skb is successfully built or not.
> +  */
>   dma_unmap_single(dev->dev.parent, phys_addr,
>MVNETA_RX_BUF_SIZE(pp->pkt_size), 
> DMA_FROM_DEVICE);
>  
> + if (!skb)
> + goto err_drop_frame;
> +
>   rcvd_pkts++;
>   rcvd_bytes += rx_bytes;
>  
> -- 
> 1.8.3.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


signature.asc
Description: Digital signature

Re: [PATCH v2] net: mvneta: fix refilling for Rx DMA buffers

[Simon Guinot]

On Mon, Sep 14, 2015 at 01:22:12PM -0700, Oren Laskin wrote:
> I had to undo this change on my Amada 370 based board.  It was causing
> corrupt data to make it through on large downloads.  I'm using wget to get
> the same 30MB file many times and the SHA would occasionally be different.
> I tracked it down to this commit.  In it, I would find on the order of a
> few hundred bytes to simply be wrong data.

Hi Oren,

Well, you are right. This patch actually introduces a pretty serious
bug. I still don't understand how I missed that in a first place and
I apologize for the inconvenience.

Anyway, I am about to send a patch fixing the issue. Please, can you
test it ?

Thanks in advance,

Simon

> On Tue, Jul 21, 2015 at 12:30 AM, David Miller <da...@davemloft.net> wrote:
> 
> > From: Simon Guinot <simon.gui...@sequanux.org>
> > Date: Sun, 19 Jul 2015 13:00:53 +0200
> >
> > > With the actual code, if a memory allocation error happens while
> > > refilling a Rx descriptor, then the original Rx buffer is both passed
> > > to the networking stack (in a SKB) and let in the Rx ring. This leads
> > > to various kernel oops and crashes.
> > >
> > > As a fix, this patch moves Rx descriptor refilling ahead of building
> > > SKB with the associated Rx buffer. In case of a memory allocation
> > > failure, data is dropped and the original DMA buffer is put back into
> > > the Rx ring.
> > >
> > > Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
> > network unit")
> > > Cc: <sta...@vger.kernel.org> # v3.8+
> > > Tested-by: Yoann Sculo <yo...@sculo.fr>
> >
> > Applied, thanks.
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >


signature.asc
Description: Digital signature

[PATCH] net: mvneta: fix DMA buffer unmapping in mvneta_rx()

[Simon Guinot]

This patch fixes a regression introduced by the commit a84e32894191
("net: mvneta: fix refilling for Rx DMA buffers"). Due to this commit
the newly allocated Rx buffers are DMA-unmapped in place of those passed
to the networking stack. Obviously, this causes data corruptions.

This patch fixes the issue by ensuring that the right Rx buffers are
DMA-unmapped.

Reported-by: Oren Laskin <o...@igneous.io>
Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
Fixes: a84e32894191 ("net: mvneta: fix refilling for Rx DMA buffers")
Cc: <sta...@vger.kernel.org> # v3.8+
---
 drivers/net/ethernet/marvell/mvneta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 62e48bc0cb23..03e052a1cf5a 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1479,6 +1479,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
struct sk_buff *skb;
unsigned char *data;
+   dma_addr_t phys_addr;
u32 rx_status;
int rx_bytes, err;
 
@@ -1486,6 +1487,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (unsigned char *)rx_desc->buf_cookie;
+   phys_addr = rx_desc->buf_phys_addr;
 
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
(rx_status & MVNETA_RXD_ERR_SUMMARY)) {
@@ -1534,7 +1536,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
if (!skb)
goto err_drop_frame;
 
-   dma_unmap_single(dev->dev.parent, rx_desc->buf_phys_addr,
+   dma_unmap_single(dev->dev.parent, phys_addr,
 MVNETA_RX_BUF_SIZE(pp->pkt_size), 
DMA_FROM_DEVICE);
 
rcvd_pkts++;
-- 
2.1.4

--
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 v2] net: mvneta: fix refilling for Rx DMA buffers

[Simon Guinot]

Hi Oren,

On Mon, Sep 14, 2015 at 01:22:12PM -0700, Oren Laskin wrote:
> I had to undo this change on my Amada 370 based board.  It was causing
> corrupt data to make it through on large downloads.  I'm using wget to get
> the same 30MB file many times and the SHA would occasionally be different.

During your tests, can you see some "Linux processing - Can't refill"
messages along with the data corruptions ?

> I tracked it down to this commit.  In it, I would find on the order of a
> few hundred bytes to simply be wrong data.

I am little bit surprised here. For me, this patch is very simple and
does the exact opposite. It does fix kernel crashes and data corruptions
in case of refilling errors. This can happen for example if you run
large data transfers with jumbo frames enabled...

But anyway, I'll try to reproduce the issue tomorrow. I only have to
wget the same file (size 30MB) in a loop and to check its md5sum ?
That's it ? And how long should I wait for the error ?

Thanks,

Simon

> 
> Thanks,
> 
> Oren
> 
> On Tue, Jul 21, 2015 at 12:30 AM, David Miller <da...@davemloft.net> wrote:
> 
> > From: Simon Guinot <simon.gui...@sequanux.org>
> > Date: Sun, 19 Jul 2015 13:00:53 +0200
> >
> > > With the actual code, if a memory allocation error happens while
> > > refilling a Rx descriptor, then the original Rx buffer is both passed
> > > to the networking stack (in a SKB) and let in the Rx ring. This leads
> > > to various kernel oops and crashes.
> > >
> > > As a fix, this patch moves Rx descriptor refilling ahead of building
> > > SKB with the associated Rx buffer. In case of a memory allocation
> > > failure, data is dropped and the original DMA buffer is put back into
> > > the Rx ring.
> > >
> > > Signed-off-by: Simon Guinot <simon.gui...@sequanux.org>
> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
> > network unit")
> > > Cc: <sta...@vger.kernel.org> # v3.8+
> > > Tested-by: Yoann Sculo <yo...@sculo.fr>
> >
> > Applied, thanks.
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >


signature.asc
Description: Digital signature

[PATCH v2] net: mvneta: fix refilling for Rx DMA buffers

[Simon Guinot]

With the actual code, if a memory allocation error happens while
refilling a Rx descriptor, then the original Rx buffer is both passed
to the networking stack (in a SKB) and let in the Rx ring. This leads
to various kernel oops and crashes.

As a fix, this patch moves Rx descriptor refilling ahead of building
SKB with the associated Rx buffer. In case of a memory allocation
failure, data is dropped and the original DMA buffer is put back into
the Rx ring.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
unit)
Cc: sta...@vger.kernel.org # v3.8+
Tested-by: Yoann Sculo yo...@sculo.fr
---
 drivers/net/ethernet/marvell/mvneta.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

Changes for v2:
- Update commit message.

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..ac3da11e63a0 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1455,7 +1455,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 struct mvneta_rx_queue *rxq)
 {
struct net_device *dev = pp-dev;
-   int rx_done, rx_filled;
+   int rx_done;
u32 rcvd_pkts = 0;
u32 rcvd_bytes = 0;
 
@@ -1466,7 +1466,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
rx_todo = rx_done;
 
rx_done = 0;
-   rx_filled = 0;
 
/* Fairness NAPI loop */
while (rx_done  rx_todo) {
@@ -1477,7 +1476,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
int rx_bytes, err;
 
rx_done++;
-   rx_filled++;
rx_status = rx_desc-status;
rx_bytes = rx_desc-data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (unsigned char *)rx_desc-buf_cookie;
@@ -1517,6 +1515,14 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
continue;
}
 
+   /* Refill processing */
+   err = mvneta_rx_refill(pp, rx_desc);
+   if (err) {
+   netdev_err(dev, Linux processing - Can't refill\n);
+   rxq-missed++;
+   goto err_drop_frame;
+   }
+
skb = build_skb(data, pp-frag_size  PAGE_SIZE ? 0 : 
pp-frag_size);
if (!skb)
goto err_drop_frame;
@@ -1536,14 +1542,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
mvneta_rx_csum(pp, rx_status, skb);
 
napi_gro_receive(pp-napi, skb);
-
-   /* Refill processing */
-   err = mvneta_rx_refill(pp, rx_desc);
-   if (err) {
-   netdev_err(dev, Linux processing - Can't refill\n);
-   rxq-missed++;
-   rx_filled--;
-   }
}
 
if (rcvd_pkts) {
@@ -1556,7 +1554,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
}
 
/* Update rxq management counters */
-   mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_filled);
+   mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_done);
 
return rx_done;
 }
-- 
2.1.4

--
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] net: mvneta: fix refilling for Rx DMA buffers

[Simon Guinot]

On Wed, Jul 15, 2015 at 03:52:56PM -0700, David Miller wrote:
 From: Simon Guinot simon.gui...@sequanux.org
 Date: Mon, 13 Jul 2015 00:04:57 +0200
 
  On some Armada 370-based NAS, I have experimented kernel bugs and
  crashes when the mvneta Ethernet driver fails to refill Rx DMA buffers
  due to memory shortage.
  
  With the actual code, if the memory allocation fails while refilling a
  Rx DMA buffer, then the corresponding descriptor is let with the address
  of an unmapped DMA buffer already passed to the network stack. Since the
  driver still increments the non-occupied counter for Rx descriptor (if a
  next packet is handled successfully), then the Ethernet controller is
  allowed to reuse the unfilled Rx descriptor...
  
  As a fix, this patch first refills a Rx descriptor before handling the
  stored data and unmapping the associated Rx DMA buffer. Additionally the
  occupied and non-occupied counters for the Rx descriptors queues are now
  both updated with the rx_done value: the number of descriptors ready to
  be returned to the networking controller. Moreover note that there is no
  point in using different values for this counters because both the
  mvneta driver and the Ethernet controller are unable to handle holes in
  the Rx descriptors queues.
  
  Signed-off-by: Simon Guinot simon.gui...@sequanux.org
  Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
  unit)
  Cc: sta...@vger.kernel.org # v3.8+
  Tested-by: Yoann Sculo yo...@sculo.fr

Hi David,

 
 Failed memory allocations, are normal and if necessary kernel log
 messages will be triggered by the core memory allocator.  So there is
 zero reason to do anything other than bump the drop counter in your
 driver.

Sure.

 
 If I understand the rest of your logic, if the allocator fails, we
 will reuse the original SKB and place it back into the RX ring?
 Right?

Yes that's what does the patch.

Without this patch, in case of memory allocation error, the original
buffer is both passed to the networking stack in a SKB _and_ let in
the Rx ring. This leads to various kernel oops and crashes.

Here are the problems with the original code:

- Rx descriptor refilling is tried after passing the SKB to the
  networking stack.
- In case of refilling error, the buffer (passed in the SKB) is also
  let in the Rx ring (as an unmapped DMA buffer).
- And (always in case of refilling error), the non-occupied counter of
  the Rx queue (hardware register) is not incremented. The idea was
  maybe to prevent the networking controller to reuse the non-refilled
  descriptor. But since this counter is incremented as soon as a next
  descriptor is handled successfully, it is not correct.

The patch:

- Moves Rx descriptor refilling ahead of passing the SKB to the
  networking stack.
- places the buffer back into the Rx ring in case of refilling error.
- And updates correctly the non-occupied descriptors counter of the Rx
  queue.

Simon


signature.asc
Description: Digital signature

[PATCH] net: mvneta: fix refilling for Rx DMA buffers

[Simon Guinot]

On some Armada 370-based NAS, I have experimented kernel bugs and
crashes when the mvneta Ethernet driver fails to refill Rx DMA buffers
due to memory shortage.

With the actual code, if the memory allocation fails while refilling a
Rx DMA buffer, then the corresponding descriptor is let with the address
of an unmapped DMA buffer already passed to the network stack. Since the
driver still increments the non-occupied counter for Rx descriptor (if a
next packet is handled successfully), then the Ethernet controller is
allowed to reuse the unfilled Rx descriptor...

As a fix, this patch first refills a Rx descriptor before handling the
stored data and unmapping the associated Rx DMA buffer. Additionally the
occupied and non-occupied counters for the Rx descriptors queues are now
both updated with the rx_done value: the number of descriptors ready to
be returned to the networking controller. Moreover note that there is no
point in using different values for this counters because both the
mvneta driver and the Ethernet controller are unable to handle holes in
the Rx descriptors queues.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
unit)
Cc: sta...@vger.kernel.org # v3.8+
Tested-by: Yoann Sculo yo...@sculo.fr
---
 drivers/net/ethernet/marvell/mvneta.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..ac3da11e63a0 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1455,7 +1455,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
 struct mvneta_rx_queue *rxq)
 {
struct net_device *dev = pp-dev;
-   int rx_done, rx_filled;
+   int rx_done;
u32 rcvd_pkts = 0;
u32 rcvd_bytes = 0;
 
@@ -1466,7 +1466,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
rx_todo = rx_done;
 
rx_done = 0;
-   rx_filled = 0;
 
/* Fairness NAPI loop */
while (rx_done  rx_todo) {
@@ -1477,7 +1476,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
int rx_bytes, err;
 
rx_done++;
-   rx_filled++;
rx_status = rx_desc-status;
rx_bytes = rx_desc-data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
data = (unsigned char *)rx_desc-buf_cookie;
@@ -1517,6 +1515,14 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
continue;
}
 
+   /* Refill processing */
+   err = mvneta_rx_refill(pp, rx_desc);
+   if (err) {
+   netdev_err(dev, Linux processing - Can't refill\n);
+   rxq-missed++;
+   goto err_drop_frame;
+   }
+
skb = build_skb(data, pp-frag_size  PAGE_SIZE ? 0 : 
pp-frag_size);
if (!skb)
goto err_drop_frame;
@@ -1536,14 +1542,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
mvneta_rx_csum(pp, rx_status, skb);
 
napi_gro_receive(pp-napi, skb);
-
-   /* Refill processing */
-   err = mvneta_rx_refill(pp, rx_desc);
-   if (err) {
-   netdev_err(dev, Linux processing - Can't refill\n);
-   rxq-missed++;
-   rx_filled--;
-   }
}
 
if (rcvd_pkts) {
@@ -1556,7 +1554,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
}
 
/* Update rxq management counters */
-   mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_filled);
+   mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_done);
 
return rx_done;
 }
-- 
2.1.4

--
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 v3 1/3] net: mvneta: introduce compatible string marvell,armada-xp-neta

[Simon Guinot]

The mvneta driver supports the Ethernet IP found in the Armada 370, XP,
380 and 385 SoCs. Since at least one more hardware feature is available
for the Armada XP SoCs then a way to identify them is needed.

This patch introduces a new compatible string marvell,armada-xp-neta.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
unit)
Cc: sta...@vger.kernel.org # v3.8+
Acked-by: Gregory CLEMENT gregory.clem...@free-electrons.com
---
 Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +-
 drivers/net/ethernet/marvell/mvneta.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt 
b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 750d577e8083..f5a8ca29aff0 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -1,7 +1,7 @@
 * Marvell Armada 370 / Armada XP Ethernet Controller (NETA)
 
 Required properties:
-- compatible: should be marvell,armada-370-neta.
+- compatible: marvell,armada-370-neta or marvell,armada-xp-neta.
 - reg: address and length of the register set for the device.
 - interrupts: interrupt for the device
 - phy: See ethernet.txt file in the same directory.
diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..aceb977b104d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev)
 
 static const struct of_device_id mvneta_match[] = {
{ .compatible = marvell,armada-370-neta },
+   { .compatible = marvell,armada-xp-neta },
{ }
 };
 MODULE_DEVICE_TABLE(of, mvneta_match);
-- 
2.1.4

--
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 v3 2/3] ARM: mvebu: update Ethernet compatible string for Armada XP

[Simon Guinot]

This patch updates the Ethernet DT nodes for Armada XP SoCs with the
compatible string marvell,armada-xp-neta.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: 77916519cba3 (arm: mvebu: Armada XP MV78230 has only three Ethernet 
interfaces)
Cc: sta...@vger.kernel.org # v3.8+
Acked-by: Gregory CLEMENT gregory.clem...@free-electrons.com
---
 arch/arm/boot/dts/armada-370-xp.dtsi |  2 --
 arch/arm/boot/dts/armada-370.dtsi|  8 
 arch/arm/boot/dts/armada-xp-mv78260.dtsi |  2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |  2 +-
 arch/arm/boot/dts/armada-xp.dtsi | 10 +-
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi 
b/arch/arm/boot/dts/armada-370-xp.dtsi
index ec96f0b36346..06a2f2ae9d1e 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -270,7 +270,6 @@
};
 
eth0: ethernet@7 {
-   compatible = marvell,armada-370-neta;
reg = 0x7 0x4000;
interrupts = 8;
clocks = gateclk 4;
@@ -286,7 +285,6 @@
};
 
eth1: ethernet@74000 {
-   compatible = marvell,armada-370-neta;
reg = 0x74000 0x4000;
interrupts = 10;
clocks = gateclk 3;
diff --git a/arch/arm/boot/dts/armada-370.dtsi 
b/arch/arm/boot/dts/armada-370.dtsi
index 00b50db57c9c..ca4257b2f77d 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -307,6 +307,14 @@
dmacap,memset;
};
};
+
+   ethernet@7 {
+   compatible = marvell,armada-370-neta;
+   };
+
+   ethernet@74000 {
+   compatible = marvell,armada-370-neta;
+   };
};
};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi 
b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index 8479fdc9e9c2..c5fdc99f0dbe 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -318,7 +318,7 @@
};
 
eth3: ethernet@34000 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x34000 0x4000;
interrupts = 14;
clocks = gateclk 1;
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi 
b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 661d54c81580..0e24f1a38540 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -356,7 +356,7 @@
};
 
eth3: ethernet@34000 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x34000 0x4000;
interrupts = 14;
clocks = gateclk 1;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 013d63f69e36..8fdd6d7c0ab1 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -177,7 +177,7 @@
};
 
eth2: ethernet@3 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x3 0x4000;
interrupts = 12;
clocks = gateclk 2;
@@ -220,6 +220,14 @@
};
};
 
+   ethernet@7 {
+   compatible = marvell,armada-xp-neta;
+   };
+
+   ethernet@74000 {
+   compatible = marvell,armada-xp-neta;
+   };
+
xor@f0900 {
compatible = marvell,orion-xor;
reg = 0xF0900 0x100
-- 
2.1.4

--
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 v3 3/3] ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

[Simon Guinot]

The Ethernet controller found in the Armada 370, 380 and 385 SoCs don't
support TCP/IP checksumming with frame sizes larger than 1600 bytes.

This patch fixes the issue by disabling the features NETIF_F_IP_CSUM and
NETIF_F_TSO for the Armada 370 and compatibles SoCs when the MTU is set
to a value greater than 1600 bytes.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
unit)
Cc: sta...@vger.kernel.org # v3.8+
---
 drivers/net/ethernet/marvell/mvneta.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index aceb977b104d..9b2b7478fe47 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -310,6 +310,7 @@ struct mvneta_port {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+   unsigned int tx_csum_limit;
int use_inband_status:1;
 };
 
@@ -2502,8 +2503,10 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
 
dev-mtu = mtu;
 
-   if (!netif_running(dev))
+   if (!netif_running(dev)) {
+   netdev_update_features(dev);
return 0;
+   }
 
/* The interface is running, so we have to force a
 * reallocation of the queues
@@ -2532,9 +2535,26 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
mvneta_start_dev(pp);
mvneta_port_up(pp);
 
+   netdev_update_features(dev);
+
return 0;
 }
 
+static netdev_features_t mvneta_fix_features(struct net_device *dev,
+netdev_features_t features)
+{
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (pp-tx_csum_limit  dev-mtu  pp-tx_csum_limit) {
+   features = ~(NETIF_F_IP_CSUM | NETIF_F_TSO);
+   netdev_info(dev,
+   Disable IP checksum for MTU greater than %dB\n,
+   pp-tx_csum_limit);
+   }
+
+   return features;
+}
+
 /* Get mac address */
 static void mvneta_get_mac_addr(struct mvneta_port *pp, unsigned char *addr)
 {
@@ -2856,6 +2876,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
.ndo_set_rx_mode = mvneta_set_rx_mode,
.ndo_set_mac_address = mvneta_set_mac_addr,
.ndo_change_mtu  = mvneta_change_mtu,
+   .ndo_fix_features= mvneta_fix_features,
.ndo_get_stats64 = mvneta_get_stats64,
.ndo_do_ioctl= mvneta_ioctl,
 };
@@ -3101,6 +3122,9 @@ static int mvneta_probe(struct platform_device *pdev)
}
}
 
+   if (of_device_is_compatible(dn, marvell,armada-370-neta))
+   pp-tx_csum_limit = 1600;
+
pp-tx_ring_size = MVNETA_MAX_TXD;
pp-rx_ring_size = MVNETA_MAX_RXD;
 
-- 
2.1.4

--
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 v4 1/3] net: mvneta: introduce compatible string marvell,armada-xp-neta

[Simon Guinot]

The mvneta driver supports the Ethernet IP found in the Armada 370, XP,
380 and 385 SoCs. Since at least one more hardware feature is available
for the Armada XP SoCs then a way to identify them is needed.

This patch introduces a new compatible string marvell,armada-xp-neta.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
unit)
Cc: sta...@vger.kernel.org # v3.8+
Acked-by: Gregory CLEMENT gregory.clem...@free-electrons.com
---
 Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +-
 drivers/net/ethernet/marvell/mvneta.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt 
b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 750d577e8083..f5a8ca29aff0 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -1,7 +1,7 @@
 * Marvell Armada 370 / Armada XP Ethernet Controller (NETA)
 
 Required properties:
-- compatible: should be marvell,armada-370-neta.
+- compatible: marvell,armada-370-neta or marvell,armada-xp-neta.
 - reg: address and length of the register set for the device.
 - interrupts: interrupt for the device
 - phy: See ethernet.txt file in the same directory.
diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..aceb977b104d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev)
 
 static const struct of_device_id mvneta_match[] = {
{ .compatible = marvell,armada-370-neta },
+   { .compatible = marvell,armada-xp-neta },
{ }
 };
 MODULE_DEVICE_TABLE(of, mvneta_match);
-- 
2.1.4

--
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 v4 0/3] Fix Ethernet jumbo frames support for Armada 370 and 38x

[Simon Guinot]

Hello,

This patch series fixes the Ethernet jumbo frames support for the SoCs
Armada 370, 380 and 385. Unlike Armada XP, the Ethernet controller for
this SoCs don't support TCP/IP checksumming with a frame size larger
than 1600 bytes.

This patches should be applied to the -stable kernels 3.8 and onwards.

Changes since v1:
- Use a new compatible string for the Ethernet IP found in Armada XP
  SoCs (instead of using an optional property).
- Fix the issue for the Armada 380 and 385 SoCs as well.

Changes since v2:
- Add Acked-by from Gregory Clement.
- Add Fixes: tag to each commits.

Changes since v3:
- Fix patch 3 name: replace prefix ARM: mvebu: with net: mvneta:.

Regards,

Simon

Simon Guinot (3):
  net: mvneta: introduce compatible string marvell,armada-xp-neta
  ARM: mvebu: update Ethernet compatible string for Armada XP
  net: mvneta: disable IP checksum with jumbo frames for Armada 370

 .../bindings/net/marvell-armada-370-neta.txt   |  2 +-
 arch/arm/boot/dts/armada-370-xp.dtsi   |  2 --
 arch/arm/boot/dts/armada-370.dtsi  |  8 +++
 arch/arm/boot/dts/armada-xp-mv78260.dtsi   |  2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi   |  2 +-
 arch/arm/boot/dts/armada-xp.dtsi   | 10 +++-
 drivers/net/ethernet/marvell/mvneta.c  | 27 +-
 7 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.1.4

--
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 v4 3/3] net: mvneta: disable IP checksum with jumbo frames for Armada 370

[Simon Guinot]

The Ethernet controller found in the Armada 370, 380 and 385 SoCs don't
support TCP/IP checksumming with frame sizes larger than 1600 bytes.

This patch fixes the issue by disabling the features NETIF_F_IP_CSUM and
NETIF_F_TSO for the Armada 370 and compatibles SoCs when the MTU is set
to a value greater than 1600 bytes.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: c5aff18204da (net: mvneta: driver for Marvell Armada 370/XP network 
unit)
Cc: sta...@vger.kernel.org # v3.8+
---
 drivers/net/ethernet/marvell/mvneta.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index aceb977b104d..9b2b7478fe47 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -310,6 +310,7 @@ struct mvneta_port {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+   unsigned int tx_csum_limit;
int use_inband_status:1;
 };
 
@@ -2502,8 +2503,10 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
 
dev-mtu = mtu;
 
-   if (!netif_running(dev))
+   if (!netif_running(dev)) {
+   netdev_update_features(dev);
return 0;
+   }
 
/* The interface is running, so we have to force a
 * reallocation of the queues
@@ -2532,9 +2535,26 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
mvneta_start_dev(pp);
mvneta_port_up(pp);
 
+   netdev_update_features(dev);
+
return 0;
 }
 
+static netdev_features_t mvneta_fix_features(struct net_device *dev,
+netdev_features_t features)
+{
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (pp-tx_csum_limit  dev-mtu  pp-tx_csum_limit) {
+   features = ~(NETIF_F_IP_CSUM | NETIF_F_TSO);
+   netdev_info(dev,
+   Disable IP checksum for MTU greater than %dB\n,
+   pp-tx_csum_limit);
+   }
+
+   return features;
+}
+
 /* Get mac address */
 static void mvneta_get_mac_addr(struct mvneta_port *pp, unsigned char *addr)
 {
@@ -2856,6 +2876,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
.ndo_set_rx_mode = mvneta_set_rx_mode,
.ndo_set_mac_address = mvneta_set_mac_addr,
.ndo_change_mtu  = mvneta_change_mtu,
+   .ndo_fix_features= mvneta_fix_features,
.ndo_get_stats64 = mvneta_get_stats64,
.ndo_do_ioctl= mvneta_ioctl,
 };
@@ -3101,6 +3122,9 @@ static int mvneta_probe(struct platform_device *pdev)
}
}
 
+   if (of_device_is_compatible(dn, marvell,armada-370-neta))
+   pp-tx_csum_limit = 1600;
+
pp-tx_ring_size = MVNETA_MAX_TXD;
pp-rx_ring_size = MVNETA_MAX_RXD;
 
-- 
2.1.4

--
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 v4 2/3] ARM: mvebu: update Ethernet compatible string for Armada XP

[Simon Guinot]

This patch updates the Ethernet DT nodes for Armada XP SoCs with the
compatible string marvell,armada-xp-neta.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Fixes: 77916519cba3 (arm: mvebu: Armada XP MV78230 has only three Ethernet 
interfaces)
Cc: sta...@vger.kernel.org # v3.8+
Acked-by: Gregory CLEMENT gregory.clem...@free-electrons.com
---
 arch/arm/boot/dts/armada-370-xp.dtsi |  2 --
 arch/arm/boot/dts/armada-370.dtsi|  8 
 arch/arm/boot/dts/armada-xp-mv78260.dtsi |  2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |  2 +-
 arch/arm/boot/dts/armada-xp.dtsi | 10 +-
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi 
b/arch/arm/boot/dts/armada-370-xp.dtsi
index ec96f0b36346..06a2f2ae9d1e 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -270,7 +270,6 @@
};
 
eth0: ethernet@7 {
-   compatible = marvell,armada-370-neta;
reg = 0x7 0x4000;
interrupts = 8;
clocks = gateclk 4;
@@ -286,7 +285,6 @@
};
 
eth1: ethernet@74000 {
-   compatible = marvell,armada-370-neta;
reg = 0x74000 0x4000;
interrupts = 10;
clocks = gateclk 3;
diff --git a/arch/arm/boot/dts/armada-370.dtsi 
b/arch/arm/boot/dts/armada-370.dtsi
index 00b50db57c9c..ca4257b2f77d 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -307,6 +307,14 @@
dmacap,memset;
};
};
+
+   ethernet@7 {
+   compatible = marvell,armada-370-neta;
+   };
+
+   ethernet@74000 {
+   compatible = marvell,armada-370-neta;
+   };
};
};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi 
b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index 8479fdc9e9c2..c5fdc99f0dbe 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -318,7 +318,7 @@
};
 
eth3: ethernet@34000 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x34000 0x4000;
interrupts = 14;
clocks = gateclk 1;
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi 
b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 661d54c81580..0e24f1a38540 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -356,7 +356,7 @@
};
 
eth3: ethernet@34000 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x34000 0x4000;
interrupts = 14;
clocks = gateclk 1;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 013d63f69e36..8fdd6d7c0ab1 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -177,7 +177,7 @@
};
 
eth2: ethernet@3 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x3 0x4000;
interrupts = 12;
clocks = gateclk 2;
@@ -220,6 +220,14 @@
};
};
 
+   ethernet@7 {
+   compatible = marvell,armada-xp-neta;
+   };
+
+   ethernet@74000 {
+   compatible = marvell,armada-xp-neta;
+   };
+
xor@f0900 {
compatible = marvell,orion-xor;
reg = 0xF0900 0x100
-- 
2.1.4

--
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 v3 0/3] Fix Ethernet jumbo frames support for Armada 370 and 38x

[Simon Guinot]

Hello,

This patch series fixes the Ethernet jumbo frames support for the SoCs
Armada 370, 380 and 385. Unlike Armada XP, the Ethernet controller for
this SoCs don't support TCP/IP checksumming with a frame size larger
than 1600 bytes.

This patches should be applied to the -stable kernels 3.8 and onwards.

Changes since v1:
- Use a new compatible string for the Ethernet IP found in Armada XP
  SoCs (instead of using an optional property).
- Fix the issue for the Armada 380 and 385 SoCs as well.

Changes since v2:
- Add Acked-by from Gregory Clement.
- Add Fixes: tag to each commits.

Regards,

Simon


Simon Guinot (3):
  net: mvneta: introduce compatible string marvell,armada-xp-neta
  ARM: mvebu: update Ethernet compatible string for Armada XP
  ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

 .../bindings/net/marvell-armada-370-neta.txt   |  2 +-
 arch/arm/boot/dts/armada-370-xp.dtsi   |  2 --
 arch/arm/boot/dts/armada-370.dtsi  |  8 +++
 arch/arm/boot/dts/armada-xp-mv78260.dtsi   |  2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi   |  2 +-
 arch/arm/boot/dts/armada-xp.dtsi   | 10 +++-
 drivers/net/ethernet/marvell/mvneta.c  | 27 +-
 7 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.1.4

--
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 v2 1/3] net: mvneta: introduce compatible string marvell, armada-xp-neta

[Simon Guinot]

On Fri, Jun 19, 2015 at 02:32:53PM +0200, Simon Guinot wrote:
 On Wed, Jun 17, 2015 at 05:01:12PM +, Jason Cooper wrote:
  Hi Gregory,
  
  On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote:
   On 17/06/2015 17:12, Gregory CLEMENT wrote:
On 17/06/2015 15:19, Simon Guinot wrote:
The mvneta driver supports the Ethernet IP found in the Armada 370, XP,
380 and 385 SoCs. Since at least one more hardware feature is available
for the Armada XP SoCs then a way to identify them is needed.
   
This patch introduces a new compatible string marvell,armada-xp-neta.

Let's be future proof by going further. I would like to have an 
compatible string
for each SoC even if we currently we don't use them.
  
  I disagree with this.  We can't predict what incosistencies we'll discover 
  in
  the future.  We should only assign new compatible strings based on known IP
  variations when we discover them.  This seems fraught with demons since we
  can't predict the scope of affected IP blocks (some steppings of one SoC, 
  three
  SoCs plus two steppings of a fourth, etc)
  
  imho, the 'future-proofing' lies in being specific as to the naming of the
  compatible strings against known hardware variations at the time.
 
 So, should I add more compatible strings or not ?

Hi Gregory and Jason,

How do you want me to handle this ? Did you reach an agreement ?

Thanks,

Simon


signature.asc
Description: Digital signature

Re: [PATCH v2 1/3] net: mvneta: introduce compatible string marvell, armada-xp-neta

[Simon Guinot]

On Wed, Jun 17, 2015 at 05:01:12PM +, Jason Cooper wrote:
 Hi Gregory,
 
 On Wed, Jun 17, 2015 at 05:15:28PM +0200, Gregory CLEMENT wrote:
  On 17/06/2015 17:12, Gregory CLEMENT wrote:
   On 17/06/2015 15:19, Simon Guinot wrote:
   The mvneta driver supports the Ethernet IP found in the Armada 370, XP,
   380 and 385 SoCs. Since at least one more hardware feature is available
   for the Armada XP SoCs then a way to identify them is needed.
  
   This patch introduces a new compatible string marvell,armada-xp-neta.
   
   Let's be future proof by going further. I would like to have an 
   compatible string
   for each SoC even if we currently we don't use them.
 
 I disagree with this.  We can't predict what incosistencies we'll discover in
 the future.  We should only assign new compatible strings based on known IP
 variations when we discover them.  This seems fraught with demons since we
 can't predict the scope of affected IP blocks (some steppings of one SoC, 
 three
 SoCs plus two steppings of a fourth, etc)
 
 imho, the 'future-proofing' lies in being specific as to the naming of the
 compatible strings against known hardware variations at the time.

So, should I add more compatible strings or not ?

Simon


signature.asc
Description: Digital signature

Re: [PATCH v2 0/3] Fix Ethernet jumbo frames support for Armada 370 and 38x

[Simon Guinot]

On Wed, Jun 17, 2015 at 05:24:58PM +0200, Thomas Petazzoni wrote:
 Dear Simon Guinot,
 
 On Wed, 17 Jun 2015 15:19:19 +0200, Simon Guinot wrote:
 
  This patch series fixes the Ethernet jumbo frames support for the SoCs
  Armada 370, 380 and 385. Unlike Armada XP, the Ethernet controller for
  this SoCs don't support TCP/IP checksumming with a frame size larger
  than 1600 bytes.
  
  This patches should be applied to the -stable kernels 3.8 and onwards.
 
 You should add a Fixes: tag to each commit to indicate which commit is
 being fixed by your patches.
 
 Also, I was a bit surprised by your statement that Armada 38x is also
 affected by the problem, since Armada 38x is more recent than Armada
 XP. but indeed, according to the Armada 38x datasheet:
 
   IPv4 and TCP/UDP over IPv4/IPv6 checksum generation on transmit
   frames for standard Ethernet packet size
 
 While the Armada XP datasheet says:
 
   Long frames transmission (including jumbo frames), with
   IPv4/v6/TCP/UDP checksum generation
 
 So it seems like you're right about this!

At first, I though this was an error in the Armada 38x datasheet (maybe
a sloppy copy/paster). Therefore I have checked on an DB-88F6820-GP
board and then I can confirm that the Armada 38x is also affected.

Simon


signature.asc
Description: Digital signature

[PATCH v2 2/3] ARM: mvebu: update Ethernet compatible string for Armada XP

[Simon Guinot]

This patch updates the Ethernet DT nodes for Armada XP SoCs with the
compatible string marvell,armada-xp-neta.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Cc: sta...@vger.kernel.org # v3.8+
---
 arch/arm/boot/dts/armada-370-xp.dtsi |  2 --
 arch/arm/boot/dts/armada-370.dtsi|  8 
 arch/arm/boot/dts/armada-xp-mv78260.dtsi |  2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |  2 +-
 arch/arm/boot/dts/armada-xp.dtsi | 10 +-
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi 
b/arch/arm/boot/dts/armada-370-xp.dtsi
index ec96f0b36346..06a2f2ae9d1e 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -270,7 +270,6 @@
};
 
eth0: ethernet@7 {
-   compatible = marvell,armada-370-neta;
reg = 0x7 0x4000;
interrupts = 8;
clocks = gateclk 4;
@@ -286,7 +285,6 @@
};
 
eth1: ethernet@74000 {
-   compatible = marvell,armada-370-neta;
reg = 0x74000 0x4000;
interrupts = 10;
clocks = gateclk 3;
diff --git a/arch/arm/boot/dts/armada-370.dtsi 
b/arch/arm/boot/dts/armada-370.dtsi
index 00b50db57c9c..ca4257b2f77d 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -307,6 +307,14 @@
dmacap,memset;
};
};
+
+   ethernet@7 {
+   compatible = marvell,armada-370-neta;
+   };
+
+   ethernet@74000 {
+   compatible = marvell,armada-370-neta;
+   };
};
};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi 
b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index 8479fdc9e9c2..c5fdc99f0dbe 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -318,7 +318,7 @@
};
 
eth3: ethernet@34000 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x34000 0x4000;
interrupts = 14;
clocks = gateclk 1;
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi 
b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 661d54c81580..0e24f1a38540 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -356,7 +356,7 @@
};
 
eth3: ethernet@34000 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x34000 0x4000;
interrupts = 14;
clocks = gateclk 1;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 013d63f69e36..8fdd6d7c0ab1 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -177,7 +177,7 @@
};
 
eth2: ethernet@3 {
-   compatible = marvell,armada-370-neta;
+   compatible = marvell,armada-xp-neta;
reg = 0x3 0x4000;
interrupts = 12;
clocks = gateclk 2;
@@ -220,6 +220,14 @@
};
};
 
+   ethernet@7 {
+   compatible = marvell,armada-xp-neta;
+   };
+
+   ethernet@74000 {
+   compatible = marvell,armada-xp-neta;
+   };
+
xor@f0900 {
compatible = marvell,orion-xor;
reg = 0xF0900 0x100
-- 
2.1.4

--
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 v2 1/3] net: mvneta: introduce compatible string marvell,armada-xp-neta

[Simon Guinot]

The mvneta driver supports the Ethernet IP found in the Armada 370, XP,
380 and 385 SoCs. Since at least one more hardware feature is available
for the Armada XP SoCs then a way to identify them is needed.

This patch introduces a new compatible string marvell,armada-xp-neta.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Cc: sta...@vger.kernel.org # v3.8+
---
 Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt | 2 +-
 drivers/net/ethernet/marvell/mvneta.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt 
b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 750d577e8083..f5a8ca29aff0 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -1,7 +1,7 @@
 * Marvell Armada 370 / Armada XP Ethernet Controller (NETA)
 
 Required properties:
-- compatible: should be marvell,armada-370-neta.
+- compatible: marvell,armada-370-neta or marvell,armada-xp-neta.
 - reg: address and length of the register set for the device.
 - interrupts: interrupt for the device
 - phy: See ethernet.txt file in the same directory.
diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..aceb977b104d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3179,6 +3179,7 @@ static int mvneta_remove(struct platform_device *pdev)
 
 static const struct of_device_id mvneta_match[] = {
{ .compatible = marvell,armada-370-neta },
+   { .compatible = marvell,armada-xp-neta },
{ }
 };
 MODULE_DEVICE_TABLE(of, mvneta_match);
-- 
2.1.4

--
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 v2 3/3] ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

[Simon Guinot]

The Ethernet controller found in the Armada 370, 380 and 385 SoCs don't
support TCP/IP checksumming with frame sizes larger than 1600 bytes.

This patch fixes the issue by disabling the features NETIF_F_IP_CSUM and
NETIF_F_TSO for the Armada 370 and compatibles SoCs when the MTU is set
to a value greater than 1600 bytes.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Cc: sta...@vger.kernel.org # v3.8+
---
 drivers/net/ethernet/marvell/mvneta.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index aceb977b104d..9b2b7478fe47 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -310,6 +310,7 @@ struct mvneta_port {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+   unsigned int tx_csum_limit;
int use_inband_status:1;
 };
 
@@ -2502,8 +2503,10 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
 
dev-mtu = mtu;
 
-   if (!netif_running(dev))
+   if (!netif_running(dev)) {
+   netdev_update_features(dev);
return 0;
+   }
 
/* The interface is running, so we have to force a
 * reallocation of the queues
@@ -2532,9 +2535,26 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
mvneta_start_dev(pp);
mvneta_port_up(pp);
 
+   netdev_update_features(dev);
+
return 0;
 }
 
+static netdev_features_t mvneta_fix_features(struct net_device *dev,
+netdev_features_t features)
+{
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (pp-tx_csum_limit  dev-mtu  pp-tx_csum_limit) {
+   features = ~(NETIF_F_IP_CSUM | NETIF_F_TSO);
+   netdev_info(dev,
+   Disable IP checksum for MTU greater than %dB\n,
+   pp-tx_csum_limit);
+   }
+
+   return features;
+}
+
 /* Get mac address */
 static void mvneta_get_mac_addr(struct mvneta_port *pp, unsigned char *addr)
 {
@@ -2856,6 +2876,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
.ndo_set_rx_mode = mvneta_set_rx_mode,
.ndo_set_mac_address = mvneta_set_mac_addr,
.ndo_change_mtu  = mvneta_change_mtu,
+   .ndo_fix_features= mvneta_fix_features,
.ndo_get_stats64 = mvneta_get_stats64,
.ndo_do_ioctl= mvneta_ioctl,
 };
@@ -3101,6 +3122,9 @@ static int mvneta_probe(struct platform_device *pdev)
}
}
 
+   if (of_device_is_compatible(dn, marvell,armada-370-neta))
+   pp-tx_csum_limit = 1600;
+
pp-tx_ring_size = MVNETA_MAX_TXD;
pp-rx_ring_size = MVNETA_MAX_RXD;
 
-- 
2.1.4

--
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 v2 0/3] Fix Ethernet jumbo frames support for Armada 370 and 38x

[Simon Guinot]

Hello,

This patch series fixes the Ethernet jumbo frames support for the SoCs
Armada 370, 380 and 385. Unlike Armada XP, the Ethernet controller for
this SoCs don't support TCP/IP checksumming with a frame size larger
than 1600 bytes.

This patches should be applied to the -stable kernels 3.8 and onwards.

Changes since v1:
- Use a new compatible string for the Ethernet IP found in Armada XP
  SoCs (instead of using an optional property).
- Fix the issue for the Armada 380 and 385 SoCs as well.

Regards,

Simon

Simon Guinot (3):
  net: mvneta: introduce compatible string marvell,armada-xp-neta
  ARM: mvebu: update Ethernet compatible string for Armada XP
  ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

 .../bindings/net/marvell-armada-370-neta.txt   |  2 +-
 arch/arm/boot/dts/armada-370-xp.dtsi   |  2 --
 arch/arm/boot/dts/armada-370.dtsi  |  8 +++
 arch/arm/boot/dts/armada-xp-mv78260.dtsi   |  2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi   |  2 +-
 arch/arm/boot/dts/armada-xp.dtsi   | 10 +++-
 drivers/net/ethernet/marvell/mvneta.c  | 27 +-
 7 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.1.4

--
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 0/2] Fix Ethernet jumbo frames support for Armada 370

[Simon Guinot]

Hello,

This patch series fixes the Ethernet jumbo frames support for Armada 370
SoCs. Unlike Armada XP, the Ethernet controller in Armada 370 SoCs don't
support TCP/IP checksumming with frames largest than 1600 Bytes.

This patches should be applied to the -stable kernels 3.8 and onwards.

Regards,

Simon

Simon Guinot (2):
  net: mvneta: introduce tx_csum_limit property
  ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

 .../bindings/net/marvell-armada-370-neta.txt   |  3 +++
 arch/arm/boot/dts/armada-370.dtsi  |  8 +++
 drivers/net/ethernet/marvell/mvneta.c  | 25 +-
 3 files changed, 35 insertions(+), 1 deletion(-)

--
Cc: sta...@vger.kernel.org
-- 
2.1.4

--
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 2/2] ARM: mvebu: disable IP checksum with jumbo frames for Armada 370

[Simon Guinot]

The Ethernet controller found in Armada 370 SoCs don't support TCP/IP
checksumming with frames largest than 1600 Bytes.

This patch sets accordingly the tx_csum_limit property in Ethernet nodes
for Armada 370.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Cc: sta...@vger.kernel.org # v3.8+
---
 arch/arm/boot/dts/armada-370.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370.dtsi 
b/arch/arm/boot/dts/armada-370.dtsi
index 00b50db57c9c..046489df351d 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -307,6 +307,14 @@
dmacap,memset;
};
};
+
+   ethernet@7 {
+   tx_csum_limit = 1600;
+   };
+
+   ethernet@74000 {
+   tx_csum_limit = 1600;
+   };
};
};
 };
-- 
2.1.4

--
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 1/2] net: mvneta: introduce tx_csum_limit property

[Simon Guinot]

This patch introduces the tx_csum_limit DT property. This allows to
configure the maximum frame size for which the Ethernet controller is
able to perform TCP/IP checksumming. If MTU is set to a value greater
than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
are disabled.

Signed-off-by: Simon Guinot simon.gui...@sequanux.org
Cc: sta...@vger.kernel.org # v3.8+
---
 .../bindings/net/marvell-armada-370-neta.txt   |  3 +++
 drivers/net/ethernet/marvell/mvneta.c  | 25 +-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt 
b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 750d577e8083..db48c83ff0f5 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -8,6 +8,9 @@ Required properties:
 - phy-mode: See ethernet.txt file in the same directory
 - clocks: a pointer to the reference clock for this device.
 
+Optional properties:
+- tx_csum_limit: max tx packet size for hardware checksum.
+
 Example:
 
 ethernet@d007 {
diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index ce5f7f9cff06..1a724e0a2a26 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -310,6 +310,7 @@ struct mvneta_port {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+   unsigned int tx_csum_limit;
int use_inband_status:1;
 };
 
@@ -2502,8 +2503,10 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
 
dev-mtu = mtu;
 
-   if (!netif_running(dev))
+   if (!netif_running(dev)) {
+   netdev_update_features(dev);
return 0;
+   }
 
/* The interface is running, so we have to force a
 * reallocation of the queues
@@ -2532,9 +2535,26 @@ static int mvneta_change_mtu(struct net_device *dev, int 
mtu)
mvneta_start_dev(pp);
mvneta_port_up(pp);
 
+   netdev_update_features(dev);
+
return 0;
 }
 
+static netdev_features_t mvneta_fix_features(struct net_device *dev,
+netdev_features_t features)
+{
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (pp-tx_csum_limit  dev-mtu  pp-tx_csum_limit) {
+   features = ~(NETIF_F_IP_CSUM | NETIF_F_TSO);
+   netdev_info(dev,
+   Disable IP checksum for MTU greater than %dB\n,
+   pp-tx_csum_limit);
+   }
+
+   return features;
+}
+
 /* Get mac address */
 static void mvneta_get_mac_addr(struct mvneta_port *pp, unsigned char *addr)
 {
@@ -2856,6 +2876,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
.ndo_set_rx_mode = mvneta_set_rx_mode,
.ndo_set_mac_address = mvneta_set_mac_addr,
.ndo_change_mtu  = mvneta_change_mtu,
+   .ndo_fix_features= mvneta_fix_features,
.ndo_get_stats64 = mvneta_get_stats64,
.ndo_do_ioctl= mvneta_ioctl,
 };
@@ -3101,6 +3122,8 @@ static int mvneta_probe(struct platform_device *pdev)
}
}
 
+   of_property_read_u32(dn, tx_csum_limit, pp-tx_csum_limit);
+
pp-tx_ring_size = MVNETA_MAX_TXD;
pp-rx_ring_size = MVNETA_MAX_RXD;
 
-- 
2.1.4

--
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 1/2] net: mvneta: introduce tx_csum_limit property

[Simon Guinot]

On Mon, Jun 15, 2015 at 04:49:52PM +0200, Thomas Petazzoni wrote:
 Dear Simon Guinot,
 
 On Mon, 15 Jun 2015 16:27:22 +0200, Simon Guinot wrote:
  This patch introduces the tx_csum_limit DT property. This allows to
  configure the maximum frame size for which the Ethernet controller is
  able to perform TCP/IP checksumming. If MTU is set to a value greater
  than tx_csum_limit, then the features NETIF_F_IP_CSUM and NETIF_F_TSO
  are disabled.
  
  Signed-off-by: Simon Guinot simon.gui...@sequanux.org
  Cc: sta...@vger.kernel.org # v3.8+
  ---
   .../bindings/net/marvell-armada-370-neta.txt   |  3 +++
   drivers/net/ethernet/marvell/mvneta.c  | 25 
  +-
   2 files changed, 27 insertions(+), 1 deletion(-)
  
  diff --git 
  a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt 
  b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
  index 750d577e8083..db48c83ff0f5 100644
  --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
  +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
  @@ -8,6 +8,9 @@ Required properties:
   - phy-mode: See ethernet.txt file in the same directory
   - clocks: a pointer to the reference clock for this device.
   
  +Optional properties:
  +- tx_csum_limit: max tx packet size for hardware checksum.
 
 To be honest, I'd prefer to have a different compatible string to
 identify the two different versions of the hardware block.
 
 The current armada-370-neta would limit the HW checksumming features to
 packets smaller than 1600 bytes, while a new armada-xp-neta would not
 have this limit.

This was also my first idea. But by doing this, we take the risk of
losing the HW checksumming feature with jumbo frames on some currently
working Armada XP setups. This may happen for example if a user is able
to update the kernel but not the on-board DTB. In order to fix a feature
on a SoC, we are breaking the DTB-kernel compatibility for the very same
feature on an another SoC. I am not sure it is OK.

Are you comfortable with that ?

By adding a new optional property, at least we ensure that the things
will still work the same way with Armada-XP SoCs (for every DTB-Linux
combination).

Please, let me know if you still want to introduce a compatible string
for Armada-XP.

Simon

 
 Yet another case where we should have used armada-soc-neta,
 armada-370-neta in the .dtsi files for each SoC so that such
 modification do not require changing the Device Trees.
 
 Best regards,
 
 Thomas
 -- 
 Thomas Petazzoni, CTO, Free Electrons
 Embedded Linux, Kernel and Android engineering
 http://free-electrons.com


signature.asc
Description: Digital signature