Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Wolfram,

On Sun, Feb 17, 2019 at 1:41 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> Add a fault injector simulating 'arbitration lost' from multi-master
> setups. Read the docs for its usage.
>
> A helper function for future fault injectors using SCL interrupts is
> created to achieve this.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> Change since RFC:
>
> * user supplied value is now duration of interference instead of number of
>   interferences
> * refactored code to suppy a resusable helper function to install SCL interrupt
>   handlers
> * added error checks to interrupt number
> * slightly renamed the SCL interrupt when registering

Thanks for the update!

> --- a/Documentation/i2c/gpio-fault-injection
> +++ b/Documentation/i2c/gpio-fault-injection
> @@ -83,3 +83,28 @@ This is why bus recovery (up to 9 clock pulses) must either check SDA or send
>  additional STOP conditions to ensure the bus has been released. Otherwise
>  random data will be written to a device!
>
> +Lost arbitration
> +================
> +
> +Here, we want to simulate the condition where the master under tests loses the

test

> +bus arbitration against another master in a multi-master setup.
> +
> +"lose_arbitration"
> +------------------
> +
> +This file is write only and you need to write the duration of the arbitration
> +interference (in us). The calling process will then sleep and wait for the

µs

We do UTF-8 in documentation, don't we?

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

> @@ -162,6 +167,67 @@ 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 int i2c_gpio_fi_act_on_scl_irq(struct i2c_gpio_private_data *priv,
> +                                      irqreturn_t handler(int, void*))
> +{
> +       int ret, irq = gpiod_to_irq(priv->scl);
> +
> +       if (irq < 0)
> +               return irq;
> +
> +       i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
> +
> +       ret = gpiod_direction_input(priv->scl);
> +       if (ret)
> +               goto unlock;
> +
> +       reinit_completion(&priv->scl_irq_completion);
> +
> +       ret = request_irq(irq, handler, IRQF_TRIGGER_FALLING,
> +                         "i2c_gpio_fault_injector_scl_irq", priv);
> +       if (ret)
> +               goto output;
> +
> +       wait_for_completion_interruptible(&priv->scl_irq_completion);

Error checking/propagation (-ERESTARTSYS)?

> +
> +       free_irq(irq, priv);
> + output:
> +       ret = gpiod_direction_output(priv->scl, 1);

This may overwrite the error code returned by request_irq().

> + unlock:
> +       i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
> +
> +       return ret;
> +}
> +
> +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(priv->scl_irq_data);

On 32-bit, u64 scl_irq_data is truncated...

> +       setsda(&priv->bit_data, 1);
> +
> +       complete(&priv->scl_irq_completion);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int fops_lose_arbitration_set(void *data, u64 duration)
> +{
> +       struct i2c_gpio_private_data *priv = data;
> +
> +       priv->scl_irq_data = duration;

... since calling udelay() with large numbers can be dangerous, perhaps you
want to limit it to say 100 ms max anyway?

> +       /*
> +        * Interrupt on falling SCL. This ensures that the master under test has
> +        * really started the transfer. Interrupt on falling SDA did only
> +        * exercise 'bus busy' detection on some HW but not 'arbitration lost'.
> +        * Note that the interrupt latency may cause the first bits to be
> +        * transmitted correctly.
> +        */
> +       return i2c_gpio_fi_act_on_scl_irq(priv, lose_arbitration_irq);
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux