On Thu, 2024-06-06 at 14:28 +0100, Diogo Ivo wrote:
> On 6/6/24 11:32 AM, Paolo Abeni wrote:
> > On Tue, 2024-06-04 at 14:15 +0100, Diogo Ivo wrote:
> > > @@ -571,6 +573,57 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
> > > return ret;
> > > }
> > >
> > > +static void icss_iep_cap_cmp_work(struct work_struct *work)
> > > +{
> > > + struct icss_iep *iep = container_of(work, struct icss_iep, work);
> > > + const u32 *reg_offs = iep->plat_data->reg_offs;
> > > + struct ptp_clock_event pevent;
> > > + unsigned int val;
> > > + u64 ns, ns_next;
> > > +
> > > + spin_lock(&iep->irq_lock);
> >
> > 'irq_lock' is always acquired with the irqsave variant, and here we are
> > in process context. This discrepancy would at least deserve a comment;
> > likely the above lock type is not correct.
>
> If my reasoning is correct I believe this variant is correct here. The
> register accesses in the IRQ handler and icss_iep_cap_cmp_work() are
> orthogonal, so there should be no need to guard against the IRQ handler
> here. This is the case for the other places where the _irqsave() variant
> is used, so using the _irqsave() variant is overkill there.
>
> From my understanding this is a remnant of the SDK's version of the
> driver, where all of the processing now done in icss_iep_cap_cmp_work()
> was done directly in the IRQ handler, meaning that we had to guard
> against some other thread calling icss_iep_ptp_enable() and accessing
> for example ICSS_IEP_CMP1_REG0 concurrently. This can be seen in the
> comment on line 114:
>
> struct icss_iep {
> ...
> spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
> ...
> };
>
> For v3 I can add a comment with a condensed version of this argument in
> icss_iep_cap_cmp_work().
Please have run with LOCKDEP enabled, I think it should splat with the
mix of plain spinlock and spinlock_irqsave this patch brings in.
> With this said it should be possible to change this spinlock to a mutex as
> well, since all the possibilities for concurrency happen outside of
> interrupt context. I can add a patch to this series doing that if you
> agree with my reasoning above and find it beneficial. For this some
> comments from TI would also be good to have in case I missed something
> or there is some other factor that I am not aware of.
It looks like that most critical section protected by iep->irq_lock are
already under ptp_clk_mutex protection. AFAICS all except the one
introduced by this patch.
If so, you could acquire such mutex even in icss_iep_cap_cmp_work() and
completely remove iep->irq_lock.
Cheers,
Paolo
