| Message ID | 20230417170741.1714310-2-shmuel.h@siklu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mvpp2: tai: add extts support | expand |
On Mon, Apr 17, 2023 at 08:07:39PM +0300, Shmuel Hazan wrote: > In some configurations, a single TAI can be responsible for multiple > mvpp2 interfaces. However, the mvpp2 driver will call mvpp22_tai_stop > and mvpp22_tai_start per interface RX timestamp disable/enable. > > As a result, disabling timestamping for one interface would stop the > worker and corrupt the other interface's RX timestamps. > > This commit solves the issue by introducing a simpler ref count for each > TAI instance. > > Fixes: ce3497e2072e ("net: mvpp2: ptp: add support for receive timestamping") > Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com> > --- > .../net/ethernet/marvell/mvpp2/mvpp2_tai.c | 30 ++++++++++++++++--- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c > index 95862aff49f1..2e3d43b1bac1 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c > @@ -61,6 +61,7 @@ struct mvpp2_tai { > u64 period; // nanosecond period in 32.32 fixed point > /* This timestamp is updated every two seconds */ > struct timespec64 stamp; > + u16 poll_worker_refcount; > }; > > static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set) > @@ -368,18 +369,39 @@ void mvpp22_tai_tstamp(struct mvpp2_tai *tai, u32 tstamp, > hwtstamp->hwtstamp = timespec64_to_ktime(ts); > } > > +static void mvpp22_tai_start_unlocked(struct mvpp2_tai *tai) > +{ > + tai->poll_worker_refcount++; > + if (tai->poll_worker_refcount > 1) > + return; > + > + ptp_schedule_worker(tai->ptp_clock, 0); So the old code used to have delay here, not 0. And delay would be returned by mvpp22_tai_aux_work() and have a value of 2000ms. So this is a clear change in behaviour. Why is it O.K. not to delay for 2 seconds? Should you actually still have the delay, pass it as a parameter into this function? Andrew
On Tue, 2023-04-18 at 02:44 +0200, Andrew Lunn wrote: > Caution: This is an external email. Please take care when clicking links or opening attachments. > > > On Mon, Apr 17, 2023 at 08:07:39PM +0300, Shmuel Hazan wrote: > > In some configurations, a single TAI can be responsible for multiple > > mvpp2 interfaces. However, the mvpp2 driver will call mvpp22_tai_stop > > and mvpp22_tai_start per interface RX timestamp disable/enable. > > > > As a result, disabling timestamping for one interface would stop the > > worker and corrupt the other interface's RX timestamps. > > > > This commit solves the issue by introducing a simpler ref count for each > > TAI instance. > > > > Fixes: ce3497e2072e ("net: mvpp2: ptp: add support for receive timestamping") > > Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com> > > --- > > .../net/ethernet/marvell/mvpp2/mvpp2_tai.c | 30 ++++++++++++++++--- > > 1 file changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c > > index 95862aff49f1..2e3d43b1bac1 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c > > @@ -61,6 +61,7 @@ struct mvpp2_tai { > > u64 period; // nanosecond period in 32.32 fixed point > > /* This timestamp is updated every two seconds */ > > struct timespec64 stamp; > > + u16 poll_worker_refcount; > > }; > > > > static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set) > > @@ -368,18 +369,39 @@ void mvpp22_tai_tstamp(struct mvpp2_tai *tai, u32 tstamp, > > hwtstamp->hwtstamp = timespec64_to_ktime(ts); > > } > > > > +static void mvpp22_tai_start_unlocked(struct mvpp2_tai *tai) > > +{ > > + tai->poll_worker_refcount++; > > + if (tai->poll_worker_refcount > 1) > > + return; > > + > > + ptp_schedule_worker(tai->ptp_clock, 0); > > So the old code used to have delay here, not 0. And delay would be > returned by mvpp22_tai_aux_work() and have a value of 2000ms. So this > is a clear change in behaviour. Why is it O.K. not to delay for 2 > seconds? Should you actually still have the delay, pass it as a > parameter into this function? Hi Andrew, Thanks for your feedback. I will add more context about this change in the next version. Before of this change, we ran the first iteration internally (on mvpp22_tai_start), and then schedule another iteration in a delay to run on the worker context. Howver, since the iteration itself locks tai->lock, it is not possible to run it from mvpp22_tai_start anymore as tai->lock is already locked. So, to summarize, from my point of view, this change should not introduce any change in behavior as we will still run the first iteration immediately, and then run it each 2s, as we did before. The only change is that the first iteration will also be done from the worker thread. However, I let me know if I am missing anything. --- Thanks, Shmuel. > > Andrew
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c index 95862aff49f1..2e3d43b1bac1 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c @@ -61,6 +61,7 @@ struct mvpp2_tai { u64 period; // nanosecond period in 32.32 fixed point /* This timestamp is updated every two seconds */ struct timespec64 stamp; + u16 poll_worker_refcount; }; static void mvpp2_tai_modify(void __iomem *reg, u32 mask, u32 set) @@ -368,18 +369,39 @@ void mvpp22_tai_tstamp(struct mvpp2_tai *tai, u32 tstamp, hwtstamp->hwtstamp = timespec64_to_ktime(ts); } +static void mvpp22_tai_start_unlocked(struct mvpp2_tai *tai) +{ + tai->poll_worker_refcount++; + if (tai->poll_worker_refcount > 1) + return; + + ptp_schedule_worker(tai->ptp_clock, 0); +} + void mvpp22_tai_start(struct mvpp2_tai *tai) { - long delay; + unsigned long flags; - delay = mvpp22_tai_aux_work(&tai->caps); + spin_lock_irqsave(&tai->lock, flags); + mvpp22_tai_start_unlocked(tai); + spin_unlock_irqrestore(&tai->lock, flags); +} - ptp_schedule_worker(tai->ptp_clock, delay); +static void mvpp22_tai_stop_unlocked(struct mvpp2_tai *tai) +{ + tai->poll_worker_refcount--; + if (tai->poll_worker_refcount) + return; + ptp_cancel_worker_sync(tai->ptp_clock); } void mvpp22_tai_stop(struct mvpp2_tai *tai) { - ptp_cancel_worker_sync(tai->ptp_clock); + unsigned long flags; + + spin_lock_irqsave(&tai->lock, flags); + mvpp22_tai_stop_unlocked(tai); + spin_unlock_irqrestore(&tai->lock, flags); } static void mvpp22_tai_remove(void *priv)
In some configurations, a single TAI can be responsible for multiple mvpp2 interfaces. However, the mvpp2 driver will call mvpp22_tai_stop and mvpp22_tai_start per interface RX timestamp disable/enable. As a result, disabling timestamping for one interface would stop the worker and corrupt the other interface's RX timestamps. This commit solves the issue by introducing a simpler ref count for each TAI instance. Fixes: ce3497e2072e ("net: mvpp2: ptp: add support for receive timestamping") Signed-off-by: Shmuel Hazan <shmuel.h@siklu.com> --- .../net/ethernet/marvell/mvpp2/mvpp2_tai.c | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-)