RE: [PATCH] dpaa_eth: Add dpaa_change_carrier()

[Madalin-cristian Bucur]

> -Original Message-
> From: Joakim Tjernlund 
> Sent: Thursday, December 6, 2018 5:32 PM
> To: netdev @ vger . kernel . org ; Madalin-
> cristian Bucur 
> Cc: jo...@infinera.com 
> Subject: [PATCH] dpaa_eth: Add dpaa_change_carrier()
> 
> This allows to control carrier from /sys/class/net/ethX/carrier

Hi,

can you please explain why it's needed?

Thanks,
Madalin

> Signed-off-by: Joakim Tjernlund 
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index a202c50d6fc7..0e93dfa6df0a 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2502,12 +2502,23 @@ static int dpaa_ioctl(struct net_device *net_dev,
> struct ifreq *rq, int cmd)
>   return phy_mii_ioctl(net_dev->phydev, rq, cmd);
>  }
> 
> +static int dpaa_change_carrier(struct net_device *dev, bool new_carrier)
> +{
> + struct phy_device *phydev = dev->phydev;
> +
> + if (phydev && phydev->phy_link_change)
> + phydev->phy_link_change(phydev, new_carrier, 1);
> +
> + return 0;
> +}
> +
>  static const struct net_device_ops dpaa_ops = {
>   .ndo_open = dpaa_open,
>   .ndo_start_xmit = dpaa_start_xmit,
>   .ndo_stop = dpaa_eth_stop,
>   .ndo_tx_timeout = dpaa_tx_timeout,
>   .ndo_get_stats64 = dpaa_get_stats64,
> + .ndo_change_carrier = dpaa_change_carrier,
>   .ndo_set_mac_address = dpaa_set_mac_address,
>   .ndo_validate_addr = eth_validate_addr,
>   .ndo_set_rx_mode = dpaa_set_rx_mode,
> --
> 2.18.1

RE: [PATCH v2 2/2] dpaa_eth: add ethtool coalesce control

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller 
> Sent: Saturday, November 17, 2018 5:42 AM
> To: Madalin-cristian Bucur 
> Subject: Re: [PATCH v2 2/2] dpaa_eth: add ethtool coalesce control
> 
> From: Madalin Bucur 
> Date: Tue, 13 Nov 2018 18:29:51 +0200
> 
> > +   for_each_cpu(cpu, cpus) {
> > +   portal = qman_get_affine_portal(cpu);
> > +   res = qman_portal_set_iperiod(portal, period);
> > +   if (res)
> > +   return res;
> > +   res = qman_dqrr_set_ithresh(portal, thresh);
> > +   if (res)
> > +   return res;
> 
> Nope, you can't do it like this.
> 
> If any intermediate change fails, you have to unwind all of the
> changes made up until that point.
> 
> Which means you'll have to store the previous setting somewhere
> and reinstall those saved values in the error path.

Thank you, I'll come back with a v3.

Madalin

RE: [PATCH v2 00/22] SMMU enablement for NXP LS1043A and LS1046A

[Madalin-cristian Bucur]

> -Original Message-
> From: laurentiu.tu...@nxp.com [mailto:laurentiu.tu...@nxp.com]
> Sent: Wednesday, September 26, 2018 4:22 PM
> To: devicet...@vger.kernel.org; netdev@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Cc: Roy Pledge ; Madalin-cristian Bucur
> ; da...@davemloft.net; shawn...@kernel.org; Leo Li
> ; robin.mur...@arm.com; Bharat Bhushan
> ; Laurentiu Tudor 
> Subject: [PATCH v2 00/22] SMMU enablement for NXP LS1043A and LS1046A
> 
> From: Laurentiu Tudor 
> 
> This patch series adds SMMU support for NXP LS1043A and LS1046A chips
> and consists mostly in important driver fixes and the required device
> tree updates. It touches several subsystems and consists of three main
> parts:
>  - changes in soc/drivers/fsl/qbman drivers adding iommu mapping of
>reserved memory areas, fixes and defered probe support
>  - changes in drivers/net/ethernet/freescale/dpaa_eth drivers
>consisting in misc dma mapping related fixes and probe ordering
>  - addition of the actual arm smmu device tree node together with
>various adjustments to the device trees
> 
> Performance impact
> 
> Running iperf benchmarks in a back-to-back setup (both sides
> having smmu enabled) on a 10GBps port show an important
> networking performance degradation of around 40% (9.48Gbps
> linerate vs 5.45Gbps) and around 30%-35% with iommu.strict=1.
> If you need performance but without SMMU support you can use
> "iommu.passthrough=1" to disable SMMU.
> 
> The patch set is based on net-next so, if generally agreed, I'd suggest
> to get the patches through the netdev tree after getting all the Acks.
> 
> Changes in v2:
>  - dropped confusing comments in smmu interrupt property (Robin)
>  - add an intermediate simple-bus for usb to fix dma size issue (Robin)
>  - use defines instead of numbers in smmu interrupt definition
>  - drop redundant double iommu_get_domain_for_dev() call in few qbman
>patches
> 
> Laurentiu Tudor (22):
>   soc/fsl/qman: fixup liodns only on ppc targets
>   soc/fsl/bman: map FBPR area in the iommu
>   soc/fsl/qman: map FQD and PFDR areas in the iommu
>   soc/fsl/qman-portal: map CENA area in the iommu
>   soc/fsl/qbman: add APIs to retrieve the probing status
>   soc/fsl/qman_portals: defer probe after qman's probe
>   soc/fsl/bman_portals: defer probe after bman's probe
>   soc/fsl/qbman_portals: add APIs to retrieve the probing status
>   fsl/fman: backup and restore ICID registers
>   fsl/fman: add API to get the device behind a fman port
>   dpaa_eth: defer probing after qbman
>   dpaa_eth: base dma mappings on the fman rx port
>   dpaa_eth: fix iova handling for contiguous frames
>   dpaa_eth: fix iova handling for sg frames
>   dpaa_eth: fix SG frame cleanup
>   arm64: dts: ls1046a: add smmu node
>   arm64: dts: ls1043a: add smmu node
>   arm64: dts: ls104xa: set mask to drop TBU ID from StreamID
>   arm64: dts: ls104x: add missing dma ranges property
>   arm64: dts: ls104x: add iommu-map to pci controllers
>   arm64: dts: ls104x: make dma-coherent global to the SoC
>   arm64: dts: ls104x: use a pseudo-bus to constrain usb dma size
> 
>  .../arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 106 ++
>  .../arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 102 ++---
>  .../net/ethernet/freescale/dpaa/dpaa_eth.c| 136 --
>  drivers/net/ethernet/freescale/fman/fman.c|  35 -
>  drivers/net/ethernet/freescale/fman/fman.h|   4 +
>  .../net/ethernet/freescale/fman/fman_port.c   |  14 ++
>  .../net/ethernet/freescale/fman/fman_port.h   |   2 +
>  drivers/soc/fsl/qbman/bman_ccsr.c |  22 +++
>  drivers/soc/fsl/qbman/bman_portal.c   |  20 ++-
>  drivers/soc/fsl/qbman/qman_ccsr.c |  28 
>  drivers/soc/fsl/qbman/qman_portal.c   |  35 +
>  include/soc/fsl/bman.h|  16 +++
>  include/soc/fsl/qman.h|  17 +++
>  13 files changed, 438 insertions(+), 99 deletions(-)
> 
> --
> 2.17.1

For the fsl/fman and dpaa_eth:

Acked-by: Madalin Bucur 

Thank you

RE: [PATCH] fsl/fman: remove unnecessary set_dma_ops() call and HAS_DMA dependency

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, March 22, 2018 8:32 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: geert.uytterhoe...@gmail.com; netdev@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] fsl/fman: remove unnecessary set_dma_ops() call and
> HAS_DMA dependency
> 
> From: Madalin Bucur <madalin.bu...@nxp.com>
> Date: Wed, 21 Mar 2018 03:58:19 -0500
> 
> > The platform device is no longer used for DMA mapping so the
> > (questionable) setting of the DMA ops done here is no longer
> > needed. Removing it together with the HAS_DMA dependency that
> > it required.
> >
> > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> 
> This doesn't apply to any of my trees.

Sorry, it's caused by a patch in my tree that adds ARM 32b support, resulting 
in differences
in the context:

-   depends on FSL_SOC || ARCH_LAYERSCAPE || COMPILE_TEST
+   depends on ARM || ARCH_LAYERSCAPE || FSL_SOC || COMPILE_TEST

I'll send a v2 based on a clean tree.

Madalin

RE: [PATCH v2 10/21] lightnvm: Remove depends on HAS_DMA in case of platform dependency

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Geert Uytterhoeven
> Sent: Friday, March 16, 2018 3:52 PM
> To: Christoph Hellwig ; Marek Szyprowski
> ; Robin Murphy ;
> Felipe Balbi ; Greg Kroah-Hartman
> ; James E . J . Bottomley
> ; Martin K . Petersen
> ; Andrew Morton  foundation.org>; Mark Brown ; Liam Girdwood
> ; Tejun Heo ; Herbert Xu
> ; David S . Miller ;
> Bartlomiej Zolnierkiewicz ; Stefan Richter
> ; Alan Tull ; Moritz Fischer
> ; Wolfram Sang ; Jonathan Cameron
> ; Joerg Roedel ; Matias Bjorling
> ; Jassi Brar ; Mauro Carvalho
> Chehab ; Ulf Hansson ; David
> Woodhouse ; Brian Norris
> ; Marek Vasut ;
> Cyrille Pitchen ; Boris Brezillon
> ; Richard Weinberger ;
> Kalle Valo ; Ohad Ben-Cohen ;
> Bjorn Andersson ; Eric Anholt ;
> Stefan Wahren 
> Cc: io...@lists.linux-foundation.org; linux-...@vger.kernel.org; linux-
> s...@vger.kernel.org; alsa-de...@alsa-project.org; linux-...@vger.kernel.org;
> linux-cry...@vger.kernel.org; linux-fb...@vger.kernel.org; linux1394-
> de...@lists.sourceforge.net; linux-f...@vger.kernel.org; linux-
> i...@vger.kernel.org; linux-...@vger.kernel.org; linux-bl...@vger.kernel.org;
> linux-me...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> m...@lists.infradead.org; netdev@vger.kernel.org; linux-
> remotep...@vger.kernel.org; linux-ser...@vger.kernel.org; linux-
> s...@vger.kernel.org; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org; Geert Uytterhoeven 
> Subject: [PATCH v2 10/21] lightnvm: Remove depends on HAS_DMA in case of
> platform dependency
> 
> Remove dependencies on HAS_DMA where a Kconfig symbol depends on
> another
> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
> In most cases this other symbol is an architecture or platform specific
> symbol, or PCI.
> 
> Generic symbols and drivers without platform dependencies keep their
> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
> cannot work anyway.
> 
> This simplifies the dependencies, and allows to improve compile-testing.
> 
> Notes:
>   - FSL_FMAN keeps its dependency on HAS_DMA, as it calls set_dma_ops(),
> which does not exist if HAS_DMA=n (Do we need a dummy? The use of
> set_dma_ops() in this driver is questionable),

Hi,

The set_dma_ops() is no longer required in the fsl/fman, I'll send a patch to 
remove it.

Thanks

>   - SND_SOC_LPASS_IPQ806X and SND_SOC_LPASS_PLATFORM loose their
> dependency on HAS_DMA, as they are selected from
> SND_SOC_APQ8016_SBC.
> 
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Mark Brown 
> Acked-by: Robin Murphy 
> ---
> v2:
>   - Add Reviewed-by, Acked-by,
>   - Drop RFC state,
>   - Split per subsystem.
> ---
>  drivers/lightnvm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/lightnvm/Kconfig b/drivers/lightnvm/Kconfig
> index 10c08982185a572f..9c03f35d9df113c6 100644
> --- a/drivers/lightnvm/Kconfig
> +++ b/drivers/lightnvm/Kconfig
> @@ -4,7 +4,7 @@
> 
>  menuconfig NVM
>   bool "Open-Channel SSD target support"
> - depends on BLOCK && HAS_DMA && PCI
> + depends on BLOCK && PCI
>   select BLK_DEV_NVME
>   help
> Say Y here to get to enable Open-channel SSDs.
> --
> 2.7.4

RE: [PATCH 0/5] DPAA Ethernet fixes

[Madalin-cristian Bucur]

> -Original Message-
> From: Joakim Tjernlund [mailto:joakim.tjernl...@infinera.com]
> Sent: Wednesday, March 14, 2018 8:43 PM
> To: da...@davemloft.net; Madalin-cristian Bucur
> <madalin.bu...@nxp.com>
> 
> On Wed, 2018-03-14 at 08:37 -0500, Madalin Bucur wrote:
> > Hi,
> >
> > This patch set is addressing several issues in the DPAA Ethernet
> > driver suite:
> >
> >  - module unload crash caused by wrong reference to device being left
> >in the cleanup code after the DSA related changes
> >  - scheduling wile atomic bug in QMan code revealed during dpaa_eth
> >module unload
> >  - a couple of error counter fixes, a duplicated init in dpaa_eth.
> 
> hmm, some of these(all?) bugs are in stable too, CC: stable perhaps?

Hi Jocke,

I did not check that, they should be added.

Thanks,
Madalin

> >
> > Madalin
> >
> > Camelia Groza (3):
> >   dpaa_eth: remove duplicate initialization
> >   dpaa_eth: increment the RX dropped counter when needed
> >   dpaa_eth: remove duplicate increment of the tx_errors counter
> >
> > Madalin Bucur (2):
> >   soc/fsl/qbman: fix issue in qman_delete_cgr_safe()
> >   dpaa_eth: fix error in dpaa_remove()
> >
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c |  8 
> >  drivers/soc/fsl/qbman/qman.c   | 28 
> > +-
> >  2 files changed, 9 insertions(+), 27 deletions(-)
> >
> > --
> > 2.1.0
> >

RE: [PATCH] fsl/fman: avoid sleeping in atomic context while adding an address

[Madalin-cristian Bucur]

> -Original Message-
> From: Denis Kirjanov [mailto:k...@linux-powerpc.org]
> Sent: Sunday, March 4, 2018 8:48 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; Denis Kirjanov <k...@linux-powerpc.org>
> Subject: [PATCH] fsl/fman: avoid sleeping in atomic context while adding an
> address
> 
> __dev_mc_add grabs an adress spinlock so use
> atomic context in kmalloc.
> 
> / # ifconfig eth0 inet 192.168.0.111
> [   89.331622] BUG: sleeping function called from invalid context at
> mm/slab.h:420
> [   89.339002] in_atomic(): 1, irqs_disabled(): 0, pid: 1035, name: ifconfig
> [   89.345799] 2 locks held by ifconfig/1035:
> [   89.349908]  #0:  (rtnl_mutex){+.+.}, at: [<(ptrval)>]
> devinet_ioctl+0xc0/0x8a0
> [   89.357258]  #1:  (_xmit_ETHER){+...}, at: [<(ptrval)>]
> __dev_mc_add+0x28/0x80
> [   89.364520] CPU: 1 PID: 1035 Comm: ifconfig Not tainted 4.16.0-rc3-dirty
> #8
> [   89.371464] Call Trace:
> [   89.373908] [e959db60] [c066f948] dump_stack+0xa4/0xfc (unreliable)
> [   89.380177] [e959db80] [c00671d8] ___might_sleep+0x248/0x280
> [   89.385833] [e959dba0] [c01aec34]
> kmem_cache_alloc_trace+0x174/0x320
> [   89.392179] [e959dbd0] [c04ab920]
> dtsec_add_hash_mac_address+0x130/0x240
> [   89.398874] [e959dc00] [c04a9d74] set_multi+0x174/0x1b0
> [   89.404093] [e959dc30] [c04afb68] dpaa_set_rx_mode+0x68/0xe0
> [   89.409745] [e959dc40] [c057baf8] __dev_mc_add+0x58/0x80
> [   89.415052] [e959dc60] [c060fd64] igmp_group_added+0x164/0x190
> [   89.420878] [e959dca0] [c060ffa8] ip_mc_inc_group+0x218/0x460
> [   89.426617] [e959dce0] [c06120fc] ip_mc_up+0x3c/0x190
> [   89.431662] [e959dd10] [c0607270] inetdev_event+0x250/0x620
> [   89.437227] [e959dd50] [c005f190] notifier_call_chain+0x80/0xf0
> [   89.443138] [e959dd80] [c0573a74] __dev_notify_flags+0x54/0xf0
> [   89.448964] [e959dda0] [c05743f8] dev_change_flags+0x48/0x60
> [   89.454615] [e959ddc0] [c0606744] devinet_ioctl+0x544/0x8a0
> [   89.460180] [e959de10] [c060987c] inet_ioctl+0x9c/0x1f0
> [   89.465400] [e959de80] [c05479a8] sock_ioctl+0x168/0x460
> [   89.470708] [e959ded0] [c01cf3ec] do_vfs_ioctl+0xac/0x8c0
> [   89.476099] [e959df20] [c01cfc40] SyS_ioctl+0x40/0xc0
> [   89.481147] [e959df40] [c0011318] ret_from_syscall+0x0/0x3c
> [   89.486715] --- interrupt: c01 at 0x1006943c
> [   89.486715] LR = 0x100c45ec
> 
> Signed-off-by: Denis Kirjanov <k...@linux-powerpc.org>
> ---
>  drivers/net/ethernet/freescale/fman/fman_dtsec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> index ea43b4974149..7af31ddd093f 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> @@ -1100,7 +1100,7 @@ int dtsec_add_hash_mac_address(struct
> fman_mac *dtsec, enet_addr_t *eth_addr)
>   set_bucket(dtsec->regs, bucket, true);
> 
>   /* Create element to be added to the driver hash table */
> - hash_entry = kmalloc(sizeof(*hash_entry), GFP_KERNEL);
> + hash_entry = kmalloc(sizeof(*hash_entry), GFP_ATOMIC);
>   if (!hash_entry)
>   return -ENOMEM;
>   hash_entry->addr = addr;
> --
> 2.13.6

Acked-by: Madalin Bucur <madalin.bu...@nxp.com>

RE: [PATCH net-next] dpaa_eth: fix pause capability advertisement logic

[Madalin-cristian Bucur]

> -Original Message-
> From: Jake Moroni [mailto:m...@jakemoroni.com]
> Sent: Sunday, February 18, 2018 10:26 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; Jake Moroni <m...@jakemoroni.com>
> Subject: [PATCH net-next] dpaa_eth: fix pause capability advertisement
> logic
> 
> The ADVERTISED_Asym_Pause bit was being improperly set when both
> rx and tx pause were enabled. When rx and tx are both enabled, only
> the ADVERTISED_Pause bit is supposed to be set.
> 
> Signed-off-by: Jake Moroni <m...@jakemoroni.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> index faea674..85306d1 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> @@ -211,7 +211,7 @@ static int dpaa_set_pauseparam(struct net_device
> *net_dev,
>   if (epause->rx_pause)
>   newadv = ADVERTISED_Pause | ADVERTISED_Asym_Pause;
>   if (epause->tx_pause)
> - newadv |= ADVERTISED_Asym_Pause;
> + newadv ^= ADVERTISED_Asym_Pause;
> 
>   oldadv = phydev->advertising &
>   (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
> --
> 2.7.4

Acked-by: Madalin Bucur <madalin.bu...@nxp.com>

RE: DPAA Ethernet traffice troubles with Linux kernel

[Madalin-cristian Bucur]

> -Original Message-
> From: Madalin-cristian Bucur
> Sent: Wednesday, January 17, 2018 4:25 PM
> To: David S . Miller <da...@davemloft.net>
> Cc: linuxppc-...@lists.ozlabs.org; netdev@vger.kernel.org;
> madskate...@gmail.com; 'Madalin-cristian Bucur' <madalin.bu...@nxp.com>;
> Andrew Lunn <and...@lunn.ch>; Joakim Tjernlund
> <joakim.tjernl...@infinera.com>
> Subject: RE: DPAA Ethernet traffice troubles with Linux kernel
> 
> > -Original Message-
> > From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> > On Behalf Of Madalin-cristian Bucur
> > Sent: Wednesday, January 17, 2018 4:16 PM
> > To: Andrew Lunn <and...@lunn.ch>; Joakim Tjernlund
> > <joakim.tjernl...@infinera.com>
> > Cc: linuxppc-...@lists.ozlabs.org; netdev@vger.kernel.org;
> > madskate...@gmail.com; David S . Miller <da...@davemloft.net>
> > Subject: RE: DPAA Ethernet traffice troubles with Linux kernel
> >
> > > -Original Message-
> > > From: Andrew Lunn [mailto:and...@lunn.ch]
> > > Sent: Wednesday, January 17, 2018 3:44 PM
> > > To: Joakim Tjernlund <joakim.tjernl...@infinera.com>
> > > Subject: Re: DPAA Ethernet traffice troubles with Linux kernel
> > >
> > > > That doesn't work really, having users to hit the bug, debug it, fix
> > it
> > > and then
> > > > find it fixed already in upstream, then specifically request it to
> be
> > > backported to stable.
> > > > I don't need this fix to be backported, already got it. Someone else
> > > might though.
> > >
> > > The "someone else might though" is a big point of asking for it to
> > > added to stable. The other reason is it means one less patch you need
> > > to maintain in your build.
> >
> > I've sent that patch [1] for net but I guess the timing was wrong and
> > it was merged to net-next.
> >
> > > > I would be interested in bug fixes upstream which fixes:
> > >
> > > Did you try upstream? Does it give the same errors?
> > >
> > > Andrew
> >
> > [1] https://patchwork.kernel.org/patch/10146119/
> >
> > Madalin
> 
> Hi Dave,
> 
> Can you please add the fix [1] to stable?
> 
> Thank you,
> Madalin

Sorry,

I've provided the wrong link towards the patch (v1 instead of v3),
here's the correct one:

https://patchwork.kernel.org/patch/10151969/

Madalin

RE: DPAA Ethernet traffice troubles with Linux kernel

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Madalin-cristian Bucur
> Sent: Wednesday, January 17, 2018 4:16 PM
> To: Andrew Lunn <and...@lunn.ch>; Joakim Tjernlund
> <joakim.tjernl...@infinera.com>
> Cc: linuxppc-...@lists.ozlabs.org; netdev@vger.kernel.org;
> madskate...@gmail.com; David S . Miller <da...@davemloft.net>
> Subject: RE: DPAA Ethernet traffice troubles with Linux kernel
> 
> > -Original Message-
> > From: Andrew Lunn [mailto:and...@lunn.ch]
> > Sent: Wednesday, January 17, 2018 3:44 PM
> > To: Joakim Tjernlund <joakim.tjernl...@infinera.com>
> > Subject: Re: DPAA Ethernet traffice troubles with Linux kernel
> >
> > > That doesn't work really, having users to hit the bug, debug it, fix
> it
> > and then
> > > find it fixed already in upstream, then specifically request it to be
> > backported to stable.
> > > I don't need this fix to be backported, already got it. Someone else
> > might though.
> >
> > The "someone else might though" is a big point of asking for it to
> > added to stable. The other reason is it means one less patch you need
> > to maintain in your build.
> 
> I've sent that patch [1] for net but I guess the timing was wrong and
> it was merged to net-next.
> 
> > > I would be interested in bug fixes upstream which fixes:
> >
> > Did you try upstream? Does it give the same errors?
> >
> > Andrew
> 
> [1] https://patchwork.kernel.org/patch/10146119/
> 
> Madalin

Hi Dave,

Can you please add the fix [1] to stable?

Thank you,
Madalin

RE: DPAA Ethernet traffice troubles with Linux kernel

[Madalin-cristian Bucur]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Wednesday, January 17, 2018 3:44 PM
> To: Joakim Tjernlund 
> Subject: Re: DPAA Ethernet traffice troubles with Linux kernel
> 
> > That doesn't work really, having users to hit the bug, debug it, fix it
> and then
> > find it fixed already in upstream, then specifically request it to be
> backported to stable.
> > I don't need this fix to be backported, already got it. Someone else
> might though.
> 
> The "someone else might though" is a big point of asking for it to
> added to stable. The other reason is it means one less patch you need
> to maintain in your build.

I've sent that patch [1] for net but I guess the timing was wrong and
it was merged to net-next.

> > I would be interested in bug fixes upstream which fixes:
> 
> Did you try upstream? Does it give the same errors?
> 
> Andrew

[1] https://patchwork.kernel.org/patch/10146119/

Madalin

RE: DPAA Ethernet traffice troubles with Linux kernel

[Madalin-cristian Bucur]

> -Original Message-
> From: Joakim Tjernlund [mailto:joakim.tjernl...@infinera.com]
> Sent: Tuesday, January 16, 2018 7:58 PM
> To: and...@lunn.ch
> Subject: Re: DPAA Ethernet traffice troubles with Linux kernel
> 
> On Thu, 1970-01-01 at 00:00 +, Andrew Lunn wrote:
> >
> > Hi Joakim
> >
> > You appear to be using an old kernel. Take a look at:
> 
> Not really, I am using 4.14.x and I don't think that is old. Seems like
> this
> patch hasn't been sent to 4.14.x.
> 
> I wonder if I might be missing something else, we just moved to 4.14 and
> notic that all
> our fixed PHYs are non functioning:
> fsl_mac ffe4e2000.ethernet: FMan MEMAC
> fsl_mac ffe4e2000.ethernet: FMan MAC address: 00:06:9c:0b:06:20
> fsl_mac dpaa-ethernet.0: __devm_request_mem_region(mac) failed
> fsl_mac: probe of dpaa-ethernet.0 failed with error -16
> fsl_mac ffe4e4000.ethernet: FMan MEMAC
> fsl_mac ffe4e4000.ethernet: FMan MAC address: 00:06:9c:0b:06:21
> fsl_mac dpaa-ethernet.1: __devm_request_mem_region(mac) failed
> fsl_mac: probe of dpaa-ethernet.1 failed with error -16
> fsl_mac ffe4e6000.ethernet: FMan MEMAC
> fsl_mac ffe4e6000.ethernet: FMan MAC address: 00:06:9c:0b:06:22
> fsl_mac dpaa-ethernet.2: __devm_request_mem_region(mac) failed
> fsl_mac: probe of dpaa-ethernet.2 failed with error -16
> fsl_mac ffe4e8000.ethernet: FMan MEMAC
> fsl_mac ffe4e8000.ethernet: FMan MAC address: 00:06:9c:0b:06:23
> fsl_mac dpaa-ethernet.3: __devm_request_mem_region(mac) failed
> fsl_mac: probe of dpaa-ethernet.3 failed with error -16
> 
> Feels like FMAN still think there are real PHYs there ?

Hi Joakim,

These errors are issued when trying to probe the second time the same
MAC node. The issue was introduced by this commit:

commit 4d8ee1935bcd666360311dfdadeee235d682d69a
Author: Florian Fainelli 
Date: Tue Aug 22 15:24:47 2017 -0700
fsl/man: Inherit parent device and of_node

and was later addressed by this patch set:

http://patchwork.ozlabs.org/project/netdev/list/?series=8462=*

Even with these errors printed, all is working fine, it's just the
second probing that fails. Adding the latter patches or reverting
the one above makes the errors prints dissapear.

Madalin

RE: DPAA Ethernet problems with mainstream Linux kernels

[Madalin-cristian Bucur]

> -Original Message-
> From: Darren Stevens [mailto:dar...@stevens-zone.net]
> Sent: Tuesday, January 16, 2018 12:40 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: Jamie Krueger <ja...@bitbybitsoftwaregroup.com>; linuxppc-
> d...@lists.ozlabs.org; netdev@vger.kernel.org
> Subject: Re: DPAA Ethernet problems with mainstream Linux kernels
> 
> Hello Madalin-cristian
> 
> On 15/01/2018, Madalin-cristian Bucur wrote:
> >> > The device tree that you mention, cyrus_p5020.eth.dts is not found in
> >> > the Linux kernel sources. The cyrus_p5020.dts file from the fsl ppc
> >> > device tree folder does not include the PHY information for the DPAA
> >> > interfaces. The problems that you experience may be caused by some
> >> > issues with the PHY configuration (i.e. internal delay).
> >> The cyrus_p5020.eth.dts is a modified version of the cyrus_p5020.dts,
> >> which of course was based off the original p5020ds.dts file. As you
> >> noted, the current cyrus_p5020.dts file is incomplete, and does not
> >> map the Ethernet connections properly.
> 
> This is because the current linux kernel version of cyrus_p5020.dts
> includes 'p5020si-pre.dtsi' and 'p5020si-post.dtsi' include files, which
> orginally gave us working ethernet (when we used the SDK kernel) However
> at some point you moved the ethernet aliases from the board dts file to
> the p5020si-pre.dtsi file breaking the linkages for our board.
> 
> cyrus-pre.dtsi is simply p5020si-pre.dtsi with only the correct aliases
> in.
> 
> >> ** I have attached both the cyrus_p5020.eth.dts and cyrus-pre.dtsi
> >>   files with this email for comparison. Please let me know if you
> see
> >>   any corrections that should be made to either file.
> >
> > At a first glance they look fine to me.
> 
> That's good to know.
> 
> >> I have started testing along that line, using Wireshark to view the
> >> traffic on the X5000/20 itself, and from another machine connected
> >> on the same subnet. So far (as indicated by some details of in my
> >> initial email), I can see outgoing broadcast requests (for DHCP)
> >> being sent out from the X5000/20, and these requests are correctly
> >> constructed and visible outside the X5000/20.
> >>
> >> However, no responses to the DHCP broadcasts appear to reach
> >> to X5000/20's DPAA Ethernet. I will need to setup some further
> >> tests to determine if the DHCP server saw the requests and responded
> >> to them. (I assume the DHCP server is getting them, and responding,
> >> as I can always get a successful DHCP response to the X5000/20
> >> when using an add-on Ethernet PICe card on the same subnet).
> 
> This matches what I see, the interface I have connected to the network
> shows an increasing number of transmitted packets, but no received ones.
> 
> Jamie also noticed the following error in dmesg (right after the ethernet
> port becomes active)
> 
> [4.112165] fsl_dpa dpaa-ethernet.0 eth0: Probed interface eth0
> [4.116616] fsl_dpa dpaa-ethernet.1 eth1: Probed interface eth1
> [ ... ]
> [  106.501055] IPv6: ADDRCONF(NETDEV_UP): eth1: link is not ready
> [  106.570944] IPv6: ADDRCONF(NETDEV_UP): eth1: link is not ready
> [  106.605044] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
> [  106.674918] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
> [  108.702771] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [  109.032798] fsl-pamu: pamu_av_isr: access violation interrupt
> [  109.032806] fsl-pamu: pamu_av_isr: POES1=
> [  109.032808] fsl-pamu: pamu_av_isr: POES2=
> [  109.032809] fsl-pamu: pamu_av_isr: AVS1=002d0081
> [  109.032811] fsl-pamu: pamu_av_isr: AVS2=0081
> [  109.032813] fsl-pamu: pamu_av_isr: AVA=0001f1328000
> [  109.032815] fsl-pamu: pamu_av_isr: UDAD=002d0001
> [  109.032817] fsl-pamu: pamu_av_isr: POEA=
> 
> I haven't seen this anywhere else, and wondered if it is relevant.
> 
> Regards
> Darren

The PAMU related errors may be relevant to the issue, if you have incorrect
settings you may have no traffic passing through. The PAMU configuration
should be made by the bootloader. Can you try to disable CONFIG_FSL_PAMU?

Madalin

RE: DPAA Ethernet traffice troubles with Linux kernel

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Andrew Lunn
> Sent: Tuesday, January 16, 2018 5:04 PM
> To: mad skateman <madskate...@gmail.com>
> Cc: Christian Zigotzky <chzigot...@xenosoft.de>; Joakim Tjernlund
> <joakim.tjernl...@infinera.com>; linuxppc-...@lists.ozlabs.org; Madalin-
> cristian Bucur <madalin.bu...@nxp.com>; netdev@vger.kernel.org
> Subject: Re: DPAA Ethernet traffice troubles with Linux kernel
> 
> > When i use mii-tool too Kick the tranceiver... it comes alive.. i can
> > ping the eth0 itself
> >
> > root@X5000LNX:/home/skateman# mii-tool -R eth0
> > resetting the transceiver...
> > root@X5000LNX:/home/skateman# ping 192.168.22.44
> > PING 192.168.22.44 (192.168.22.44) 56(84) bytes of data.
> > 64 bytes from 192.168.22.44: icmp_seq=1 ttl=64 time=0.045 ms
> > 64 bytes from 192.168.22.44: icmp_seq=2 ttl=64 time=0.046 ms
> > 64 bytes from 192.168.22.44: icmp_seq=3 ttl=64 time=0.047 ms
> > 64 bytes from 192.168.22.44: icmp_seq=4 ttl=64 time=0.048 ms
> 
> What PHY driver are you using?
> 
> This smells a bit like an RGMII-ID problem.
> 
>  Andrew

Hi Andrew,

>From another thread[1] on the same topic:

> I am not sure what PHY hardware/configuration you are using on the
> DS and RDB platforms, but I can confirm that AmigaONE X5000/20
> (Cyrus Motherboard with p5020 SoC), has dTSEC 4 and dTSEC 5
> wired to two Micrel KSZ9021RN Gigabit Ethernet PHYs, using the
> RGMII protocol.

[1] https://www.spinics.net/lists/netdev/msg478062.html

RE: DPAA Ethernet traffice troubles with Linux kernel

[Madalin-cristian Bucur]

> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+madalin.bucur=nxp@lists.ozlabs.org] On Behalf Of mad skateman
> Sent: Wednesday, January 10, 2018 10:39 PM
> To: linuxppc-...@lists.ozlabs.org
> Subject: DPAA Ethernet traffice troubles with Linux kernel
> 
> Hi linux devs,
> 
> Like mentioned in this thread
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-January/167630.html
> i also experience the exact same issues.
> I am also trying to find out why the network traffic is not flowing
> the way it should (out for example ).
> 
> My linux knowledge is very basic but i hope i can contribute anyway.
> 
> I am using the AmigaOne X5000 with a P5020
> Most detailed technical information regarding this issue can be found
> in the Thread by Jamie Krueger mentioned above.
> 
> In this screenshot, the ETH0 and ETH1 seem up and running (probed) ..
> even due to the FSL_DPAA_MAC error messages that DMESG shows.
> http://www.skateman.nl/wp-content/uploads/2018/01/Screenshot-at-2018-01-08-21_22_06_ETH_NIC_ERROR.png
> 
> http://www.skateman.nl/wp-content/uploads/2018/01/Screenshot-at-2018-01-08-22_16_28.png
> 
> I was able to use some tooling like ETHTOOL to adjust some settings
> and check if the interface responded. This all seems fine.
> 
> Hope that someone can find a fix, so the Ethernet adapter can be used.
> 
> Thanks!!

Hi,

Please use text logs instead of pictures next time, it's easier to read.
The errors you see are related to missing MAC addresses for the unused
interfaces, you can ignore these are they are not relevant for the issue
you encounter. Normally the unused interfaces should have status disabled
in the device tree but there is not a big deal if they fail like that.
As I've advised Jamie on the other thread, please try to connect the device
back 2 back to a known good machine and determine what is broken - Rx/Tx?
Is there another software version that does work on these machines?

Madalin

RE: DPAA Ethernet problems with mainstream Linux kernels

[Madalin-cristian Bucur]

> -Original Message-
> From: Jamie Krueger [mailto:ja...@bitbybitsoftwaregroup.com]
> Sent: Friday, January 12, 2018 6:36 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; linuxppc-
> d...@lists.ozlabs.org
> Cc: netdev@vger.kernel.org
> Subject: Re: DPAA Ethernet problems with mainstream Linux kernels
> 
> On 01/12/2018 08:22 AM, Madalin-cristian Bucur wrote:
> >> -Original Message-
> >> From: Linuxppc-dev [mailto:linuxppc-dev-
> >> bounces+madalin.bucur=nxp@lists.ozlabs.org] On Behalf Of Jamie
> Krueger
> >> Sent: Wednesday, January 10, 2018 5:57 PM
> >> To: linuxppc-...@lists.ozlabs.org
> >> Subject: DPAA Ethernet problems with mainstream Linux kernels
> >>
> >> Hello all @ linuxppc-dev,
> >>
> >> I have been working with a team of people maintaining PowerPC
> >> Linux for the new AmigaONE X5000/20 (a Freescale p5020 SoC based
> >> machine).
> >>
> >> We are trying to determine why the submitted Data Path Acceleration
> >> Architecture (DPAA) Ethernet Driver is not fully functional with
> >> the mainstream Linux kernels.
> > Hi Jamie,
> Hi Madalin,
> > We are testing the DPAA driver on several DS and RDB platforms and it
> > is working properly. The issues you encounter with it on the X5000/20
> > are likely caused by some issues specific to that particular platform.
> It is good to hear that the DPAA driver is functioning correctly
> on the reference platforms. I am positive you are correct that
> the issue is the difference in implementation on the X5000/20
> (Cyrus) motherboard, as compared to the reference boards.
> 
> Can you verify which Linux Kernel sources your tests are being
> performed on? We have been testing using the mainstream
> Linux sources up to linux-4.15-rc6 thus far.

Latest run is on 4.15.0-rc7-00200-gc92a9a4.

> > The device tree that you mention, cyrus_p5020.eth.dts is not found in
> > the Linux kernel sources. The cyrus_p5020.dts file from the fsl ppc
> > device tree folder does not include the PHY information for the DPAA
> > interfaces. The problems that you experience may be caused by some
> > issues with the PHY configuration (i.e. internal delay).
> The cyrus_p5020.eth.dts is a modified version of the cyrus_p5020.dts,
> which of course was based off the original p5020ds.dts file. As you
> noted, the current cyrus_p5020.dts file is incomplete, and does not
> map the Ethernet connections properly.
> 
> The cyrus_p5020.eth.dts file, along with it's cyrus-pre.dtsi dependent
> file, are an attempt to correctly define the Ethernet hardware, as it is
> implemented on the X5000/20.
> 
> ** I have attached both the cyrus_p5020.eth.dts and cyrus-pre.dtsi
>   files with this email for comparison. Please let me know if you see
>   any corrections that should be made to either file.

At a first glance they look fine to me.

> I am not sure what PHY hardware/configuration you are using on the
> DS and RDB platforms, but I can confirm that AmigaONE X5000/20
> (Cyrus Motherboard with p5020 SoC), has dTSEC 4 and dTSEC 5
> wired to two Micrel KSZ9021RN Gigabit Ethernet PHYs, using the
> RGMII protocol.

Since it's RGMII, I think you should look into RGMII internal delay
requirements for this board.

> >   I suggest
> > that you connect the DPAA interface to a traffic analyzer or directly
> > to another device on which you can capture the incoming traffic and
> > check that the received frames are correct.
> I have started testing along that line, using Wireshark to view the
> traffic on the X5000/20 itself, and from another machine connected
> on the same subnet. So far (as indicated by some details of in my
> initial email), I can see outgoing broadcast requests (for DHCP)
> being sent out from the X5000/20, and these requests are correctly
> constructed and visible outside the X5000/20.
> 
> However, no responses to the DHCP broadcasts appear to reach
> to X5000/20's DPAA Ethernet. I will need to setup some further
> tests to determine if the DHCP server saw the requests and responded
> to them. (I assume the DHCP server is getting them, and responding,
> as I can always get a successful DHCP response to the X5000/20
> when using an add-on Ethernet PICe card on the same subnet).
> 
> I will setup some more direct machine-to-machine testing to
> see what else I can glean from the network traffic.

That will provide more clarity to the actual issue.

> Please have a look at the attached dts files, maybe there is something
> obvious there we are not seeing.
> 
> Also, given that the X5000/20 uses Micrel KSZ9021RN PHYs in RGMII
> mode, what changes to the DPAA har

RE: [PATCH net-next] net: dpaa: change while() to if() in dpaa_fq_setup()

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of yuan linyu
> Sent: Saturday, January 13, 2018 11:25 AM
> To: netdev@vger.kernel.org
> Cc: David S . Miller ; yuan linyu
> 
> Subject: Re: [PATCH net-next] net: dpaa: change while() to if() in
> dpaa_fq_setup()
> 
> i am wrong, ignore it.
> 

Yes, the wile is required.

Madalin

> On 六, 2018-01-13 at 17:15 +0800, yuan linyu wrote:
> > From: yuan linyu 
> >
> > while loop is not needed, because list_for_each_entry() already check
> all fq.
> >
> > Signed-off-by: yuan linyu 
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 7caa8da..fd0e411 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -931,7 +931,7 @@ static void dpaa_fq_setup(struct dpaa_priv *priv,
> >     }
> >
> >      /* Make sure all CPUs receive a corresponding Tx queue. */
> > -   while (egress_cnt < DPAA_ETH_TXQ_NUM) {
> > +   if (egress_cnt < DPAA_ETH_TXQ_NUM) {
> >     list_for_each_entry(fq, >dpaa_fq_list, list) {
> >     if (fq->fq_type != FQ_TYPE_TX)
> >     continue;

RE: DPAA Ethernet problems with mainstream Linux kernels

[Madalin-cristian Bucur]

> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+madalin.bucur=nxp@lists.ozlabs.org] On Behalf Of Jamie Krueger
> Sent: Wednesday, January 10, 2018 5:57 PM
> To: linuxppc-...@lists.ozlabs.org
> Subject: DPAA Ethernet problems with mainstream Linux kernels
> 
> Hello all @ linuxppc-dev,
> 
> I have been working with a team of people maintaining PowerPC
> Linux for the new AmigaONE X5000/20 (a Freescale p5020 SoC based
> machine).
> 
> We are trying to determine why the submitted Data Path Acceleration
> Architecture (DPAA) Ethernet Driver is not fully functional with
> the mainstream Linux kernels.

Hi Jamie,

We are testing the DPAA driver on several DS and RDB platforms and it
is working properly. The issues you encounter with it on the X5000/20
are likely caused by some issues specific to that particular platform.
The device tree that you mention, cyrus_p5020.eth.dts is not found in
the Linux kernel sources. The cyrus_p5020.dts file from the fsl ppc
device tree folder does not include the PHY information for the DPAA
interfaces. The problems that you experience may be caused by some
issues with the PHY configuration (i.e. internal delay). I suggest
that you connect the DPAA interface to a traffic analyzer or directly
to another device on which you can capture the incoming traffic and
check that the received frames are correct.

Madalin

> Here is the results from my latest tests. They were performed using
> the linux-4.10.17 ppc64, since that represents when the DPAA Ethernet
> code was introduced.
> 
> Similar tests, with similar results, were also performed
> using the latest Linux kernels:
> 
> linux-4.15-rc5
> linux-4.15-rc6
> linux-4.15-rc7
> 
> (Hence the reason for falling back to test the kernel right
>   after the introduction of the DPAA Ethernet driver sources)
> 
> ---
> 
> All Kernel builds had the DPAA Ethernet enabled in the kernel,
> and are using the correct cyrus_p5020.eth.dtb device tree file
> (for use on the X5000/20).
> 
> The results are quite similar for all kernels in regards to the DPAA
> Ethernet.
> 
> All tested kernels setup the two Ethernet interfaces correctly
> as eth0 and eth1, and pull the correct MAC addresses from U-Boot
> environment variables ethaddr and eth1addr respectively.
> 
> So at this point Linux has what it believes is fully configured
> hardware, waiting to have an IP Address/Netmask/Gateway
> to be set and to bring the interface online.
> 
> However, all attempts to communicate with the outside world
> do not make it out the physical (PHY) hardware - or do they?
> 
> ** The following results were captured under linux-4.10.17 **
> 
> When I bring the interface up using a static address, in this case
> 192.168.1.21, I see the following (NOTE TX bytes says 154.0 KB,
> while RX bytes says 0.0 B):
> 
> jamie@X5000-Linux:$ ifconfig
> eth0  Link encap:Ethernet  HWaddr 00:80:10:11:11:11
>    inet addr:192.168.1.21  Bcast:192.168.1.255 Mask:255.255.255.0
>    inet6 addr: fe80::280:10ff:fe11:/64 Scope:Link
>    UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
>    RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>    TX packets:1428 errors:0 dropped:0 overruns:0 carrier:0
>    collisions:0 txqueuelen:1000
>    RX bytes:0 (0.0 B)  TX bytes:154066 (154.0 KB)
>    Memory:fe4e6000-fe4e6fff
> 
> eth1  Link encap:Ethernet  HWaddr 00:80:10:22:22:22
>    UP BROADCAST MULTICAST  MTU:1500  Metric:1
>    RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>    TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>    collisions:0 txqueuelen:1000
>    RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
>    Memory:fe4e8000-fe4e8fff
> 
> lo    Link encap:Local Loopback
>    inet addr:127.0.0.1  Mask:255.0.0.0
>    inet6 addr: ::1/128 Scope:Host
>    UP LOOPBACK RUNNING  MTU:65536  Metric:1
>    RX packets:1869 errors:0 dropped:0 overruns:0 frame:0
>    TX packets:1869 errors:0 dropped:0 overruns:0 carrier:0
>    collisions:0 txqueuelen:1000
>    RX bytes:156932 (156.9 KB)  TX bytes:156932 (156.9 KB)
> 
> Checking the routing table, everything looks fine there:
> 
> jamie@X5000-Linux:$ netstat -r
> Kernel IP routing table
> Destination Gateway Genmask Flags   MSS Window  irtt
> Iface
> default 192.168.1.1 0.0.0.0 UG    0 0  0
> eth0
> link-local  *   255.255.0.0 U 0 0  0
> eth0
> 192.168.1.0 *   255.255.255.0   U 0 0  0
> eth0
> 
> Attempting to PING the interface itself works:
> 
> jamie@X5000-Linux:$ ping 192.168.1.21
> PING 192.168.1.21 (192.168.1.21) 56(84) bytes of data.
> 64 bytes from 192.168.1.21: icmp_seq=1 ttl=64 time=0.037 ms
> 64 bytes from 192.168.1.21: icmp_seq=2 ttl=64 time=0.045 ms
> 64 bytes from 192.168.1.21: icmp_seq=3 ttl=64 

RE: [PATCH net] of_mdio: avoid MDIO bus removal when a PHY is missing

[Madalin-cristian Bucur]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Friday, January 05, 2018 5:13 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Subject: Re: [PATCH net] of_mdio: avoid MDIO bus removal when a PHY is
> missing
> 
> On Fri, Jan 05, 2018 at 11:36:14AM +0200, Madalin Bucur wrote:
> > If one of the child devices is missing the of_mdiobus_register_phy()
> > call will return -ENODEV. When a missing device is encountered the
> > registration of the remaining PHYs is stopped and the MDIO bus will
> > fail to register. Propagate all errors except ENODEV to avoid it.
> 
> Hi Madalin
> 
> This is is not clear cut. If the PHY is in device tree, the PHY should
> exist. So returning ENODEV is justified. The device tree blob is
> broken. But i can also see the value for continuing. There is a chance
> some of your other interfaces come up, allowing you to get the correct
> device tree blob for the hardware.
> 
> Please add
> 
> dev_err(>dev, "MDIO device at address %d is missing.\n");
> 
>   Andrew

This appears on boards that include in the device tree the description
for the PHYs found on an optional riser card. When the riser card is
removed, this issue is triggered. I'll send a v2 with the dev_err()
included.

Thanks,
Madalin

RE: [RFC] Support for SGMII 2500

[Madalin-cristian Bucur]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, November 28, 2017 4:13 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Subject: Re: [RFC] Support for SGMII 2500
> 
> > Hi Andrew,
> >
> > Bhaskar is working on enabling a PFE [1] MAC connected to an Aquantia
> AQR107
> > PHY [2] on a LS1012AQDS board. Initially I've indicated 2500Base-X too,
> but it
> > seems the HW actually works in SGMII mode. The QDS boards are lower
> volume,
> > higher spec boards than the RDBs [3], they exercise most of the HW
> capabilities.
> 
> The webpage for the AQR107 lists 2500Base-X, so i assume the issue is
> with the MAC? Ideally you want to use 2500Base-X, since this is wider
> known.
> 
> Anyway, you seem to have a legitimate need for it.
> 
> However, i would prefer a different name. The convention is to put the
> number first. So PHY_INTERFACE_MODE_2500SGMII.
> 
>Andrew

OK,

I just wanted to make sure 2.5G SGMII is to be added separately from the
"normal" SGMII, as it was done in u-boot. Thanks also for the naming hint.

Madalin

RE: [RFC] Support for SGMII 2500

[Madalin-cristian Bucur]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, November 28, 2017 3:30 PM
> Subject: Re: [RFC] Support for SGMII 2500
> 
> On Tue, Nov 28, 2017 at 07:25:48AM +, Madalin-cristian Bucur wrote:
> > Hi,
> >
> > There is a disconnect between the SGMII 2500 support in u-boot and
> Linux.
> > Bhaskar is trying to add support for a SGMII interface working at
> 2.5Gbps
> > by using the PHY connection type "sgmii-2500" in the device tree:
> 
> Hi Madalin
> 
> What MAC and PHY are you using?
> 
> I did a quick search for SGMII 2.5, and all i keep coming across is
> 2500BASE-X. I just want to make sure you really do need SGMII at 2500,
> and not 2500BASE-X, which is already supported.
> 
> Thanks
>   Andrew

Hi Andrew,

Bhaskar is working on enabling a PFE [1] MAC connected to an Aquantia AQR107 
PHY [2] on a LS1012AQDS board. Initially I've indicated 2500Base-X too, but it
seems the HW actually works in SGMII mode. The QDS boards are lower volume,
higher spec boards than the RDBs [3], they exercise most of the HW capabilities.

Regards,
Madalin

[1] 
https://www.nxp.com/products/processors-and-microcontrollers/arm-based-processors-and-mcus/qoriq-layerscape-arm-processors/qoriq-layerscape-1012a-low-power-communication-processor:LS1012A
[2] https://www.aquantia.com/products/enterprise/aqr107/
[3] 
https://www.nxp.com/support/developer-resources/software-development-tools/qoriq-developer-resources/qoriq-ls1012a-reference-design-board:LS1012A-RDB

[RFC] Support for SGMII 2500

[Madalin-cristian Bucur]

Hi,

There is a disconnect between the SGMII 2500 support in u-boot and Linux.
Bhaskar is trying to add support for a SGMII interface working at 2.5Gbps
by using the PHY connection type "sgmii-2500" in the device tree:

phy-connection-type = "sgmii-2500";

This is supported by u-boot, in include/phy.h:

typedef enum {
PHY_INTERFACE_MODE_MII,
PHY_INTERFACE_MODE_GMII,
PHY_INTERFACE_MODE_SGMII,
PHY_INTERFACE_MODE_SGMII_2500,
...

static const char *phy_interface_strings[] = {
[PHY_INTERFACE_MODE_MII]= "mii",
[PHY_INTERFACE_MODE_GMII]   = "gmii",
[PHY_INTERFACE_MODE_SGMII]  = "sgmii",
[PHY_INTERFACE_MODE_SGMII_2500] = "sgmii-2500",
...

since this commit:

commit c35f8693942d8284c635592f263a0fe11abe1d1d 
Author: Shengzhou Liu 
Date:   Thu Oct 23 17:20:57 2014 +0800

net/fm: add 2.5G SGMII support

As auto-negotiation is not supported for 2.5G SGMII, we need
to add a new type PHY_INTERFACE_MODE_SGMII_2500 to differentiate
SGMII-1G and SGMII-2.5G with different setting for auto-negotiation.

Signed-off-by: Shaohui Xie 
Signed-off-by: Shengzhou Liu 
Reviewed-by: York Sun 

In the Linux kernel we do not have a separate define for SGMII_2500, should we 
add
something like the change below?

Thanks,
Madalin

---
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07..086f7a3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -68,6 +68,7 @@ typedef enum {
PHY_INTERFACE_MODE_MII,
PHY_INTERFACE_MODE_GMII,
PHY_INTERFACE_MODE_SGMII,
+   PHY_INTERFACE_MODE_SGMII_2500,
PHY_INTERFACE_MODE_TBI,
PHY_INTERFACE_MODE_REVMII,
PHY_INTERFACE_MODE_RMII,
@@ -123,6 +124,8 @@ static inline const char *phy_modes(phy_interface_t 
interface)
return "gmii";
case PHY_INTERFACE_MODE_SGMII:
return "sgmii";
+   case PHY_INTERFACE_MODE_SGMII_2500:
+   return "sgmii-2500";
case PHY_INTERFACE_MODE_TBI:
return "tbi";
case PHY_INTERFACE_MODE_REVMII:

RE: [PATCH 0/4] fsl/fman: Fix some error handling code in mac_probe

[Madalin-cristian Bucur]

Hi Christophe,

I'll review and test your fixes.

Thank you!
Madalin

> -Original Message-
> From: Christophe JAILLET [mailto:christophe.jail...@wanadoo.fr]
> Sent: Monday, November 06, 2017 11:53 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET
> <christophe.jail...@wanadoo.fr>
> Subject: [PATCH 0/4] fsl/fman: Fix some error handling code in mac_probe
> 
> Commit c6e26ea8c893 ("dpaa_eth: change device used") generated some
> conflicts in my patches waiting for submission. So I took a closer look at
> it.
> 
> 
> So here is a serie of 4 patches.
> 
> The 1st one is just about a spurious call to 'dev_set_drvdata()', which is
> done in only 1 error handling path in the function.
> 
> The 2nd one removes some devm_iounmap/release/kfree functions which look
> useless to me.
> 
> The 3rd one fixes a missing of_node_put.
> 
> The 4th one is just cosmetic and removes a useless message.
> 
> 
> Christophe JAILLET (4):
>   fsl/fman: Remove a useless call to 'dev_set_drvdata()'
>   fsl/fman: Remove some useless code
>   fsl/fman: Add a missing 'of_node_put()' call in an error handling path
>   fsl/fman: Remove a useless 'dev_err()' call
> 
>  drivers/net/ethernet/freescale/fman/mac.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> --
> 2.14.1

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Madalin-cristian Bucur]

> +Eric
> 
> > -Original Message-
> > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> sbell.com]
> >
> > I just saw below accepted commit, it said "per cpu allocations are
> already
> > zeroed, no need to clear them again."
> >
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fdavem%2Fnet-
> next.git%2Fcommit%2F%3Fid%3Dbfd8e5a407133e58a92a38ccf3d0ba6db81f22d8=
> 02%7C01%7Cmadalin.bucur%40nxp.com%7C4116c18371684740c41a08d51f65c6e9%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636449444348895856=PBvP4J0ch
> 8o%2Bvif3C8D77dVxGW4vjUlCYwJzJcIzhFk%3D=0
> >
> > I will not touch memory sub-system, so I will not change this function
> > description.
> >
> 
> Your particular change removes redundancy, it's fine. Having the memset
> documented
> somewhere would prevent similar redundancy from being added in the future.
> 
> > > -Original Message-
> > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > Sent: Monday, October 30, 2017 1:56 PM
> > > To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu;
> netdev@vger.kernel.org;
> > > gre...@linuxfoundation.org
> > > Cc: David S . Miller
> > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> done
> > in
> > > per-cpu allocation
> > >
> > > Also here:
> > >
> > >
> > >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Felixir.f
> ree-
> electrons.com%2Flinux%2Flatest%2Fsource%2Fmm%2Fpercpu.c%23L717=02%7C0
> 1%7Cmadalin.bucur%40nxp.com%7C4116c18371684740c41a08d51f65c6e9%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C636449444348895856=G9AEcoAPE7yWH%2
> FHi56DP3zns8LaIixJ4xkuQCTJDI5E%3D=0
> > >
> > > Should the fact that the memory is zeroed be included in the function
> > > description?
> > >
> > > /**
> > >  * pcpu_alloc - the percpu allocator
> > >  * @size: size of area to allocate in bytes
> > >  * @align: alignment of area (max PAGE_SIZE)
> > >  * @reserved: allocate from the reserved chunk if available
> > >  * @gfp: allocation flags
> > >  *
> > >  * Allocate percpu area of @size bytes aligned at @align.  If @gfp
> > doesn't
> > >  * contain %GFP_KERNEL, the allocation is atomic.
> > >  *
> > >  * RETURNS:
> > >  * Percpu pointer to the allocated area on success, NULL on failure.
> > >  */
> > >
> > > Now it seems to be an implementation detail rather than a guarantee.
> > >
> > > Looking at Documentation/driver-model/devres.txt, the memset is not
> > > mentioned there either.
> > >
> > > > -Original Message-
> > > > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> > sbell.com]
> > > > Sent: Monday, October 30, 2017 7:21 AM
> > > > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > > > <cug...@163.com>; netdev@vger.kernel.org
> > > > Cc: David S . Miller <da...@davemloft.net>
> > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > > > done in per-cpu allocation
> > > >
> > > >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Felixir.f
> ree-
> electrons.com%2Flinux%2Flatest%2Fsource%2Fmm%2Fpercpu.c%23L1018=02%7C
> 01%7Cmadalin.bucur%40nxp.com%7C4116c18371684740c41a08d51f65c6e9%7C686ea1d3
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636449444348895856=%2FOCOMao4jJti
> ptOqUUXBbZMt5TNRXdFfyrqa1zIekAI%3D=0
> > > >
> > > > > -Original Message-
> > > > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > > > Sent: Monday, October 30, 2017 1:15 PM
> > > > > To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu;
> > netdev@vger.kernel.org
> > > > > Cc: David S . Miller
> > > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > > > > done in per-cpu allocation
> > > > >
> > > > > Is the memset part documented?
> > > > > Can you point to the specific comment & code that does it?
> > > > >
> > > > > Thanks
> > > > >
> > > > > > -Original Message-
> > > > > > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> > > > sbell.com]
> > > > > > Sent: Monday, October 30, 2017 7:12

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Madalin-cristian Bucur]

+Eric

> -Original Message-
> From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.y...@nokia-sbell.com]
> 
> I just saw below accepted commit, it said "per cpu allocations are already
> zeroed, no need to clear them again."
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=bfd8e5a407133e58a92a38ccf3d0ba6db81f22d8
> 
> I will not touch memory sub-system, so I will not change this function
> description.
> 

Your particular change removes redundancy, it's fine. Having the memset 
documented
somewhere would prevent similar redundancy from being added in the future.

> > -Original Message-
> > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > Sent: Monday, October 30, 2017 1:56 PM
> > To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu; netdev@vger.kernel.org;
> > gre...@linuxfoundation.org
> > Cc: David S . Miller
> > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done
> in
> > per-cpu allocation
> >
> > Also here:
> >
> >
> > http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L717
> >
> > Should the fact that the memory is zeroed be included in the function
> > description?
> >
> > /**
> >  * pcpu_alloc - the percpu allocator
> >  * @size: size of area to allocate in bytes
> >  * @align: alignment of area (max PAGE_SIZE)
> >  * @reserved: allocate from the reserved chunk if available
> >  * @gfp: allocation flags
> >  *
> >  * Allocate percpu area of @size bytes aligned at @align.  If @gfp
> doesn't
> >  * contain %GFP_KERNEL, the allocation is atomic.
> >  *
> >  * RETURNS:
> >  * Percpu pointer to the allocated area on success, NULL on failure.
> >  */
> >
> > Now it seems to be an implementation detail rather than a guarantee.
> >
> > Looking at Documentation/driver-model/devres.txt, the memset is not
> > mentioned there either.
> >
> > > -Original Message-
> > > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> sbell.com]
> > > Sent: Monday, October 30, 2017 7:21 AM
> > > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > > <cug...@163.com>; netdev@vger.kernel.org
> > > Cc: David S . Miller <da...@davemloft.net>
> > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > > done in per-cpu allocation
> > >
> > > http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L1018
> > >
> > > > -Original Message-
> > > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > > Sent: Monday, October 30, 2017 1:15 PM
> > > > To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu;
> netdev@vger.kernel.org
> > > > Cc: David S . Miller
> > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > > > done in per-cpu allocation
> > > >
> > > > Is the memset part documented?
> > > > Can you point to the specific comment & code that does it?
> > > >
> > > > Thanks
> > > >
> > > > > -Original Message-
> > > > > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> > > sbell.com]
> > > > > Sent: Monday, October 30, 2017 7:12 AM
> > > > > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > > > > <cug...@163.com>; netdev@vger.kernel.org
> > > > > Cc: David S . Miller <da...@davemloft.net>
> > > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > > done in
> > > > > per-cpu allocation
> > > > >
> > > > > Hi,
> > > > >
> > > > > devm_alloc_percpu() will allocate per-cpu memory and memset
> allocated
> > > > > block content to 0.
> > > > >
> > > > > > -Original Message-
> > > > > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > > > > Sent: Monday, October 30, 2017 1:08 PM
> > > > > > To: yuan linyu; netdev@vger.kernel.org
> > > > > > Cc: David S . Miller; Yuan, Linyu (NSB - CN/Shanghai)
> > > > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which
> > > > > > already done in per-cpu allocation
> > > > > >
> > > > > > Hi Yuan,
> > > > > >
> > > > > > Can you please give mo

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Madalin-cristian Bucur]

Also here:

http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L717

Should the fact that the memory is zeroed be included in the function 
description?

/**
 * pcpu_alloc - the percpu allocator
 * @size: size of area to allocate in bytes
 * @align: alignment of area (max PAGE_SIZE)
 * @reserved: allocate from the reserved chunk if available
 * @gfp: allocation flags
 *
 * Allocate percpu area of @size bytes aligned at @align.  If @gfp doesn't
 * contain %GFP_KERNEL, the allocation is atomic.
 *
 * RETURNS:
 * Percpu pointer to the allocated area on success, NULL on failure.
 */

Now it seems to be an implementation detail rather than a guarantee.

Looking at Documentation/driver-model/devres.txt, the memset is not mentioned 
there either.

> -Original Message-
> From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.y...@nokia-sbell.com]
> Sent: Monday, October 30, 2017 7:21 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> <cug...@163.com>; netdev@vger.kernel.org
> Cc: David S . Miller <da...@davemloft.net>
> Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> per-cpu allocation
> 
> http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L1018
> 
> > -----Original Message-
> > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > Sent: Monday, October 30, 2017 1:15 PM
> > To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu; netdev@vger.kernel.org
> > Cc: David S . Miller
> > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done
> in
> > per-cpu allocation
> >
> > Is the memset part documented?
> > Can you point to the specific comment & code that does it?
> >
> > Thanks
> >
> > > -Original Message-
> > > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> sbell.com]
> > > Sent: Monday, October 30, 2017 7:12 AM
> > > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > > <cug...@163.com>; netdev@vger.kernel.org
> > > Cc: David S . Miller <da...@davemloft.net>
> > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> done in
> > > per-cpu allocation
> > >
> > > Hi,
> > >
> > > devm_alloc_percpu() will allocate per-cpu memory and memset allocated
> > > block content to 0.
> > >
> > > > -Original Message-
> > > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > > Sent: Monday, October 30, 2017 1:08 PM
> > > > To: yuan linyu; netdev@vger.kernel.org
> > > > Cc: David S . Miller; Yuan, Linyu (NSB - CN/Shanghai)
> > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> done
> > > in
> > > > per-cpu allocation
> > > >
> > > > Hi Yuan,
> > > >
> > > > Can you please give more details about this change you are
> proposing?
> > > >
> > > > Regards,
> > > > Madalin
> > > >
> > > > > -Original Message-
> > > > > From: netdev-ow...@vger.kernel.org
> > > > [mailto:netdev-ow...@vger.kernel.org]
> > > > > On Behalf Of yuan linyu
> > > > > Sent: Sunday, October 29, 2017 3:49 AM
> > > > > To: netdev@vger.kernel.org
> > > > > Cc: David S . Miller <da...@davemloft.net>; yuan linyu
> > > > > <linyu.y...@alcatel-sbell.com.cn>
> > > > > Subject: [PATCH net-next] net: dpaa: remove init which already
> done in
> > > > > per-cpu allocation
> > > > >
> > > > > From: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > > > >
> > > > > Signed-off-by: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > > > > ---
> > > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 
> > > > >  1 file changed, 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > index a8d0be8..1ccc316 100644
> > > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > @@ -2814,10 +2814,6 @@ static int dpaa_eth_probe(struct
> > > > platform_device
> > > > > *pdev)
> > > > >   err = -ENOMEM;
> > > > >   goto free_dpaa_fqs;
> > > > >   }
> > > > > - for_each_possible_cpu(i) {
> > > > > - percpu_priv = per_cpu_ptr(priv->percpu_priv, i);
> > > > > - memset(percpu_priv, 0, sizeof(*percpu_priv));
> > > > > - }
> > > > >
> > > > >   priv->num_tc = 1;
> > > > >   netif_set_real_num_tx_queues(net_dev, priv->num_tc *
> > > > > DPAA_TC_TXQ_NUM);
> > > > > --
> > > > > 2.7.4
> > > > >

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Madalin-cristian Bucur]

Is the memset part documented?
Can you point to the specific comment & code that does it?

Thanks

> -Original Message-
> From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.y...@nokia-sbell.com]
> Sent: Monday, October 30, 2017 7:12 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> <cug...@163.com>; netdev@vger.kernel.org
> Cc: David S . Miller <da...@davemloft.net>
> Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> per-cpu allocation
> 
> Hi,
> 
> devm_alloc_percpu() will allocate per-cpu memory and memset allocated
> block content to 0.
> 
> > -Original Message-
> > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > Sent: Monday, October 30, 2017 1:08 PM
> > To: yuan linyu; netdev@vger.kernel.org
> > Cc: David S . Miller; Yuan, Linyu (NSB - CN/Shanghai)
> > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done
> in
> > per-cpu allocation
> >
> > Hi Yuan,
> >
> > Can you please give more details about this change you are proposing?
> >
> > Regards,
> > Madalin
> >
> > > -Original Message-
> > > From: netdev-ow...@vger.kernel.org
> > [mailto:netdev-ow...@vger.kernel.org]
> > > On Behalf Of yuan linyu
> > > Sent: Sunday, October 29, 2017 3:49 AM
> > > To: netdev@vger.kernel.org
> > > Cc: David S . Miller <da...@davemloft.net>; yuan linyu
> > > <linyu.y...@alcatel-sbell.com.cn>
> > > Subject: [PATCH net-next] net: dpaa: remove init which already done in
> > > per-cpu allocation
> > >
> > > From: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > >
> > > Signed-off-by: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > > ---
> > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > index a8d0be8..1ccc316 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > @@ -2814,10 +2814,6 @@ static int dpaa_eth_probe(struct
> > platform_device
> > > *pdev)
> > >   err = -ENOMEM;
> > >   goto free_dpaa_fqs;
> > >   }
> > > - for_each_possible_cpu(i) {
> > > - percpu_priv = per_cpu_ptr(priv->percpu_priv, i);
> > > - memset(percpu_priv, 0, sizeof(*percpu_priv));
> > > - }
> > >
> > >   priv->num_tc = 1;
> > >   netif_set_real_num_tx_queues(net_dev, priv->num_tc *
> > > DPAA_TC_TXQ_NUM);
> > > --
> > > 2.7.4
> > >

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Madalin-cristian Bucur]

Hi Yuan,

Can you please give more details about this change you are proposing?

Regards,
Madalin

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of yuan linyu
> Sent: Sunday, October 29, 2017 3:49 AM
> To: netdev@vger.kernel.org
> Cc: David S . Miller ; yuan linyu
> 
> Subject: [PATCH net-next] net: dpaa: remove init which already done in
> per-cpu allocation
> 
> From: yuan linyu 
> 
> Signed-off-by: yuan linyu 
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index a8d0be8..1ccc316 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2814,10 +2814,6 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
>   err = -ENOMEM;
>   goto free_dpaa_fqs;
>   }
> - for_each_possible_cpu(i) {
> - percpu_priv = per_cpu_ptr(priv->percpu_priv, i);
> - memset(percpu_priv, 0, sizeof(*percpu_priv));
> - }
> 
>   priv->num_tc = 1;
>   netif_set_real_num_tx_queues(net_dev, priv->num_tc *
> DPAA_TC_TXQ_NUM);
> --
> 2.7.4
> 

RE: [PATCH v2 2/5] dpaa_eth: move of_phy_connect() to the eth driver

[Madalin-cristian Bucur]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Sunday, October 15, 2017 9:34 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; da...@davemloft.net; f.faine...@gmail.com;
> vivien.dide...@savoirfairelinux.com; jun...@outlook.com; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v2 2/5] dpaa_eth: move of_phy_connect() to the eth
> driver
> 
> On Fri, Oct 13, 2017 at 05:50:09PM +0300, Madalin Bucur wrote:
> > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 48 +++--
> >  drivers/net/ethernet/freescale/fman/mac.c  | 97 ++-
> ---
> >  drivers/net/ethernet/freescale/fman/mac.h  |  5 +-
> >  3 files changed, 66 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 4225806..7cf61d6 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2435,6 +2435,48 @@ static void dpaa_eth_napi_disable(struct
> dpaa_priv *priv)
> > }
> >  }
> >
> > +static void dpaa_adjust_link(struct net_device *net_dev)
> > +{
> > +   struct mac_device *mac_dev;
> > +   struct dpaa_priv *priv;
> > +
> > +   priv = netdev_priv(net_dev);
> > +   mac_dev = priv->mac_dev;
> > +   mac_dev->adjust_link(mac_dev);
> > +}
> > +
> > +static int dpaa_phy_init(struct net_device *net_dev)
> > +{
> > +   struct mac_device *mac_dev;
> > +   struct phy_device *phy_dev;
> > +   struct dpaa_priv *priv;
> > +
> > +   priv = netdev_priv(net_dev);
> > +   mac_dev = priv->mac_dev;
> > +
> > +   phy_dev = of_phy_connect(net_dev, mac_dev->phy_node,
> > +_adjust_link, 0,
> > +mac_dev->phy_if);
> > +   if (!phy_dev) {
> > +   netif_err(priv, ifup, net_dev, "init_phy() failed\n");
> > +   return -ENODEV;
> > +   }
> > +
> > +   /* Remove any features not supported by the controller */
> > +   phy_dev->supported &= mac_dev->if_support;
> > +
> > +   /* Enable the symmetric and asymmetric PAUSE frame advertisements,
> > +* as most of the PHY drivers do not enable them by default.
> > +*/
> 
> Hi Madalin
> 
> This is just moving code around, so the patch is O.K. However, it
> would be nice to have a followup patch. This comment is wrong. The phy
> driver should never enable symmetric and asymmetric PAUSE frames. The
> MAC needs to, because only the MAC knows if the MAC supports pause
> frames.
> 
>   Andrew

Hi Andrew,

This is obsolete and it will be removed, I'll send a v3. It remained there from
a time it used to be valid (the original DPAA Ethernet driver was developed
and maintained out of tree since about 9 years ago). I see this thread on the
subject which is relatively recent:

https://www.spinics.net/lists/netdev/msg404288.html

Thanks,
Madalin

RE: [PATCH v2 5/5] fsl/fman: add dpaa in module names

[Madalin-cristian Bucur]

> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Friday, October 13, 2017 8:39 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>;
> netdev@vger.kernel.org; da...@davemloft.net
> Cc: and...@lunn.ch; vivien.dide...@savoirfairelinux.com;
> jun...@outlook.com; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v2 5/5] fsl/fman: add dpaa in module names
> 
> On 10/13/2017 07:50 AM, Madalin Bucur wrote:
> > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> 
> You should provide a line or two to explain why are you making this
> change, it is to resolve modular build configurations?

This change just renames the FMan driver modules, using a common prefix for
the DPAA FMan and DPAA Ethernet drivers. Besides making the names more aligned,
this allows writing udev rules that match on either driver name, if needed,
using the fsl_dpaa_* prefix. The change of netdev dev required for the DSA
probing makes the previous rules written using this prefix fail, this change
makes them work again.

> > ---
> >  drivers/net/ethernet/freescale/fman/Makefile | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fman/Makefile
> b/drivers/net/ethernet/freescale/fman/Makefile
> > index 2c38119..4ae524a 100644
> > --- a/drivers/net/ethernet/freescale/fman/Makefile
> > +++ b/drivers/net/ethernet/freescale/fman/Makefile
> > @@ -1,9 +1,9 @@
> >  subdir-ccflags-y +=  -I$(srctree)/drivers/net/ethernet/freescale/fman
> >
> > -obj-$(CONFIG_FSL_FMAN) += fsl_fman.o
> > -obj-$(CONFIG_FSL_FMAN) += fsl_fman_port.o
> > -obj-$(CONFIG_FSL_FMAN) += fsl_mac.o
> > +obj-$(CONFIG_FSL_FMAN) += fsl_dpaa_fman.o
> > +obj-$(CONFIG_FSL_FMAN) += fsl_dpaa_fman_port.o
> > +obj-$(CONFIG_FSL_FMAN) += fsl_dpaa_mac.o
> >
> > -fsl_fman-objs  := fman_muram.o fman.o fman_sp.o fman_keygen.o
> > -fsl_fman_port-objs := fman_port.o
> > -fsl_mac-objs:= mac.o fman_dtsec.o fman_memac.o fman_tgec.o
> > +fsl_dpaa_fman-objs := fman_muram.o fman.o fman_sp.o fman_keygen.o
> > +fsl_dpaa_fman_port-objs := fman_port.o
> > +fsl_dpaa_mac-objs:= mac.o fman_dtsec.o fman_memac.o fman_tgec.o
> >
> 
> 
> --
> Florian

RE: [PATCH 3/4] dpaa_eth: change device used

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, October 12, 2017 12:02 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Subject: Re: [PATCH 3/4] dpaa_eth: change device used
> 
> From: Madalin Bucur <madalin.bu...@nxp.com>
> Date: Tue, 10 Oct 2017 17:10:17 +0300
> 
> > @@ -2696,7 +2681,13 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
> > int err = 0, i, channel;
> > struct device *dev;
> >
> > -   dev = >dev;
> > +   /* device used for DMA mapping */
> > +   dev = pdev->dev.parent;
> > +   err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
> > +   if (err) {
> > +   dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> > +   goto dev_mask_failed;
> > +   }
> >
> > /* Allocate this early, so we can store relevant information in
> >  * the private area
> 
> Since you are moving this code up before the netdev allocation, you must
> adjust the failure path goto label used.
> 
> Your change as-is will cause an OOPS because we'll pass a NULL pointer
> to free_netdev().

Thank you, besides this new issue I was introducing I see there other problems,
I'll include a cleanup of these error paths in v2.

Madalin

RE: [PATCH] fsl/fman: remove of_node

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Madalin-cristian Bucur
> Sent: Wednesday, October 04, 2017 12:54 PM
> To: David Miller <da...@davemloft.net>
> Subject: RE: [PATCH] fsl/fman: remove of_node
> 
> > -Original Message-
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Wednesday, October 04, 2017 7:44 AM
> > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> > Cc: netdev@vger.kernel.org; and...@lunn.ch; f.faine...@gmail.com; linux-
> > ker...@vger.kernel.org
> > Subject: Re: [PATCH] fsl/fman: remove of_node
> >
> > From: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> > Date: Tue, 3 Oct 2017 08:49:31 +
> >
> > > My patch removes the of_node that was set to a device that was not an
> > > of_device, preventing duplicated probing of both the real of_device
> > > and the "fake" one created through this assignment.
> > >
> > > I understand that the DSA issue that triggered the initial change
> > > was related to DSA finding the network devices using
> > > of_find_net_device_by_node(), something that will not work for the
> > > DPAA case where the netdevice does not have an of_node. I do not know
> > > enough about DSA to come up with a solution for this problem now.
> > > Andrew, Florian, can you please comment on this?
> >
> > It sounds like you're knowingly breaking DSA.
> 
> It never worked, even with the change I'm reverting.

I'll resend this change as part of a patch set that changes the device
used as net_dev dev to ensure DSA will find a of_device there. To make
that work some changes to adjust link (that also make it cleaner) were
needed. Also, to keep the old udev rules happy, I've changed the names
of the FMan kernel modules from fsl_fman_* to fsl_dpaa_fman*.

I do not have a DSA setup to test so I just tested the part related to
of_find_net_device_by_node() being able to determine the net_device
based on a device tree handle using an artificial device tree and code
construct. I hope that will help the initial reporter of the DSA issue
on DPAA (Junote Cai).

Madalin

RE: [PATCH] fsl/fman: remove of_node

[Madalin-cristian Bucur]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, October 03, 2017 4:01 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: David Miller <da...@davemloft.net>; netdev@vger.kernel.org;
> f.faine...@gmail.com; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] fsl/fman: remove of_node
> 
> On Tue, Oct 03, 2017 at 08:49:31AM +, Madalin-cristian Bucur wrote:
> > > -Original Message-
> > > From: David Miller [mailto:da...@davemloft.net]
> > > Sent: Tuesday, October 03, 2017 2:05 AM
> > > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> > > Subject: Re: [PATCH] fsl/fman: remove of_node
> > >
> > > From: Madalin Bucur <madalin.bu...@nxp.com>
> > > Date: Mon, 2 Oct 2017 13:31:37 +0300
> > >
> > > > The FMan MAC driver allocates a platform device for the Ethernet
> > > > driver to probe on. Setting pdev->dev.of_node with the MAC node
> > > > triggers the MAC driver probing of the new platform device. While
> > > > this fails quickly and does not affect the functionality of the
> > > > drivers, it is incorrect and must be removed. This was added to
> > > > address a report that DSA code using of_find_net_device_by_node()
> > > > is unable to use the DPAA interfaces. Error message seen before
> > > > this fix:
> > > >
> > > > fsl_mac dpaa-ethernet.0: __devm_request_mem_region(mac) failed
> > > > fsl_mac: probe of dpaa-ethernet.0 failed with error -16
> > > >
> > > > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> > >
> > > Is the DSA issue no longer something we need to be concerned
> > > about?  If not, why?  You have to explain this.
> >
> > My patch removes the of_node that was set to a device that was not an
> > of_device, preventing duplicated probing of both the real of_device
> > and the "fake" one created through this assignment.
> >
> > I understand that the DSA issue that triggered the initial change
> > was related to DSA finding the network devices using
> > of_find_net_device_by_node(), something that will not work for the
> > DPAA case where the netdevice does not have an of_node. I do not know
> > enough about DSA to come up with a solution for this problem now.
> > Andrew, Florian, can you please comment on this?
> >
> > Thanks,
> 
> Hi Madalin
> 
> I guess the real fix is to throw away the platform device. But that is
> a big change.
> 
> I've not looked at the code in detail. Why is the platform device
> needed?
> 
>   Andrew

There are multiple components that are aggregated by the DPAA Ethernet
netdevice, the FMan MAC is one of them. There is no entry in the device
tree for the Ethernet device as this is just a software construct that
binds together multiple pieces from the DPAA FMan, QMan, BMan. The probing
of the Ethernet driver is performed against a platform device that is created
in the FMan MAC. Setting the of_node in the new platform device makes it
look like an of_device without being one, thus the errors at the MAC driver
probing against the platform device.

Does DSA work for systems that do not use a device tree to boot, i.e. ACPI?

Madalin

RE: [PATCH] fsl/fman: remove of_node

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Wednesday, October 04, 2017 7:44 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; and...@lunn.ch; f.faine...@gmail.com; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] fsl/fman: remove of_node
> 
> From: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Date: Tue, 3 Oct 2017 08:49:31 +
> 
> > My patch removes the of_node that was set to a device that was not an
> > of_device, preventing duplicated probing of both the real of_device
> > and the "fake" one created through this assignment.
> >
> > I understand that the DSA issue that triggered the initial change
> > was related to DSA finding the network devices using
> > of_find_net_device_by_node(), something that will not work for the
> > DPAA case where the netdevice does not have an of_node. I do not know
> > enough about DSA to come up with a solution for this problem now.
> > Andrew, Florian, can you please comment on this?
> 
> It sounds like you're knowingly breaking DSA.

It never worked, even with the change I'm reverting.

RE: [PATCH] fsl/fman: remove of_node

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, October 03, 2017 2:05 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Subject: Re: [PATCH] fsl/fman: remove of_node
> 
> From: Madalin Bucur <madalin.bu...@nxp.com>
> Date: Mon, 2 Oct 2017 13:31:37 +0300
> 
> > The FMan MAC driver allocates a platform device for the Ethernet
> > driver to probe on. Setting pdev->dev.of_node with the MAC node
> > triggers the MAC driver probing of the new platform device. While
> > this fails quickly and does not affect the functionality of the
> > drivers, it is incorrect and must be removed. This was added to
> > address a report that DSA code using of_find_net_device_by_node()
> > is unable to use the DPAA interfaces. Error message seen before
> > this fix:
> >
> > fsl_mac dpaa-ethernet.0: __devm_request_mem_region(mac) failed
> > fsl_mac: probe of dpaa-ethernet.0 failed with error -16
> >
> > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> 
> Is the DSA issue no longer something we need to be concerned
> about?  If not, why?  You have to explain this.

My patch removes the of_node that was set to a device that was not an
of_device, preventing duplicated probing of both the real of_device
and the "fake" one created through this assignment.

I understand that the DSA issue that triggered the initial change
was related to DSA finding the network devices using 
of_find_net_device_by_node(), something that will not work for the
DPAA case where the netdevice does not have an of_node. I do not know
enough about DSA to come up with a solution for this problem now.
Andrew, Florian, can you please comment on this?

Thanks,
Madalin

RE: [PATCH] fsl/fman: make arrays port_ids static, reduces object code size

[Madalin-cristian Bucur]

> -Original Message-
> From: Colin King [mailto:colin.k...@canonical.com]
> Sent: Thursday, August 31, 2017 4:25 PM
> Subject: [PATCH] fsl/fman: make arrays port_ids static, reduces object
> code size
> 
> From: Colin Ian King 
> 
> Don't populate the arrays port_ids on the stack, instead make them static.
> Makes the object code smaller by over 700 bytes:
> 
> Before:
>text  data bss dec hex filename
>   28785  5832 192   3480987f9 fman.o
> 
> After:
>text  data bss dec hex filename
>   27921  5992 192   341058539 fman.o
> 
> Signed-off-by: Colin Ian King 

Thanks,
Madalin

RE: [PATCH v3 0/7] Add RSS to DPAA 1.x Ethernet driver

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Subject: Re: [PATCH v3 0/7] Add RSS to DPAA 1.x Ethernet driver
> 
> From: David Miller 
> Date: Thu, 24 Aug 2017 09:42:20 -0700 (PDT)
> 
> > From: Madalin Bucur 
> > Date: Thu, 24 Aug 2017 10:28:21 +0300
> >
> >> This patch set introduces Receive Side Scaling for the DPAA Ethernet
> >> driver. Documentation is updated with details related to the new
> >> feature and limitations that apply.
> >> Added also a small fix.
> >>
> >> v2: removed a C++ style comment
> >> v3: move struct fman to header file to avoid exporting a function
> >
> > Series applied, thanks.
> 
> Actually I'm reverting, this doesn't even compile.

Hi,

Sorry for this blunder, I've only tested on PPC, where it works.
Will come back with a proper patch set.

Madalin

RE: [PATCH v2 1/6] fsl/fman: enable FMan Keygen

[Madalin-cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Wednesday, August 23, 2017 7:47 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v2 1/6] fsl/fman: enable FMan Keygen
> 
> From: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Date: Wed, 23 Aug 2017 04:36:56 +
> 
> > The struct fman is only visible in the fman file, the fman port
> > module uses struct fman as an opaque pointer, thus this export.
> 
> Don't use that programming model.
> 
> Export the datastructure properly to it's users.
> 
> This abstraction scheme is so wasteful and costly.

Normally does not come with this cost, it's this case where one of the
sub-modules needs to call into another that gets things complicated.
I'll move struct fman to the header file.

Thanks,
Madalin

RE: [PATCH v2 1/6] fsl/fman: enable FMan Keygen

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of David Miller
> Sent: Wednesday, August 23, 2017 12:35 AM
> Subject: Re: [PATCH v2 1/6] fsl/fman: enable FMan Keygen
> 
> From: Madalin Bucur 
> Date: Tue, 22 Aug 2017 20:31:01 +0300
> 
> >  /**
> > + * fman_get_keygen
> > + *
> > + * @fman:  A Pointer to FMan device
> > + *
> > + * Get the handle to KeyGen module part of FM driver
> > + *
> > + * Return: Handle to KeyGen
> > + */
> > +struct fman_keygen *fman_get_keygen(struct fman *fman)
> > +{
> > +   return fman->keygen;
> > +}
> > +EXPORT_SYMBOL(fman_get_keygen);
> 
> Please don't do this.
> 
> Just directly derefence the pointer in the source code to
> get the keygen.
> 
> Thank you.

Hi,

The struct fman is only visible in the fman file, the fman port module uses 
struct
fman as an opaque pointer, thus this export.

Madalin

RE: [PATCH net-next] fsl/fman: implement several errata workarounds

[Madalin-cristian Bucur]

> -Original Message-
> From: Florinel Iordache [mailto:florinel.iorda...@nxp.com]
> Subject: [PATCH net-next] fsl/fman: implement several errata workarounds
> 
> Implemented workarounds for the following dTSEC Erratum:
> A002, A004, A0012, A0014, A004839 on several operations
> that involve MAC CFG register changes: adjust link,
> rx pause frames, modify MAC address.
> 
> Signed-off-by: Florinel Iordache 

Acked-by: Madalin Bucur 

RE: [PATCH] dpaa_eth: use correct device for DMA mapping API

[Madalin-cristian Bucur]

> -Original Message-
> From: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] On Behalf Of
> Arnd Bergmann
> Subject: Re: [PATCH] dpaa_eth: use correct device for DMA mapping API
> 
> On Tue, Jul 11, 2017 at 10:50 AM, Madalin-cristian Bucur
> <madalin.bu...@nxp.com> wrote:
> 
> > Hi Arnd,
> >
> > Thanks for looking into this, I've tested your fix, it seems to need
> more work:
> >
> > [0.894968]  platform: DMA map failed
> > [0.898627]  platform: DMA map failed
> > [0.902288]  platform: DMA map failed
> > [0.905947]  platform: DMA map failed
> > [0.909606]  platform: DMA map failed
> > [0.913265]  platform: DMA map failed
> 
> I see: the assignment ended up after the first use, so ->dev was still
> NULL here.
> 
> This should fix the problem you saw here:
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index f7b0b928cd53..988c0212ce7e 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2650,6 +2650,8 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
>   for (i = 0; i < DPAA_BPS_NUM; i++) {
>   int err;
> 
> + /* DMA operations are done on the platform-provided device */
> + dpaa_bps[i]->dev = dev->parent;
>   dpaa_bps[i] = dpaa_bp_alloc(dev);

Your new change de-references dpaa_bps[i] before it is set, this won't work 
either.

>   if (IS_ERR(dpaa_bps[i]))
>   return PTR_ERR(dpaa_bps[i]);
> @@ -2657,8 +2659,6 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
>   dpaa_bps[i]->raw_size = bpool_buffer_raw_size(i, DPAA_BPS_NUM);
>   /* avoid runtime computations by keeping the usable size here */
>   dpaa_bps[i]->size = dpaa_bp_size(dpaa_bps[i]->raw_size);
> - /* DMA operations are done on the platform-provided device */
> - dpaa_bps[i]->dev = dev->parent;
> 
>   err = dpaa_bp_alloc_pool(dpaa_bps[i]);
>   if (err < 0) {
> 
> > @@ -2806,7 +2799,6 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
> > dpaa_bps_free(priv);
> >  bp_create_failed:
> >  fq_probe_failed:
> > -dev_mask_failed:
> >  mac_probe_failed:
> > dev_set_drvdata(dev, NULL);
> > free_netdev(net_dev);
> >
> > I'll try to address your concern about performing the DMA mapping on a
> different
> > device than the one set up by the platform code.
> 
> Thanks!
> 
>  Arnd

RE: [PATCH] dpaa_eth: use correct device for DMA mapping API

[Madalin-cristian Bucur]

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Monday, July 10, 2017 6:14 PM
> Subject: [PATCH] dpaa_eth: use correct device for DMA mapping API
> 
> Geert Uytterhoeven ran into a build error without CONFIG_HAS_DMA,
> as a result of the driver calling set_dma_ops(). While we can
> fix the build error in the dma-mapping implementation, there is
> another problem in this driver:
> 
> The configuration for the DMA is done by the platform code,
> looking up information about the system from the device tree.
> This copies the information only in an incomplete way, setting
> the dma_map_ops and forcing a specific mask, but ignoring all
> settings regarding IOMMU, coherence etc.
> 
> A better way to avoid the problem is to only ever pass a device
> into the dma_mapping implementation that has been setup by the
> platform code. In this case, that is the parent device, so we
> can get that pointer at probe time. Fortunately, we already have
> a pointer in the device specific structure for that, so we only
> need to modify that.
> 
> Fixes: fb52728a9294 ("dpaa_eth: reuse the dma_ops provided by the FMan MAC
> device")
> Signed-off-by: Arnd Bergmann 
> ---
> Not tested, please see if this works before applying!
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 11 ++-
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 757b873735a5..f7b0b928cd53 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2646,14 +2646,6 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
>   priv->buf_layout[RX].priv_data_size = DPAA_RX_PRIV_DATA_SIZE; /* Rx
> */
>   priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx
> */
> 
> - /* device used for DMA mapping */
> - set_dma_ops(dev, get_dma_ops(>dev));
> - err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
> - if (err) {
> - dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> - goto dev_mask_failed;
> - }
> -
>   /* bp init */
>   for (i = 0; i < DPAA_BPS_NUM; i++) {
>   int err;
> @@ -2665,7 +2657,8 @@ static int dpaa_eth_probe(struct platform_device
> *pdev)
>   dpaa_bps[i]->raw_size = bpool_buffer_raw_size(i,
> DPAA_BPS_NUM);
>   /* avoid runtime computations by keeping the usable size here
> */
>   dpaa_bps[i]->size = dpaa_bp_size(dpaa_bps[i]->raw_size);
> - dpaa_bps[i]->dev = dev;
> + /* DMA operations are done on the platform-provided device */
> + dpaa_bps[i]->dev = dev->parent;
> 
>   err = dpaa_bp_alloc_pool(dpaa_bps[i]);
>   if (err < 0) {
> --
> 2.9.0

Hi Arnd,

Thanks for looking into this, I've tested your fix, it seems to need more work:

[0.894968]  platform: DMA map failed
[0.898627]  platform: DMA map failed
[0.902288]  platform: DMA map failed
[0.905947]  platform: DMA map failed
[0.909606]  platform: DMA map failed
[0.913265]  platform: DMA map failed

You also missed this related change:

@@ -2806,7 +2799,6 @@ static int dpaa_eth_probe(struct platform_device *pdev)
dpaa_bps_free(priv);
 bp_create_failed:
 fq_probe_failed:
-dev_mask_failed:
 mac_probe_failed:
dev_set_drvdata(dev, NULL);
free_netdev(net_dev);

I'll try to address your concern about performing the DMA mapping on a different
device than the one set up by the platform code.

Regards,
Madalin

RE: [PATCH NET V6 1/2] net: phy: Add phy loopback support in net phy framework

[Madalin-cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Lin Yun Sheng
> Sent: Tuesday, June 27, 2017 2:01 PM
> To: da...@davemloft.net; and...@lunn.ch; f.faine...@gmail.com
> Cc: huangda...@hisilicon.com; xuw...@hisilicon.com;
> liguo...@hisilicon.com; yisen.zhu...@huawei.com;
> gabriele.paol...@huawei.com; john.ga...@huawei.com; linux...@huawei.com;
> yisen.zhu...@huawei.com; salil.me...@huawei.com; lipeng...@huawei.com;
> trem...@gmail.com; netdev@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: [PATCH NET V6 1/2] net: phy: Add phy loopback support in net phy
> framework
> 
> This patch add set_loopback in phy_driver, which is used by Mac
> driver to enable or disable a phy. it also add a generic
> genphy_loopback function, which use BMCR loopback bit to enable
> or disable a phy.

"disable a phy" or disable the PHY loopback function?

> 
> Signed-off-by: Lin Yun Sheng 
> ---
>  drivers/net/phy/marvell.c|  1 +
>  drivers/net/phy/phy_device.c | 51
> 
>  include/linux/phy.h  |  5 +
>  3 files changed, 57 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 57297ba..01a1586 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -2094,6 +2094,7 @@ static int m88e1510_probe(struct phy_device *phydev)
>   .get_sset_count = marvell_get_sset_count,
>   .get_strings = marvell_get_strings,
>   .get_stats = marvell_get_stats,
> + .set_loopback = genphy_loopback,
>   },
>   {
>   .phy_id = MARVELL_PHY_ID_88E1540,
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 1219eea..1e08d62 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1123,6 +1123,39 @@ int phy_resume(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_resume);
> 
> +int phy_loopback(struct phy_device *phydev, bool enable)
> +{
> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
> + int ret = 0;
> +
> + mutex_lock(>lock);
> +
> + if (enable && phydev->loopback_enabled) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + if (!enable && !phydev->loopback_enabled) {
> + ret = -EINVAL;
> + goto out;
> + }
> +

if (enable == phydev->loopback_enabled)

> + if (phydev->drv && phydrv->set_loopback)
> + ret = phydrv->set_loopback(phydev, enable);
> + else
> + ret = -EOPNOTSUPP;
> +
> + if (ret)
> + goto out;
> +
> + phydev->loopback_enabled = enable;
> +
> +out:
> + mutex_unlock(>lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(phy_loopback);
> +
>  /* Generic PHY support and helper functions */
> 
>  /**
> @@ -1628,6 +1661,23 @@ static int gen10g_resume(struct phy_device *phydev)
>   return 0;
>  }
> 
> +int genphy_loopback(struct phy_device *phydev, bool enable)
> +{
> + int value;
> +
> + value = phy_read(phydev, MII_BMCR);
> + if (value < 0)
> + return value;
> +
> + if (enable)
> + value |= BMCR_LOOPBACK;
> + else
> + value &= ~BMCR_LOOPBACK;
> +
> + return phy_write(phydev, MII_BMCR, value);
> +}
> +EXPORT_SYMBOL(genphy_loopback);
> +
>  static int __set_phy_supported(struct phy_device *phydev, u32 max_speed)
>  {
>   /* The default values for phydev->supported are provided by the PHY
> @@ -1874,6 +1924,7 @@ void phy_drivers_unregister(struct phy_driver *drv,
> int n)
>   .read_status= genphy_read_status,
>   .suspend= genphy_suspend,
>   .resume = genphy_resume,
> + .set_loopback   = genphy_loopback,
>  }, {
>   .phy_id = 0x,
>   .phy_id_mask= 0x,
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index e76e4ad..49c903dc 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -364,6 +364,7 @@ struct phy_c45_device_ids {
>   * is_pseudo_fixed_link: Set to true if this phy is an Ethernet switch,
> etc.
>   * has_fixups: Set to true if this phy has fixups/quirks.
>   * suspended: Set to true if this phy has been suspended successfully.
> + * loopback_enabled: Set true if this phy has been loopbacked
> successfully.
>   * state: state of the PHY for management purposes
>   * dev_flags: Device-specific flags used by the PHY driver.
>   * link_timeout: The number of timer firings to wait before the
> @@ -400,6 +401,7 @@ struct phy_device {
>   bool is_pseudo_fixed_link;
>   bool has_fixups;
>   bool suspended;
> + bool loopback_enabled;
> 
>   enum phy_state state;
> 
> @@ -639,6 +641,7 @@ struct phy_driver {
>   int (*set_tunable)(struct phy_device *dev,
>   struct ethtool_tunable *tuna,
>   const void *data);
> + int 

RE: [PATCH 1/2] fsl/fman: propagate dma_ops

[Madalin-cristian Bucur]

> -Original Message-
> From: geert.uytterhoe...@gmail.com [mailto:geert.uytterhoe...@gmail.com]
> On Behalf Of Geert Uytterhoeven
> Sent: Monday, June 26, 2017 7:24 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>;
> linuxppc-...@lists.ozlabs.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 1/2] fsl/fman: propagate dma_ops
> 
> Hi Madalin,
> 
> On Mon, Jun 26, 2017 at 4:55 PM, Madalin-cristian Bucur
> <madalin.bu...@nxp.com> wrote:
> >> -Original Message-
> >> From: geert.uytterhoe...@gmail.com
> [mailto:geert.uytterhoe...@gmail.com]
> >> On Behalf Of Geert Uytterhoeven
> >> Sent: Monday, June 26, 2017 10:49 AM
> >> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> >> Cc: netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>;
> >> linuxppc-...@lists.ozlabs.org; linux-ker...@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] fsl/fman: propagate dma_ops
> >>
> > On Mon, Jun 19, 2017 at 5:04 PM, Madalin Bucur <madalin.bu...@nxp.com>
> >> wrote:
> >> > Make sure dma_ops are set, to be later used by the Ethernet driver.
> >> >
> >> > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> >> > ---
> >> >  drivers/net/ethernet/freescale/fman/mac.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/drivers/net/ethernet/freescale/fman/mac.c
> >> b/drivers/net/ethernet/freescale/fman/mac.c
> >> > index 0b31f85..6e67d22f 100644
> >> > --- a/drivers/net/ethernet/freescale/fman/mac.c
> >> > +++ b/drivers/net/ethernet/freescale/fman/mac.c
> >> > @@ -623,6 +623,8 @@ static struct platform_device
> >> *dpaa_eth_add_device(int fman_id,
> >> > goto no_mem;
> >> > }
> >> >
> >> > +   set_dma_ops(>dev, get_dma_ops(priv->dev));
> >> > +
> >>
> >> When compile-testing with f NO_DMA=y:
> >>
> >> drivers/net/ethernet/freescale/fman/mac.c: In function
> >> ‘dpaa_eth_add_device’:
> >> drivers/net/ethernet/freescale/fman/mac.c:626: error: implicit
> >> declaration of function ‘set_dma_ops’
> >>
> >> Reverting commit 5567e989198b5a8d fixes this regression in v4.12-rc7.
> >>
> >> Why is this change needed?
> >> There's no single other call to the DMA API in this file?
> >
> > We're setting here the dma_ops that are later used in the other
> driver/patch.
> > The problem is we now depend upon DMA but do not explicitly declare it:
> >
> > < HAS_DMA'
> > in its Kconfig>>
> 
> Sure. But only if the driver really uses DMA.
> I can stick a set_dma_ops() call in whatever driver, but that doesn't
> mean it will
> suddenly use DMA.
> Why does the FMan driver suddenly has a dependency on DMA, if it doesn't
> use DMA?
> 
> > I'll need to add this to the FMan driver Kconfig.
> 
> Why does the FMan driver need this?
> Why can't his call be done in the driver that uses the DMA APIO?

The DPAA Ethernet driver makes use of DMA ops. It used to get them from
an API call (arch_setup_dma_ops) that was not exported. The DPAA Ethernet
that makes use of the FMan devices does not get the dma_ops as it does not
probe neither as an OF platform device nor thorough ACPI. It probes as a
platform device based on information prepared by the FMan driver. What the
FMan change [1] does is supplement the information shared with the Ethernet
driver with the dma_ops that the FMan driver gets during OF probing. There
are no scenarios one can use the DPAA drivers with NO_DMA, as far as I know.

For general info on the DPAA drivers please refer to the documentation
found in Documentation/networking/dpaa.txt. For the probing of the Ethernet
driver see change [2] and dpaa_eth_add_device() in fsl/fman, dpaa_eth_probe()
in dpaa_eth.
 
[1] 5567e989198b5a8d fsl/fman: propagate dma_ops
[2] fb52728a9294d97d dpaa_eth: reuse the dma_ops provided by the FMan MAC device

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds

Thanks,
Madalin

RE: [PATCH] fsl/fman: add dependency on HAS_DMA

[Madalin-cristian Bucur]

> -Original Message-
> From: geert.uytterhoe...@gmail.com [mailto:geert.uytterhoe...@gmail.com]
> On Behalf Of Geert Uytterhoeven
> Sent: Monday, June 26, 2017 7:17 PM
> To: Fabio Estevam <feste...@gmail.com>
> Cc: Madalin-cristian Bucur <madalin.bu...@nxp.com>;
> netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>; linuxppc-
> d...@lists.ozlabs.org; linux-kernel <linux-ker...@vger.kernel.org>
> Subject: Re: [PATCH] fsl/fman: add dependency on HAS_DMA
> 
> On Mon, Jun 26, 2017 at 5:20 PM, Fabio Estevam <feste...@gmail.com> wrote:
> > On Mon, Jun 26, 2017 at 12:12 PM, Madalin Bucur <madalin.bu...@nxp.com>
> wrote:
> >> A previous commit inserted a dependency on DMA API that requires
> >> HAS_DMA to be added in Kconfig.
> >
> > It would be nice to specify the commit that caused this.
> 
> That would be commit 5567e989198b5a8d ("fsl/fman: propagate dma_ops").
> 
> However, none of the fman code uses any DMA API calls, so IMHO
> the set_dma_ops() should be done somewhere else.

The Ethernet driver is making use of the DMA ops set here.

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds

RE: [PATCH] fsl/fman: add dependency on HAS_DMA

[Madalin-cristian Bucur]

> -Original Message-
> From: Fabio Estevam [mailto:feste...@gmail.com]
> Sent: Monday, June 26, 2017 6:21 PM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>; Geert
> Uytterhoeven <ge...@linux-m68k.org>; linuxppc-...@lists.ozlabs.org; linux-
> kernel <linux-ker...@vger.kernel.org>
> Subject: Re: [PATCH] fsl/fman: add dependency on HAS_DMA
> 
> On Mon, Jun 26, 2017 at 12:12 PM, Madalin Bucur <madalin.bu...@nxp.com>
> wrote:
> > A previous commit inserted a dependency on DMA API that requires
> > HAS_DMA to be added in Kconfig.
> 
> It would be nice to specify the commit that caused this.

Sent v2, thanks.

Madalin

RE: [PATCH 1/2] fsl/fman: propagate dma_ops

[Madalin-cristian Bucur]

> -Original Message-
> From: geert.uytterhoe...@gmail.com [mailto:geert.uytterhoe...@gmail.com]
> On Behalf Of Geert Uytterhoeven
> Sent: Monday, June 26, 2017 10:49 AM
> To: Madalin-cristian Bucur <madalin.bu...@nxp.com>
> Cc: netdev@vger.kernel.org; David S. Miller <da...@davemloft.net>;
> linuxppc-...@lists.ozlabs.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 1/2] fsl/fman: propagate dma_ops
> 
> Hi Madalin,
> 
> On Mon, Jun 19, 2017 at 5:04 PM, Madalin Bucur <madalin.bu...@nxp.com>
> wrote:
> > Make sure dma_ops are set, to be later used by the Ethernet driver.
> >
> > Signed-off-by: Madalin Bucur <madalin.bu...@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/fman/mac.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/fman/mac.c
> b/drivers/net/ethernet/freescale/fman/mac.c
> > index 0b31f85..6e67d22f 100644
> > --- a/drivers/net/ethernet/freescale/fman/mac.c
> > +++ b/drivers/net/ethernet/freescale/fman/mac.c
> > @@ -623,6 +623,8 @@ static struct platform_device
> *dpaa_eth_add_device(int fman_id,
> > goto no_mem;
> > }
> >
> > +   set_dma_ops(>dev, get_dma_ops(priv->dev));
> > +
> 
> When compile-testing with f NO_DMA=y:
> 
> drivers/net/ethernet/freescale/fman/mac.c: In function
> ‘dpaa_eth_add_device’:
> drivers/net/ethernet/freescale/fman/mac.c:626: error: implicit
> declaration of function ‘set_dma_ops’
> 
> Reverting commit 5567e989198b5a8d fixes this regression in v4.12-rc7.
> 
> Why is this change needed?
> There's no single other call to the DMA API in this file?

We're setting here the dma_ops that are later used in the other driver/patch.
The problem is we now depend upon DMA but do not explicitly declare it:

<>

I'll need to add this to the FMan driver Kconfig. 

> If it's really needed, can't set_dma_ops() be called from the driver that
> needs it, cfr. your other patch "[PATCH 2/2] dpaa_eth: reuse the dma_ops
> provided by the FMan MAC device"?
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds

Thanks,
Madalin

RE: [PATCH] dt-bindings: net: move FMan binding

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, May 15, 2017 5:31 PM
> Subject: Re: [PATCH] dt-bindings: net: move FMan binding
> 
> From: Madalin Bucur 
> Date: Mon, 15 May 2017 16:36:38 +0300
> 
> > Besides the PPC SoCs, the QorIQ DPAA FMan is also present on ARM SoCs,
> > moving the device tree binding document into the bindings/net folder.
> >
> > Signed-off-by: Madalin Bucur 
> 
> What tree is this targetted at for merging?

I hope Rob Herring will take this into his tree, it's a device tree binding
change. 

Thanks,
Madalin

RE: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Madalin-Cristian Bucur]

On March 27, 2017 2:59 PM, Roger Quadros wrote:
> The Ethernet link on an interrupt driven PHY was not coming up if the
> Ethernet cable was plugged before the Ethernet interface was brought up.
> 
> The PHY state machine seems to be stuck from RUNNING to AN state
> with no new interrupts from the PHY. So it doesn't know when the
> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
> state with ANEG done thus netif_carrier_on() is never called.
> 
> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
> advertisement parameters didn't change.
> 
> Fix this by scheduling the PHY state machine in phy_start_aneg().
> There is no way of knowing in phy.c whether auto-negotiation was
> restarted or not by the PHY driver so we just wait for the next
> poll/interrupt to update the PHY state machine.
> 
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and
> not polling.")
> Cc: stable  # v4.9+
> Signed-off-by: Roger Quadros 
> ---
> v3: Fix typo in commit message
> 
>  drivers/net/phy/phy.c | 4 
>  1 file changed, 4 insertions(+)

Tested-by: Madalin Bucur 

RE: [PATCH 0/9] QorIQ DPAA 1 updates

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, February 21, 2017 8:20 PM
> 
> From: Madalin Bucur 
> Date: Tue, 21 Feb 2017 13:52:45 +0200
> 
> > This patch set introduces a series of fixes and features to the DPAA 1
> > drivers. Besides activating hardware Rx checksum offloading, four
> traffic
> > classes are added for Tx traffic prioritisation.
> 
> The net-next tree is closed, please resubmit this after the merge window
> when
> the net-next tree opens back up.
> 
> Thanks.

Sorry, I'll resend.

Meanwhile I should start baking some cakes...

Fixed link for 10G

[Madalin-Cristian Bucur]

Hi Florian,

I'm trying to add a fixed-link property that declares 10G speed
for a XGMII PHY and I'm encountering some issues as the fixed
link infrastructure does not seem to support this speed.

I'm using this device tree snippet (using the legacy format, but it
should not matter):

ethernet@f2000 { /* 10GEC2 */
fixed-link = <0 1 1 0 0>;
phy-connection-type = "xgmii";
};

and I get this error:

[0.464238] swphy: unknown speed
[0.467464] fsl_mac: probe of 1af2000.ethernet failed with error -22

Looking at the code, fixed_phy_register() seems to check for speeds up
to 1G and swphy only caters 1G and lower speeds, the swphy_decode_speed()
returning -EINVAL for 10G, triggering the error printed above in
swphy_validate_state().

What would be the proper way to add support for the 10G fixed link speed?

Thank you,
Madalin

RE: [PATCH net] tcp: add a missing barrier in tcp_tasklet_func()

[Madalin-Cristian Bucur]

> -Original Message-
> From: Eric Dumazet [mailto:eric.duma...@gmail.com]
> Sent: Wednesday, December 21, 2016 3:43 PM
> 
> Madalin reported crashes happening in tcp_tasklet_func() on powerpc64
> 
> Before TSQ_QUEUED bit is cleared, we must ensure the changes done
> by list_del(>tsq_node); are committed to memory, otherwise
> corruption might happen, as an other cpu could catch TSQ_QUEUED
> clearance too soon.
> 
> We can notice that old kernels were immune to this bug, because
> TSQ_QUEUED was cleared after a bh_lock_sock(sk)/bh_unlock_sock(sk)
> section, but they could have missed a kick to write additional bytes,
> when NIC interrupts for a given flow are spread to multiple cpus.
> 
> Affected TCP flows would need an incoming ACK or RTO timer to add more
> packets to the pipe. So overall situation should be better now.
> 
> Fixes: b223feb9de2a ("tcp: tsq: add shortcut in tcp_tasklet_func()")
> Signed-off-by: Eric Dumazet 
> Reported-by: Madalin Bucur 
> Tested-by: Madalin Bucur 

It's actually tested by Xing Lei:

Tested-by: Xing Lei 

Thank you for the fast resolution.

> ---
>  net/ipv4/tcp_output.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index
> b45101f3d2bd2e0f0077305a061add4f7ea0de27..31a255b555ad86a3537c077862e3ea38
> f9b44284 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -769,6 +769,7 @@ static void tcp_tasklet_func(unsigned long data)
>   list_del(>tsq_node);
> 
>   sk = (struct sock *)tp;
> + smp_mb__before_atomic();
>   clear_bit(TSQ_QUEUED, >sk_tsq_flags);
> 
>   if (!sk->sk_lock.owned &&
> 

RE: [upstream-release] [PATCH net v3 2/4] powerpc: fsl/fman: remove fsl, fman from of_device_ids[]

[Madalin-Cristian Bucur]

> From: Scott Wood [mailto:o...@buserror.net]
> Sent: Monday, December 19, 2016 9:46 PM
> 
> On Mon, 2016-12-19 at 18:13 +0200, Madalin Bucur wrote:
> > The fsl/fman drivers will use of_platform_populate() on all
> > supported platforms. Call of_platform_populate() to probe the
> > FMan sub-nodes.
> >
> > Signed-off-by: Igal Liberman 
> > Signed-off-by: Madalin Bucur 
> > ---
> >  arch/powerpc/platforms/85xx/corenet_generic.c | 3 ---
> >  drivers/net/ethernet/freescale/fman/fman.c| 8 
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c
> > b/arch/powerpc/platforms/85xx/corenet_generic.c
> > index 1179115..824b7f1 100644
> > --- a/arch/powerpc/platforms/85xx/corenet_generic.c
> > +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
> > @@ -117,9 +117,6 @@ static const struct of_device_id of_device_ids[] = {
> >     {
> >     .compatible = "fsl,qe",
> >     },
> > -   {
> > -   .compatible= "fsl,fman",
> > -   },
> >     /* The following two are for the Freescale hypervisor */
> >     {
> >     .name   = "hypervisor",
> 
> For this part:
> 
> Acked-by: Scott Wood 

Thank you, added to v4.

> > diff --git a/drivers/net/ethernet/freescale/fman/fman.c
> > b/drivers/net/ethernet/freescale/fman/fman.c
> > index dafd9e1..0b7f711 100644
> > --- a/drivers/net/ethernet/freescale/fman/fman.c
> > +++ b/drivers/net/ethernet/freescale/fman/fman.c
> > @@ -2868,6 +2868,14 @@ static struct fman *read_dts_node(struct
> > platform_device *of_dev)
> >
> >     fman->dev = _dev->dev;
> >
> > +   /* call of_platform_populate in order to probe sub-nodes on arm64
> > */
> > +   err = of_platform_populate(fm_node, NULL, NULL, _dev->dev);
> > +   if (err) {
> > +   dev_err(_dev->dev, "%s: of_platform_populate()
> > failed\n",
> > +   __func__);
> > +   goto fman_free;
> > +   }
> 
> The "on arm64" comment is no longer accurate (and the rest of the comment
> seems unnecessary).
> 
> -Scott

Removed in v4.

RE: [PATCH net v2 2/5] powerpc: remove fsl,fman from of_device_ids[]

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, December 19, 2016 5:37 PM
> 
> From: Madalin Bucur 
> Date: Mon, 19 Dec 2016 11:22:20 +0200
> 
> > The fsl/fman drivers will use of_platform_populate() on all
> > supported platforms.
> >
> > Signed-off-by: Madalin Bucur 
> 
> It seems that this creates a failure point between patches #2 and
> #3.  If the cases handled by this "fsl,fman" entry are only afterwards
> covered by the of_platform_populate() code added in patch #3 then you
> cannot split these two changes up like this.
> 
> The two changes must be done at the same time, otherwise probing will
> fail for some people between patches #2 and #3.

You are right, that will happen. I was not convinced these need to be
merged due to the different places they touch. I'll send a v3.

RE: [upstream-release] [PATCH net 2/4] fsl/fman: arm: call of_platform_populate() for arm64 platfrom

[Madalin-Cristian Bucur]

> From: Scott Wood
> Sent: Thursday, December 15, 2016 8:42 PM
> 
> On 12/15/2016 07:11 AM, Madalin Bucur wrote:
> > From: Igal Liberman 
> >
> > Signed-off-by: Igal Liberman 
> > ---
> >  drivers/net/ethernet/freescale/fman/fman.c | 10 ++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/fman/fman.c
> b/drivers/net/ethernet/freescale/fman/fman.c
> > index dafd9e1..f36b4eb 100644
> > --- a/drivers/net/ethernet/freescale/fman/fman.c
> > +++ b/drivers/net/ethernet/freescale/fman/fman.c
> > @@ -2868,6 +2868,16 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
> >
> > fman->dev = _dev->dev;
> >
> > +#ifdef CONFIG_ARM64
> > +   /* call of_platform_populate in order to probe sub-nodes on arm64 */
> > +   err = of_platform_populate(fm_node, NULL, NULL, _dev->dev);
> > +   if (err) {
> > +   dev_err(_dev->dev, "%s: of_platform_populate() failed\n",
> > +   __func__);
> > +   goto fman_free;
> > +   }
> > +#endif
> 
> Should we remove fsl,fman from the PPC of_device_ids[], so this doesn't
> need an ifdef?
> 
> Why is it #ifdef CONFIG_ARM64 rather than #ifndef CONFIG_PPC?
> 
> -Scott

Igal was working on adding ARM64 support when this patch was created, thus the
choice of #ifdef CONFIG_ARM64. Unifying this for PPC and ARM64 by always calling
of_platform_populate() sounds like the best approach. I would need to 
synchronize
the introduction of this code with the removal of the fsl,fman entry from the
of_device_ids[] array.

Dave, Michael, Scott, is it ok to add to v2 of this patch set the patch that 
removes
the compatible "fsl,fman" from arch/powerpc/platforms/85xx/corenet_generic.c?

Thanks,
Madalin

RE: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

[Madalin-Cristian Bucur]

> From: netdev-ow...@vger.kernel.org On Behalf Of Florian Fainelli
> Sent: Thursday, December 08, 2016 7:54 PM
> To: Johan Hovold 
> On 12/08/2016 09:01 AM, Johan Hovold wrote:
> > On Thu, Dec 08, 2016 at 08:47:54AM -0800, Florian Fainelli wrote:
> >> On 12/08/2016 08:27 AM, Johan Hovold wrote:
> >>> On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
>  Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way
> we
>  dealt with MDIO bus module reference count, but sort of introduced a
>  regression in that, if an Ethernet driver registers its own MDIO bus
>  driver, as is common, we will end up with the Ethernet driver's
>  module->refnct set to 1, thus preventing this driver from any
> removal.
> 
>  Fix this by comparing the network device's device driver owner
> against
>  the MDIO bus driver owner, and only if they are different, increment
> the
>  MDIO bus module refcount.
> 
>  Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
>  Signed-off-by: Florian Fainelli 
>  ---
>  Russell,
> 
>  I verified this against the ethoc driver primarily (on a TS7300
> board)
>  and bcmgenet.
> 
>  Thanks!
> 
>   drivers/net/phy/phy_device.c | 16 +---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
>  diff --git a/drivers/net/phy/phy_device.c
> b/drivers/net/phy/phy_device.c
>  index 1a4bf8acad78..c4ceb082e970 100644
>  --- a/drivers/net/phy/phy_device.c
>  +++ b/drivers/net/phy/phy_device.c
>  @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
>   int phy_attach_direct(struct net_device *dev, struct phy_device
> *phydev,
> u32 flags, phy_interface_t interface)
>   {
>  +struct module *ndev_owner = dev->dev.parent->driver->owner;
> >>>
> >>> Is this really safe? A driver does not need to set a parent device,
> and
> >>> in that case you get a NULL-deref here (I tried using cpsw).
> >>
> >> Humm, cpsw does call SET_NETDEV_DEV() which should take care of that,
> is
> >> the call made too late? Do you have an example oops?
> >
> > Sorry if I was being unclear, cpsw does set a parent device, but there
> > are network driver that do not. Perhaps such drivers will never hit this
> > code path, but I can't say for sure and everything appear to work for
> > cpsw if you comment out that SET_NETDEV_DEV (well, at least before this
> > patch).
> 
> You were clear, I did not understand that you exercised this with cpsw
> to see whether this was safe in all conditions.
> 
> >
> >> I don't mind safeguarding this with a check against dev->dev.parent,
> but
> >> I would like to fix the drivers where relevant too, since
> >> SET_NETDEV_DEV() should really be called, otherwise a number of things
> >> just don't work
> >
> > I grepped for for register_netdev and think I saw a number of drivers
> > which do not call SET_NETDEV_DEV.
> >
> > Again, perhaps they will never hit this path, but thought I should ask.
> 
> You are absolutely right, this is a potential problem, so far I found
> two legitimate drivers that do not call SET_NETDEV_DEV (lantiq_etop.c
> and cpmac.c, both fixed), and Freescale's FMAN driver, which I have a
> hard time understanding what it does with mac_dev->net_dev...
> 
> Thanks!
> --
> Florian

Hi Florian,

The Freescale DPAA Ethernet driver is in drivers/net/ethernet/freescale/dpaa:

drivers/net/ethernet/freescale/dpaa/dpaa_eth.c:2501:SET_NETDEV_DEV(net_dev, 
dev);

and it is making use of the MAC and ports driver in the FMan driver (and of the
QBMan drivers in drivers/soc/fsl/qbman but that's off topic). You need to look
at the net-next tree for this, the drivers were gradually added.

Madalin

RE: [PATCH net v2 4/5] net: fsl/fman: fix fixed-link-phydev reference leak

[Madalin-Cristian Bucur]

> -Original Message-
> From: Johan Hovold [mailto:jhov...@gmail.com] On Behalf Of Johan Hovold
> Sent: Thursday, November 24, 2016 8:22 PM
> 
> Make sure to drop the reference taken by of_phy_find_device() when
> looking up a fixed-link phydev during probe.
> 
> Fixes: 57ba4c9b56d8 ("fsl/fman: Add FMan MAC support")
> Signed-off-by: Johan Hovold 
> ---
>  drivers/net/ethernet/freescale/fman/mac.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/mac.c
> b/drivers/net/ethernet/freescale/fman/mac.c
> index 8fe6b3e253fa..736db9d9b0ad 100644
> --- a/drivers/net/ethernet/freescale/fman/mac.c
> +++ b/drivers/net/ethernet/freescale/fman/mac.c
> @@ -892,6 +892,8 @@ static int mac_probe(struct platform_device *_of_dev)
>   priv->fixed_link->duplex = phy->duplex;
>   priv->fixed_link->pause = phy->pause;
>   priv->fixed_link->asym_pause = phy->asym_pause;
> +
> + put_device(>mdio.dev);
>   }
> 
>   err = mac_dev->init(mac_dev);
> --
> 2.7.3

Thank you,
Madalin

RE: [PATCH net-next v7 03/10] dpaa_eth: add option to use one buffer pool set

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Sunday, November 13, 2016 7:46 PM
> 
> From: Madalin Bucur 
> Date: Fri, 11 Nov 2016 10:20:00 +0200
> 
> > @@ -8,3 +8,12 @@ menuconfig FSL_DPAA_ETH
> >   supporting the Freescale QorIQ chips.
> >   Depends on Freescale Buffer Manager and Queue Manager
> >   driver and Frame Manager Driver.
> > +
> > +if FSL_DPAA_ETH
> > +config FSL_DPAA_ETH_COMMON_BPOOL
> > +   bool "Use a common buffer pool set for all the interfaces"
> > +   ---help---
> > + The DPAA Ethernet netdevices require buffer pools for storing the
> buffers
> > + used by the FMan hardware for reception. One can use a single
> buffer pool
> > + set for all interfaces or a dedicated buffer pool set for each
> interface.
> > +endif # FSL_DPAA_ETH
> 
> This in no way belongs in Kconfig.  If you want to support this,
> support it wit a run time configuration choice via ethtool flags
> or similar.  Do not use debugfs, do not use sysfs, do not use
> module options.
> 
> If you put it in Kconfig, distributions will have to pick one way or
> another which means that users who want the other choice lose.  This
> never works.

I've introduced this Kconfig option as a backwards compatible option, to
be able to run comparative tests between the independent buffer pool setup
and the previous common buffer pool setup. There are not so many reasons
to use the same buffer pool besides "having the old setup", the memory
saving is marginal, in all other aspects the separate buffer pools setup
fares better.

I'll remove this patch from the next submission. Should anyone care for
this I can add an entry to the feature backlog to add runtime support but
it will be quite low in priority.

Thank you for your review. 

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: Madalin-Cristian Bucur
> Sent: Monday, November 07, 2016 5:43 PM
> 
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Thursday, November 03, 2016 9:58 PM
> >
> > From: Madalin Bucur <madalin.bu...@nxp.com>
> > Date: Wed, 2 Nov 2016 22:17:26 +0200
> >
> > > This introduces the Freescale Data Path Acceleration Architecture


> > > + int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
> >  ...
> > > + cpustats = (u64 *)_priv->stats;
> > > +
> > > + for (j = 0; j < numstats; j++)
> > > + netstats[j] += cpustats[j];
> >
> > This is a memcpy() on well-typed datastructures which requires no
> > casting or special handling whatsoever, so use memcpy instead of
> > needlessly open coding the operation.
> 
> Will fix.

Took a second look at this, it's not copying but adding the percpu
statistics into consolidated results.

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 07, 2016 5:55 PM
> 
> From: Madalin-Cristian Bucur <madalin.bu...@nxp.com>
> Date: Mon, 7 Nov 2016 15:43:26 +
> 
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Thursday, November 03, 2016 9:58 PM
> >>
> >> Why?  By clearing this, you disallow an important fundamental way to do
> >> performane testing, via pktgen.
> >
> > The Tx path in DPAA requires one to insert a back-pointer to the skb
> into
> > the Tx buffer. On the Tx confirmation path the back-pointer in the
> buffer
> > is used to release the skb. If Tx buffer is shared we'd alter the back-
> pointer
> > and leak/double free skbs. See also
> 
> Then have your software state store an array of SKB pointers, one for each
> TX ring entry, just like every other driver does.

There is no Tx ring in DPAA. Frames are send out on QMan HW queues towards
the FMan for Tx and then received back on Tx confirmation queues for cleanup.
Array traversal would for sure cost more than using the back-pointer. Also,
we can now process confirmations on a different core than the one doing Tx,
we'd have to keep the arrays percpu and force the Tx conf on the same core.
Or add locks.

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 07, 2016 6:40 PM
> 
> From: Madalin-Cristian Bucur <madalin.bu...@nxp.com>
> Date: Mon, 7 Nov 2016 16:32:16 +
> 
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Monday, November 07, 2016 5:55 PM
> >>
> >> From: Madalin-Cristian Bucur <madalin.bu...@nxp.com>
> >> Date: Mon, 7 Nov 2016 15:43:26 +
> >>
> >> >> From: David Miller [mailto:da...@davemloft.net]
> >> >> Sent: Thursday, November 03, 2016 9:58 PM
> >> >>
> >> >> Why?  By clearing this, you disallow an important fundamental way to
> >> >> do performane testing, via pktgen.
> >> >
> >> > The Tx path in DPAA requires one to insert a back-pointer to the skb
> >> > into
> >> > the Tx buffer. On the Tx confirmation path the back-pointer in the
> >> > buffer
> >> > is used to release the skb. If Tx buffer is shared we'd alter the
> >> > back-pointer
> >> > and leak/double free skbs. See also
> >>
> >> Then have your software state store an array of SKB pointers, one for
> each
> >> TX ring entry, just like every other driver does.
> >
> > There is no Tx ring in DPAA. Frames are send out on QMan HW queues
> > towards the FMan for Tx and then received back on Tx confirmation queues
> > for cleanup.
> > Array traversal would for sure cost more than using the back-pointer.
> > Also, we can now process confirmations on a different core than the one
> > doing Tx,
> > we'd have to keep the arrays percpu and force the Tx conf on the same
> > core. Or add locks.
> 
> Report back an integer index, like every scsi driver out there which
> completes tagged queued block I/O operations asynchronously.  You can
> associate the array with a specific TX confirmation queue.

>From HW? It only gives you back the buffer start address (plus length, etc).
"buff_2_skb()" needs to be solved in SW, expensively using array (lists? As
the number of frames in flight can be large/variable) or cheaply with the back
pointer. The back-pointer approach has its tradeoffs: no shared skbs, imposed
non-zero needed_headroom.

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, November 03, 2016 9:58 PM
> 
> From: Madalin Bucur 
> Date: Wed, 2 Nov 2016 22:17:26 +0200
> 
> > This introduces the Freescale Data Path Acceleration Architecture
> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> > +{
> > +   u8 i;
> > +   size_t res = DPAA_BP_RAW_SIZE / 2;
> 
> Always order local variable declarations from longest to shortest line,
> also know as Reverse Christmas Tree Format.
> 
> Please audit your entire submission for this problem, it occurs
> everywhere.

Thank you, I'll resolve this.

> > +   /* we do not want shared skbs on TX */
> > +   net_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> 
> Why?  By clearing this, you disallow an important fundamental way to do
> performane testing, via pktgen.

The Tx path in DPAA requires one to insert a back-pointer to the skb into
the Tx buffer. On the Tx confirmation path the back-pointer in the buffer
is used to release the skb. If Tx buffer is shared we'd alter the back-pointer
and leak/double free skbs. See also 

static int dpaa_start_xmit(struct sk_buff *skb, struct net_device 
*net_dev)
{
...
  if (!nonlinear) {
/* We're going to store the skb backpointer at the 
beginning
 * of the data buffer, so we need a privately owned skb
   *
 * We've made sure skb is not shared in dev->priv_flags,
 * we need to verify the skb head is not cloned
   */
if (skb_cow_head(skb, priv->tx_headroom))
goto enomem;

  WARN_ON(skb_is_nonlinear(skb));
}
...

> > +   int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
>  ...
> > +   cpustats = (u64 *)_priv->stats;
> > +
> > +   for (j = 0; j < numstats; j++)
> > +   netstats[j] += cpustats[j];
> 
> This is a memcpy() on well-typed datastructures which requires no
> casting or special handling whatsoever, so use memcpy instead of
> needlessly open coding the operation.

Will fix.

> > +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> > +{
> > +   const int max_mtu = dpaa_get_max_mtu();
> > +
> > +   /* Make sure we don't exceed the Ethernet controller's MAXFRM */
> > +   if (new_mtu < 68 || new_mtu > max_mtu) {
> > +   netdev_err(net_dev, "Invalid L3 mtu %d (must be between %d and
> %d).\n",
> > +  new_mtu, 68, max_mtu);
> > +   return -EINVAL;
> > +   }
> > +   net_dev->mtu = new_mtu;
> > +
> > +   return 0;
> > +}
> 
> MTU restrictions are handled in the net-next tree via net_dev->min_mtu and
> net_dev->max_mtu.  Use that and do not define this NDO operation as you do
> not need it.

OK
 
> > +static int dpaa_set_features(struct net_device *dev, netdev_features_t
> features)
> > +{
> > +   /* Not much to do here for now */
> > +   dev->features = features;
> > +   return 0;
> > +}
> 
> Do not define unnecessary NDO operations, let the defaults do their job.
> 
> > +static netdev_features_t dpaa_fix_features(struct net_device *dev,
> > +  netdev_features_t features)
> > +{
> > +   netdev_features_t unsupported_features = 0;
> > +
> > +   /* In theory we should never be requested to enable features that
> > +* we didn't set in netdev->features and netdev->hw_features at
> probe
> > +* time, but double check just to be on the safe side.
> > +*/
> > +   unsupported_features |= NETIF_F_RXCSUM;
> > +
> > +   features &= ~unsupported_features;
> > +
> > +   return features;
> > +}
> 
> Unless you can show that your need this, do not "guess" by implement this
> NDO operation.  You don't need it.

Will remove it.
 
> > +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME
> > +static int dpaa_mac_hw_index_get(struct platform_device *pdev)
> > +{
> > +   struct device *dpaa_dev;
> > +   struct dpaa_eth_data *eth_data;
> > +
> > +   dpaa_dev = >dev;
> > +   eth_data = dpaa_dev->platform_data;
> > +
> > +   return eth_data->mac_hw_id;
> > +}
> > +
> > +static int dpaa_mac_fman_index_get(struct platform_device *pdev)
> > +{
> > +   struct device *dpaa_dev;
> > +   struct dpaa_eth_data *eth_data;
> > +
> > +   dpaa_dev = >dev;
> > +   eth_data = dpaa_dev->platform_data;
> > +
> > +   return eth_data->fman_hw_id;
> > +}
> > +#endif
> 
> Do not play network device naming games like this, use the standard name
> assignment done by the kernel and have userspace entities do geographic or
> device type specific naming.
> 
> I want to see this code completely removed.

I'll remove the option, udev rules like these can achieve the same effect:

SUBSYSTEM=="net", DRIVERS=="fsl_dpa*", ATTR{device_addr}=="ffe4e", 
NAME="fm1-mac1"
SUBSYSTEM=="net", DRIVERS=="fsl_dpa*", ATTR{device_addr}=="ffe4e2000", 
NAME="fm1-mac2"
 
> > +static int 

RE: [net-next 08/13] fsl/fman: check pcsphy pointer before use

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Laight [mailto:david.lai...@aculab.com]
> Sent: Tuesday, October 04, 2016 5:44 PM
> To: Madalin-Cristian Bucur <madalin.bu...@nxp.com>;
> netdev@vger.kernel.org
> Cc: linuxdev.baldr...@gmail.com; linuxppc-...@lists.ozlabs.org;
> da...@davemloft.net; linux-ker...@vger.kernel.org
> Subject: RE: [net-next 08/13] fsl/fman: check pcsphy pointer before use
> 
> From: Madalin Bucur
> > Sent: 04 October 2016 08:33
> > Subject: [net-next 08/13] fsl/fman: check pcsphy pointer before use
> ..
> > --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> > +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> > @@ -507,6 +507,9 @@ static void setup_sgmii_internal_phy(struct
> fman_mac *memac,
> >  {
> > u16 tmp_reg16;
> >
> > +   if (WARN_ON(!memac->pcsphy))
> > +   return;
> > +
> 
> Why?
> 
> Either it can validly be NULL in which case you don't want the message.
> Or it shouldn't be NULL in which case you need to find and fix the bug.
> The later kernel OOPS will make the bug much easier to find.
> 
>   David

You can get into that situation if you have a broken device tree, state into
which someone may get while trying to create the device tree for a new
board. Avoiding a crash here allows the user to look at the device trees as
seen by the kernel / add some debug code if needed.

Madalin

RE: [v9, 6/6] fsl/fman: Add FMan MAC driver

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> 
> From: Scott Wood 
> Date: Tue, 8 Dec 2015 16:44:38 -0600
> 
> > On Tue, 2015-12-08 at 14:18 -0600, Andy Fleming wrote:
> >> On Thu, Dec 3, 2015 at 1:19 AM,   wrote:
> >> > From: Igal Liberman 
> >> >
> >> > This patch adds the Ethernet MAC driver supporting the three
> >> > different types of MACs: dTSEC, tGEC and mEMAC.
> >> >
> >> > Signed-off-by: Igal Liberman 
> >>
> >> [...]
> >>
> >> > +
> >> > +MODULE_LICENSE("Dual BSD/GPL");
> >> > +
> >> > +MODULE_AUTHOR("Emil Medve ");
> >>
> >> I imagine this email address doesn't exist anymore, or won't soon.
> >> This is also an issue in the ethernet driver (with my old address).
> >> I'm not sure what the right approach is, but we shouldn't be putting
> >> obsolete email addresses in the kernel.
> >
> > I don't think a MODULE_AUTHOR tag makes sense for drivers like this that
> had a
> > lot of people work on them.  git history is better for giving credit in
> such
> > cases.
> 
> Agreed.

I've already removed the MODULE_AUTHOR tag from the dpaa_eth code, this will
be removed as well. We are required to keep the Signed-off by: tags of the
original authors for the patches and also were instructed to keep the original
Freescale email addresses even if the patch authors have different email
addresses now. To complicate things further, after the Freescale - NXP merger
the freescale.com employee emails will be replaced by nxp.com email addresses
so our future submissions will have a mix of legacy freescale.com emails for
the former employees and nxp.com for the current ones. Given that the Freescale
brand is replaced with NXP, we may need to reflect this in the folder structure,
prefixes used in the code, etc. Copyright will also need to reflect this change,
any further changes will be attributed to NXP. Is there a norm/custom followed
in such situations?

Madalin
--
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: [net-next v5 0/8] dpaa_eth: Add the Freescale DPAA Ethernet driver

[Madalin-Cristian Bucur]

Hi Timur,

I've managed somehow to make got send-email to move the From: line in the body 
instead of the header, probably typed something wrong when asked to confirm the 
sender. I've resent the series.

Regards,
Madalin

From: Timur Tabi 
Sent: Saturday, December 5, 2015 6:40:11 AM
To: Bucur Madalin-Cristian-B32716
Cc: netdev@vger.kernel.org; linuxppc-...@lists.ozlabs.org; lkml; David Miller; 
Wood Scott-B07421; Liberman Igal-B31950; p...@mindchasers.com; Joe Perches; 
pebo...@tiscali.nl; Joakim Tjernlund; Greg Kroah-Hartman
Subject: Re: [net-next v5 0/8] dpaa_eth: Add the Freescale DPAA Ethernet driver

On Thu, Dec 3, 2015 at 6:08 AM,  <> wrote:
> From: Madalin Bucur 
>
> This patch series adds the Ethernet driver for the Freescale
> QorIQ Data Path Acceleration Architecture (DPAA).

Please fix your git-send-email configuration, so that your emails are
formatted properly.  This is the From: header:

From: <>

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
--
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: [net-next v4 4/8] dpaa_eth: add driver's Tx queue selection

[Madalin-Cristian Bucur]

> -Original Message-
> From: Wood Scott-B07421
> Sent: Wednesday, December 02, 2015 11:41 PM
> 
> On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > Allow the selection of the transmission queue based on the CPU id.
> 
> Explain why.

I'll add more details in the commit log. This is a customer generated
feature. Bypassing the standard XPS can increase performance by making use
of the DPAA HW particularities.

> >
> > Signed-off-by: Madalin Bucur 
> > ---
> >  drivers/net/ethernet/freescale/dpaa/Kconfig   | 10 ++
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c|  3 +++
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.h|  6 ++
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c |  8 
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.h |  4 
> >  5 files changed, 31 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/Kconfig
> > b/drivers/net/ethernet/freescale/dpaa/Kconfig
> > index 022d5aa..2577aac 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/Kconfig
> > +++ b/drivers/net/ethernet/freescale/dpaa/Kconfig
> > @@ -11,6 +11,16 @@ menuconfig FSL_DPAA_ETH
> >
> >  if FSL_DPAA_ETH
> >
> > +config FSL_DPAA_ETH_USE_NDO_SELECT_QUEUE
> > +   bool "Use driver's Tx queue selection mechanism"
> > +   default y
> > +   ---help---
> > + The DPAA Ethernet driver defines a ndo_select_queue() callback
> > for optimal selection
> > + of the egress FQ. That will override the XPS support for this
> > netdevice.
> > + If for whatever reason you want to be in control of the egress FQ
> > -to-CPU selection and mapping,
> > + or simply don't want to use the driver's ndo_select_queue()
> > callback, then unselect this
> > + and use the standard XPS support instead.
> 
> Is there a use case for needing this to be configurable?

If the standard XPS is desired, the Kconfig option allows the driver user to
select that.

> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index 31d55b4..894f1a7 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -390,6 +390,9 @@ static const struct net_device_ops dpa_private_ops =
> {
> > .ndo_get_stats64 = dpa_get_stats64,
> > .ndo_set_mac_address = dpa_set_mac_address,
> > .ndo_validate_addr = eth_validate_addr,
> > +#ifdef CONFIG_FSL_DPAA_ETH_USE_NDO_SELECT_QUEUE
> > +   .ndo_select_queue = dpa_select_queue,
> > +#endif
> > .ndo_change_mtu = dpa_change_mtu,
> > .ndo_set_rx_mode = dpa_set_rx_mode,
> > .ndo_init = dpa_ndo_init,
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > index 1ba6617..87577cf 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> > @@ -420,9 +420,15 @@ static inline void _dpa_assign_wq(struct dpa_fq
> *fq)
> > }
> >  }
> >
> > +#ifdef CONFIG_FSL_DPAA_ETH_USE_NDO_SELECT_QUEUE
> > +/* Use in lieu of skb_get_queue_mapping() */
> > +#define dpa_get_queue_mapping(skb) \
> > +   raw_smp_processor_id()
> > +#else
> >  /* Use the queue selected by XPS */
> >  #define dpa_get_queue_mapping(skb) \
> > skb_get_queue_mapping(skb)
> > +#endif
> 
> Why is this necessary?  Shouldn't providing a custom .ndo_select_queue()
> be
> sufficient to ensure that skb_get_queue_mapping() returns the same thing?

dpa_get_queue_mapping() is used in more than one place, the ndo function cannot
be used directly in all places, the current setup is justified.

> And if this goes away, it's just a matter of a function pointer, so if it
> does
> need to be configurable it could be a runtime option.
> 
> -Scott

It's used on the hot path, adding an extra indirection layer to make it 
selectable
at runtime would defeat the purpose...

Madalin

RE: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> -Original Message-
> From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
> 
> On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > +   if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> > +   if (net_ratelimit())
> > +   netif_warn(priv, hw, net_dev, "FD status =
> 0x%08x\n",
> > +  fd_status & FM_FD_STAT_RX_ERRORS);
> > +
> > +   percpu_stats->rx_errors++;
> > +   goto _release_frame;
> > +   }
> 
> I cannot find any detailed error accounting(maybe I am not looking hard
> enough) but I
> would appreciate if both TX and RX errors where better
> accounted(rx_length_errors, rx_frame_errors,
> rx_crc_errors, rx_fifo_errors etc.). This has helped me many times in the
> past diagnosing
> board HW problems.
> 
>  Jocke

Hi Jocke,

There are some error counters exported through ethtool (used to be debugfs).
FMan HW provides more debug information than we currently export, that will be
improved in the future but given the current priority of having a codebase as
small and reviewable as possible we had to drop some things from the initial
submission.

Madalin
--
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: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> -Original Message-
> From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
> 
> On Tue, 2015-11-03 at 09:37 +0000, Madalin-Cristian Bucur wrote:
> > > -Original Message-
> > > From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
> > >
> > > On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > > > +   if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> > > > +   if (net_ratelimit())
> > > > +   netif_warn(priv, hw, net_dev, "FD status =
> > > 0x%08x\n",
> > > > +  fd_status &
> FM_FD_STAT_RX_ERRORS);
> > > > +
> > > > +   percpu_stats->rx_errors++;
> > > > +   goto _release_frame;
> > > > +   }
> > >
> > > I cannot find any detailed error accounting(maybe I am not looking
> hard
> > > enough) but I
> > > would appreciate if both TX and RX errors where better
> > > accounted(rx_length_errors, rx_frame_errors,
> > > rx_crc_errors, rx_fifo_errors etc.). This has helped me many times in
> the
> > > past diagnosing
> > > board HW problems.
> > >
> > >  Jocke
> >
> > Hi Jocke,
> >
> > There are some error counters exported through ethtool (used to be
> debugfs).
> > FMan HW provides more debug information than we currently export, that
> will be
> > improved in the future but given the current priority of having a
> codebase as
> > small and reviewable as possible we had to drop some things from the
> initial
> > submission.
> 
> I know, but ethtool is not always available.
> Even the old fec_main.c has it:
>   if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO |
>  BD_ENET_RX_CR | BD_ENET_RX_OV)) {
>   ndev->stats.rx_errors++;
>   if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH)) {
>   /* Frame too long or too short. */
>   ndev->stats.rx_length_errors++;
>   }
>   if (status & BD_ENET_RX_NO) /* Frame alignment */
>   ndev->stats.rx_frame_errors++;
>   if (status & BD_ENET_RX_CR) /* CRC Error */
>   ndev->stats.rx_crc_errors++;
>   if (status & BD_ENET_RX_OV) /* FIFO overrun */
>   ndev->stats.rx_fifo_errors++;
>   }
> so it is just a few more lines ... Pretty please ? :)
> 
>  Jocke

It may be more that just a few lines to add complete debug details.
Your request is noted and will be among the first features to work on
after the driver is accepted upstream.

Thanks,
Madalin
--
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: [net-next v4 3/8] dpaa_eth: add support for S/G frames

[Madalin-Cristian Bucur]

> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> 
> On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > Add support for Scater/Gather (S/G) frames. The FMan can place
> > the frame content into multiple buffers and provide a S/G Table
> > (SGT) into one first buffer with references to the others.
> 
> trivia: scatter
> 
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c
> []
> > @@ -1177,10 +1177,42 @@ void dpaa_eth_init_ports(struct mac_device
> *mac_dev,
> >   port_fqs->rx_defq, _layout[RX]);
> >  }
> >
> > +void dpa_release_sgt(struct qm_sg_entry *sgt)
> > +{
> > +   struct dpa_bp *dpa_bp;
> > +   struct bm_buffer bmb[DPA_BUFF_RELEASE_MAX];
> 
> Where is "struct bm_buffer" defined?
> 

Thank you, I'll address this and the other observations you have sent.
The struct bm_buffer is defined in the Buffer Manager driver header file,
in include/soc/fsl/bman.h.

Madalin
--
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: [v3 1/8] devres: add devm_alloc_percpu()

[Madalin-Cristian Bucur]

> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, October 02, 2015 4:01 AM
> 
> On Thu, Sep 24, 2015 at 06:00:12PM +0300, Madalin Bucur wrote:
> > Introduce managed counterparts for alloc_percpu() and free_percpu().
> > Add devm_alloc_percpu() and devm_free_percpu() into the managed
> > interfaces list.
> >
> > Signed-off-by: Madalin Bucur <madalin.bu...@freescale.com>
> > Tested-by: Madalin-Cristian Bucur <madalin.bu...@freescale.com>
> > ---
> >  Documentation/driver-model/devres.txt |  4 +++
> >  drivers/base/devres.c | 64
> +++
> >  include/linux/device.h| 19 +++
> >  3 files changed, 87 insertions(+)
> 
> Greg KH needs to be CCed on any drivers/base changes.
> 
> -Scott

Thank you, I will add him.

Madalin
--
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: [net-next:master 1512/1524] net/ipv4/af_inet.c:1486:26: error: 'offt' undeclared

[Madalin-Cristian Bucur]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> Subject: Re: [net-next:master 1512/1524] net/ipv4/af_inet.c:1486:26: error:
> 'offt' undeclared
> 
> From: kbuild test robot 
> Date: Mon, 31 Aug 2015 13:06:10 +0800
> 
> >net/ipv4/af_inet.c: In function 'snmp_get_cpu_field64':
> >>> net/ipv4/af_inet.c:1486:26: error: 'offt' undeclared (first use in this
> function)
> >   v = *(((u64 *)bhptr) + offt);
> >  ^
> >net/ipv4/af_inet.c:1486:26: note: each undeclared identifier is reported
> only once for each function it appears in
> >net/ipv4/af_inet.c: In function 'snmp_fold_field64':
> >>> net/ipv4/af_inet.c:1499:39: error: 'offct' undeclared (first use in this
> function)
> >   res += snmp_get_cpu_field(mib, cpu, offct, syncp_offset);
> >   ^
> >>> net/ipv4/af_inet.c:1499:10: error: too many arguments to function
> 'snmp_get_cpu_field'
> >   res += snmp_get_cpu_field(mib, cpu, offct, syncp_offset);
> >  ^
> >net/ipv4/af_inet.c:1455:5: note: declared here
> > u64 snmp_get_cpu_field(void __percpu *mib, int cpu, int offt)
> > ^
> >net/ipv4/af_inet.c:1499: confused by earlier errors, bailing out
> 
> Thanks, this should fix it:
> 
> 
> [PATCH] ipv4: Fix 32-bit build.
> 
>net/ipv4/af_inet.c: In function 'snmp_get_cpu_field64':
> >> net/ipv4/af_inet.c:1486:26: error: 'offt' undeclared (first use in this
> function)
>   v = *(((u64 *)bhptr) + offt);
>  ^
>net/ipv4/af_inet.c:1486:26: note: each undeclared identifier is reported
> only once for each function it appears in
>net/ipv4/af_inet.c: In function 'snmp_fold_field64':
> >> net/ipv4/af_inet.c:1499:39: error: 'offct' undeclared (first use in this
> function)
>   res += snmp_get_cpu_field(mib, cpu, offct, syncp_offset);
>   ^
> >> net/ipv4/af_inet.c:1499:10: error: too many arguments to function
> 'snmp_get_cpu_field'
>   res += snmp_get_cpu_field(mib, cpu, offct, syncp_offset);
>  ^
>net/ipv4/af_inet.c:1455:5: note: declared here
> u64 snmp_get_cpu_field(void __percpu *mib, int cpu, int offt)
> ^
> 
> Reported-by: kbuild test robot 
> Signed-off-by: David S. Miller 
> ---
>  net/ipv4/af_inet.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 0c69c0b..c2d0ebc 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1471,7 +1471,7 @@ EXPORT_SYMBOL_GPL(snmp_fold_field);
> 
>  #if BITS_PER_LONG==32
> 
> -u64 snmp_get_cpu_field64(void __percpu *mib, int cpu, int offct,
> +u64 snmp_get_cpu_field64(void __percpu *mib, int cpu, int offt,
>size_t syncp_offset)
>  {
>   void *bhptr;
> @@ -1496,7 +1496,7 @@ u64 snmp_fold_field64(void __percpu *mib, int
> offt, size_t syncp_offset)
>   int cpu;
> 
>   for_each_possible_cpu(cpu) {
> - res += snmp_get_cpu_field(mib, cpu, offct, syncp_offset);
> + res += snmp_get_cpu_field(mib, cpu, offt, syncp_offset);
>   }
>   return res;
>  }
> --
> 2.1.0
> 
> --

Hi,

shouldn't that be snmp_get_cpu_field64() ?

-   res += snmp_get_cpu_field(mib, cpu, offct, syncp_offset);
+   res += snmp_get_cpu_field64(mib, cpu, offt, syncp_offset);

Madalin


--
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: [v2] net: phy: fixed: propagate fixed link values to struct

[Madalin-Cristian Bucur]

 -Original Message-
 From: Stas Sergeev [mailto:s...@list.ru]
 Sent: Wednesday, August 26, 2015 6:51 PM
 To: Bucur Madalin-Cristian-B32716 madalin.bu...@freescale.com;
 f.faine...@gmail.com
 Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org; Liberman Igal-
 B31950 igal.liber...@freescale.com
 Subject: Re: [v2] net: phy: fixed: propagate fixed link values to struct
 
 26.08.2015 17:58, Madalin Bucur пишет:
  The fixed link values parsed from the device tree are stored in
  the struct fixed_phy member status. The struct phy_device members
  speed, duplex were not updated.
 
 ACK, but IMHO it will make more sense if you include that
 into your upcoming patch set rather than sending separately,
 as otherwise there is simply no in-kernel users of that new
 functionality (all the current users likely do not access
 these fields as early as you want to, so they don't care).
 In any case, the patch looks good to me and the policy is
 up to others.

Given that it's more of a fix than a feature, I think it can be picked up 
separate
from a certain driver that accesses those fields early but I guess Florian, 
David
will decide this.

Thanks,
Madalin

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

 -Original Message-
 From: Stas Sergeev [mailto:s...@list.ru]

 12.08.2015 16:26, Madalin-Cristian Bucur пишет:
  I've tried to make the smallest changes that allow me to retrieve those
  without modifying existing API.
  Why is it important to hide the default values from the MAC driver?
  My worry is that the fixed values are not really fixed, and
  therefore are not always useful to access directly. It is likely
  not a problem for your use-case, as, as you say, the AN is
  disabled, but this is probably not the best to do in general.
  Yes, not a problem in my case.
 
  And also you do:
  ---
 
  -  err = of_phy_register_fixed_link(mac_node);
  -  if (err)
  +  struct phy_device *phy;
  +
  +  mac_dev-fixed_link = kzalloc(sizeof(*mac_dev-
  fixed_link),
  +GFP_KERNEL);
  +  if (of_phy_parse_fixed_link(mac_node, mac_dev-
  fixed_link))
  +  goto _return_dev_set_drvdata;
  +
  +  phy = fixed_phy_register(PHY_POLL, mac_dev-fixed_link,
  +   mac_node);
 
  ---
 
  which means you really want to circumvent the current OF
  api quite a lot, without saying why in the patch description.
  I circumvent the API because I din not want to change existing API.
  If I could get a reference to the status struct without changing any code
  or without being required to call by myself fixed_phy_register(), I
  would of done that. Given the existing code in
 of_phy_register_fixed_link(),
  this was my only option. I could have broken of_phy_register_fixed_link()
  in two functions:
 
  of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter
 doing only
  the call to fixed_phy_register()
 
  that would allow to keep of_phy_register_fixed_link() as it is, broken in
 two stages:
 
  - parsing
  - registering
 
  than can be used by other drivers in order to get the status but I think 
  it's
 overkill.
 What I referred to as circumventing an API is that you do
 phy = fixed_phy_register(PHY_POLL, mac_dev-fixed_link, + mac_node);
 by hands, instead of letting the of_phy_register_fixed_link() doing so.
 
 How about a smaller circumvention, like this for instance:
 ---
 err = of_phy_register_fixed_link(mac_node);
 phy = of_phy_find_device(dn);
 status = fixed_phy_get_link_status(phy);// no such func, to be coded up
 ---
 
 Or even like this:
 ---
 err = of_phy_register_fixed_link(mac_node);
 phy = of_phy_find_device(dn);
 set_speed_and_duplex(phy-speed, phy-duplex);// not sure if these
 values are available that early
 ---

After my patch, all that of_phy_register_fixed_link() does is to call
the new parsing function I introduced then register the fixed PHY.
I could have done this (pseudocode):

- add of_phy_parse_fixed_link() as seen in the patch
- add of_phy_register_fixed_phy() that just calls fixed_phy_register():

int of_phy_register_fixed_phy(node)
{
phy = fixed_phy_register(PHY_POLL, mac_dev-fixed_link,
 mac_node);
return (!phy);
}

- change of_phy_register_fixed_link() to contain only this:

int of_phy_register_fixed_link(node)
{
of_phy_parse_fixed_link(node, status);

return of_phy_register_fixed_phy(node);
}

Then I could call only of_* functions but the end result would be the same and
of_phy_register_fixed_phy() would not justify its existence that much...

The getter for status you suggest would be fine, but not sure how one would 
retrieve
it from the mac_node unless we change of_phy_register_fixed_link() to return the
pointer to phy (and all the drivers that use it...).

 
 Also I meant the description should have been in the patch,
 not in the e-mail. :) You only wrote _what_ the patch does
 (which is of course obvious from the code itself), but not
 _why_ and _what was fixed_ (what didn't work).
 


If you refer to the first, separation patch, I thought the description was 
enough:

of: separate fixed link parsing from registration

Some drivers may need to parse the fixed link values before registering
the fixed link phy. Separate the parsing from the actual registration
and provide an export for the added parsing function.

Signed-off-by: Madalin Bucur madalin.bu...@freescale.com

For this one it was a bit brief, I admit - the longer version would be that 
before it
we were not using from fixed link anything else but the fact the link was fixed
(ignored actual speed, duplex values there) and this patch tries to fix that.
In the end this patch will be squashed in a new FMan patch set, let me use that 
as
an excuse for the brief commit log :)

snip

You are concerned about people
  abusing this API to read fixed link status when the link is not really 
  fixed, I'm
 concerned
  about declaring the link as fixed-link when it's not. Maybe the
 naming/binding needs to be
  revised to cover the case when all is fixed but the link.
 Yes, naming

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

 -Original Message-
 From: Stas Sergeev [mailto:s...@list.ru]

snip
 But have you looked into the patch I pointed previously?
 https://lkml.org/lkml/2015/7/20/711
 You code will likely clash with it because my patch extends
 of_phy_register_fixed_link().
 

I admin I failed to grasp the details of your change - the lack of ample context
Lines makes it a bit difficult. I'm sure your change could be merged then the
of parsing could be separated from the actual fixed_phy_register() call if 
anyone
cares about that.

  Then I could call only of_* functions but the end result would be the same
  and  of_phy_register_fixed_phy() would not justify its existence that 
  much...
 You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
 Since it is there (and even changed by me in a way your
 patch will likely clash), IMHO it would be better if it is used,
 rather than copy/pasted into the driver.

Please note I was referring to a fictional new function that would embed the 
call to 
fixed_phy_register(). I was not talking about some existing API, just about a 
new 
of_call  named of_phy_register_fixed_phy()  that would in the end be called by
of_phy_register_fixed_link() and by some drivers that want to get in the middle
of things and get a hold on status.

Maybe the fact we're reviewing two patches in one thread is what makes the
discussion less than optimal.

  The getter for status you suggest would be fine, but not sure how one
  would retrieve
  it from the mac_node unless we change of_phy_register_fixed_link() to
  return the
  pointer to phy (and all the drivers that use it...).
 If you look for instance to mvneta.c, you'll find the following:
 ---
 err = of_phy_register_fixed_link(dn);
 /* In the case of a fixed PHY, the DT node associated
   * to the PHY is the Ethernet MAC DT node.
   */
   phy_node = of_node_get(dn);
 ...
 phy = of_phy_find_device(dn);
 ---
 
 So the answer is: just use the same mac_node for both.

I understand, I'll use this approach although is suboptimal imho to
scan the device tree again to get a phy pointer that you need just
to get some of info that was parsed in a call you just made.

  Also I meant the description should have been in the patch,
  not in the e-mail. :) You only wrote _what_ the patch does
  (which is of course obvious from the code itself), but not
  _why_ and _what was fixed_ (what didn't work).
 
  If you refer to the first, separation patch, I thought the description was
 enough:
 
   of: separate fixed link parsing from registration
 
   Some drivers may need
 may need? I don't understand.
 If it is a fix, then they _do need_, and in this case it should
 be specified what was broken and what is fixed.
 If it is just a clean-up, then may need may suffice, but it
 was not mentioned it is a clean-up. So I still don't know what
 this patch is all about.
 Some drivers - which ones? The ones that are limited to
 the purely fixed links, and never support AN or MDIO?
 Or some other drivers too?
 So really, the description sounds very cryptic to me.

Mine, when there is a fixed link node, maybe others. When there isn't any
fixed link node, the internal PHY config defaults to 1G full duplex AN enabled
and adjust link takes care of things.

 
to parse the fixed link values before registering
   the fixed link phy. Separate the parsing from the actual registration
   and provide an export for the added parsing function.
 
   Signed-off-by: Madalin Bucur madalin.bu...@freescale.com
 
  For this one it was a bit brief, I admit - the longer version would be that
 before it
  we were not using from fixed link anything else but the fact the link was
 fixed
  (ignored actual speed, duplex values there)
 And what didn't work as the result?
 
and this patch tries to fix that.
 What started to work after that patch that didn't without it?

10M half duplex for instance

I'd close this thread for now and use in my driver of_phy_find_device(mac_node).

Thank you,
Madalin

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

 -Original Message-
 From: Stas Sergeev [mailto:s...@list.ru]
 
 11.08.2015 19:33, Madalin-Cristian Bucur пишет:
  + Joakim, Shaohui
 
  -Original Message-
  From: Stas Sergeev [mailto:s...@list.ru]
 
  08.08.2015 20:32, Florian Fainelli пишет:
  CC'ing Stas,
  Hi.
 
  Le 08/05/15 07:42, Madalin Bucur a écrit :
  The FMan MAC configuration code needs the speed and duplex
  information
  for fixed-link interfaces that is parsed now by the of function
  of_phy_register_fixed_link(). This parses the fixed-link parameters but
  does not expose to the caller neither the phy_device pointer nor the
  status struct where it loads the fixed-link params.
snip

  I've tried to make the smallest changes that allow me to retrieve those
  without modifying existing API.
  Why is it important to hide the default values from the MAC driver?
 My worry is that the fixed values are not really fixed, and
 therefore are not always useful to access directly. It is likely
 not a problem for your use-case, as, as you say, the AN is
 disabled, but this is probably not the best to do in general.

Yes, not a problem in my case.

 And also you do:
 ---
 
 - err = of_phy_register_fixed_link(mac_node);
 - if (err)
 + struct phy_device *phy;
 +
 + mac_dev-fixed_link = kzalloc(sizeof(*mac_dev-
 fixed_link),
 +   GFP_KERNEL);
 + if (of_phy_parse_fixed_link(mac_node, mac_dev-
 fixed_link))
 + goto _return_dev_set_drvdata;
 +
 + phy = fixed_phy_register(PHY_POLL, mac_dev-fixed_link,
 +  mac_node);
 
 ---
 
 which means you really want to circumvent the current OF
 api quite a lot, without saying why in the patch description.

I circumvent the API because I din not want to change existing API.
If I could get a reference to the status struct without changing any code 
or without being required to call by myself fixed_phy_register(), I
would of done that. Given the existing code in of_phy_register_fixed_link(),
this was my only option. I could have broken of_phy_register_fixed_link()
in two functions:

of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter doing 
only
the call to fixed_phy_register()

that would allow to keep of_phy_register_fixed_link() as it is, broken in two 
stages:

- parsing
- registering

than can be used by other drivers in order to get the status but I think it's 
overkill.

 As such, it may be difficult to review. Could you please write
 a more complete description to the patch?

To better understand this patch, think of it as just a refactoring of the
of_phy_register_fixed_link() that does two things inside:

- parsing of fixed link node (2 bindings supported)
- register phy by calling fixed_phy_register() in the same way, in the same 
codebase

I've extracted the parsing in a separate function ( following the one function 
should
do one thing rule).

Then I've exported this function to make status available to callers.

 As to your problem: would it be possible to set speed  duplex
 after you do of_phy_connect()? It returns the phy_device
 pointer, and perhaps you can look into phydev-speed and
 phydev-duplex at that point?

It would be possible but un-natural as I'd have probing information only 
available at
runtime. That would just complicate matters for my particular case ans I 
suspect there
will be other drivers that get into this situation. You are concerned about 
people
abusing this API to read fixed link status when the link is not really fixed, 
I'm concerned
about declaring the link as fixed-link when it's not. Maybe the naming/binding 
needs to be
revised to cover the case when all is fixed but the link.

RE: [v2 8/9] dpaa_eth: add debugfs entries

[Madalin-Cristian Bucur]

 -Original Message-
 From: David Miller [mailto:da...@davemloft.net]
 
 From: Madalin Bucur madalin.bu...@freescale.com
 Date: Wed, 5 Aug 2015 18:41:28 +0300
 
  Export per CPU counters through debugfs.
 
  Signed-off-by: Madalin Bucur madalin.bu...@freescale.com
 
 This is absolutely inappropriate.
 
 You can export these just fine via ethtool statistics.  There is zero reason
 to add ugly debugfs crap for something like this.

If you have such a strong adversity about the debugfs, then I'll export these
debug counters through ethtool statistics in the next version.

Madalin
--
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 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

+ Joakim, Shaohui

 -Original Message-
 From: Stas Sergeev [mailto:s...@list.ru]
 
 08.08.2015 20:32, Florian Fainelli пишет:
  CC'ing Stas,
 Hi.
 
  Le 08/05/15 07:42, Madalin Bucur a écrit :
  The FMan MAC configuration code needs the speed and duplex
 information
  for fixed-link interfaces that is parsed now by the of function
  of_phy_register_fixed_link(). This parses the fixed-link parameters but
  does not expose to the caller neither the phy_device pointer nor the
  status struct where it loads the fixed-link params.
 I have only barely touched that code, but IMO both things
 are by design. There are some API deficiencies, and so, many
 drivers still use of_phy_find_device() to circumvent the encapsulation
 and get the phy_device pointer, but this is unlikely a good thing
 to do. I even proposed some API extensions, but there was no
 interest.
 
By extracting the
  fixed-link parsing code from of_phy_register_fixed_link() into a
  separate function the parsed values are made available without changing
  the existing API. This change also removes a small redundancy in the
  previous code calling fixed_phy_register().
 Today, the fixed_link is not always fixed.
 See for example this patch (already mainlined):
 https://lkml.org/lkml/2015/7/20/711
 of_phy_is_fixed_link() returns 'true' if you have
 managed=in-band-status, and so the SGMII in-band status
 can update fixed-link params.
 
 So my question is: why do you even need to know whether
 the link is fixed or not? IIRC you can check the phy_device
 pointer in the adjust_link callback of of_phy_connect() to get
 the current link status values. Why is this not enough for your
 task? Maybe the patch description should be updated to include
 why the current technique is bad, what is actually fixed by the
 change.
 I think using the fixed-link DT values directly is not something
 to be done. The encapsulation is there for a reason, so maybe
 instead we can see what API additions do we need to avoid the
 current limitations that force people to use of_phy_find_device()
 and other work-arounds.

I need to be able to determine the imposed speed and duplex for fixed link
external PHYs because I need to configure the internal PHY with matching
values. If I do not set the same speed, given the fact that AN needs to be off,
there will be no link and no adjust link to fix things later (and the internal 
PHY is
not updated by adjust link anyway). I do not have access at the phy pointer at
the time I need the speed and duplex, to retrieve the defaults from there and
I've tried to make the smallest changes that allow me to retrieve those without
modifying existing API.
Why is it important to hide the default values from the MAC driver?

Thank you,
Madalin

RE: [PATCH 03/10] dpaa_eth: add configurable bpool thresholds

[Madalin-Cristian Bucur]

 -Original Message-
 From: Joe Perches [mailto:j...@perches.com]
 On Wed, 2015-07-22 at 19:16 +0300, Madalin Bucur wrote:
  Allow the user to tweak the refill threshold and the total number
  of buffers in the buffer pool. The provided values are for one CPU.
 
 Any value in making these module parameters instead?

I expect one would (hardly ever) change these to improve some corner
cases then use them with the new values. It may help in the tuning process
but afterwards the bloat to the bootcmd would probably be  a nuisance.

  +config FSL_DPAA_ETH_MAX_BUF_COUNT
  +   int Maximum number of buffers in private bpool
  +   range 64 2048
  +   default 128
  +   ---help---
  + The maximum number of buffers to be by default allocated in the
 DPAA-Ethernet private port's
  + buffer pool. One needn't normally modify this, as it has probably
 been tuned for performance
  + already. This cannot be lower than DPAA_ETH_REFILL_THRESHOLD.
  +
  +config FSL_DPAA_ETH_REFILL_THRESHOLD
  +   int Private bpool refill threshold
  +   range 32 FSL_DPAA_ETH_MAX_BUF_COUNT
  +   default 80
  +   ---help---
  + The DPAA-Ethernet driver will start replenishing buffer pools whose
 count
  + falls below this threshold. This must be related to
 DPAA_ETH_MAX_BUF_COUNT. One needn't normally
  + modify this value unless one has very specific performance reasons.
  +
   config FSL_DPAA_CS_THRESHOLD_1G
  hex Egress congestion threshold on 1G ports
  range 0x1000 0x1000
 
 
--
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 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

 -Original Message-
 From: Joe Perches [mailto:j...@perches.com]
 On Wed, 2015-07-22 at 19:16 +0300, Madalin Bucur wrote:
  This introduces the Freescale Data Path Acceleration Architecture
  (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
  BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
  the Freescale DPAA QorIQ platforms.
 
 trivia:
 
  +static void __hot _dpa_tx_conf(struct net_device   *net_dev,
  +  const struct dpa_priv_s  *priv,
  +  struct dpa_percpu_priv_s *percpu_priv,
  +  const struct qm_fd   *fd,
  +  u32  fqid)
  +{
 []
  +static struct dpa_bp * __cold
  +dpa_priv_bp_probe(struct device *dev)
 
 Do the __hot and __cold markings really matter?
 Some of them may be questionable.

Some may be, yes. I need to go through all of them.

  +static int __init dpa_load(void)
  +{
 []
  +   err = platform_driver_register(dpa_driver);
  +   if (unlikely(err  0)) {
  +   pr_err(KBUILD_MODNAME
  +   : %s:%hu:%s(): platform_driver_register() = %d\n,
  +   KBUILD_BASENAME .c, __LINE__, __func__, err);
  +   }
  +
  +   pr_debug(KBUILD_MODNAME : %s:%s() -\n,
  +KBUILD_BASENAME .c, __func__);
 
 Perhaps these should use pr_fmt

Agree.

  +static void __exit dpa_unload(void)
  +{
  +   pr_debug(KBUILD_MODNAME : - %s:%s()\n,
  +KBUILD_BASENAME .c, __func__);
 
 dynamic debug has __func__ available and perhaps
 the function tracer might be used instead.
 
  diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
 b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
 []
  +#define __hot
 
 curious.
 
 Maybe it'd be good to add a real __hot to compiler.h

They're mostly there to make readers aware the code is critical, any
changes could mess performance.

  +struct dpa_buffer_layout_s {
  +   u16 priv_data_size;
  +   boolparse_results;
  +   booltime_stamp;
  +   boolhash_results;
  +   u16 data_align;
  +};
 
  +struct dpa_fq {
  +   struct qman_fq   fq_base;
  +   struct list_head list;
  +   struct net_device   *net_dev;
 
 some inconsistent indentation here and there

Yes, I've tried to align the style but given the many editors along the time 
the code existed 
there still are areas out of sync.

  +struct dpa_bp {
  +   struct bman_pool*pool;
  +   u8  bpid;
  +   struct device   *dev;
  +   union {
  +   /* The buffer pools used for the private ports are initialized
  +* with target_count buffers for each CPU; at runtime the
  +* number of buffers per CPU is constantly brought back to
 this
  +* level
  +*/
  +   int target_count;
  +   /* The configured value for the number of buffers in the
 pool,
  +* used for shared port buffer pools
  +*/
  +   int config_count;
  +   };
 
 Anonymous unions are relatively rare

We liked the direct access to members...
In this particular case the use is a bit excessive, we can do without it.

  +   struct {
  +   /**
 
 Maybe the /** style should be avoided

Will fix.

  +* All egress queues to a given net device belong to one
  +* (and the same) congestion group.
  +*/
  +   struct qman_cgr cgr;
  +   } cgr_data;
 
 []
 
  +int dpa_stop(struct net_device *net_dev)
  +{
 []
  +   err = mac_dev-stop(mac_dev);
  +   if (unlikely(err  0))
  +   netif_err(priv, ifdown, net_dev, mac_dev-stop() = %d\n,
  + err);
 
 Some of the likely/unlikely uses may not
 be useful/necessary.

In this particular case it's gratuitous, I'll go through all of them.

  +
  +   for_each_port_device(i, mac_dev-port_dev) {
  +   error = fm_port_disable(
  +   fm_port_drv_handle(mac_dev-
 port_dev[i]));
  +   err = error ? error : err;
 
   if (error)
   err = error;
 
 is more obvious to me.

Yes, it's more readable.

Thank you,
Madalin

--
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 RFC 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

Hi Joakim

 -Original Message-
 From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
 Sent: Monday, July 20, 2015 10:57 AM
 To: linuxppc-...@lists.ozlabs.org; netdev@vger.kernel.org; Bucur Madalin-
 Cristian-B32716
 Cc: linux-ker...@vger.kernel.org
 Subject: Re: [PATCH RFC 02/10] dpaa_eth: add support for DPAA Ethernet
 
 On Mon, 2015-07-20 at 09:54 +0200, Joakim Tjernlund wrote:
  On Wed, 2015-04-01 at 19:19 +0300, Madalin Bucur wrote:
   This introduces the Freescale Data Path Acceleration Architecture
   (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
   BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
   the Freescale DPAA QorIQ platforms.
  
   Signed-off-by: Madalin Bucur madalin.bu...@freescale.com
   ---
  
   + snprintf(net_dev-name, IFNAMSIZ, fm%d-mac%d,
   +  dpa_mac_fman_index_get(pdev),
   +  dpa_mac_hw_index_get(pdev));
 
  Should ethernet drivers dictate interface name in user space nowadays?
  I would prefer if you didn't.

The preformatted interface name was thought as a helper for quick interface
identification. It also ensures constant naming of the interfaces, i.e. if you
add/remove PCI network cards. One can make use of udev rules to override
default interface names (eth%d) in userspace.

Another reason for using this is that the interface name was also used for the
debugfs file name and when compiling dpaa_eth as a module there was a
problem with udev concurrently renaming interfaces from eth0 to something
like fmx-macy, making the next probed DPAA interface temporarily get the
eth0 name (before being renamed fmx-macw). Subsequently,
the debugfs_create_file(net_dev-name,...) call failed because of duplicated
names.

If this is considered more of a bug than a feature, I can remove it and only 
change
the naming of the debugfs entries to avoid the udev issue.

  I am trying these patches on a custom T1042 board using Linux 4.1 but
  I cannot get Fixed PHY to work:
  libphy: PHY fixed-0:00 not found
  fsl_dpa dpaa-ethernet.2 eth2: Could not connect to PHY fixed-0:00
  fsl_dpa dpaa-ethernet.2 eth2: init_phy() = -19
 
  Not sure what I have missed here, any ideas?
 
 I meant I am using
 http://git.freescale.com/git/cgit.cgi/ppc/upstream/linux.git/
 on top of 4.1
 
  Jocke

Please make sure you have CONFIG_FIXED_PHY=y in your .config.
Can you please share the device tree part where you've added the fixed-link 
entry?

Madalin
--
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: [RFC,v3,12/12] fsl/fman: Add FMan MAC driver

[Madalin-Cristian Bucur]

Hi Joakim,

It seems we just need to align to the API introduced by Thomas Petazzoni
in 3be2a49e.

Madalin

 -Original Message-
 From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
 Sent: Monday, July 20, 2015 3:16 PM
 To: netdev@vger.kernel.org; Liberman Igal-B31950
 Cc: linuxppc-...@lists.ozlabs.org; linux-ker...@vger.kernel.org; Bucur
 Madalin-Cristian-B32716
 Subject: Re: [RFC,v3,12/12] fsl/fman: Add FMan MAC driver
 
 On Wed, 2015-04-29 at 12:29 +0300, Igal.Liberman wrote:
  From: Igal Liberman igal.liber...@freescale.com
 
  This patch adds the Ethernet MAC driver support.
 
  Signed-off-by: Igal Liberman igal.liber...@freescale.com
  ---
   drivers/net/ethernet/freescale/fman/inc/mac.h |  125 +
   drivers/net/ethernet/freescale/fman/mac/Makefile  |3 +-
   drivers/net/ethernet/freescale/fman/mac/mac-api.c |  605
 +
   drivers/net/ethernet/freescale/fman/mac/mac.c |  527
 ++
   4 files changed, 1259 insertions(+), 1 deletion(-)
   create mode 100644 drivers/net/ethernet/freescale/fman/inc/mac.h
   create mode 100644 drivers/net/ethernet/freescale/fman/mac/mac-api.c
   create mode 100644 drivers/net/ethernet/freescale/fman/mac/mac.c
 
  diff --git a/drivers/net/ethernet/freescale/fman/inc/mac.h
 b/drivers/net/ethernet/freescale/fman/inc/mac.h
  new file mode 100644
  index 000..2d27331
  --- /dev/null
  +++ b/drivers/net/ethernet/freescale/fman/inc/mac.h
 .
  +   /* Get the rest of the PHY information */
  +   mac_dev-phy_node = of_parse_phandle(mac_node, phy-handle,
 0);
  +   if (!mac_dev-phy_node) {
  +   int sz;
  +   const u32 *phy_id = of_get_property(mac_node, fixed-
 link,
  +   sz);
  +   if (!phy_id || sz  sizeof(*phy_id)) {
  +   dev_err(dev, No PHY (or fixed link) found\n);
  +   _errno = -EINVAL;
  +   goto _return_dev_set_drvdata;
  +   }
  +
  +   sprintf(mac_dev-fixed_bus_id, PHY_ID_FMT, fixed-0,
  +   phy_id[0]);
  +   }
 
 The above for fixed PHY does not work for me, changing it to does:
 
 diff --git a/drivers/net/ethernet/freescale/fman/mac/mac.c
 b/drivers/net/ethernet/freescale/fman/mac/mac.c
 index 4eb8f7c..a8be96a 100644
 --- a/drivers/net/ethernet/freescale/fman/mac/mac.c
 +++ b/drivers/net/ethernet/freescale/fman/mac/mac.c
 @@ -42,6 +42,7 @@
  #include linux/module.h
  #include linux/of_address.h
  #include linux/of_platform.h
 +#include linux/of_mdio.h
  #include linux/of_net.h
  #include linux/device.h
  #include linux/phy.h
 @@ -399,7 +400,7 @@ static int __cold mac_probe(struct platform_device
 *_of_dev)
 
 /* Get the rest of the PHY information */
 mac_dev-phy_node = of_parse_phandle(mac_node, phy-handle, 0);
 -   if (!mac_dev-phy_node) {
 +   if (0  !mac_dev-phy_node) {
 int sz;
 const u32 *phy_id = of_get_property(mac_node, fixed-link,
 sz);
 @@ -412,6 +413,16 @@ static int __cold mac_probe(struct platform_device
 *_of_dev)
 sprintf(mac_dev-fixed_bus_id, PHY_ID_FMT, fixed-0,
 phy_id[0]);
 }
 +   if (!mac_dev-phy_node  of_phy_is_fixed_link(mac_node)) {
 +   /*
 +* In the case of a fixed PHY, the DT node associated
 +* to the PHY is the Ethernet MAC DT node.
 +*/
 +   _errno = of_phy_register_fixed_link(mac_node);
 +   if (_errno)
 +   return _errno;
 +   mac_dev-phy_node = of_node_get(mac_node);
 +   }
--
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: [RFC,v3,12/12] fsl/fman: Add FMan MAC driver

[Madalin-Cristian Bucur]

 -Original Message-
 From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
 Sent: Monday, July 20, 2015 3:57 PM
 To: netdev@vger.kernel.org; Liberman Igal-B31950; Bucur Madalin-Cristian-
 B32716
 Cc: linuxppc-...@lists.ozlabs.org; linux-ker...@vger.kernel.org
 Subject: Re: [RFC,v3,12/12] fsl/fman: Add FMan MAC driver
 
 On Mon, 2015-07-20 at 12:28 +, Madalin-Cristian Bucur wrote:
  Hi Joakim,
 
  It seems we just need to align to the API introduced by Thomas Petazzoni
  in 3be2a49e.
 
  Madalin
 
 So it seems, any idea when the next spin will be ready?
 Could you also push it onto
   http://git.freescale.com/git/cgit.cgi/ppc/upstream/linux.git/
 ?
 
  Jocke

We're working on addressing all the feedback received to date (you've just added
a bit more) then we'll re-submit the FMan driver together with the DPAA Ethernet
driver. A push in the public git is also going to take place after the patches 
are sent
for review.

Madalin
--
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: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

 -Original Message-
 From: Eric Dumazet [mailto:eric.duma...@gmail.com]
 
 On Wed, 2015-06-10 at 07:38 +, Madalin-Cristian Bucur wrote:
  Hi Eric,
 
  Can you please tell us if this change would be for the better?
 
  I was about to say yes to this request but checked and no other Ethernet
 driver seems to use the queue trans_start.
  I was able to find your patch net: tx scalability works : trans_start [
 http://patchwork.ozlabs.org/patch/27104/ ] but did not find more about this
 topic.
 
 
 Drivers no longer have to worry about updating trans_start themselves.
 Core networking stack handles this.
 
 Check txq_trans_update() done from netdev_start_xmit()

I'll remove the updates, thank you.

Madalin
N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

 -Original Message-
 From: Eric Dumazet [mailto:eric.duma...@gmail.com]
 
 On Wed, 2015-04-29 at 17:56 +0300, Madalin Bucur wrote:
  This introduces the Freescale Data Path Acceleration Architecture
  (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
  BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
  the Freescale DPAA QorIQ platforms.
 ...
 
  +   /* We're going to store the skb backpointer at the beginning
  +* of the data buffer, so we need a privately owned skb
  +*/
  +
  +   /* Code borrowed from skb_unshare(). */
  +   if (skb_cloned(skb)) {
  +   struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
  +
  +   /* Finally, create a contig FD from this skb */
  +   skb_to_contig_fd(priv, skb, fd, countptr, offset);
  +
  +   kfree_skb(skb);
  +   skb = nskb;
  +   /* skb_copy() has now linearized the skbuff. */
  +   }
  +
 
 You know that TCP packets are clones, right ?
 This code is killing performance.
 
 TCP allows the headers part being modified by a driver if needed.
 
 You should use skb_header_cloned() instead.

Thank you, I'll address this. I plan to do something like this:

+   if (!nonlinear) {
+   /* We're going to store the skb backpointer at the beginning
+* of the data buffer, so we need a privately owned skb
+*/
+
+   /* make sure skb is not shared, skb_cow_head() assumes it's not 
*/
+   skb = skb_share_check(skb, GFP_ATOMIC);
+   if (!skb)
+   goto enomem;
+
+   /* verify the skb head is not cloned */
+   if (skb_cow_head(skb, priv-tx_headroom))
+   goto enomem;
+
+   nonlinear = skb_is_nonlinear(skb);
+   }

but I'm not sure the skb_share_check() is required on tx.
I'm also a bit puzzled by the aliasing between shared and cloned terms
(i.e. skb_unshare() could be named something like skb_unclone();
 the skb_share_check() not only checks but also unshares an skb so
 the already taken skb_unshare() name would probably fit too).

Madalin

RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

Hi Eric,

Can you please tell us if this change would be for the better?

I was about to say yes to this request but checked and no other Ethernet driver 
seems to use the queue trans_start.
I was able to find your patch net: tx scalability works : trans_start [ 
http://patchwork.ozlabs.org/patch/27104/ ] but did not find more about this 
topic.
 
Thank you,
Madalin

 -Original Message-
 From: Xie Jianhua-B29408
 Sent: Wednesday, June 10, 2015 9:00 AM
 To: Bucur Madalin-Cristian-B32716; netdev@vger.kernel.org; linuxppc-
 d...@lists.ozlabs.org
 Cc: linux-ker...@vger.kernel.org
 Subject: RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
 
 
 
  -Original Message-
  From: Linuxppc-dev [mailto:linuxppc-dev-
  bounces+jianhua.xie=freescale@lists.ozlabs.org] On Behalf Of
 Madalin
  Bucur
  Sent: Wednesday, April 29, 2015 10:57 PM
  To: netdev@vger.kernel.org; linuxppc-...@lists.ozlabs.org
  Cc: linux-ker...@vger.kernel.org; Bucur Madalin-Cristian-B32716
  Subject: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
 
  This introduces the Freescale Data Path Acceleration Architecture
  (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
  BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
  the Freescale DPAA QorIQ platforms.
 
 Snip..
 
  +
  +   if (unlikely(dpa_xmit(priv, percpu_stats, queue_mapping, fd)  0))
  +   goto xmit_failed;
  +
  +   net_dev-trans_start = jiffies;
 
 It is probably better to use netdev_queue-trans_start to instead of
 net_dev-trans_start on SMP.
 
 Best Regards,
 Jianhua
 
  +   return NETDEV_TX_OK;
  +
  +xmit_failed:
  +   if (fd.cmd  FM_FD_CMD_FCO) {
  +   (*countptr)--;
  +   dpa_fd_release(net_dev, fd);
  +   percpu_stats-tx_errors++;
  +   return NETDEV_TX_OK;
  +   }
  +   _dpa_cleanup_tx_fd(priv, fd);
  +   percpu_stats-tx_errors++;
  +   dev_kfree_skb(skb);
  +   return NETDEV_TX_OK;
  +}
  --
  1.7.11.7
 
  ___
  Linuxppc-dev mailing list
  linuxppc-...@lists.ozlabs.org
  https://lists.ozlabs.org/listinfo/linuxppc-dev
N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥