Re: [PATCH v2] pci/aer_inject: switching inject_lock to raw_spinlock_t

[Guangbo Cui]

On Fri, Oct 10, 2025 at 09:15:55AM +0200, Sebastian Andrzej Siewior wrote:
> On 2025-10-09 15:06:50 [+0000], Guangbo Cui wrote:
> > index 91acc7b17f68..6dd231d9fccd 100644
> > --- a/drivers/pci/pcie/aer_inject.c
> > +++ b/drivers/pci/pcie/aer_inject.c
> > @@ -523,8 +523,8 @@ static int __init aer_inject_init(void)
> >  static void __exit aer_inject_exit(void)
> >  {
> >     struct aer_error *err, *err_next;
> > -   unsigned long flags;
> >     struct pci_bus_ops *bus_ops;
> > +   LIST_HEAD(einjected_to_free);
> >
> >     misc_deregister(&aer_inject_device);
> >
> > @@ -533,12 +533,14 @@ static void __exit aer_inject_exit(void)
> >             kfree(bus_ops);
> >     }
> >
> > -   spin_lock_irqsave(&inject_lock, flags);
> > -   list_for_each_entry_safe(err, err_next, &einjected, list) {
> > +   scoped_guard(raw_spinlock_irqsave, &inject_lock) {
> > +           list_splice_init(&einjected, &einjected_to_free);
> > +   }
>
> I would either convert _all_ instance of the lock usage to guard
> notation or none. But not one.


> Also I wouldn't split everything to another list just to free it later.
> I would argue here that locking in aer_inject_exit() locking is not
> required because the misc device is removed (no one can keep it open as
> it would not allow module removal) and so this is the only possible
> user.
> Therefore you can iterate over the list and free items lock less.
> This reasoning of this change needs to be part of your commit
> description. Also _why_ this becomes a problem. You do not mention this
> change at all.

Hi, Sebastian

Yeah, you're right. Once misc_deregister is called, no new user can add
err injection, and aer_inj_pci_ops was deleted by pci_bus_ops_pop below.
All places that access einjected have already been released, so freeing
einjected can be done without locking.

I will drop the lock in aer_inject_exit, and update commit msg.

Best regards,
Guangbo