On 7/25/22 08:53, Marc Zyngier wrote:
On Sun, 24 Jul 2022 19:15:26 +0100,
Marek Vasut <marex@xxxxxxx> wrote:
On 7/24/22 19:50, Marc Zyngier wrote:
[...]
+++ b/drivers/gpio/gpio-mxc.c
@@ -147,6 +147,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
{
struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
struct mxc_gpio_port *port = gc->private;
+ unsigned long flags;
u32 bit, val;
u32 gpio_idx = d->hwirq;
int edge;
@@ -185,6 +186,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
return -EINVAL;
}
+ spin_lock_irqsave(&port->gc.bgpio_lock, flags);
In my tree, bgpio is a raw spinlock, and has been since 3c938cc5cebcb.
Now, looking a bit closer at this code, I have to withdraw my earlier
comment about the lack of mutual exclusion in the existing code. All
writes are of the form:
writel(single_bit_mask, some_addr + MXS_{SET,CLR});
which indicates that the write side can be accessed with a hot-bit
pattern, avoiding a RWM pattern and thus the need for a lock.
Only for the ISR/IMR, not for the GDIR register, that's why the locks
are added only around the RMW which don't have these "hot bits".
Only your patch adds any GDIR access.
Your second patch, however requires the lock. I'm not sure it is safe
to do after the interrupt type has been configured though. You may
want to refer to the TRM for this.
There is in fact another unprotected RMW in gpio_set_irq_type() , look
for GPIO_EDGE_SEL, so the locks should be valid as they are now, right
?
I seem to be confused with gpio-mxs.c, and gpio-mxc indeed needs the
lock.
However, you have totally ignored my earlier comments in your v4:
- This doesn't compile, as bgpio_lock has been changed to a *raw*
spinlock. You obviously haven't even bothered testing your patch.
Yes indeed, I tested every single one on 5.18.y . I noticed the raw
spinlock change is only in next.
- I asked for a cover letter for any series with multiple patch.
That's not exactly a new requirement.
So we got 4 versions in just over 24 hours, none of which actually
work. Do you see the overarching problem?
Lemme rebase this on next and send v5.
