On Thu, Feb 06, 2025 at 04:07:23PM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Markus Elfring <Markus.Elfring@xxxxxx>
> > Sent: Wednesday, February 5, 2025 8:40 PM
> > To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx>; linux-
> > pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Bjorn Helgaas
> > <bhelgaas@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Krzysztof
> > Kozlowski <krzk+dt@xxxxxxxxxx>; Krzysztof Wilczyński <kw@xxxxxxxxx>;
> > Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>; Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>
> > Cc: LKML <linux-kernel@xxxxxxxxxxxxxxx>; Gogada, Bharat Kumar
> > <bharat.kumar.gogada@xxxxxxx>; Jingoo Han <jingoohan1@xxxxxxxxx>;
> > Simek, Michal <michal.simek@xxxxxxx>
> > Subject: Re: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> >
> > …
> > > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> > > @@ -0,0 +1,476 @@
> > …
> > > +static void amd_mdb_mask_intx_irq(struct irq_data *data)
> > > +{
> > …
> > > + raw_spin_lock_irqsave(&port->lock, flags);
> > > + val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> > > + pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_MISC);
> > > + raw_spin_unlock_irqrestore(&port->lock, flags);
> > > +}
> > …
> >
> > Under which circumstances would you become interested to apply a
> > statement like “guard(raw_spinlock_irqsave)(&port->lock);”?
> > https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/spinlock.h#L551
>
> -Thanks for review comments, raw_spin_lock_irqsave is lightweight
> and performance oriented.
>
> Achieves it by not performing few checks related to preemption, lock
> deprecation that was originally in spin_lock_irqsave.
>
> If you add guard around guard around the raw_spin_lock_irqsave then
> it should provide those additional safety checks.
>
> Its need is based on the environment, if you needs those
> checks/features. We need to check the implementation/code to
> exactly see what are those. So choose to prevent my interrupt
> handler from being affected by latency pruning
IIUC, using guard() would not add any additional checks; it only helps
prevent errors like returning from the function without releasing the
lock.
That makes sense in larger functions with multiple exits, but IMO
there's no real benefit in a case like this where the function is so
short, there's only one exit, and it's completely obvious that we
lock, do something, and unlock.
I don't *really* like guard() anyway because it's kind of magic in
that the unlock doesn't actually appear in the code, and it's kind of
hard to unravel what guard() is and how it works. But I guess that's
mostly because it's just a new idiom that takes time to internalize.
Bjorn
