linux-kernel - Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure

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]

Message-ID: <20210802134021.flrkpmlrcjfxdrdr@mobilestation>
Date:   Mon, 2 Aug 2021 16:40:21 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Lee Jones <lee.jones@...aro.org>, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org,
        Hoan Tran <hoan@...amperecomputing.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in
 get_irq() and configure_irqs()

Hello Andy
Thanks for the cleanup series. A tiny note is below.

On Mon, Jul 26, 2021 at 03:54:33PM +0300, Andy Shevchenko wrote:
> Shared IRQ is only enabled for ACPI enumeration, there is no need
> to have a special flag for that, since we simple can test if device
> has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
> and dwapb_configure_irqs().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
>  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
>  include/linux/platform_data/gpio-dwapb.h |  1 -
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 3eb13d6d31ef..f6ae69d5d644 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;
>  #endif
>  

> -	if (!pp->irq_shared) {
> -		girq->num_parents = pirq->nr_irqs;
> -		girq->parents = pirq->irq;
> -		girq->parent_handler_data = gpio;
> -		girq->parent_handler = dwapb_irq_handler;
> -	} else {
> +	if (has_acpi_companion(gpio->dev)) {

Before this patch the platform flag irq_shared has been as kind of a
hint regarding the shared IRQ case being covered here. But now it
doesn't seem obvious why we've got the ACPI and ACPI-less cases
differently handled. What about adding a small comment about that?
E.g. like this: "Intel ACPI-based platforms mostly have the DW APB
GPIO IRQ lane shared between several devices. In that case the
parental IRQ has to be handled in the shared way so to be properly
delivered to all the connected devices." or something more detailed
for your preference. After that the rest of the comments in the
if-clause could be discarded.

Other than that, feel free to add:
>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
Acked-by: Serge Semin <fancer.lancer@...il.com>
Tested-by: Serge Semin <fancer.lancer@...il.com>

-Sergey

>  		/* This will let us handle the parent IRQ in the driver */
>  		girq->num_parents = 0;
>  		girq->parents = NULL;
> @@ -458,6 +453,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  			dev_err(gpio->dev, "error requesting IRQ\n");
>  			goto err_kfree_pirq;
>  		}
> +	} else {
> +		girq->num_parents = pirq->nr_irqs;
> +		girq->parents = pirq->irq;
> +		girq->parent_handler_data = gpio;
> +		girq->parent_handler = dwapb_irq_handler;
>  	}
>  
>  	girq->chip = &pirq->irqchip;
> @@ -581,7 +581,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  			pp->ngpio = DWAPB_MAX_GPIOS;
>  		}
>  
> -		pp->irq_shared	= false;
>  		pp->gpio_base	= -1;
>  
>  		/*
> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> index 01935ae4e9e1..a43993e38b6e 100644
> --- a/drivers/mfd/intel_quark_i2c_gpio.c
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -227,7 +227,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev)
>  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
>  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
>  	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
> -	pdata->properties->irq_shared	= true;
>  
>  	cell->platform_data = pdata;
>  	cell->pdata_size = sizeof(*pdata);
> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
> index 0aa5c6720259..535e5ed549d9 100644
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -14,7 +14,6 @@ struct dwapb_port_property {
>  	unsigned int	ngpio;
>  	unsigned int	gpio_base;
>  	int		irq[DWAPB_MAX_GPIOS];
> -	bool		irq_shared;
>  };
>  
>  struct dwapb_platform_data {
> -- 
> 2.30.2
>