Re: [cip-dev] [RFC PATCH 4.19.y-cip 24/50] PCI: endpoint: Replace spinlock with mutex

Pavel Machek
2020-10-21T18:34:27Z

[-- Attachment #1.1: Type: text/plain, Size: 1683 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 420 bytes --]

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#5633): https://lists.cip-project.org/g/cip-dev/message/5633
Mute This Topic: https://lists.cip-project.org/mt/77461695/4520388