[PATCH 0/2] i2c: gpio: fault-injector: add two new injectors

[PATCH 0/2] i2c: gpio: fault-injector: add two new injectors

[Wolfram Sang]

After sending out the 'lose_arbitration' injector as RFC, here is now a patch
series with that injector updated and another one added.

The first one successfully triggers an ARBITRATION_LOST interrupt on my Renesas
Lager board (R-Car H2). The other one leaves SCL low on the same board because
of the Kernel panic, so reboot handlers need to handle that. But more
interestingly, it causes a deadlock because it needs this I2C adapter to
trigger a reboot via the PMIC. This is a good testcase for the soon to be
developed master_xfer_atomic callback for the I2C core.

Wolfram Sang (2):
  i2c: gpio: fault-injector: add 'lose_arbitration' injector
  i2c: gpio: fault-injector: add 'inject_panic' injector

 Documentation/i2c/gpio-fault-injection | 51 ++++++++++++++++++
 drivers/i2c/busses/i2c-gpio.c          | 97 ++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+)

-- 
2.11.0

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

[Wolfram Sang]

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@sang-engineering.com>
---

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 again, Geert, for the review!

 Documentation/i2c/gpio-fault-injection | 25 ++++++++++++
 drivers/i2c/busses/i2c-gpio.c          | 71 ++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)

diff --git a/Documentation/i2c/gpio-fault-injection b/Documentation/i2c/gpio-fault-injection
index 1a44e3edc0c4..5262801438e6 100644
--- 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
+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
+next bus clock. 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 point for using this fault injector on an otherwise
+idle bus is:
+
+# echo 200 > 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 2d532cc042f5..f3645148d120 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,9 @@ struct i2c_gpio_private_data {
 	struct i2c_gpio_platform_data pdata;
 #ifdef CONFIG_I2C_GPIO_FAULT_INJECTOR
 	struct dentry *debug_dir;
+	/* these must be protected by bus lock */
+	struct completion scl_irq_completion;
+	u64 scl_irq_data;
 #endif
 };
 
@@ -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);
+
+	free_irq(irq, priv);
+ output:
+	ret = gpiod_direction_output(priv->scl, 1);
+ 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);
+	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;
+	/*
+	 * 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);
+}
+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 +247,15 @@ static void i2c_gpio_fault_injector_init(struct platform_device *pdev)
 	if (!priv->debug_dir)
 		return;
 
+	init_completion(&priv->scl_irq_completion);
+
 	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

[PATCH 2/2] i2c: gpio: fault-injector: add 'inject_panic' injector

[Wolfram Sang]

Add a fault injector simulating a Kernel panic happening after starting
a transfer. Read the docs for its usage.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/gpio-fault-injection | 26 ++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-gpio.c          | 28 +++++++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/i2c/gpio-fault-injection b/Documentation/i2c/gpio-fault-injection
index 5262801438e6..fc85c1ab6f68 100644
--- a/Documentation/i2c/gpio-fault-injection
+++ b/Documentation/i2c/gpio-fault-injection
@@ -108,3 +108,29 @@ idle bus is:
 
 # echo 200 > lose_arbitration &
 # i2cget -y <bus_to_test> 0x3f
+
+Panic during transfer
+=====================
+
+This fault injector will create a Kernel panic once the master under test
+started a transfer. This usually means that the state machine of the bus master
+driver will be ungracefully interrupted and the bus may end up in an unusual
+state. Use this to check if your shutdown/reboot/boot code can handle this
+scenario.
+
+"inject_panic"
+--------------
+
+This file is write only and you need to write the delay between the detected
+start of a transmission and the induced Kernel panic (in us). The calling
+process will then sleep and wait for the next bus clock. The process is
+interruptible, though.
+
+Start of a transfer is detected by waiting for SCL going down by the master
+under test.  A good starting point for using this fault injector is:
+
+# echo 0 > inject_panic &
+# i2cget -y <bus_to_test> <some_address>
+
+Note that there doesn't need to be a device listening to the address you are
+using. Results may vary depending on that, though.
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index f3645148d120..a07544dc2da5 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -228,6 +228,29 @@ static int fops_lose_arbitration_set(void *data, u64 duration)
 }
 DEFINE_DEBUGFS_ATTRIBUTE(fops_lose_arbitration, NULL, fops_lose_arbitration_set, "%llu\n");
 
+static irqreturn_t inject_panic_irq(int irq, void *dev_id)
+{
+	struct i2c_gpio_private_data *priv = dev_id;
+
+	udelay(priv->scl_irq_data);
+	panic("I2C fault injector induced panic");
+
+	return IRQ_HANDLED;
+}
+
+static int fops_inject_panic_set(void *data, u64 duration)
+{
+	struct i2c_gpio_private_data *priv = data;
+
+	priv->scl_irq_data = duration;
+	/*
+	 * Interrupt on falling SCL. This ensures that the master under test has
+	 * really started the transfer.
+	 */
+	return i2c_gpio_fi_act_on_scl_irq(priv, inject_panic_irq);
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_inject_panic, NULL, fops_inject_panic_set, "%llu\n");
+
 static void i2c_gpio_fault_injector_init(struct platform_device *pdev)
 {
 	struct i2c_gpio_private_data *priv = platform_get_drvdata(pdev);
@@ -253,9 +276,12 @@ static void i2c_gpio_fault_injector_init(struct platform_device *pdev)
 				   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)
+	if (priv->bit_data.getscl) {
+		debugfs_create_file_unsafe("inject_panic", 0200, priv->debug_dir,
+					   priv, &fops_inject_panic);
 		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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Sun, Feb 17, 2019 at 1:41 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> 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@sang-engineering.com>
> ---
>
> 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@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

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

[Wolfram Sang]

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

Hi Geert,

thanks for this review, too!

> >
> > +Lost arbitration
> > +================
> > +
> > +Here, we want to simulate the condition where the master under tests loses the
> 
> test

Ack.

> > +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?

Dunno, I can change. Only 4 occcurences of 'µs' in Documentation/ so far.

> > +       wait_for_completion_interruptible(&priv->scl_irq_completion);
> 
> Error checking/propagation (-ERESTARTSYS)?

Are you sure? ERESTARTSYS belongs to the "These should never be seen by
user programs." group.

> 
> > +
> > +       free_irq(irq, priv);
> > + output:
> > +       ret = gpiod_direction_output(priv->scl, 1);
> 
> This may overwrite the error code returned by request_irq().

Yeah. What do you think about this, is this too dense?

	ret = gpiod_direction_output(priv->scl, 1) ?: ret;

> > +       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?

Yeah, good idea. Will apply it for both injectors.

Happy hacking,

   Wolfram


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

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

[Peter Rosin]

On 2019-02-18 21:41, Wolfram Sang wrote:
> Hi Geert,
>>> +
>>> +       free_irq(irq, priv);
>>> + output:
>>> +       ret = gpiod_direction_output(priv->scl, 1);
>>
>> This may overwrite the error code returned by request_irq().
> 
> Yeah. What do you think about this, is this too dense?
> 
> 	ret = gpiod_direction_output(priv->scl, 1) ?: ret;

That may also overwrite the error code, of course. Isn't it
usually best to return the first error? I have no clue if that
guideline does not apply here, though...

Cheers,
Peter

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

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Feb 18, 2019 at 9:41 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > +       wait_for_completion_interruptible(&priv->scl_irq_completion);
> >
> > Error checking/propagation (-ERESTARTSYS)?
>
> Are you sure? ERESTARTSYS belongs to the "These should never be seen by
> user programs." group.

How else can you inform the user the operation has been interrupted?

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

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

[Wolfram Sang]

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


> >>> +       ret = gpiod_direction_output(priv->scl, 1);
> >>
> >> This may overwrite the error code returned by request_irq().
> > 
> > Yeah. What do you think about this, is this too dense?
> > 
> > 	ret = gpiod_direction_output(priv->scl, 1) ?: ret;
> 
> That may also overwrite the error code, of course. Isn't it
> usually best to return the first error? I have no clue if that
> guideline does not apply here, though...

I am neither entirely sure here. My take was that the above was the more
severe error. Because if setting to output fails, the GPIO I2C bus will
be broken. If the former stuff fails, well, the injection didn't work or
was interrupted.

However, the GPIO was set to output before the injector. So, if setting
it back fails, then the system likely has more severe problems anyhow?

I am open to any better solution. However, let's not forget, this is
debug code aimed to be used by devs.


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

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

[Wolfram Sang]

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


> > > > +       wait_for_completion_interruptible(&priv->scl_irq_completion);
> > >
> > > Error checking/propagation (-ERESTARTSYS)?
> >
> > Are you sure? ERESTARTSYS belongs to the "These should never be seen by
> > user programs." group.
> 
> How else can you inform the user the operation has been interrupted?

Definately not by using something which is marked "should never be seen
by user programs" :)

In the worst case, I'll add:
	if (ret) ret = -EINTR;

I tested the current code with CTRL+C, there we get EOWNERDEAD back to
userspace, even with my code not propagating anything. With sending
SIGKILL, I got 143 which is not defined. I want to double check the
latter first, might be my tests were flaky...


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Feb 19, 2019 at 2:18 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > > > +       wait_for_completion_interruptible(&priv->scl_irq_completion);
> > > >
> > > > Error checking/propagation (-ERESTARTSYS)?
> > >
> > > Are you sure? ERESTARTSYS belongs to the "These should never be seen by
> > > user programs." group.
> >
> > How else can you inform the user the operation has been interrupted?
>
> Definately not by using something which is marked "should never be seen
> by user programs" :)
>
> In the worst case, I'll add:
>         if (ret) ret = -EINTR;
>
> I tested the current code with CTRL+C, there we get EOWNERDEAD back to
> userspace, even with my code not propagating anything. With sending
> SIGKILL, I got 143 which is not defined. I want to double check the
> latter first, might be my tests were flaky...

Where's the kernel code that returns EOWNERDEAD?
Must be hidden in a complex preprocessor macro, as git grep cannot find it :-(

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

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

[Wolfram Sang]

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


> > > > > > +       wait_for_completion_interruptible(&priv->scl_irq_completion);
> > > > >
> > > > > Error checking/propagation (-ERESTARTSYS)?
> > > >
> > > > Are you sure? ERESTARTSYS belongs to the "These should never be seen by
> > > > user programs." group.
> > >
> > > How else can you inform the user the operation has been interrupted?
> >
> > Definately not by using something which is marked "should never be seen
> > by user programs" :)
> >
> > In the worst case, I'll add:
> >         if (ret) ret = -EINTR;
> >
> > I tested the current code with CTRL+C, there we get EOWNERDEAD back to
> > userspace, even with my code not propagating anything. With sending
> > SIGKILL, I got 143 which is not defined. I want to double check the
> > latter first, might be my tests were flaky...
> 
> Where's the kernel code that returns EOWNERDEAD?
> Must be hidden in a complex preprocessor macro, as git grep cannot find it :-(

I rather think it is glibc returning it when it discovered that the
owner of a robust mutex was gone:

http://man7.org/linux/man-pages/man3/pthread_mutexattr_setrobust.3.html


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

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

[Peter Rosin]

On 2019-02-19 14:13, Wolfram Sang wrote:
> 
>>>>> +       ret = gpiod_direction_output(priv->scl, 1);
>>>>
>>>> This may overwrite the error code returned by request_irq().
>>>
>>> Yeah. What do you think about this, is this too dense?
>>>
>>> 	ret = gpiod_direction_output(priv->scl, 1) ?: ret;
>>
>> That may also overwrite the error code, of course. Isn't it
>> usually best to return the first error? I have no clue if that
>> guideline does not apply here, though...
> 
> I am neither entirely sure here. My take was that the above was the more
> severe error. Because if setting to output fails, the GPIO I2C bus will
> be broken. If the former stuff fails, well, the injection didn't work or
> was interrupted.
> 
> However, the GPIO was set to output before the injector. So, if setting
> it back fails, then the system likely has more severe problems anyhow?

One way to end up with that is if there is an irq attached to the gpio
pin. If you disable the irq, you are (sometimes) allowed to change the
gpio to output, but not if the irq is active. So, if some other part of
the driver "steals" the gpio by activating an irq while the injector is
doing its thing, it is possible to end up with this.

But that seems like a gigantic corner case.

> I am open to any better solution. However, let's not forget, this is
> debug code aimed to be used by devs.
> 

You are right, please ignore me.

Cheers,
Peter

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

[Wolfram Sang]

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


> I tested the current code with CTRL+C, there we get EOWNERDEAD back to
> userspace, even with my code not propagating anything. With sending
> SIGKILL, I got 143 which is not defined. I want to double check the
> latter first, might be my tests were flaky...

Yeah, I mixed it up. That was SIGTERM, of course. SIGKILL gives 137. A
signal gives you errorcode 128 + signal_number.

So, I don't think we need to propagate the wait_for_completion result
value.


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