linux-kernel - Re: [PATCH v2 1/2] gpio: tegra: add multiple interrupt support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 6 Sep 2021 06:51:20 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Prathamesh Shete <pshete@...dia.com>
Cc:     linus.walleij@...aro.org, bgolaszewski@...libre.com,
        jonathanh@...dia.com, linux-gpio@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
        smangipudi@...dia.com
Subject: Re: [PATCH v2 1/2] gpio: tegra: add multiple interrupt support

On Fri, Sep 03, 2021 at 03:45:11PM +0530, Prathamesh Shete wrote:
> From: pshete <pshete@...dia.com>
> 
> T19x GPIO controller's support multiple interrupts. The GPIO
> controller is capable to route 8 interrupts per controller in
> case of NON-AON GPIO's and 4 interrupts per controller in AON GPIO.
> This is new feature starting T194
> The interrupt route map determines which interrupt line is to be used.
> 
> Signed-off-by: Prathamesh Shete <pshete@...dia.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index d38980b9923a..36bd8de6d401 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2016-2017 NVIDIA Corporation
> + * Copyright (c) 2016-2021 NVIDIA Corporation
>   *
>   * Author: Thierry Reding <treding@...dia.com>
>   */
> @@ -68,6 +68,7 @@ struct tegra_gpio_soc {
>  	unsigned int num_ports;
>  	const char *name;
>  	unsigned int instance;
> +	bool multi_ints;

Do we really have to add this? Can we not simply derive it from the
number of interrupts actually read from device tree? Doing so would also
make it easier to keep the code backwards-compatible. Remember that this
code must not fail if fed with an old device tree where not 8 interrupts
have been specified per controller.

>  
>  	const struct tegra186_pin_range *pin_ranges;
>  	unsigned int num_pin_ranges;
> @@ -451,6 +452,7 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	unsigned int parent = irq_desc_get_irq(desc);
>  	unsigned int i, offset = 0;
> +	int j, flag;

j can be unsigned in, so you can put it after i in the line above. Also,
maybe name the flag variable to something more specific to make it clear
what it's used for.

>  
>  	chained_irq_enter(chip, desc);
>  
> @@ -462,9 +464,20 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
>  
>  		base = gpio->base + port->bank * 0x1000 + port->port * 0x200;
>  
> -		/* skip ports that are not associated with this bank */
> -		if (parent != gpio->irq[port->bank])
> -			goto skip;
> +		if (!gpio->soc->multi_ints) {
> +			/* skip ports that are not associated with this bank */
> +			if (parent != gpio->irq[port->bank])
> +				goto skip;
> +
> +		} else {
> +			flag = 0;
> +			for (j = 0; j < 8; j++) {
> +				if (parent != gpio->irq[(port->bank * 8) + j])
> +					flag++;
> +			}
> +			if (!(flag & 0xF))
> +				goto skip;
> +		}
>  
>  		value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
>  
> @@ -772,6 +785,7 @@ static const struct tegra_gpio_soc tegra186_main_soc = {
>  	.ports = tegra186_main_ports,
>  	.name = "tegra186-gpio",
>  	.instance = 0,
> +	.multi_ints = false,
>  };
>  
>  #define TEGRA186_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
> @@ -798,6 +812,7 @@ static const struct tegra_gpio_soc tegra186_aon_soc = {
>  	.ports = tegra186_aon_ports,
>  	.name = "tegra186-gpio-aon",
>  	.instance = 1,
> +	.multi_ints = false,
>  };
>  
>  #define TEGRA194_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
> @@ -852,6 +867,7 @@ static const struct tegra_gpio_soc tegra194_main_soc = {
>  	.num_pin_ranges = ARRAY_SIZE(tegra194_main_pin_ranges),
>  	.pin_ranges = tegra194_main_pin_ranges,
>  	.pinmux = "nvidia,tegra194-pinmux",
> +	.multi_ints = true,
>  };
>  
>  #define TEGRA194_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
> @@ -875,6 +891,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
>  	.ports = tegra194_aon_ports,
>  	.name = "tegra194-gpio-aon",
>  	.instance = 1,
> +	.multi_ints = true,
>  };
>  
>  static const struct of_device_id tegra186_gpio_of_match[] = {

Going over this patch reminded me that I had written a similar patch a
while ago, which does things a bit differently. I've attached both
patches below. Please take a look. It's slightly bigger that your
version above, but it addresses the backwards-compatibility issue. It
also has a couple of comments that describe why the interrupt routing is
done the way it is.

For completeness I should say that I'm not sure if I've ever tested the
second patch because I had it marked "WIP", which I usually do if there
is work I know remains to be done and since there's no TODO comments or
anything in the code, I assume that I never tested it completely.
Looking at the history of the branch where I have that patch, I don't
see changes to the device tree files, so I probably never got around to
adding the multiple interrupts per bank and hence couldn't test it
properly. I can do that based on your second patch, but it'd be great if
you could go over the attached patches and let me know what you think.

Thierry

View attachment "0001-gpio-tegra186-Force-one-interrupt-per-bank.patch" of type "text/plain" (4430 bytes)

View attachment "0002-WIP-gpio-tegra186-Support-multiple-interrupts-per-ba.patch" of type "text/plain" (5098 bytes)

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)