Re: Re: Re: [PATCH v3 2/3] clk: eswin: Add eic7700 HSP clock driver

[Brian Masney <bmasney@redhat.com>]

Hi Stephen,

On Tue, Apr 28, 2026 at 05:21:03PM +0800, Xuyang Dong wrote:
> > Quoting Brian Masney (2026-04-24 04:15:46)
> > > On Fri, Apr 24, 2026 at 6:45 AM Xuyang Dong
> > > <dongxuyang@eswincomputing.com> wrote:
> > > > Thanks for the feedback. I did some research based on your comments.
> > > >
> > > > lock_ctx is a local variable declared inside the function body. It is not
> > > > in scope at the attribute site. The attribute expands to
> > > > __attribute__((acquire_capability(lock_ctx->lock))), and since lock_ctx
> > > > doesn't exist at the declaration point, clang's analysis cannot resolve it
> > > > and silently drops the annotation. That's why you see no warnings from
> > > > make C=2 or -Wthread-safety.
> > > >
> > > > Why -Wthread-safety produces no output
> > > > Two reasons:
> > > >   1. The lock_ctx->lock expression is unresolvable at the attribute site,
> > > >   so clang drops the annotation silently — no acquire/release tracking,
> > > >   no warnings.
> > > >   2. Even if the expression were resolvable, spinlock_t in this driver is
> > > >   a plain pointer field (spinlock_t *lock) accessed through a void *
> > > >   callback — the analysis can't track lock state through that indirection.
> > > >
> > > > The closest correct expression would be:
> > > > __acquires(((struct eic7700_hsp_regmap_lock *)arg)->lock)
> > > > But that also won't work: arg is void *, and clang's thread-safety
> > > > analysis is type-based. It can't trace through a void pointer cast to
> > > > determine which spinlock_t instance is being acquired. The analysis
> > > > would still silently ignore it.
> > > >
> > > > For void * regmap callbacks, there is no clean way to make __acquires()
> > > > work, because the lock is always hidden behind the opaque pointer.
> > > > The annotations should be dropped.
> > > >
> > > > Based on the above analysis, I suggest removing the annotations entirely.
> > > > However, I'd like to hear your thoughts on this approach.
> > > 
> > > I agree to remove the annotations. Before you post a new version,
> > > let's let this series sit out on the list for a week or two, and see
> > > if anyone else replies with the proper way to do this.
> > > 
> > 
> > Why not use a regmap instead? That would enforce locking on registers
> > and then you use the right regmap APIs to update the register under the
> > lock (like regmap_update_bits() or something).
> 
> Hi Stephen,
> 
> The common gate API, the HSP private API, and the reset driver all access 
> the same register space.
> Therefore, they need to be protected by the same data->lock.

To add to Xuyang's response: regmap is already used by this driver.
These annotations are for the lock and unlock ops on the regmap.

Brian