Hi Geert, On Mon, Feb 11, 2019 at 11:30:56AM +0100, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Mon, Jan 21, 2019 at 3:29 PM Wolfram Sang > <[email protected]> wrote: > > Here is a fault injector simulating 'arbitration lost' from multi-master > > setups. Read the docs for its usage. > > > > Signed-off-by: Wolfram Sang <[email protected]> > > 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.
Thanks for the review and glad you like the use case and implementation
approach.
> > + struct i2c_gpio_private_data *priv = dev_id;
> > +
> > + setsda(&priv->bit_data, 0);
> > + udelay(200);
>
> Please add a #define for this value.
Sure thing, this was a typical RFC marker :) It is now calculated based
on bit_data.udelay and has a comment.
> > + 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.
Will do.
> > + 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.
OK, but won't be needed after my refactoring to count this in the
interrupt.
> > + ret = request_irq(irq, lose_arbitration_irq, IRQF_TRIGGER_FALLING,
> > + "i2c-gpio-fi", priv);
>
> "i2c-gpio-fault-injection"?
OK. That was RFC style, too.
> > + 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?
Yes, I like this approach! Will do.
>
>
> > +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 ;-)
Any pointer for that information? I seem to recall that the format
string is also used to parse the user string to the set function. I
sadly can't find that documented, but all the helper macros in
fs/debugfs/file.c have a format string, too, even if WO.
I have the code ready but testing and pushing out needs to wait until
tomorrow.
Thanks again,
Wolfram
signature.asc
Description: PGP signature
