Message-ID: <CAMRc=MecZ3a8cO_yaXOZkteoA5fY=X3kTjj4ZFY-X039mn6wKA@mail.gmail.com>
Date:   Mon, 17 Dec 2018 14:59:21 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Marc Zyngier <marc.zyngier@....com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-gpio <linux-gpio@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

pon., 17 gru 2018 o 13:59 Uwe Kleine-König
<u.kleine-koenig@...gutronix.de> napisał(a):
>
> On Mon, Dec 17, 2018 at 11:32:45AM +0100, Bartosz Golaszewski wrote:
> > śr., 5 gru 2018 o 13:38 Bartosz Golaszewski
> > <bgolaszewski@...libre.com> napisał(a):
> > >
> > > śr., 5 gru 2018 o 13:20 Linus Walleij <linus.walleij@...aro.org> napisał(a):
> > > >
> > > > On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
> > > > <u.kleine-koenig@...gutronix.de> wrote:
> > > > > On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
> > > >
> > > > > > It used to live in the gpio-mockup driver and I generalized it
> > > > > > precisely because there was another driver - iio evgen - which was
> > > > > > doing basically the same thing. While I don't know if there'll be more
> > > > > > users (I'd guess it would be useful for testing purposes of other
> > > > > > subsystems) having the same functionality implemented once is better
> > > > > > than twice.
> > > > >
> > > > > The iio testing driver only needs the trigger and relies on an irq that
> > > > > then calls the registerd handler. The iio driver doesn't need to tune
> > > > > the edge sensitivity though and if your mockup driver just only calls
> > > > > the fire routine if the configured sensitivity justifies that,
> > > > > everything should work as expected.
> > > >
> > > > Simulating edges in the generic IRQ simulator codes seems
> > > > generally useful to me, even if there is just one user now.
> > > >
> > > > Certainly for any kind of IRQ testing, it could be interesting to
> > > > throw several low-to-high and high-to-low transitions
> > > > on a driver and see how it reacts.
> > > >
> > > > But it is up to the irqchip maintainers to state whether they
> > > > agree.
> > > >
> > >
> > > All that would be great, but at this point I just want to fix broken
> > > tests in user-space. After that we can think about how to
> > > improve/approach simulating interrupts all we want.
> > >
> > > Marc: is my explanation for using an int instead of bool for
> > > irq_sim_fire_edge() fine? Can Linus pick this up for fixes?
> > >
> >
> > Ping. We have only this week left to fix the regression - can we get
> > your Ack Marc?
>
> I don't understand the urge. The problem is that libgpiod's test is
> failing. And that is because when userspace requested
> IRQF_TRIGGER_FALLING the mockup driver also fires if the line rised and
> with my change libgpiod now sees that and wonders about it. Apart from
> the test failure both libgpiod and the gpio framework are entirely fine
> (AFAICT).
>
> The "fix" under discussion is to modify the mockup driver to only report
> a falling irq if the output is set to 0. But it also fires if the value
> is already 0 and is set to 0 again. So the problem isn't addressed
> completely, but only enough to make libgpiod's test suite report
> success.
>

The problem with your approach is that you're treating gpio-mockup as
a regular driver the goal of which is self-contained correctness. I
treat it as a tool to test the userspace API.

Of course libgpiod works correctly - it requests and receives the
correct type of interrupts. So does the gpiolib in-kernel part. The
problem indeed lies with the kernel testing module. But it doesn't
matter - we want to make sure the uAPI works correctly i.e. it behaves
the same with gpio-mockup as it would with a real driver for actual HW
behind. We're not testing gpio-mockup(!) here.

> In my eyes this is not how test-driven development works. Tests are
> here to bring breakage into the light. That worked just fine here. And
> now as a test fails, the goal is to fix the breakage, but not to change
> just enough details to get the test to pass and then urge them through
> because "we're already at -rc7 and userspace broke!"
>

The tests here are to find regressions in a) libgpiod and b) gpiolib
kernel-to-userspace interface. The mockup module isn't part of either.

The breakage in gpio-mockup/irq_sim is really not all that important.
Whether the userspace API works is. And with a breakage like this
we're now unable to check if it behaves correctly for events of
specified types. So yes: for 4.20 I want to fix the gpio-mockup module
just enough to keep the tests passing.

> And no, the right fix isn't hard. My concerns were expressed Tuesday
> last week, and the problem wasn't important enough since then to fix the
> patch set.
>

I already told you on several occasions that I *will* address certain
issues in irq_sim. It will *not* make you happy however because it
will not use the mechanism you suggested in gpio-simulator as I still
want to keep this relatively generic for others to use. I will fix
other problems though - among others the one with multiple subsequent
events of the same edge. I don't want to be in a hurry to propose
something fast, so I want to patch the tests now and then have time to
come up with a better solution.

Linus agrees. I see no objections to that from neither Marc nor
Thomas. I would really like to stop discussing this over and over
again after my every e-mail in this thread.

> But maybe it's just me and the Linux development process changed since I
> learned about it and today the demand on quality isn't that high any
> more.
>

First rule has always been "don't break the userspace" and everything
else came after that.

Best regards,
Bartosz Golaszewski

Powered by blists - more mailing lists