Re: [PATCH v2 1/3] net: mvpp2: tai: add refcount for ptp worker

[Shmuel Hazan <shmuel.h@siklu.com>]

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