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

[Xuyang Dong]

> > 
> > Add driver for the ESWIN EIC7700 high-speed peripherals system
> > clock controller and register an auxiliary device for system
> > reset controller which is named as "hsp-reset".
> > 
> > Signed-off-by: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> 
> Reviewed-by: Brian Masney <bmasney@xxxxxxxxxx>
> 
> There's one minor bit I am not sure of.
> 
> > +static void eic7700_hsp_regmap_lock(void *arg)
> > +__acquires(lock_ctx->lock)
> > +{
> > +	struct eic7700_hsp_regmap_lock *const lock_ctx = arg;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(lock_ctx->lock, flags);
> > +	lock_ctx->flags = flags;
> > +}
> 
> I don't know if the __acquires() is accurate syntax. If it needs to
> be arg, lock_ctx->lock, or ((struct eic7700_hsp_regmap_lock *)arg)->lock.
> 
> It looks like this code is triggered with clang, and I tried compiling
> this driver with:
> 
> make LLVM=1 KCFLAGS="-ferror-limit=10000 -DWARN_CONTEXT_ANALYSIS -Wthread-safety" \
>         drivers/clk/eswin/clk-eic7700-hsp.o
> 
> I also tried with 'make C=2' and I don't see any locking messages from
> this driver.
> 

Hi Brian,

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.

Best regards,
Xuyang Dong