linux-kernel - Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

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: <522FBED9.9000305@collabora.co.uk>
Date:	Wed, 11 Sep 2013 02:52:41 +0200
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Mark Brown <broonie@...nel.org>,
	Lars Poeschel <poeschel@...onage.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Lars Poeschel <larsi@....tu-dresden.de>,
	Grant Likely <grant.likely@...aro.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ian.campbell@...rix.com>,
	Kumar Gala <galak@...eaurora.org>,
	Pawel Moll <pawel.moll@....com>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Enric Balletbo i Serra <eballetbo@...il.com>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>,
	Santosh Shilimkar <santosh.shilimkar@...com>,
	Kevin Hilman <khilman@...aro.org>,
	Balaji T K <balajitk@...com>,
	Tony Lindgren <tony@...mide.com>,
	Jon Hunter <jgchunter@...il.com>, joelf@...com
Subject: Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

On 09/11/2013 12:34 AM, Stephen Warren wrote:
> On 09/10/2013 03:37 PM, Mark Brown wrote:
>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>> 
>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>> hence prevent the driver's own gpio_request() from succeeding,
>>> since the GPIO is already requested? If this is not a problem, it
>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>> for the GPIO.
>> 
>> Or at the very least something that's likely to break in the
>> future.
> 
> Looking at the GPIO code, it already prevents double-requests:
> 
>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>                 desc_set_label(desc, label ? : "?");
>>                 status = 0;
>>         } else {
>>                 status = -EBUSY;
>>                 module_put(chip->owner);
>>                 goto done;
>>         }
> 
> And I tested it in practice, and it really does fail.
> 

I'm a bit confused now. Doesn't the fact that gpio_request() prevents
double-requests mean that the use-case that you say that have not been covered
by this patch can't actually happen?

I mean, if when using board files an explicit call to gpio_request() is made by
platform code then a driver can't call gpio_request() for the same gpio. So this
patch shouldn't cause any regression since is just auto-requesting a GPIO when
is mapped as an IRQ in a DT which basically will be the same that was made by
board files before.

To give you an example of an use-case that this patch is trying to solve:

OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
interface with Pseudo-SRAM devices such as ethernet controllers. So with board
files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):

void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *gpmc_cfg)
{
       ....
       if (gpio_request_one(gpmc_cfg->gpio_irq, GPIOF_IN, "smsc911x irq")) {
                pr_err("Failed to request IRQ GPIO%d\n", gpmc_cfg->gpio_irq);
                goto free1;
       }
       ....
       gpmc_smsc911x_resources[1].start = gpio_to_irq(gpmc_cfg->gpio_irq);
       ...
       pdev = platform_device_register_resndata(NULL, "smsc911x", gpmc_cfg->id,
                 gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources),
                 &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config));
       ...
}

and later in the smsc911x ethernet driver probe function:

static int smsc911x_drv_probe(struct platform_device *pdev)
{
       retval = request_irq(dev->irq, smsc911x_irqhandler,
                             irq_flags | IRQF_SHARED, dev->name, dev);
       ...
       irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
       ...
       dev->irq = irq_res->start;
       ...
       retval = request_irq(dev->irq, smsc911x_irqhandler,
                             irq_flags | IRQF_SHARED, dev->name, dev);
       ...
}

The driver just knows that it has to get the IRQ from a struct resource and it
doesn't care if that is a real IRQ line from an interrupt controller or a GPIO
pin mapped as an IRQ. With linus patch I just can define on a DT (GPMC
properties omitted for simplicity):

        ethernet@5,0 {
                pinctrl-names = "default";
                pinctrl-0 = <&smsc911x_pins>;
                compatible = "smsc,lan9221", "smsc,lan9115";
                reg = <5 0 0xff>;
                bank-width = <2>;
                interrupt-parent = <&gpio6>;
                interrupts = <16 8>;
                vmmc-supply = <&vddvario>;
                vmmc_aux-supply = <&vdd33a>;
                reg-io-width = <4>;

                smsc,save-mac-address;
        };

and it will just work. Without Linus patch the call to request_irq() will fail
because a call to gpio_request() has not been made (and thus the GPIO bank was
not enabled).

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/