linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
@ 2018-03-27 13:10 Razvan Stefanescu
  2018-03-27 13:37 ` Andrew Lunn
  2018-03-28 10:15 ` kbuild test robot
  0 siblings, 2 replies; 4+ messages in thread
From: Razvan Stefanescu @ 2018-03-27 13:10 UTC (permalink / raw)
  To: gregkh
  Cc: devel, linux-kernel, netdev, andrew, alexandru.marginean,
	ruxandra.radulescu, ioana.ciornei, laurentiu.tudor, stuyoder

Previous implementation overwrites PCP value, assuming the default value is
0, instead of 7.

Avoid this by modifying helper function ethsw_port_set_tci() to
ethsw_port_set_pvid() and make it update only the vlan_id of the tci_cfg
struct.

Signed-off-by: Razvan Stefanescu <razvan.stefanescu@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h | 13 +++++++++
 drivers/staging/fsl-dpaa2/ethsw/dpsw.c     | 42 ++++++++++++++++++++++++++++++
 drivers/staging/fsl-dpaa2/ethsw/dpsw.h     |  6 +++++
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c    | 37 +++++++++++++-------------
 4 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h b/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h
index 1c203e6..da744f2 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h
+++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h
@@ -49,6 +49,8 @@
 #define DPSW_CMDID_IF_SET_FLOODING          DPSW_CMD_ID(0x047)
 #define DPSW_CMDID_IF_SET_BROADCAST         DPSW_CMD_ID(0x048)
 
+#define DPSW_CMDID_IF_GET_TCI               DPSW_CMD_ID(0x04A)
+
 #define DPSW_CMDID_IF_SET_LINK_CFG          DPSW_CMD_ID(0x04C)
 
 #define DPSW_CMDID_VLAN_ADD                 DPSW_CMD_ID(0x060)
@@ -206,6 +208,17 @@ struct dpsw_cmd_if_set_tci {
 	__le16 conf;
 };
 
+struct dpsw_cmd_if_get_tci {
+	__le16 if_id;
+};
+
+struct dpsw_rsp_if_get_tci {
+	__le16 pad;
+	__le16 vlan_id;
+	u8 dei;
+	u8 pcp;
+};
+
 #define DPSW_STATE_SHIFT	0
 #define DPSW_STATE_SIZE		4
 
diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw.c b/drivers/staging/fsl-dpaa2/ethsw/dpsw.c
index 9b9bc60..3ea957c 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/dpsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw.c
@@ -529,6 +529,48 @@ int dpsw_if_set_tci(struct fsl_mc_io *mc_io,
 }
 
 /**
+ * dpsw_if_get_tci() - Get default VLAN Tag Control Information (TCI)
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPSW object
+ * @if_id:	Interface Identifier
+ * @cfg:	Tag Control Information Configuration
+ *
+ * Return:	Completion status. '0' on Success; Error code otherwise.
+ */
+int dpsw_if_get_tci(struct fsl_mc_io *mc_io,
+		    u32 cmd_flags,
+		    u16 token,
+		    u16 if_id,
+		    struct dpsw_tci_cfg *cfg)
+{
+	struct mc_command cmd = { 0 };
+	struct dpsw_cmd_if_get_tci *cmd_params;
+	struct dpsw_rsp_if_get_tci *rsp_params;
+	int err;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPSW_CMDID_IF_GET_TCI,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpsw_cmd_if_get_tci *)cmd.params;
+	cmd_params->if_id = cpu_to_le16(if_id);
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	/* retrieve response parameters */
+	rsp_params = (struct dpsw_rsp_if_get_tci *)cmd.params;
+	cfg->pcp = rsp_params->pcp;
+	cfg->dei = rsp_params->dei;
+	cfg->vlan_id = le16_to_cpu(rsp_params->vlan_id);
+
+	return 0;
+}
+
+/**
  * dpsw_if_set_stp() - Function sets Spanning Tree Protocol (STP) state.
  * @mc_io:	Pointer to MC portal's I/O object
  * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw.h b/drivers/staging/fsl-dpaa2/ethsw/dpsw.h
index 3335add..82f80c40 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/dpsw.h
+++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw.h
@@ -306,6 +306,12 @@ int dpsw_if_set_tci(struct fsl_mc_io *mc_io,
 		    u16 if_id,
 		    const struct dpsw_tci_cfg *cfg);
 
+int dpsw_if_get_tci(struct fsl_mc_io *mc_io,
+		    u32 cmd_flags,
+		    u16 token,
+		    u16 if_id,
+		    struct dpsw_tci_cfg *cfg);
+
 /**
  * enum dpsw_stp_state - Spanning Tree Protocol (STP) states
  * @DPSW_STP_STATE_BLOCKING: Blocking state
diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index c723a04..ab81a6c 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -50,14 +50,23 @@ static int ethsw_add_vlan(struct ethsw_core *ethsw, u16 vid)
 	return 0;
 }
 
-static int ethsw_port_set_tci(struct ethsw_port_priv *port_priv,
-			      struct dpsw_tci_cfg *tci_cfg)
+static int ethsw_port_set_pvid(struct ethsw_port_priv *port_priv, u16 pvid)
 {
 	struct ethsw_core *ethsw = port_priv->ethsw_data;
 	struct net_device *netdev = port_priv->netdev;
+	struct dpsw_tci_cfg tci_cfg = { 0 };
 	bool is_oper;
 	int err, ret;
 
+	err = dpsw_if_get_tci(ethsw->mc_io, 0, ethsw->dpsw_handle,
+			      port_priv->idx, &tci_cfg);
+	if (err) {
+		netdev_err(netdev, "dpsw_if_get_tci err %d\n", err);
+		return err;
+	}
+
+	tci_cfg.vlan_id = pvid;
+
 	/* Interface needs to be down to change PVID */
 	is_oper = netif_oper_up(netdev);
 	if (is_oper) {
@@ -71,17 +80,16 @@ static int ethsw_port_set_tci(struct ethsw_port_priv *port_priv,
 	}
 
 	err = dpsw_if_set_tci(ethsw->mc_io, 0, ethsw->dpsw_handle,
-			      port_priv->idx, tci_cfg);
+			      port_priv->idx, &tci_cfg);
 	if (err) {
 		netdev_err(netdev, "dpsw_if_set_tci err %d\n", err);
 		goto set_tci_error;
 	}
 
 	/* Delete previous PVID info and mark the new one */
-	if (port_priv->pvid)
-		port_priv->vlans[port_priv->pvid] &= ~ETHSW_VLAN_PVID;
-	port_priv->vlans[tci_cfg->vlan_id] |= ETHSW_VLAN_PVID;
-	port_priv->pvid = tci_cfg->vlan_id;
+	port_priv->vlans[port_priv->pvid] &= ~ETHSW_VLAN_PVID;
+	port_priv->vlans[pvid] |= ETHSW_VLAN_PVID;
+	port_priv->pvid = pvid;
 
 set_tci_error:
 	if (is_oper) {
@@ -133,13 +141,7 @@ static int ethsw_port_add_vlan(struct ethsw_port_priv *port_priv,
 	}
 
 	if (flags & BRIDGE_VLAN_INFO_PVID) {
-		struct dpsw_tci_cfg tci_cfg = {
-			.pcp = 0,
-			.dei = 0,
-			.vlan_id = vid,
-		};
-
-		err = ethsw_port_set_tci(port_priv, &tci_cfg);
+		err = ethsw_port_set_pvid(port_priv, vid);
 		if (err)
 			return err;
 	}
@@ -819,9 +821,7 @@ static int ethsw_port_del_vlan(struct ethsw_port_priv *port_priv, u16 vid)
 		return -ENOENT;
 
 	if (port_priv->vlans[vid] & ETHSW_VLAN_PVID) {
-		struct dpsw_tci_cfg tci_cfg = { 0 };
-
-		err = ethsw_port_set_tci(port_priv, &tci_cfg);
+		err = ethsw_port_set_pvid(port_priv, 0);
 		if (err)
 			return err;
 	}
@@ -1254,7 +1254,6 @@ static int ethsw_port_init(struct ethsw_port_priv *port_priv, u16 port)
 	const char def_mcast[ETH_ALEN] = {0x01, 0x00, 0x5e, 0x00, 0x00, 0x01};
 	struct net_device *netdev = port_priv->netdev;
 	struct ethsw_core *ethsw = port_priv->ethsw_data;
-	struct dpsw_tci_cfg tci_cfg = {0};
 	struct dpsw_vlan_if_cfg vcfg;
 	int err;
 
@@ -1272,7 +1271,7 @@ static int ethsw_port_init(struct ethsw_port_priv *port_priv, u16 port)
 		return err;
 	}
 
-	err = ethsw_port_set_tci(port_priv, &tci_cfg);
+	err = ethsw_port_set_pvid(port_priv, 0);
 	if (err)
 		return err;
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
  2018-03-27 13:10 [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite Razvan Stefanescu
@ 2018-03-27 13:37 ` Andrew Lunn
  2018-03-28  6:45   ` Razvan Stefanescu
  2018-03-28 10:15 ` kbuild test robot
  1 sibling, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2018-03-27 13:37 UTC (permalink / raw)
  To: Razvan Stefanescu
  Cc: devel, stuyoder, gregkh, ioana.ciornei, alexandru.marginean,
	linux-kernel, netdev, laurentiu.tudor

On Tue, Mar 27, 2018 at 08:10:50AM -0500, Razvan Stefanescu wrote:
> Previous implementation overwrites PCP value, assuming the default value is
> 0, instead of 7.
> 
> Avoid this by modifying helper function ethsw_port_set_tci() to
> ethsw_port_set_pvid() and make it update only the vlan_id of the tci_cfg
> struct.

Hi Razvan

It is a good idea to explain acronyms, especially for staging, since
there are patches for all sorts of devices, can you cannot expect
everybody to know network specific acronyms.

By PCP you mean Priority Code Point. TCI i have no idea about.

Looking at the code, i think you are changing the flow to become
read/modify/write, instead of just write, which is overwriting the
previously configured Priority Code Point?

Please try to add more details to your change logs, to help us
understand the change.

	   Andrew
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
  2018-03-27 13:37 ` Andrew Lunn
@ 2018-03-28  6:45   ` Razvan Stefanescu
  0 siblings, 0 replies; 4+ messages in thread
From: Razvan Stefanescu @ 2018-03-28  6:45 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: gregkh, devel, linux-kernel, netdev, Alexandru Marginean,
	Ruxandra Ioana Ciocoi Radulescu, Ioana Ciornei, Laurentiu Tudor



> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Andrew Lunn
> Sent: Tuesday, March 27, 2018 4:38 PM
> To: Razvan Stefanescu <razvan.stefanescu@nxp.com>
> Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; Alexandru Marginean
> <alexandru.marginean@nxp.com>; Ruxandra Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>; stuyoder@gmail.com
> Subject: Re: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
> 
> On Tue, Mar 27, 2018 at 08:10:50AM -0500, Razvan Stefanescu wrote:
> > Previous implementation overwrites PCP value, assuming the default value
> is
> > 0, instead of 7.
> >
> > Avoid this by modifying helper function ethsw_port_set_tci() to
> > ethsw_port_set_pvid() and make it update only the vlan_id of the tci_cfg
> > struct.
> 
> Hi Razvan
> 
> It is a good idea to explain acronyms, especially for staging, since
> there are patches for all sorts of devices, can you cannot expect
> everybody to know network specific acronyms.
> 
> By PCP you mean Priority Code Point. TCI i have no idea about.
> 
> Looking at the code, i think you are changing the flow to become
> read/modify/write, instead of just write, which is overwriting the
> previously configured Priority Code Point?
> 
> Please try to add more details to your change logs, to help us
> understand the change.
> 

Thank you Andrew.  I'll address this in v2.

Best regards,
Razvan Stefanescu

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
  2018-03-27 13:10 [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite Razvan Stefanescu
  2018-03-27 13:37 ` Andrew Lunn
@ 2018-03-28 10:15 ` kbuild test robot
  1 sibling, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2018-03-28 10:15 UTC (permalink / raw)
  To: Razvan Stefanescu
  Cc: devel, andrew, stuyoder, gregkh, ioana.ciornei,
	alexandru.marginean, linux-kernel, kbuild-all, netdev,
	laurentiu.tudor

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

Hi Razvan,

I love your patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on next-20180327]
[cannot apply to v4.16-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Razvan-Stefanescu/staging-fsl-dpaa2-ethsw-Fix-TCI-values-overwrite/20180328-154703
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   drivers/staging/fsl-dpaa2/ethsw/dpsw.c: In function 'dpsw_if_get_tci':
>> drivers/staging/fsl-dpaa2/ethsw/dpsw.c:547:9: error: variable 'cmd' has initializer but incomplete type
     struct mc_command cmd = { 0 };
            ^~~~~~~~~~
>> drivers/staging/fsl-dpaa2/ethsw/dpsw.c:547:28: warning: excess elements in struct initializer
     struct mc_command cmd = { 0 };
                               ^
   drivers/staging/fsl-dpaa2/ethsw/dpsw.c:547:28: note: (near initialization for 'cmd')
>> drivers/staging/fsl-dpaa2/ethsw/dpsw.c:547:20: error: storage size of 'cmd' isn't known
     struct mc_command cmd = { 0 };
                       ^~~
   drivers/staging/fsl-dpaa2/ethsw/dpsw.c:547:20: warning: unused variable 'cmd' [-Wunused-variable]

vim +/cmd +547 drivers/staging/fsl-dpaa2/ethsw/dpsw.c

   530	
   531	/**
   532	 * dpsw_if_get_tci() - Get default VLAN Tag Control Information (TCI)
   533	 * @mc_io:	Pointer to MC portal's I/O object
   534	 * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
   535	 * @token:	Token of DPSW object
   536	 * @if_id:	Interface Identifier
   537	 * @cfg:	Tag Control Information Configuration
   538	 *
   539	 * Return:	Completion status. '0' on Success; Error code otherwise.
   540	 */
   541	int dpsw_if_get_tci(struct fsl_mc_io *mc_io,
   542			    u32 cmd_flags,
   543			    u16 token,
   544			    u16 if_id,
   545			    struct dpsw_tci_cfg *cfg)
   546	{
 > 547		struct mc_command cmd = { 0 };
   548		struct dpsw_cmd_if_get_tci *cmd_params;
   549		struct dpsw_rsp_if_get_tci *rsp_params;
   550		int err;
   551	
   552		/* prepare command */
   553		cmd.header = mc_encode_cmd_header(DPSW_CMDID_IF_GET_TCI,
   554						  cmd_flags,
   555						  token);
   556		cmd_params = (struct dpsw_cmd_if_get_tci *)cmd.params;
   557		cmd_params->if_id = cpu_to_le16(if_id);
   558	
   559		/* send command to mc*/
   560		err = mc_send_command(mc_io, &cmd);
   561		if (err)
   562			return err;
   563	
   564		/* retrieve response parameters */
   565		rsp_params = (struct dpsw_rsp_if_get_tci *)cmd.params;
   566		cfg->pcp = rsp_params->pcp;
   567		cfg->dei = rsp_params->dei;
   568		cfg->vlan_id = le16_to_cpu(rsp_params->vlan_id);
   569	
   570		return 0;
   571	}
   572	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[-- Attachment #3: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-03-28 10:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27 13:10 [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite Razvan Stefanescu
2018-03-27 13:37 ` Andrew Lunn
2018-03-28  6:45   ` Razvan Stefanescu
2018-03-28 10:15 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).