| Message ID | 20240807082918.2558282-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [resubmit,net,1/2] net: fec: Forward-declare `fec_ptp_read()` | expand |
Aw, crop, I meant to say -v2 instead of "resubmit"... That's what happens if you mindlessly re-use format-patch commands :/ I hope it doesn't bother you _too_ much... On 8/7/24 10:29, Csókás, Bence wrote: > This function is used in `fec_ptp_enable_pps()` through > struct cyclecounter read(). Forward declarations make > it clearer, what's happening. > > Fixes: 61d5e2a251fb ("fec: Fix timer capture timing in `fec_ptp_enable_pps()`") > Suggested-by: Frank Li <Frank.li@nxp.com> > Link: https://lore.kernel.org/netdev/20240805144754.2384663-1-csokas.bence@prolan.hu/T/#ma6c21ad264016c24612048b1483769eaff8cdf20 > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote: > This function is used in `fec_ptp_enable_pps()` through > struct cyclecounter read(). Forward declarations make > it clearer, what's happening. In general, forward declarations are not liked. It is better to move the code to before it is used. Since this is a minimal fix for stable, lets allow it. But please wait for net to be merged into net-next, and submit a cleanup patch which does move fec_ptp_read() earlier and remove the forward declaration. Andrew
On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote: > This function is used in `fec_ptp_enable_pps()` through > struct cyclecounter read(). Forward declarations make > it clearer, what's happening. > > Fixes: 61d5e2a251fb ("fec: Fix timer capture timing in `fec_ptp_enable_pps()`") > Suggested-by: Frank Li <Frank.li@nxp.com> > Link: https://lore.kernel.org/netdev/20240805144754.2384663-1-csokas.bence@prolan.hu/T/#ma6c21ad264016c24612048b1483769eaff8cdf20 > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/net/ethernet/freescale/fec_ptp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c > index e32f6724f568..fdbf61069a05 100644 > --- a/drivers/net/ethernet/freescale/fec_ptp.c > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > @@ -90,6 +90,8 @@ > #define FEC_PTP_MAX_NSEC_PERIOD 4000000000ULL > #define FEC_PTP_MAX_NSEC_COUNTER 0x80000000ULL > > +static u64 fec_ptp_read(const struct cyclecounter *cc); > + > /** > * fec_ptp_enable_pps > * @fep: the fec_enet_private structure handle > @@ -136,7 +138,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable) > * NSEC_PER_SEC - ts.tv_nsec. Add the remaining nanoseconds > * to current timer would be next second. > */ > - tempval = fep->cc.read(&fep->cc); > + tempval = fec_ptp_read(&fep->cc); > /* Convert the ptp local counter to 1588 timestamp */ > ns = timecounter_cyc2time(&fep->tc, tempval); > ts = ns_to_timespec64(ns); > -- > 2.34.1 > >
On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote: > On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote: > > This function is used in `fec_ptp_enable_pps()` through > > struct cyclecounter read(). Forward declarations make > > it clearer, what's happening. > > In general, forward declarations are not liked. It is better to move > the code to before it is used. > > Since this is a minimal fix for stable, lets allow it. But please wait > for net to be merged into net-next, and submit a cleanup patch which > does move fec_ptp_read() earlier and remove the forward declaration. That makes sense. However, is this a fix? It's not clear to me that it is. And if it is a pre-requisite for patch 2/2, well that doesn't seem to be a fix. So in all, I'm somewhat confused. And wonder if all changes can go via net-next.
On 8/8/24 11:41, Simon Horman wrote: > On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote: >> On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote: >>> This function is used in `fec_ptp_enable_pps()` through >>> struct cyclecounter read(). Forward declarations make >>> it clearer, what's happening. >> >> In general, forward declarations are not liked. It is better to move >> the code to before it is used. >> >> Since this is a minimal fix for stable, lets allow it. But please wait >> for net to be merged into net-next, and submit a cleanup patch which >> does move fec_ptp_read() earlier and remove the forward declaration. > > That makes sense. > > However, is this a fix? > It's not clear to me that it is. Well, it's not clear to me either what constitutes as a "fix" versus "just a cleanup". But, whatever floats Andrew's boat... > And if it is a pre-requisite for patch 2/2, > well that doesn't seem to be a fix. It indeed is. > So in all, I'm somewhat confused. > And wonder if all changes can go via net-next. That's probably what will be happening. Bence
On Thu, Aug 08, 2024 at 11:49:29AM +0200, Csókás Bence wrote: > On 8/8/24 11:41, Simon Horman wrote: > > On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote: > > > On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote: > > > > This function is used in `fec_ptp_enable_pps()` through > > > > struct cyclecounter read(). Forward declarations make > > > > it clearer, what's happening. > > > > > > In general, forward declarations are not liked. It is better to move > > > the code to before it is used. > > > > > > Since this is a minimal fix for stable, lets allow it. But please wait > > > for net to be merged into net-next, and submit a cleanup patch which > > > does move fec_ptp_read() earlier and remove the forward declaration. > > > > That makes sense. > > > > However, is this a fix? > > It's not clear to me that it is. > > Well, it's not clear to me either what constitutes as a "fix" versus "just a > cleanup". But, whatever floats Andrew's boat... Let me state my rule of thumb: a fix addresses a user-visible bug. > > And if it is a pre-requisite for patch 2/2, > > well that doesn't seem to be a fix. > > It indeed is. > > > So in all, I'm somewhat confused. > > And wonder if all changes can go via net-next. > > That's probably what will be happening. It does seem like the cleanest, and coincidently easiest, path.
On Thu, Aug 08, 2024 at 12:37:41PM +0100, Simon Horman wrote: > On Thu, Aug 08, 2024 at 11:49:29AM +0200, Csókás Bence wrote: > > On 8/8/24 11:41, Simon Horman wrote: > > > On Wed, Aug 07, 2024 at 03:53:17PM +0200, Andrew Lunn wrote: > > > > On Wed, Aug 07, 2024 at 10:29:17AM +0200, Csókás, Bence wrote: > > > > > This function is used in `fec_ptp_enable_pps()` through > > > > > struct cyclecounter read(). Forward declarations make > > > > > it clearer, what's happening. > > > > > > > > In general, forward declarations are not liked. It is better to move > > > > the code to before it is used. > > > > > > > > Since this is a minimal fix for stable, lets allow it. But please wait > > > > for net to be merged into net-next, and submit a cleanup patch which > > > > does move fec_ptp_read() earlier and remove the forward declaration. > > > > > > That makes sense. > > > > > > However, is this a fix? > > > It's not clear to me that it is. > > > > Well, it's not clear to me either what constitutes as a "fix" versus "just a > > cleanup". But, whatever floats Andrew's boat... > > Let me state my rule of thumb: a fix addresses a user-visible bug. > > > > And if it is a pre-requisite for patch 2/2, > > > well that doesn't seem to be a fix. > > > > It indeed is. > > > > > So in all, I'm somewhat confused. > > > And wonder if all changes can go via net-next. > > > > That's probably what will be happening. > > It does seem like the cleanest, and coincidently easiest, path. If it does not really fix anything, then net-next. Andrew
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c index e32f6724f568..fdbf61069a05 100644 --- a/drivers/net/ethernet/freescale/fec_ptp.c +++ b/drivers/net/ethernet/freescale/fec_ptp.c @@ -90,6 +90,8 @@ #define FEC_PTP_MAX_NSEC_PERIOD 4000000000ULL #define FEC_PTP_MAX_NSEC_COUNTER 0x80000000ULL +static u64 fec_ptp_read(const struct cyclecounter *cc); + /** * fec_ptp_enable_pps * @fep: the fec_enet_private structure handle @@ -136,7 +138,7 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable) * NSEC_PER_SEC - ts.tv_nsec. Add the remaining nanoseconds * to current timer would be next second. */ - tempval = fep->cc.read(&fep->cc); + tempval = fec_ptp_read(&fep->cc); /* Convert the ptp local counter to 1588 timestamp */ ns = timecounter_cyc2time(&fep->tc, tempval); ts = ns_to_timespec64(ns);
This function is used in `fec_ptp_enable_pps()` through struct cyclecounter read(). Forward declarations make it clearer, what's happening. Fixes: 61d5e2a251fb ("fec: Fix timer capture timing in `fec_ptp_enable_pps()`") Suggested-by: Frank Li <Frank.li@nxp.com> Link: https://lore.kernel.org/netdev/20240805144754.2384663-1-csokas.bence@prolan.hu/T/#ma6c21ad264016c24612048b1483769eaff8cdf20 Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> --- drivers/net/ethernet/freescale/fec_ptp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)