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
> <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
>
> 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
Attachment:
signature.asc
Description: PGP signature
