Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
> 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. signature.asc Description: PGP signature
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
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
> > > > > > + > > > > > > 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 signature.asc Description: PGP signature
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
Hi Wolfram,
On Tue, Feb 19, 2019 at 2:18 PM Wolfram Sang 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 -- ge...@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
> > > > + 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... signature.asc Description: PGP signature
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
> >>> + 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. signature.asc Description: PGP signature
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
Hi Wolfram,
On Mon, Feb 18, 2019 at 9:41 PM Wolfram Sang 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 -- ge...@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
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
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 signature.asc Description: PGP signature
Re: [PATCH 1/2] i2c: gpio: fault-injector: add 'lose_arbitration' injector
Hi Wolfram,
On Sun, Feb 17, 2019 at 1:41 PM Wolfram Sang
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
> ---
>
> 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 -- ge...@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
