Re: [RFC PATCH] i2c: gpio: fault-injector: add 'lose_arbitration' injector

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Wolfram,

On Mon, Jan 21, 2019 at 3:29 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Here is a fault injector simulating 'arbitration lost' from multi-master
> setups. Read the docs for its usage.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks, looks like a valuable injector case!

> This is the most reliable result I came up with so far for simulating lost
> arbitration (after playing a lot with falling SDA as trigger first, but SCL
> seems the way to go). Works fine with the i2c-sh_mobile driver on a Renesas

Using SCL as the trigger makes most sense to me, too.

> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c

> @@ -162,6 +165,59 @@ static int fops_incomplete_write_byte_set(void *data, u64 addr)
>  }
>  DEFINE_DEBUGFS_ATTRIBUTE(fops_incomplete_write_byte, NULL, fops_incomplete_write_byte_set, "%llu\n");
>
> +static irqreturn_t lose_arbitration_irq(int irq, void *dev_id)
> +{
> +       struct i2c_gpio_private_data *priv = dev_id;
> +
> +       setsda(&priv->bit_data, 0);
> +       udelay(200);

Please add a #define for this value.

> +       setsda(&priv->bit_data, 1);
> +
> +       complete(&priv->irq_happened);
> +       return IRQ_HANDLED;
> +}
> +
> +static int fops_lose_arbitration_set(void *data, u64 num_faults)
> +{
> +       struct i2c_gpio_private_data *priv = data;
> +       int irq = gpiod_to_irq(priv->scl);

As request_irq() takes an unsigned int irq, please check for a negative error
code here.

> +       int ret, i;

u64 i;

Yes, 64-bit values may be a bit excessive, but that's what the caller is
passing, and truncating to int may cause unexpected behavior.

> +       ret = request_irq(irq, lose_arbitration_irq, IRQF_TRIGGER_FALLING,
> +                         "i2c-gpio-fi", priv);

"i2c-gpio-fault-injection"?

> +       if (ret)
> +               goto output;
> +
> +       for (i = 0; i < num_faults; i++) {
> +               ret = wait_for_completion_interruptible(&priv->irq_happened);
> +               if (ret)
> +                       break;
> +               reinit_completion(&priv->irq_happened);

The reinitialization may race with the interrupt handler. Probably you don't
care, though, as all of this is "best effort" anyway.

Perhaps you can do the counting in the interrupt handler instead, and only
trigger completion after the wanted number of lost arbitrations has been
performed?


> +DEFINE_DEBUGFS_ATTRIBUTE(fops_lose_arbitration, NULL, fops_lose_arbitration_set, "%llu\n");

I think you can drop the format if you don't provide a "get" method.
Or just implement the "get" method, too ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds