linux-kernel - Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver

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] [day] [month] [year] [list]

Message-ID: <CACRpkdbF_CHmfbumKYf751wpW7dX=bF6cD-V3Y=jvsGQ21bPew@mail.gmail.com>
Date:	Tue, 2 Apr 2013 10:55:32 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Tien Hock Loh <thloh@...era.com>
Cc:	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org
Subject: Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver

On Thu, Mar 28, 2013 at 12:40 PM, Tien Hock Loh <thloh@...era.com> wrote:

>> (...)
>> > +Altera GPIO specific properties:
>> > +- width: Width of the GPIO bank, range from 1-32
>> > +- level_trigger: Specifies whether the GPIO interrupt is level trigger.
>> > +  This field is required if the Altera GPIO controller used has IRQ enabled.
>>
>> This is a configuration for the irc_chip portion of the driver
>> I guess, isn't the usual custom that you don't set up
>> things like this from the device tree, but instead use the
>> kernels facilities on request_irq() such as:
>>
>> #define IRQF_TRIGGER_NONE       0x00000000
>> #define IRQF_TRIGGER_RISING     0x00000001
>> #define IRQF_TRIGGER_FALLING    0x00000002
>> #define IRQF_TRIGGER_HIGH       0x00000004
>> #define IRQF_TRIGGER_LOW        0x00000008
>>
>> ?
>>
>> I.e. that this is something you do at runtime and not a static
>> config from the device tree?
>>
>> Or shall this be percieved as some kind of default setting?
>
> The Altera GPIO IP cannot be software configurable. It is determined by
> the tool that generates the hardware, and thus I've put this as device
> tree parameter. If I understand correctly, if we implement using
> irq_set_type, it should be software configurable.
>
> Technically it can still be implemented in irq_set_type, I'm just not
> sure if it is the correct way, because the trigger type cannot be "set"
> in Altera GPIO. Please let me know if you think this should still be
> implemented as irq_set_type.

Hm I must be getting things wrong. It's correct that the driver
needs to keep track of if a certain IRQ line has a certain
trigger characteristic. So the member in this struct:

+struct altera_gpio_chip {
+       struct of_mm_gpio_chip mmchip;
+       struct irq_domain *irq; /* GPIO controller IRQ number */
+       spinlock_t gpio_lock;   /* Lock used for synchronization */
+       int level_trigger;

Is OK. But it should definately be a bool instead.

Then the irq_set_type() should check if the requested type is available.

You state that some interrupts are edge triggered, so this is
clearly wrong:

+static int altera_gpio_irq_set_type(struct irq_data *d,
+                               unsigned int type)
+{
+       if (type == IRQ_TYPE_NONE)
+               return 0;
+       return -EINVAL;
+}

I think it should rather be something like:

static int altera_gpio_irq_set_type(struct irq_data *d,
                               unsigned int type)
{
       struct altera_gpio_chip *a = irq_data_get_irq_chip_data(d);

       if (a->level_trigger) {
           if (type == IRQ_TYPE_LEVEL_HIGH)
              /* Configure the IRQ for high level */
           else if (type == IRQ_TYPE_LEVEL_LOW)
              /* Configure the IRQ for low level */
           else
             return -EINVAL;
       } else {
           /* assume it's edge triggered? */
           if (type == IRQ_TYPE_EDGE_RISING)
              /* Configure for rising edge */
           else if (type == IRQ_TYPE_EDGE_FALLING)
              /* Configure for falling edge */
           else
                return -EINVAL;
       }
}

As you can see from above a few new questions arise.

When you say an IRQ is "level trigged" does it actually mean
it is high or low level triggered? Or is this somehow configurable?

If it is *not* edge triggered, is it then implicitly edge
triggered?

If it is then implicitly edge triggered, does it trigger on
rising or falling edges or is this configurable?

As you see, the .irq_set_type needs to reflect what the
hardware can actually react to, and return errors on the
rest.

The current implementation which states that the hardware
cannot react to any IRQ is clearly wrong.

Yours,
Linus Walleij
--
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/