linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] net: ptp: use common defines for PTP message types in further drivers
@ 2020-11-22  8:26 Christian Eggers
  2020-11-22  8:26 ` [PATCH net-next 1/3] net: phy: dp83640: use new PTP_MSGTYPE_SYNC define Christian Eggers
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Christian Eggers @ 2020-11-22  8:26 UTC (permalink / raw)
  To: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski
  Cc: Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers

This series replaces further driver internal enumeration / uses of magic
numbers with the newly introduced PTP_MSGTYPE_* defines.

On Friday, 20 November 2020, 23:39:10 CET, Vladimir Oltean wrote:
> On Fri, Nov 20, 2020 at 09:41:03AM +0100, Christian Eggers wrote:
> > This series introduces commen defines for PTP event messages. Driver
> > internal defines are removed and some uses of magic numbers are replaced
> > by the new defines.
> > [...]
> 
> I understand that you don't want to spend a lifetime on this, but I see
> that there are more drivers which you did not touch.
> 
> is_sync() in drivers/net/phy/dp83640.c can be made to
> 	return ptp_get_msgtype(hdr, type) == PTP_MSGTYPE_SYNC;
> 
> this can be removed from drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h:
> enum {
> 	MLXSW_SP_PTP_MESSAGE_TYPE_SYNC,
> 	MLXSW_SP_PTP_MESSAGE_TYPE_DELAY_REQ,
> 	MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_REQ,
> 	MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_RESP,
> };
I think that I have found an addtional one in the Microsemi VSC85xx PHY driver.




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

* [PATCH net-next 1/3] net: phy: dp83640: use new PTP_MSGTYPE_SYNC define
  2020-11-22  8:26 [PATCH net-next 0/3] net: ptp: use common defines for PTP message types in further drivers Christian Eggers
@ 2020-11-22  8:26 ` Christian Eggers
  2020-11-22  8:26 ` [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions Christian Eggers
  2020-11-22  8:26 ` [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines Christian Eggers
  2 siblings, 0 replies; 12+ messages in thread
From: Christian Eggers @ 2020-11-22  8:26 UTC (permalink / raw)
  To: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski
  Cc: Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers, Christian Eggers,
	Kurt Kanzenbach

Replace use of magic number with recently introduced define.

Signed-off-by: Christian Eggers <ceggers@gmx.de>
Cc: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/phy/dp83640.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index f2caccaf4408..9757ca0d9633 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -964,15 +964,12 @@ static void decode_status_frame(struct dp83640_private *dp83640,
 static int is_sync(struct sk_buff *skb, int type)
 {
 	struct ptp_header *hdr;
-	u8 msgtype;
 
 	hdr = ptp_parse_header(skb, type);
 	if (!hdr)
 		return 0;
 
-	msgtype = ptp_get_msgtype(hdr, type);
-
-	return (msgtype & 0xf) == 0;
+	return ptp_get_msgtype(hdr, type) == PTP_MSGTYPE_SYNC;
 }
 
 static void dp83640_free_clocks(void)
-- 
Christian Eggers
Embedded software developer


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

* [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22  8:26 [PATCH net-next 0/3] net: ptp: use common defines for PTP message types in further drivers Christian Eggers
  2020-11-22  8:26 ` [PATCH net-next 1/3] net: phy: dp83640: use new PTP_MSGTYPE_SYNC define Christian Eggers
@ 2020-11-22  8:26 ` Christian Eggers
  2020-11-22 14:35   ` Ido Schimmel
  2020-11-22  8:26 ` [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines Christian Eggers
  2 siblings, 1 reply; 12+ messages in thread
From: Christian Eggers @ 2020-11-22  8:26 UTC (permalink / raw)
  To: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski
  Cc: Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers, Christian Eggers, Petr Machata,
	Jiri Pirko, Ido Schimmel

Use recently introduced PTP wide defines instead of a driver internal
enumeration.

Signed-off-by: Christian Eggers <ceggers@gmx.de>
Cc: Petr Machata <petrm@mellanox.com>
Cc: Jiri Pirko <jiri@nvidia.com>
Cc: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c | 8 ++++----
 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h | 7 -------
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index ca8090a28dec..d6e9ecb14681 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -828,10 +828,10 @@ struct mlxsw_sp_ptp_state *mlxsw_sp1_ptp_init(struct mlxsw_sp *mlxsw_sp)
 		goto err_hashtable_init;
 
 	/* Delive these message types as PTP0. */
-	message_type = BIT(MLXSW_SP_PTP_MESSAGE_TYPE_SYNC) |
-		       BIT(MLXSW_SP_PTP_MESSAGE_TYPE_DELAY_REQ) |
-		       BIT(MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_REQ) |
-		       BIT(MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_RESP);
+	message_type = BIT(PTP_MSGTYPE_SYNC) |
+		       BIT(PTP_MSGTYPE_DELAY_REQ) |
+		       BIT(PTP_MSGTYPE_PDELAY_REQ) |
+		       BIT(PTP_MSGTYPE_PDELAY_RESP);
 	err = mlxsw_sp_ptp_mtptpt_set(mlxsw_sp, MLXSW_REG_MTPTPT_TRAP_ID_PTP0,
 				      message_type);
 	if (err)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
index 8c386571afce..1d43a3755285 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
@@ -11,13 +11,6 @@ struct mlxsw_sp;
 struct mlxsw_sp_port;
 struct mlxsw_sp_ptp_clock;
 
-enum {
-	MLXSW_SP_PTP_MESSAGE_TYPE_SYNC,
-	MLXSW_SP_PTP_MESSAGE_TYPE_DELAY_REQ,
-	MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_REQ,
-	MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_RESP,
-};
-
 static inline int mlxsw_sp_ptp_get_ts_info_noptp(struct ethtool_ts_info *info)
 {
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-- 
Christian Eggers
Embedded software developer


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

* [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines
  2020-11-22  8:26 [PATCH net-next 0/3] net: ptp: use common defines for PTP message types in further drivers Christian Eggers
  2020-11-22  8:26 ` [PATCH net-next 1/3] net: phy: dp83640: use new PTP_MSGTYPE_SYNC define Christian Eggers
  2020-11-22  8:26 ` [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions Christian Eggers
@ 2020-11-22  8:26 ` Christian Eggers
  2020-11-22  9:42   ` kernel test robot
  2020-11-23  9:05   ` Antoine Tenart
  2 siblings, 2 replies; 12+ messages in thread
From: Christian Eggers @ 2020-11-22  8:26 UTC (permalink / raw)
  To: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski
  Cc: Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers, Christian Eggers, Quentin Schulz,
	Antoine Tenart, Antoine Tenart

Use recently introduced PTP_MSGTYPE_SYNC and PTP_MSGTYPE_DELAY_REQ
defines instead of a driver internal enumeration.

Signed-off-by: Christian Eggers <ceggers@gmx.de>
Cc: Quentin Schulz <quentin.schulz@bootlin.com>
Cc: Antoine Tenart <atenart@kernel.org>
Cc: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mscc/mscc_ptp.c | 14 +++++++-------
 drivers/net/phy/mscc/mscc_ptp.h |  5 -----
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
index d8a61456d1ce..924ed5b034a4 100644
--- a/drivers/net/phy/mscc/mscc_ptp.c
+++ b/drivers/net/phy/mscc/mscc_ptp.c
@@ -506,9 +506,9 @@ static int vsc85xx_ptp_cmp_init(struct phy_device *phydev, enum ts_blk blk)
 {
 	struct vsc8531_private *vsc8531 = phydev->priv;
 	bool base = phydev->mdio.addr == vsc8531->ts_base_addr;
-	enum vsc85xx_ptp_msg_type msgs[] = {
-		PTP_MSG_TYPE_SYNC,
-		PTP_MSG_TYPE_DELAY_REQ
+	u8 msgs[] = {
+		PTP_MSGTYPE_SYNC,
+		PTP_MSGTYPE_DELAY_REQ
 	};
 	u32 val;
 	u8 i;
@@ -847,9 +847,9 @@ static int vsc85xx_ts_ptp_action_flow(struct phy_device *phydev, enum ts_blk blk
 static int vsc85xx_ptp_conf(struct phy_device *phydev, enum ts_blk blk,
 			    bool one_step, bool enable)
 {
-	enum vsc85xx_ptp_msg_type msgs[] = {
-		PTP_MSG_TYPE_SYNC,
-		PTP_MSG_TYPE_DELAY_REQ
+	u8 msgs[] = {
+		PTP_MSGTYPE_SYNC,
+		PTP_MSGTYPE_DELAY_REQ
 	};
 	u32 val;
 	u8 i;
@@ -858,7 +858,7 @@ static int vsc85xx_ptp_conf(struct phy_device *phydev, enum ts_blk blk,
 		if (blk == INGRESS)
 			vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
 						   PTP_WRITE_NS);
-		else if (msgs[i] == PTP_MSG_TYPE_SYNC && one_step)
+		else if (msgs[i] == PTP_MSGTYPE_SYNC && one_step)
 			/* no need to know Sync t when sending in one_step */
 			vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
 						   PTP_WRITE_1588);
diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h
index 3ea163af0f4f..da3465360e90 100644
--- a/drivers/net/phy/mscc/mscc_ptp.h
+++ b/drivers/net/phy/mscc/mscc_ptp.h
@@ -436,11 +436,6 @@ enum ptp_cmd {
 	PTP_SAVE_IN_TS_FIFO = 11, /* invalid when writing in reg */
 };
 
-enum vsc85xx_ptp_msg_type {
-	PTP_MSG_TYPE_SYNC,
-	PTP_MSG_TYPE_DELAY_REQ,
-};
-
 struct vsc85xx_ptphdr {
 	u8 tsmt; /* transportSpecific | messageType */
 	u8 ver;  /* reserved0 | versionPTP */
-- 
Christian Eggers
Embedded software developer


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

* Re: [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines
  2020-11-22  8:26 ` [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines Christian Eggers
@ 2020-11-22  9:42   ` kernel test robot
  2020-11-23  9:05   ` Antoine Tenart
  1 sibling, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-11-22  9:42 UTC (permalink / raw)
  To: Christian Eggers, Richard Cochran, Andrew Lunn, Heiner Kallweit,
	Jakub Kicinski
  Cc: kbuild-all, Vladimir Oltean, Russell King, David S . Miller,
	netdev, linux-kernel, Christian Eggers

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

Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Christian-Eggers/net-ptp-use-common-defines-for-PTP-message-types-in-further-drivers/20201122-163319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git f9e425e99b0756c1479042afe761073779df2a30
config: sparc-randconfig-r012-20201122 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/78cc4b0e1739511ed9712c9466a48ddc6885d153
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christian-Eggers/net-ptp-use-common-defines-for-PTP-message-types-in-further-drivers/20201122-163319
        git checkout 78cc4b0e1739511ed9712c9466a48ddc6885d153
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc 

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/phy/mscc/mscc_ptp.c: In function 'vsc85xx_ptp_cmp_init':
   drivers/net/phy/mscc/mscc_ptp.c:510:3: error: 'PTP_MSGTYPE_SYNC' undeclared (first use in this function); did you mean 'CSD_TYPE_SYNC'?
     510 |   PTP_MSGTYPE_SYNC,
         |   ^~~~~~~~~~~~~~~~
         |   CSD_TYPE_SYNC
   drivers/net/phy/mscc/mscc_ptp.c:510:3: note: each undeclared identifier is reported only once for each function it appears in
   drivers/net/phy/mscc/mscc_ptp.c:511:3: error: 'PTP_MSGTYPE_DELAY_REQ' undeclared (first use in this function)
     511 |   PTP_MSGTYPE_DELAY_REQ
         |   ^~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mscc/mscc_ptp.c: In function 'vsc85xx_ptp_conf':
   drivers/net/phy/mscc/mscc_ptp.c:851:3: error: 'PTP_MSGTYPE_SYNC' undeclared (first use in this function); did you mean 'CSD_TYPE_SYNC'?
     851 |   PTP_MSGTYPE_SYNC,
         |   ^~~~~~~~~~~~~~~~
         |   CSD_TYPE_SYNC
>> drivers/net/phy/mscc/mscc_ptp.c:851:3: warning: initialization of 'unsigned char' from 'u8 *' {aka 'unsigned char *'} makes integer from pointer without a cast [-Wint-conversion]
   drivers/net/phy/mscc/mscc_ptp.c:851:3: note: (near initialization for 'msgs[0]')
   drivers/net/phy/mscc/mscc_ptp.c:852:3: error: 'PTP_MSGTYPE_DELAY_REQ' undeclared (first use in this function)
     852 |   PTP_MSGTYPE_DELAY_REQ
         |   ^~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mscc/mscc_ptp.c:852:3: warning: initialization of 'unsigned char' from 'u8 *' {aka 'unsigned char *'} makes integer from pointer without a cast [-Wint-conversion]
   drivers/net/phy/mscc/mscc_ptp.c:852:3: note: (near initialization for 'msgs[1]')
>> drivers/net/phy/mscc/mscc_ptp.c:861:20: warning: comparison between pointer and integer
     861 |   else if (msgs[i] == PTP_MSGTYPE_SYNC && one_step)
         |                    ^~

vim +851 drivers/net/phy/mscc/mscc_ptp.c

   846	
   847	static int vsc85xx_ptp_conf(struct phy_device *phydev, enum ts_blk blk,
   848				    bool one_step, bool enable)
   849	{
   850		u8 msgs[] = {
 > 851			PTP_MSGTYPE_SYNC,
   852			PTP_MSGTYPE_DELAY_REQ
   853		};
   854		u32 val;
   855		u8 i;
   856	
   857		for (i = 0; i < ARRAY_SIZE(msgs); i++) {
   858			if (blk == INGRESS)
   859				vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
   860							   PTP_WRITE_NS);
 > 861			else if (msgs[i] == PTP_MSGTYPE_SYNC && one_step)
   862				/* no need to know Sync t when sending in one_step */
   863				vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
   864							   PTP_WRITE_1588);
   865			else
   866				vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
   867							   PTP_SAVE_IN_TS_FIFO);
   868	
   869			val = vsc85xx_ts_read_csr(phydev, blk,
   870						  MSCC_ANA_PTP_FLOW_ENA(i));
   871			val &= ~PTP_FLOW_ENA;
   872			if (enable)
   873				val |= PTP_FLOW_ENA;
   874			vsc85xx_ts_write_csr(phydev, blk, MSCC_ANA_PTP_FLOW_ENA(i),
   875					     val);
   876		}
   877	
   878		return 0;
   879	}
   880	

---
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: 29585 bytes --]

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

* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22  8:26 ` [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions Christian Eggers
@ 2020-11-22 14:35   ` Ido Schimmel
  2020-11-22 19:29     ` Christian Eggers
  2020-11-23  6:59     ` Ido Schimmel
  0 siblings, 2 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-11-22 14:35 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
	Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers, Petr Machata, Jiri Pirko,
	Ido Schimmel

On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote:
> Use recently introduced PTP wide defines instead of a driver internal
> enumeration.
> 
> Signed-off-by: Christian Eggers <ceggers@gmx.de>
> Cc: Petr Machata <petrm@mellanox.com>
> Cc: Jiri Pirko <jiri@nvidia.com>
> Cc: Ido Schimmel <idosch@nvidia.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

But:

1. Checkpatch complains about:
WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers <ceggers@gmx.de>'

2. This series does not build, which fails the CI [1][2] and also
required me to fetch the dependencies that are currently under review
[3]. I believe it is generally discouraged to create dependencies
between patch sets that are under review for exactly these reasons. I
don't know what are Jakub's preferences, but had this happened on our
internal patchwork instance, I would just ask the author to submit
another version with all the patches.

Anyway, I added all six patches to our regression as we have some PTP
tests. Will let you know tomorrow.

Thanks

[1] https://lore.kernel.org/netdev/20201122082636.12451-1-ceggers@arri.de/T/#mcef35858585d23b72b8f75450a51618d5c5d3260
[2] https://patchwork.hopto.org/static/nipa/389053/11923809/build_allmodconfig_warn/summary
[3] https://patchwork.kernel.org/project/netdevbpf/cover/20201120084106.10046-1-ceggers@arri.de/

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

* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22 14:35   ` Ido Schimmel
@ 2020-11-22 19:29     ` Christian Eggers
  2020-11-22 20:01       ` Ido Schimmel
  2020-11-22 22:01       ` Vladimir Oltean
  2020-11-23  6:59     ` Ido Schimmel
  1 sibling, 2 replies; 12+ messages in thread
From: Christian Eggers @ 2020-11-22 19:29 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
	Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel

On Sunday, 22 November 2020, 15:35:55 CET, Ido Schimmel wrote:
> On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote:
> > Use recently introduced PTP wide defines instead of a driver internal
> > enumeration.
> > 
> > Signed-off-by: Christian Eggers <ceggers@gmx.de>
> > Cc: Petr Machata <petrm@mellanox.com>
> > Cc: Jiri Pirko <jiri@nvidia.com>
> > Cc: Ido Schimmel <idosch@nvidia.com>
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> 
> But:
> 
> 1. Checkpatch complains about:
> WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian
> Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers
> <ceggers@gmx.de>'
unfortunately I changed this after running checkpatch. My intention was to 
separate my (private) weekend work from the patches I do while I'm on the job.

> 2. This series does not build, which fails the CI [1][2] and also
> required me to fetch the dependencies that are currently under review
> [3]. I believe it is generally discouraged to create dependencies
> between patch sets that are under review for exactly these reasons. 
this was also not by intention. Vladimir found some files I missed in the
first series. As the whole first series had already been reviewed at that time,
I wasn't sure whether I am allowed to add further patches to it. Additionally
I didn't concern that although my local build is successful, I should wait
until the first series is applied...

> I don't know what are Jakub's preferences, but had this happened on our
> internal patchwork instance, I would just ask the author to submit
> another version with all the patches.
Please let me know how I shall proceed...

> Anyway, I added all six patches to our regression as we have some PTP
> tests. Will let you know tomorrow.
> 
> Thanks
> 
> [1]
> https://lore.kernel.org/netdev/20201122082636.12451-1-ceggers@arri.de/T/#mc
> ef35858585d23b72b8f75450a51618d5c5d3260 [2]
> https://patchwork.hopto.org/static/nipa/389053/11923809/build_allmodconfig_
> warn/summary [3]
> https://patchwork.kernel.org/project/netdevbpf/cover/20201120084106.10046-1
> -ceggers@arri.de/





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

* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22 19:29     ` Christian Eggers
@ 2020-11-22 20:01       ` Ido Schimmel
  2020-11-23 22:03         ` Jakub Kicinski
  2020-11-22 22:01       ` Vladimir Oltean
  1 sibling, 1 reply; 12+ messages in thread
From: Ido Schimmel @ 2020-11-22 20:01 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
	Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel

On Sun, Nov 22, 2020 at 08:29:22PM +0100, Christian Eggers wrote:
> On Sunday, 22 November 2020, 15:35:55 CET, Ido Schimmel wrote:
> > On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote:
> > > Use recently introduced PTP wide defines instead of a driver internal
> > > enumeration.
> > >
> > > Signed-off-by: Christian Eggers <ceggers@gmx.de>
> > > Cc: Petr Machata <petrm@mellanox.com>
> > > Cc: Jiri Pirko <jiri@nvidia.com>
> > > Cc: Ido Schimmel <idosch@nvidia.com>
> >
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> >
> > But:
> >
> > 1. Checkpatch complains about:
> > WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian
> > Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers
> > <ceggers@gmx.de>'
> unfortunately I changed this after running checkpatch. My intention was to
> separate my (private) weekend work from the patches I do while I'm on the job.

No problem. Just make sure that authorship and Signed-off-by agree. You
can use:

# git commit --amend --author="Christian Eggers <ceggers@gmx.de>"

> 
> > 2. This series does not build, which fails the CI [1][2] and also
> > required me to fetch the dependencies that are currently under review
> > [3]. I believe it is generally discouraged to create dependencies
> > between patch sets that are under review for exactly these reasons.
> this was also not by intention. Vladimir found some files I missed in the
> first series. As the whole first series had already been reviewed at that time,
> I wasn't sure whether I am allowed to add further patches to it. Additionally
> I didn't concern that although my local build is successful, I should wait
> until the first series is applied...

Yea, I saw that, no problem :)

> 
> > I don't know what are Jakub's preferences, but had this happened on our
> > internal patchwork instance, I would just ask the author to submit
> > another version with all the patches.
> Please let me know how I shall proceed...

Jakub has the final say, so I assume he will comment on that.

Regardless, thanks for the patches.

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

* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22 19:29     ` Christian Eggers
  2020-11-22 20:01       ` Ido Schimmel
@ 2020-11-22 22:01       ` Vladimir Oltean
  1 sibling, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2020-11-22 22:01 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Ido Schimmel, Richard Cochran, Andrew Lunn, Heiner Kallweit,
	Jakub Kicinski, Russell King, David S . Miller, netdev,
	linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel

On Sun, Nov 22, 2020 at 08:29:22PM +0100, Christian Eggers wrote:
> this was also not by intention. Vladimir found some files I missed in the
> first series. As the whole first series had already been reviewed at that time,
> I wasn't sure whether I am allowed to add further patches to it. Additionally
> I didn't concern that although my local build is successful, I should wait
> until the first series is applied...

When I said that, what I was thinking of (although it might have not
been clear) was that if you send further patches, you send them _after_
the initial series is merged.

Alternatively, it would have been just as valid to resend the entire
series, as a 3+3 patchset with a new revision (v3 -> v4), with review
tags collected from the first three, and the last 3 being completely
new. Jakub could have superseded v3 and applied v4.

The idea behind splicing N patches into a series is that they are
logically connected to one another. For example, a patch doesn't build
without another. You _could_ split logically-connected patches into
separate series and post them independently for review, as long as they
are build-time independent. If they aren't, well, what happens is
exactly what happened: various test robots will report build failures,
which from a maintainer's point of view inspires less confidence to
apply a patch as-is. I would not be surprised if Jakub asked you to
resend with no change at all, just to ensure that the patch series
passes build tests before merging it.

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

* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22 14:35   ` Ido Schimmel
  2020-11-22 19:29     ` Christian Eggers
@ 2020-11-23  6:59     ` Ido Schimmel
  1 sibling, 0 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-11-23  6:59 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
	Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers, Petr Machata, Jiri Pirko,
	Ido Schimmel

On Sun, Nov 22, 2020 at 04:35:58PM +0200, Ido Schimmel wrote:
> Anyway, I added all six patches to our regression as we have some PTP
> tests. Will let you know tomorrow.

Looks good

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

* Re: [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines
  2020-11-22  8:26 ` [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines Christian Eggers
  2020-11-22  9:42   ` kernel test robot
@ 2020-11-23  9:05   ` Antoine Tenart
  1 sibling, 0 replies; 12+ messages in thread
From: Antoine Tenart @ 2020-11-23  9:05 UTC (permalink / raw)
  To: Andrew Lunn, Christian Eggers, Heiner Kallweit, Jakub Kicinski,
	Richard Cochran
  Cc: Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Christian Eggers, Christian Eggers, Quentin Schulz,
	Antoine Tenart

Hello Christian,

Quoting Christian Eggers (2020-11-22 09:26:36)
> Use recently introduced PTP_MSGTYPE_SYNC and PTP_MSGTYPE_DELAY_REQ
> defines instead of a driver internal enumeration.
> 
> Signed-off-by: Christian Eggers <ceggers@gmx.de>

Reviewed-by: Antoine Tenart <atenart@kernel.org>

Thanks!
Antoine

> Cc: Quentin Schulz <quentin.schulz@bootlin.com>
> Cc: Antoine Tenart <atenart@kernel.org>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/phy/mscc/mscc_ptp.c | 14 +++++++-------
>  drivers/net/phy/mscc/mscc_ptp.h |  5 -----
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> index d8a61456d1ce..924ed5b034a4 100644
> --- a/drivers/net/phy/mscc/mscc_ptp.c
> +++ b/drivers/net/phy/mscc/mscc_ptp.c
> @@ -506,9 +506,9 @@ static int vsc85xx_ptp_cmp_init(struct phy_device *phydev, enum ts_blk blk)
>  {
>         struct vsc8531_private *vsc8531 = phydev->priv;
>         bool base = phydev->mdio.addr == vsc8531->ts_base_addr;
> -       enum vsc85xx_ptp_msg_type msgs[] = {
> -               PTP_MSG_TYPE_SYNC,
> -               PTP_MSG_TYPE_DELAY_REQ
> +       u8 msgs[] = {
> +               PTP_MSGTYPE_SYNC,
> +               PTP_MSGTYPE_DELAY_REQ
>         };
>         u32 val;
>         u8 i;
> @@ -847,9 +847,9 @@ static int vsc85xx_ts_ptp_action_flow(struct phy_device *phydev, enum ts_blk blk
>  static int vsc85xx_ptp_conf(struct phy_device *phydev, enum ts_blk blk,
>                             bool one_step, bool enable)
>  {
> -       enum vsc85xx_ptp_msg_type msgs[] = {
> -               PTP_MSG_TYPE_SYNC,
> -               PTP_MSG_TYPE_DELAY_REQ
> +       u8 msgs[] = {
> +               PTP_MSGTYPE_SYNC,
> +               PTP_MSGTYPE_DELAY_REQ
>         };
>         u32 val;
>         u8 i;
> @@ -858,7 +858,7 @@ static int vsc85xx_ptp_conf(struct phy_device *phydev, enum ts_blk blk,
>                 if (blk == INGRESS)
>                         vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
>                                                    PTP_WRITE_NS);
> -               else if (msgs[i] == PTP_MSG_TYPE_SYNC && one_step)
> +               else if (msgs[i] == PTP_MSGTYPE_SYNC && one_step)
>                         /* no need to know Sync t when sending in one_step */
>                         vsc85xx_ts_ptp_action_flow(phydev, blk, msgs[i],
>                                                    PTP_WRITE_1588);
> diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h
> index 3ea163af0f4f..da3465360e90 100644
> --- a/drivers/net/phy/mscc/mscc_ptp.h
> +++ b/drivers/net/phy/mscc/mscc_ptp.h
> @@ -436,11 +436,6 @@ enum ptp_cmd {
>         PTP_SAVE_IN_TS_FIFO = 11, /* invalid when writing in reg */
>  };
>  
> -enum vsc85xx_ptp_msg_type {
> -       PTP_MSG_TYPE_SYNC,
> -       PTP_MSG_TYPE_DELAY_REQ,
> -};
> -
>  struct vsc85xx_ptphdr {
>         u8 tsmt; /* transportSpecific | messageType */
>         u8 ver;  /* reserved0 | versionPTP */
> -- 
> Christian Eggers
> Embedded software developer
> 

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

* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
  2020-11-22 20:01       ` Ido Schimmel
@ 2020-11-23 22:03         ` Jakub Kicinski
  0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2020-11-23 22:03 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Ido Schimmel, Richard Cochran, Andrew Lunn, Heiner Kallweit,
	Vladimir Oltean, Russell King, David S . Miller, netdev,
	linux-kernel, Petr Machata, Jiri Pirko, Ido Schimmel

On Sun, 22 Nov 2020 22:01:54 +0200 Ido Schimmel wrote:
> > > I don't know what are Jakub's preferences, but had this happened on our
> > > internal patchwork instance, I would just ask the author to submit
> > > another version with all the patches.  
> > Please let me know how I shall proceed...  
> 
> Jakub has the final say, so I assume he will comment on that.

The pre-requisite was just merged, so please collect all the review tags
you got so far and repost.

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

end of thread, other threads:[~2020-11-23 22:03 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-22  8:26 [PATCH net-next 0/3] net: ptp: use common defines for PTP message types in further drivers Christian Eggers
2020-11-22  8:26 ` [PATCH net-next 1/3] net: phy: dp83640: use new PTP_MSGTYPE_SYNC define Christian Eggers
2020-11-22  8:26 ` [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions Christian Eggers
2020-11-22 14:35   ` Ido Schimmel
2020-11-22 19:29     ` Christian Eggers
2020-11-22 20:01       ` Ido Schimmel
2020-11-23 22:03         ` Jakub Kicinski
2020-11-22 22:01       ` Vladimir Oltean
2020-11-23  6:59     ` Ido Schimmel
2020-11-22  8:26 ` [PATCH net-next 3/3] net: phy: mscc: use new PTP_MSGTYPE_* defines Christian Eggers
2020-11-22  9:42   ` kernel test robot
2020-11-23  9:05   ` Antoine Tenart

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).