LinuxLists.cc - [PATCH v4 0/3] Migrate the PCIe-IDIO-24 and WS16C48 GPIO drivers to the regmap API

2023-03-08 02:33:28

by William Breathitt Gray

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] gpio: pcie-idio-24: Migrate to the regmap API

On Mon, Mar 06, 2023 at 04:24:30PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 07:59:52AM -0500, William Breathitt Gray wrote:
> > The regmap API supports IO port accessors so we can take advantage of
> > regmap abstractions rather than handling access to the device registers
> > directly in the driver.
> >
> > For the PCIe-IDIO-24 series of devices, the following BARs are
> > available:
> >
> > BAR[0]: memory mapped PEX8311
> > BAR[1]: I/O mapped PEX8311
> > BAR[2]: I/O mapped card registers
> >
> > There are 24 FET Output lines, 24 Isolated Input lines, and 8 TTL/CMOS
> > lines (which may be configured for either output or input). The GPIO
> > lines are exposed by the following card registers:
> >
> > Base +0x0-0x2 (Read/Write): FET Outputs
> > Base +0xB (Read/Write): TTL/CMOS
> > Base +0x4-0x6 (Read): Isolated Inputs
> > Base +0x7 (Read): TTL/CMOS
> >
> > In order for the device to support interrupts, the PLX PEX8311 internal
> > PCI wire interrupt and local interrupt input must first be enabled.
> >
> > The following card registers for Change-Of-State may be used:
> >
> > Base +0x8-0xA (Read): COS Status Inputs
> > Base +0x8-0xA (Write): COS Clear Inputs
> > Base +0xB (Read): COS Status TTL/CMOS
> > Base +0xB (Write): COS Clear TTL/CMOS
> > Base +0xE (Read/Write): COS Enable
> >
> > The COS Enable register is used to enable/disable interrupts and
> > configure the interrupt levels; each bit maps to a group of eight inputs
> > as described below:
> >
> > Bit 0: IRQ EN Rising Edge IN0-7
> > Bit 1: IRQ EN Rising Edge IN8-15
> > Bit 2: IRQ EN Rising Edge IN16-23
> > Bit 3: IRQ EN Rising Edge TTL0-7
> > Bit 4: IRQ EN Falling Edge IN0-7
> > Bit 5: IRQ EN Falling Edge IN8-15
> > Bit 6: IRQ EN Falling Edge IN16-23
> > Bit 7: IRQ EN Falling Edge TTL0-7
> >
> > An interrupt is asserted when a change-of-state matching the interrupt
> > level configuration respective for a particular group of eight inputs
> > with enabled COS is detected.
> >
> > The COS Status registers may be read to determine which inputs have
> > changed; if interrupts were enabled, an IRQ will be generated for the
> > set bits in these registers. Writing the value read from the COS Status
> > register back to the respective COS Clear register will clear just those
> > interrupts.
>
> ...
>
> > - Add mutex to prevent clobbering the COS_ENABLE register when masking
> > IRQ and setting their type configuration
>
> But this doesn't explain killing the raw spin lock.
>
> I don't understand how is it suppose to work then.
>
> What is this lock for and how IRQ related registers can be updated
> with RT type of kernel?
>
> If mutex is okay, doesn't mean we have to add 'can_sleep = true'?

I admit that RT locking is an area I often have trouble understanding,
so perhaps you can help me make sure whether this lock type change makes
sense here.

My reasoning for why we need a lock is that the COS Enable register is
modified in both the idio_24_set_type_config() function and the
idio_24_handle_mask_sync() function. These respectively are called by
regmap_irq_set_type() and regmap_irq_sync_unlock(), which are callbacks
for a struct irq_chip irq_set_type() and irq_bus_sync_unlock() members.

I think these callbacks could be executed in parrallel, but are allowed
to sleep, so that is why I selected mutex as the lock type here (I
probably do need to add 'can_sleep = true'). Is this reasoning correct,
or have I made a mistake here?

William Breathitt Gray

Attachments:

(No filename) (3.50 kB)
signature.asc (228.00 B)
Download all attachments

2023-03-08 02:51:40

On Wed, Mar 08, 2023 at 03:06:18PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 07, 2023 at 09:51:26PM -0500, William Breathitt Gray wrote:
> > On Mon, Mar 06, 2023 at 04:20:03PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 06, 2023 at 07:59:53AM -0500, William Breathitt Gray wrote:
>
> ...
>
> > > > - raw_spinlock_t lock;
> > > > + spinlock_t lock;
> > >
> > > This is a regression.
> > > That said, do we need a support of raw spin locks in the regmap IRQ?
> >
> > So this code has a similar need as the gpio-pcie-idio-24 patch: guard
> > registers between handle_mask_sync() and set_type_config(); however, now
> > we also need to protect registers in regmap_irq_thread(). We can't use a
> > mutex here because regmap_irq_thread() is executed in an interrupt
> > context so we cannot sleep.
> >
> > This might be a mistake in my understanding: I chose spinlock_t here
> > because I believed it to map out to a raw_spinlock_t anyway underneath,
> > whereas on RT kernels it would map out to whatever the equivalent is. I
> > suspect this is not actually the case. Would using raw_spinlock_t
> > explicitly be the correct way to go for this particular case?
>
> You may read the commit message of the 27d9098cff6e ("pinctrl: intel:
> Use raw_spinlock for locking"). TL;DR: this is only affects IRQ chips,
> so if your GPIO controller is _not_ an IRQ chip, you are fine.
>
> WRT the other driver, can_sleep may reduce scope of the use of GPIOs
> and even make a regression if any consumer don't want that behaviour
> and currently works.

Looking through kernel/irq/manage.c, I see the raw_spinlock desc->lock
is taken in __setup_irq() before potentially calling __irq_set_trigger()
which ultimate calls the chip->irq_set_type() callback. So it seems
unsafe to sleep within at least this callback which is utilized by both
drivers, so both gpio-pcie-idio-24 and gpio-ws16c48 will need the
raw_spinlock lock type afterall.

I'll make the necessary changes and release a v5 of this patchset.

As an aside, I wonder if locking is not needed if we only utilize the
set_type_config() callback, because the desc->lock taken by the irq
subsystem will be enough to guard between regmap_irq_set_type() and
regmap_irq_thread(). It's not valid for our particular case here because
we also utilize a handle_mask_sync() callback (chip_bus_lock() is not
protected by desc->chip) but it's something to think about.

William Breathitt Gray

Attachments:

(No filename) (2.38 kB)
signature.asc (228.00 B)
Download all attachments