[PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK

[Conor Dooley]

On Fri, Aug 04, 2023 at 05:53:35PM +0800, Guo Ren wrote:
> On Fri, Aug 4, 2023 at 5:06 PM Conor Dooley <conor.dooley at microchip.com> wrote:
> > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren at kernel.org wrote:
> > > From: Guo Ren <guoren at linux.alibaba.com>

> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index f8dbbe1bbd34..d9694fe40a9a 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void)
> > >                * spinlock value, the only way is to change from queued_spinlock to
> > >                * ticket_spinlock, but can not be vice.
> > >                */
> > > -             if (!force_qspinlock) {
> > > +             if (!force_qspinlock &&
> > > +                 !riscv_has_errata_thead_qspinlock()) {
> > >                       set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa);
> >
> > Is this a generic vendor extension (lol @ that misnomer) or is it an
> > erratum? Make your mind up please. As has been said on other series, NAK
> > to using march/vendor/imp IDs for feature probing.
>
> The RISCV_ISA_EXT_XTICKETLOCK is a feature extension number,

No, that is not what "ISA_EXT" means, nor what the X in "XTICKETLOCK"
would imply.

The comment above these reads:
  These macros represent the logical IDs of each multi-letter RISC-V ISA
  extension and are used in the ISA bitmap.

> and it's
> set by default for forward-compatible. We also define a vendor
> extension (riscv_has_errata_thead_qspinlock) to force all our
> processors to use qspinlock; others still stay on ticket_lock.

No, "riscv_has_errata_thead_qspinlock()" would be an _erratum_, not a
vendor extension. We need to have a discussion about how to support
non-standard extensions etc, not abuse errata. That discussion has been
started on the v0.7.1 vector patches, but has not made progress yet.

> The only possible changing direction is from qspinlock to ticket_lock
> because ticket_lock would dirty the lock value, which prevents
> changing to qspinlock next. So startup with qspinlock and change to
> ticket_lock before smp up. You also could use cmdline to try qspinlock
> (force_qspinlock).

I don't see what the relevance of this is, sorry. I am only commenting
on how you are deciding that the hardware is capable of using qspinlocks,
I don't intend getting into the detail unless the powers that be deem
this series worthwhile, as I mentioned:
> > I've got some thoughts on other parts of this series too, but I'm not
> > going to spend time on it unless the locking people and Palmer ascent
> > to this series.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20230804/5dbc680f/attachment.sig>