| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9965d0b3-f169-45c4-9345-8588865a2ca4@broadcom.com>
Date: Thu, 30 Jan 2025 13:35:28 -0800
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Artur Weber <aweber.kernel@...il.com>, Ray Jui <rjui@...adcom.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
<brgl@...ev.pl>, Scott Branden <sbranden@...adcom.com>,
Markus Mayer <markus.mayer@...aro.org>, Tim Kryger <tim.kryger@...aro.org>,
Matt Porter <matt.porter@...aro.org>, Markus Mayer <mmayer@...adcom.com>,
Christian Daudt <csd@...adcom.com>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when
requesting IRQ
On 1/30/25 09:10, Artur Weber wrote:
> The settings for all GPIOs are locked by default in bcm_kona_gpio_reset.
> The settings for a GPIO are unlocked when requesting it as a GPIO, but
> not when requesting it as an interrupt, causing the IRQ settings to not
> get applied.
>
> Fix this by making sure to unlock the right bits when an IRQ is requested.
> To avoid a situation where an IRQ being released causes a lock despite
> the same GPIO being used by a GPIO request or vice versa, add an unlock
> counter and only lock if it reaches 0.
>
> Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
> Signed-off-by: Artur Weber <aweber.kernel@...il.com>
> ---
> drivers/gpio/gpio-bcm-kona.c | 69 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 57 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
> index 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644
> --- a/drivers/gpio/gpio-bcm-kona.c
> +++ b/drivers/gpio/gpio-bcm-kona.c
> @@ -69,6 +69,22 @@ struct bcm_kona_gpio {
> struct bcm_kona_gpio_bank {
> int id;
> int irq;
> + /*
> + * Used to keep track of lock/unlock operations for each GPIO in the
> + * bank.
> + *
> + * All GPIOs are locked by default (see bcm_kona_gpio_reset), and the
> + * unlock count for all GPIOs is 0 by default. Each unlock increments
> + * the counter, and each lock decrements the counter.
> + *
> + * The lock function only locks the GPIO once its unlock counter is
> + * down to 0. This is necessary because the GPIO is unlocked in two
> + * places in this driver: once for requested GPIOs, and once for
> + * requested IRQs. Since it is possible for a GPIO to be requested
> + * as both a GPIO and an IRQ, we need to ensure that we don't lock it
> + * too early.
> + */
> + u8 gpio_unlock_count[GPIO_PER_BANK];
> /* Used in the interrupt handler */
> struct bcm_kona_gpio *kona_gpio;
> };
> @@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
> unsigned long flags;
> int bank_id = GPIO_BANK(gpio);
> int bit = GPIO_BIT(gpio);
> + struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>
> - raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> + if (bank->gpio_unlock_count[bit] == 0) {
> + dev_err(kona_gpio->gpio_chip.parent,
> + "Unbalanced locks for GPIO %u\n", gpio);
> + return;
Don't you want to release &kona_gpio->lock here?
> + }
>
> - val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> - val |= BIT(bit);
> - bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> + bank->gpio_unlock_count[bit] -= 1;
>
> - raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> + if (bank->gpio_unlock_count[bit] == 0) {
> + raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +
> + val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> + val |= BIT(bit);
> + bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +
> + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> + }
> }
>
> static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
> @@ -104,14 +131,20 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
> unsigned long flags;
> int bank_id = GPIO_BANK(gpio);
> int bit = GPIO_BIT(gpio);
> + struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>
> - raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> + if (bank->gpio_unlock_count[bit] == 0) {
> + raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>
> - val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> - val &= ~BIT(bit);
> - bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> + val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> + val &= ~BIT(bit);
> + bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
>
> - raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +
> + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> + }
> +
> + bank->gpio_unlock_count[bit] += 1;
> }
>
> static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio)
> @@ -362,6 +395,7 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d)
>
> kona_gpio = irq_data_get_irq_chip_data(d);
> reg_base = kona_gpio->reg_base;
> +
> raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>
> val = readl(reg_base + GPIO_INT_MASK(bank_id));
> @@ -384,6 +418,7 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d)
>
> kona_gpio = irq_data_get_irq_chip_data(d);
> reg_base = kona_gpio->reg_base;
> +
> raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>
> val = readl(reg_base + GPIO_INT_MSKCLR(bank_id));
> @@ -479,15 +514,25 @@ static void bcm_kona_gpio_irq_handler(struct irq_desc *desc)
> static int bcm_kona_gpio_irq_reqres(struct irq_data *d)
> {
> struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
> + unsigned int gpio = d->hwirq;
>
> - return gpiochip_reqres_irq(&kona_gpio->gpio_chip, d->hwirq);
> + /*
> + * We need to unlock the GPIO before any other operations are performed
> + * on the relevant GPIO configuration registers
> + */
> + bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
> +
> + return gpiochip_reqres_irq(&kona_gpio->gpio_chip, gpio);
> }
>
> static void bcm_kona_gpio_irq_relres(struct irq_data *d)
> {
> struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
> + unsigned int gpio = d->hwirq;
> +
> + bcm_kona_gpio_lock_gpio(kona_gpio, gpio);
>
> - gpiochip_relres_irq(&kona_gpio->gpio_chip, d->hwirq);
> + gpiochip_relres_irq(&kona_gpio->gpio_chip, gpio);
> }
>
> static struct irq_chip bcm_gpio_irq_chip = {
>
--
Florian
Powered by blists - more mailing lists