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

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

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

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@sang-engineering.com> 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>
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]