Hi!

> > > commit 3d3248dbd018502f654064c78efcd2e165ab3486 upstream.
> > >
> > > The pci_epc_ops is not intended to be invoked from interrupt context.
> > > Hence replace spin_lock_irqsave and spin_unlock_irqrestore with
> > > mutex_lock and mutex_unlock respectively.
> > 
> > Could I get some kind of explanation why this is good idea?
> > 
> Apart of one mentioned above other point I would add is on a single core machine mutex_lock/unlock would be good choice.
> 
> Also to add the callbacks in controller driver might sleep. For example in raise_irq callback  [1], [2].
> 
> > As long as code protected by the locks does not sleep, spinlocks are
> > okay... (but they should not need "_irqsave" variants).
> > 
> > They are likely to have better performance, too, when protected code
> > is small and fast.
> > 
> I do agree with the above two points *if the code isn't sleeping*.

Okay, we can't really protect sleeping code with a mutex.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/tree/drivers/pci/controller/pcie-rockchip-ep.c?h=linux-4.19.y-cip#n410

But this one is not sleeping. It is mdelay(), not msleep().

> [2] https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/tree/drivers/pci/controller/pcie-cadence-ep.c?h=linux-4.19.y-cip#n310

And same here.

If there's a place which does sleep with the spinlock held, I'd still
be curious.

OTOH, 1 msec is already threshold where mutex makes sense, so... this
is okay.

Thanks,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany