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

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 6682 bytes --]

On Mon, Jan 21, 2019 at 03:28:39PM +0100, Wolfram Sang 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>

Geert, if you would have time for a high-level review, I'd appreciate
this very much!

> ---
> 
> 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
> Lager board. I am not super-happy with the interrupt latency causing some bits
> to be non-disturbed, but for now, I don't see a way around it except for
> busy-polling which I think is too excessive. RFC for now because someone still
> might have a better idea :)
> 
> Needs my previous fault-injector cleanup patches, a branch for consuming is here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/iic-arbitration-lost
> 
> Now let's see how to fix the sh_mobile driver...
> 
>  Documentation/i2c/gpio-fault-injection | 26 +++++++++++++++
>  drivers/i2c/busses/i2c-gpio.c          | 61 ++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/Documentation/i2c/gpio-fault-injection b/Documentation/i2c/gpio-fault-injection
> index 1a44e3edc0c4..b6f36ffe55e1 100644
> --- a/Documentation/i2c/gpio-fault-injection
> +++ b/Documentation/i2c/gpio-fault-injection
> @@ -83,3 +83,29 @@ 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
> +bus arbitration against another master in a multi-master setup.
> +
> +"lose_arbitration"
> +------------------
> +
> +This file is write only and you need to write the number of desired lost
> +arbitrations in a row. The calling process will then sleep and interfere with
> +transfers from the master under test when they appear until that number is
> +reached. The process is interruptible, though.
> +
> +Arbitration lost is achieved by waiting for SCL going down by the master under
> +test and then pulling SDA low for some time. So, the I2C address sent out
> +should be corrupted and that should be detected properly. That means that the
> +address sent out should have a lot of '1' bits to be able to detect corruption.
> +There doesn't need to be a device at this address because arbitration lost
> +should be detected beforehand. Also note, that SCL going down is monitored
> +using interrupts, so the interrupt latency might cause the first bits to be not
> +corrupted. A good starting script for using this fault injector:
> +
> +# echo 1 > lose_arbitration &
> +# i2cget -y <bus_to_test> 0x3f
> +
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index ca04fa25a141..c630172b4787 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -7,12 +7,14 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +#include <linux/completion.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c-algo-bit.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/i2c-gpio.h>
> @@ -27,6 +29,7 @@ struct i2c_gpio_private_data {
>  	struct i2c_gpio_platform_data pdata;
>  #ifdef CONFIG_I2C_GPIO_FAULT_INJECTOR
>  	struct dentry *debug_dir;
> +	struct completion irq_happened;
>  #endif
>  };
>  
> @@ -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);
> +	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);
> +	int ret, i;
> +
> +	i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
> +
> +	/*
> +	 * 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.
> +	 */
> +	ret = gpiod_direction_input(priv->scl);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = request_irq(irq, lose_arbitration_irq, IRQF_TRIGGER_FALLING,
> +			  "i2c-gpio-fi", priv);
> +	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);
> +	}
> +
> +	free_irq(irq, priv);
> + output:
> +	ret = gpiod_direction_output(priv->scl, 1);
> + unlock:
> +	i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
> +
> +	return ret;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_lose_arbitration, NULL, fops_lose_arbitration_set, "%llu\n");
> +
>  static void i2c_gpio_fault_injector_init(struct platform_device *pdev)
>  {
>  	struct i2c_gpio_private_data *priv = platform_get_drvdata(pdev);
> @@ -181,10 +237,15 @@ static void i2c_gpio_fault_injector_init(struct platform_device *pdev)
>  	if (!priv->debug_dir)
>  		return;
>  
> +	init_completion(&priv->irq_happened);
> +
>  	debugfs_create_file_unsafe("incomplete_address_phase", 0200, priv->debug_dir,
>  				   priv, &fops_incomplete_addr_phase);
>  	debugfs_create_file_unsafe("incomplete_write_byte", 0200, priv->debug_dir,
>  				   priv, &fops_incomplete_write_byte);
> +	if (priv->bit_data.getscl)
> +		debugfs_create_file_unsafe("lose_arbitration", 0200, priv->debug_dir,
> +					   priv, &fops_lose_arbitration);
>  	debugfs_create_file_unsafe("scl", 0600, priv->debug_dir, priv, &fops_scl);
>  	debugfs_create_file_unsafe("sda", 0600, priv->debug_dir, priv, &fops_sda);
>  }
> -- 
> 2.11.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]