Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock

[Marc Zyngier <maz@kernel.org>]

On Sun, 24 Jul 2022 19:15:26 +0100,
Marek Vasut <marex@denx.de> 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.

- 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?

	M.

-- 
Without deviation from the norm, progress is not possible.