[PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Jean Pihet]

Some ethernet controllers (e.g. TI CPSW) pad the frames to a minimum
of 64 bytes before the FCS is appended. This causes an issue with the
KSZ tail tag which could not be the last byte before the FCS.
Solve this by padding the frame to 64 bytes minus the tail tag size,
before the tail tag is added and the frame is passed for transmission.

Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
---
 net/dsa/tag_ksz.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 0a5aa982c60d..0074702dcbbc 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -19,8 +19,13 @@ static struct sk_buff *ksz_common_xmit(struct sk_buff *skb,
 {
 	struct sk_buff *nskb;
 	int padlen;
+	const int min_len = ETH_ZLEN + ETH_FCS_LEN;
 
-	padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len;
+	/*
+	 * Pad to the minimum ethernet frame size, minus the size of the
+	 * tail tag which will be appended at the very end, before the FCS.
+	 */
+	padlen = (skb->len >= min_len) ? 0 : min_len - skb->len - len;
 
 	if (skb_tailroom(skb) >= padlen + len) {
 		/* Let dsa_slave_xmit() free skb */
-- 
2.26.2

[PATCH 2/2] net: dsa: ksz8795: adjust CPU link to host interface

[Jean Pihet]

Add support for RGMII in 100 and 1000 Mbps.

Adjust the CPU port based on the host interface settings: interface
MII type, speed, duplex.

Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
---
 drivers/net/dsa/microchip/ksz8795.c | 93 ++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 36 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 1e101ab56cea..09c1173cc607 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -916,10 +916,53 @@ static void ksz8795_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
 }
 
+static void ksz8795_mii_config(struct ksz_device *dev, struct ksz_port *p)
+{
+	u8 data8;
+
+	/* Configure MII interface for proper network communication. */
+	ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
+	data8 &= ~PORT_INTERFACE_TYPE;
+	data8 &= ~PORT_GMII_1GPS_MODE;
+	switch (p->interface) {
+	case PHY_INTERFACE_MODE_MII:
+		p->phydev.speed = SPEED_100;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		data8 |= PORT_INTERFACE_RMII;
+		p->phydev.speed = SPEED_100;
+		break;
+	case PHY_INTERFACE_MODE_GMII:
+		data8 |= PORT_GMII_1GPS_MODE;
+		data8 |= PORT_INTERFACE_GMII;
+		p->phydev.speed = SPEED_1000;
+		break;
+	default:
+		data8 &= ~PORT_RGMII_ID_IN_ENABLE;
+		data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
+		if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+		    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			data8 |= PORT_RGMII_ID_IN_ENABLE;
+		if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+		    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			data8 |= PORT_RGMII_ID_OUT_ENABLE;
+		/* Support RGMII in 100 and 1000 Mbps */
+		if (p->phydev.speed == SPEED_1000) {
+			data8 |= PORT_GMII_1GPS_MODE;
+		} else {
+			p->phydev.speed = SPEED_100;
+		}
+		data8 |= PORT_INTERFACE_RGMII;
+		break;
+	}
+	ksz_write8(dev, REG_PORT_5_CTRL_6, data8);
+	p->phydev.duplex = 1;
+}
+
 static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
 	struct ksz_port *p = &dev->ports[port];
-	u8 data8, member;
+	u8 member;
 
 	/* enable broadcast storm limit */
 	ksz_port_cfg(dev, port, P_BCAST_STORM_CTRL, PORT_BROADCAST_STORM, true);
@@ -943,41 +986,7 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 				 port);
 			p->interface = dev->compat_interface;
 		}
-
-		/* Configure MII interface for proper network communication. */
-		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
-		data8 &= ~PORT_INTERFACE_TYPE;
-		data8 &= ~PORT_GMII_1GPS_MODE;
-		switch (p->interface) {
-		case PHY_INTERFACE_MODE_MII:
-			p->phydev.speed = SPEED_100;
-			break;
-		case PHY_INTERFACE_MODE_RMII:
-			data8 |= PORT_INTERFACE_RMII;
-			p->phydev.speed = SPEED_100;
-			break;
-		case PHY_INTERFACE_MODE_GMII:
-			data8 |= PORT_GMII_1GPS_MODE;
-			data8 |= PORT_INTERFACE_GMII;
-			p->phydev.speed = SPEED_1000;
-			break;
-		default:
-			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
-			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
-			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
-				data8 |= PORT_RGMII_ID_IN_ENABLE;
-			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
-				data8 |= PORT_RGMII_ID_OUT_ENABLE;
-			data8 |= PORT_GMII_1GPS_MODE;
-			data8 |= PORT_INTERFACE_RGMII;
-			p->phydev.speed = SPEED_1000;
-			break;
-		}
-		ksz_write8(dev, REG_PORT_5_CTRL_6, data8);
-		p->phydev.duplex = 1;
-
+        ksz8795_mii_config(dev, p);
 		member = dev->port_mask;
 	} else {
 		member = dev->host_mask | p->vid_member;
@@ -1102,11 +1111,23 @@ static int ksz8795_setup(struct dsa_switch *ds)
 	return 0;
 }
 
+void ksz8795_adjust_link(struct dsa_switch *ds, int port,
+						 struct phy_device *phydev)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p = &dev->ports[port];
+
+	/* Adjust the link interface mode and speed for the CPU port */
+	if (port == dev->cpu_port)
+		ksz8795_mii_config(dev, p);
+}
+
 static const struct dsa_switch_ops ksz8795_switch_ops = {
 	.get_tag_protocol	= ksz8795_get_tag_protocol,
 	.setup			= ksz8795_setup,
 	.phy_read		= ksz_phy_read16,
 	.phy_write		= ksz_phy_write16,
+	.adjust_link		= ksz8795_adjust_link,
 	.phylink_mac_link_down	= ksz_mac_link_down,
 	.port_enable		= ksz_enable_port,
 	.get_strings		= ksz8795_get_strings,
-- 
2.26.2

Re: [PATCH 2/2] net: dsa: ksz8795: adjust CPU link to host interface

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

Hi Jean,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on ipvs/master linus/master v5.10-rc5]
[cannot apply to net-next/master next-20201127]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jean-Pihet/net-dsa-ksz-pad-frame-to-64-bytes-for-transmission/20201129-182750
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 4d521943f76bd0d1e68ea5e02df7aadd30b2838a
config: i386-randconfig-s031-20201129 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-170-g3bc348f6-dirty
        # https://github.com/0day-ci/linux/commit/5a255bc7fbe20060c6f86dfaa0bbe9fbcd024128
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Pihet/net-dsa-ksz-pad-frame-to-64-bytes-for-transmission/20201129-182750
        git checkout 5a255bc7fbe20060c6f86dfaa0bbe9fbcd024128
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/net/dsa/microchip/ksz8795.c:1114:6: sparse: sparse: symbol 'ksz8795_adjust_link' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40916 bytes --]

[RFC PATCH] net: dsa: ksz8795: ksz8795_adjust_link() can be static

[kernel test robot]

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 ksz8795.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 09c1173cc6073c..834a8dc251adba 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1111,7 +1111,7 @@ static int ksz8795_setup(struct dsa_switch *ds)
 	return 0;
 }
 
-void ksz8795_adjust_link(struct dsa_switch *ds, int port,
+static void ksz8795_adjust_link(struct dsa_switch *ds, int port,
 						 struct phy_device *phydev)
 {
 	struct ksz_device *dev = ds->priv;

Re: [PATCH 2/2] net: dsa: ksz8795: adjust CPU link to host interface

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

Hi Jean,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on ipvs/master linus/master v5.10-rc5]
[cannot apply to net-next/master next-20201127]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jean-Pihet/net-dsa-ksz-pad-frame-to-64-bytes-for-transmission/20201129-182750
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 4d521943f76bd0d1e68ea5e02df7aadd30b2838a
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/5a255bc7fbe20060c6f86dfaa0bbe9fbcd024128
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Pihet/net-dsa-ksz-pad-frame-to-64-bytes-for-transmission/20201129-182750
        git checkout 5a255bc7fbe20060c6f86dfaa0bbe9fbcd024128
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/dsa/microchip/ksz8795.c:1114:6: warning: no previous prototype for 'ksz8795_adjust_link' [-Wmissing-prototypes]
    1114 | void ksz8795_adjust_link(struct dsa_switch *ds, int port,
         |      ^~~~~~~~~~~~~~~~~~~

vim +/ksz8795_adjust_link +1114 drivers/net/dsa/microchip/ksz8795.c

  1113	
> 1114	void ksz8795_adjust_link(struct dsa_switch *ds, int port,
  1115							 struct phy_device *phydev)
  1116	{
  1117		struct ksz_device *dev = ds->priv;
  1118		struct ksz_port *p = &dev->ports[port];
  1119	
  1120		/* Adjust the link interface mode and speed for the CPU port */
  1121		if (port == dev->cpu_port)
  1122			ksz8795_mii_config(dev, p);
  1123	}
  1124	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77237 bytes --]

Re: [PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Andrew Lunn]

On Sun, Nov 29, 2020 at 11:23:59AM +0100, Jean Pihet wrote:
> Some ethernet controllers (e.g. TI CPSW) pad the frames to a minimum
> of 64 bytes before the FCS is appended. This causes an issue with the
> KSZ tail tag which could not be the last byte before the FCS.
> Solve this by padding the frame to 64 bytes minus the tail tag size,
> before the tail tag is added and the frame is passed for transmission.

Hi Jean

what tree is this based on? Have you seen

commit 88fda8eefd9a7a7175bf4dad1d02cc0840581111
Author: Christian Eggers <ceggers@arri.de>
Date:   Sun Nov 1 21:16:10 2020 +0200

    net: dsa: tag_ksz: don't allocate additional memory for padding/tagging
    
    The caller (dsa_slave_xmit) guarantees that the frame length is at least
    ETH_ZLEN and that enough memory for tail tagging is available.

Re: [PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Andrew Lunn]

Hi Jean

Please also include a patch 0/X which describes the patchset as a
whole. This will be used as the branch merge commit.

       Andrew

Re: [PATCH 2/2] net: dsa: ksz8795: adjust CPU link to host interface

[Andrew Lunn]

On Sun, Nov 29, 2020 at 11:24:00AM +0100, Jean Pihet wrote:
> Add support for RGMII in 100 and 1000 Mbps.
> 
> Adjust the CPU port based on the host interface settings: interface
> MII type, speed, duplex.

Please could you add some extra information here why this is needed. I
suspect you have back to back PHYs on the CPU port?

> +void ksz8795_adjust_link(struct dsa_switch *ds, int port,
> +						 struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	/* Adjust the link interface mode and speed for the CPU port */
> +	if (port == dev->cpu_port)

dsa_is_cpu_port(ds, port)

		    Andrew

Re: [PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Jean Pihet]

Hi Andrew,

On Sun, Nov 29, 2020 at 5:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Nov 29, 2020 at 11:23:59AM +0100, Jean Pihet wrote:
> > Some ethernet controllers (e.g. TI CPSW) pad the frames to a minimum
> > of 64 bytes before the FCS is appended. This causes an issue with the
> > KSZ tail tag which could not be the last byte before the FCS.
> > Solve this by padding the frame to 64 bytes minus the tail tag size,
> > before the tail tag is added and the frame is passed for transmission.
>
> Hi Jean
>
> what tree is this based on? Have you seen
The patches are based on the latest mainline v5.10-rc5. Is this the
recommended version to submit new patches?

>
> commit 88fda8eefd9a7a7175bf4dad1d02cc0840581111
> Author: Christian Eggers <ceggers@arri.de>
> Date:   Sun Nov 1 21:16:10 2020 +0200
>
>     net: dsa: tag_ksz: don't allocate additional memory for padding/tagging
>
>     The caller (dsa_slave_xmit) guarantees that the frame length is at least
>     ETH_ZLEN and that enough memory for tail tagging is available.
>

I cannot find this commit. Which tree/branch is it from?

Thanks for reviewing,
Jean

Re: [PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Jean Pihet]

Andrew,

On Sun, Nov 29, 2020 at 5:57 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Hi Jean
>
> Please also include a patch 0/X which describes the patchset as a
> whole. This will be used as the branch merge commit.

Sure, will submit a v2 asap.

Thx,
Jean

>
>        Andrew

Re: [PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Andrew Lunn]

On Sun, Nov 29, 2020 at 08:34:27PM +0100, Jean Pihet wrote:
> Hi Andrew,
> 
> On Sun, Nov 29, 2020 at 5:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sun, Nov 29, 2020 at 11:23:59AM +0100, Jean Pihet wrote:
> > > Some ethernet controllers (e.g. TI CPSW) pad the frames to a minimum
> > > of 64 bytes before the FCS is appended. This causes an issue with the
> > > KSZ tail tag which could not be the last byte before the FCS.
> > > Solve this by padding the frame to 64 bytes minus the tail tag size,
> > > before the tail tag is added and the frame is passed for transmission.
> >
> > Hi Jean
> >
> > what tree is this based on? Have you seen
> The patches are based on the latest mainline v5.10-rc5. Is this the
> recommended version to submit new patches?

No, that is old. Please take a read of:

https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

	Andrew

Re: [PATCH 1/2] net: dsa: ksz: pad frame to 64 bytes for transmission

[Jean Pihet]

Andrew,

On Sun, Nov 29, 2020 at 8:38 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Nov 29, 2020 at 08:34:27PM +0100, Jean Pihet wrote:
> > Hi Andrew,
> >
> > On Sun, Nov 29, 2020 at 5:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Sun, Nov 29, 2020 at 11:23:59AM +0100, Jean Pihet wrote:
> > > > Some ethernet controllers (e.g. TI CPSW) pad the frames to a minimum
> > > > of 64 bytes before the FCS is appended. This causes an issue with the
> > > > KSZ tail tag which could not be the last byte before the FCS.
> > > > Solve this by padding the frame to 64 bytes minus the tail tag size,
> > > > before the tail tag is added and the frame is passed for transmission.
> > >
> > > Hi Jean
> > >
> > > what tree is this based on? Have you seen
> > The patches are based on the latest mainline v5.10-rc5. Is this the
> > recommended version to submit new patches?
>
> No, that is old. Please take a read of:
>
> https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

Ok got it, thx!

Found the commit 88fda8ee and its parent [1] with the following
comment, which seems to indicate that my patch is not needed anymore.
Can you confirm?

/* For tail taggers, we need to pad short frames ourselves, to ensure
+ * that the tail tag does not fail at its role of being at the end of
+ * the packet, once the master interface pads the frame. Account for
+ * that pad length here, and pad later.
...

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2

Thx,
Jean

>
>         Andrew