linux-kernel - Re: [PATCH v4 05/11] iio: accel: adxl313: prepare interrupt handling

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: <CAHp75VdCMSzQt9bY0y84yndPWUmFXD+KJeu4YxvQxk9pnfgWCA@mail.gmail.com>
Date: Sun, 1 Jun 2025 22:21:07 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de, 
	Michael.Hennerich@...log.com, bagasdotme@...il.com, linux-iio@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 05/11] iio: accel: adxl313: prepare interrupt handling

On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> Evaluate the devicetree property for an optional interrupt line, and
> configure the interrupt mapping accordingly. When no interrupt line
> is defined in the devicetree, keep the FIFO in bypass mode as before.

...

>         struct adxl313_data *data;
>         struct iio_dev *indio_dev;
> -       int ret;
> +       u8 int_line;
> +       u8 int_map_msk;
> +       int irq, ret;

Why do you need a specific irq variable?

...

> +       int_line = ADXL313_INT1;

Assign this when we are sure that the INT1 is defined. Current
approach is not robust.

> +       irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +       if (irq < 0) {
> +               int_line = ADXL313_INT2;
> +               irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +               if (irq < 0)
> +                       int_line = ADXL313_INT_NONE;
> +       }

So, the below code does not use the returned vIRQ, moreover, the above
code actually does the IRQ mapping. Why do you need that if the code
doesn't use it?

> +       if (int_line != ADXL313_INT_NONE) {

Why not positive conditional? But see below...

> +               /* FIFO_STREAM mode */
> +               int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
> +                       ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
> +                       ADXL313_INT_OVERRUN;
> +               ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
> +                                        int_map_msk, int_line == ADXL313_INT2);

This is fragile. It heavily relies on the existence of exactly three
IRQ variants. Instead of defining special case (NONE) simply use
whatever is undefined as the default case

  switch (IRQ type) {
  case 'INT1':
    ...
    break;
  case 'INT2':
    ...
    break;
  default:
    // FIFO bypass mode
    ...
    break;
  }

But still, the main question and confusion here is the absence of the
users of 'irq'.

> +               if (ret)
> +                       return ret;
> +       } else {
> +               /*
> +                * FIFO_BYPASSED mode
> +                *
> +                * When no interrupt lines are specified, the driver falls back
> +                * to use the sensor in FIFO_BYPASS mode. This means turning off
> +                * internal FIFO and interrupt generation (since there is no
> +                * line specified). Unmaskable interrupts such as overrun or
> +                * data ready won't interfere. Even that a FIFO_STREAM mode w/o
> +                * connected interrupt line might allow for obtaining raw
> +                * measurements, a fallback to disable interrupts when no
> +                * interrupt lines are connected seems to be the cleaner
> +                * solution.
> +                */
> +               ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +                                  FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> +                                             ADXL313_FIFO_BYPASS));
> +               if (ret)
> +                       return ret;
> +       }
> +
>         return devm_iio_device_register(dev, indio_dev);
>  }

-- 
With Best Regards,
Andy Shevchenko