diff mbox series

[net-next,2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions

Message ID 20201122082636.12451-3-ceggers@arri.de
State New
Headers show
Series net: ptp: use common defines for PTP message types in further drivers | expand

Commit Message

Christian Eggers Nov. 22, 2020, 8:26 a.m. UTC
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(-)

Comments

Ido Schimmel Nov. 22, 2020, 2:35 p.m. UTC | #1
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/
Christian Eggers Nov. 22, 2020, 7:29 p.m. UTC | #2
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/
Ido Schimmel Nov. 22, 2020, 8:01 p.m. UTC | #3
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.
Vladimir Oltean Nov. 22, 2020, 10:01 p.m. UTC | #4
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.
Ido Schimmel Nov. 23, 2020, 6:59 a.m. UTC | #5
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
Jakub Kicinski Nov. 23, 2020, 10:03 p.m. UTC | #6
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.
diff mbox series

Patch

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 |