linux-kernel - Re: [PATCH v3] gpio-mlxbf2: only get IRQ for device instances 0 and 3

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: <aIDqWRmijLIyqcDB@smile.fi.intel.com>
Date: Wed, 23 Jul 2025 16:57:45 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: David Thompson <davthompson@...dia.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	linus.walleij@...aro.org, davem@...emloft.net, asmaa@...dia.com,
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, Shravan Kumar Ramani <shravankr@...dia.com>
Subject: Re: [PATCH v3] gpio-mlxbf2: only get IRQ for device instances 0 and 3

On Tue, Jul 22, 2025 at 04:52:02PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jul 21, 2025 at 6:22 PM David Thompson <davthompson@...dia.com> wrote:
> >
> > The gpio-mlxbf2 driver interfaces with four GPIO controllers,
> > device instances 0-3. There are two IRQ resources shared between
> > the four controllers, and they are found in the ACPI table for
> > device instances 0 and 3.  The driver should not attempt to get
> > an IRQ resource when probing device instance 1 or 2, otherwise
> > the following error is logged:
> >   mlxbf2_gpio MLNXBF22:01: error -ENXIO: IRQ index 0 not found

...

> > -       irq = platform_get_irq(pdev, 0);

So, why not simply change this to the _optional() call?

> > -       if (irq >= 0) {
> > -               girq = &gs->gc.irq;
> > -               gpio_irq_chip_set_chip(girq, &mlxbf2_gpio_irq_chip);
> > -               girq->handler = handle_simple_irq;
> > -               girq->default_type = IRQ_TYPE_NONE;
> > -               /* This will let us handle the parent IRQ in the driver */
> > -               girq->num_parents = 0;
> > -               girq->parents = NULL;
> > -               girq->parent_handler = NULL;
> > -
> > -               /*
> > -                * Directly request the irq here instead of passing
> > -                * a flow-handler because the irq is shared.
> > -                */
> > -               ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
> > -                                      IRQF_SHARED, name, gs);
> > -               if (ret) {
> > -                       dev_err(dev, "failed to request IRQ");
> > -                       return ret;
> > +       colon_ptr = strchr(dev_name(dev), ':');
> > +       if (!colon_ptr) {
> > +               dev_err(dev, "invalid device name format\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = kstrtol(++colon_ptr, 16, &num);
> > +       if (ret) {
> > +               dev_err(dev, "invalid device instance\n");
> > +               return ret;
> > +       }
> > +
> 
> That is *really* fragile. Andy, Mika: does this look remotely correct
> to you? I don't know much about ACPI systems.

I totally agree with you. This is an ugly hack and here is formal NAK from me.
The ACPI tables that doesn't provide an IRQ resources (in any of its possible
type) can be simply ignored by not requesting that IRQ. The message above
AFAICT is harmless. Above I proposed the better fix.

> > +       if (!num || num == 3) {
> > +               irq = platform_get_irq(pdev, 0);
> > +               if (irq >= 0) {
> > +                       girq = &gs->gc.irq;
> > +                       gpio_irq_chip_set_chip(girq, &mlxbf2_gpio_irq_chip);
> > +                       girq->handler = handle_simple_irq;
> > +                       girq->default_type = IRQ_TYPE_NONE;
> > +                       /* This will let us handle the parent IRQ in the driver */
> > +                       girq->num_parents = 0;
> > +                       girq->parents = NULL;
> > +                       girq->parent_handler = NULL;
> > +
> > +                       /*
> > +                        * Directly request the irq here instead of passing
> > +                        * a flow-handler because the irq is shared.
> > +                        */
> > +                       ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
> > +                                              IRQF_SHARED, name, gs);
> > +                       if (ret) {
> > +                               dev_err(dev, "failed to request IRQ");
> > +                               return ret;
> > +                       }
> >                 }
> >         }

-- 
With Best Regards,
Andy Shevchenko