linux-kernel - Re: [PATCH 2/2] Input: ili210x

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Nov 2019 16:43:32 -0500
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Adam Ford <aford173@...il.com>
Cc:     Marek Vasut <marex@...x.de>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

Hi Adam,

On Mon, Nov 4, 2019 at 4:28 PM Adam Ford <aford173@...il.com> wrote:
>
> I am using IRQ_TYPE_EDGE_RISING for the 2117A.  Is that correct?  For
> my touchscreen, the IRQ line is low until a touch is detected, so I
> assume we want to capure on the rising edge.

That is correct for the 2117A, as far as I know. I am using the same
setting.

>
> Regarding Dmitry's patch,
> Is it a good idea to use msleep in an IRQ?  It seems like using the
> schedule_delayed_work() call seems like it will get in and get out of
> the ISR faster.
>
> If we use msleep and scan again, isn't it possible to starve other processes?

I believe using msleep() is ok because this is not a "real" interrupt handler,
but a threaded one. It runs in a regular kernel thread, with its interrupt
turned off (but all other interrupts remain enabled). Its interrupt is
re-enabled automatically after the threaded handler returns.

See
https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L50

> > @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
> >                 }
> >
> >                 touch = ili210x_report_events(priv, touchdata);
> > -               keep_polling = touch || chip->continue_polling(touchdata);
> > +               keep_polling = chip->continue_polling(touchdata, touch);
> >                 if (keep_polling)
>
> Why not just check the value of touch instead of invoking the function
> pointer which takes the value of touch in as a parameter?
>

The value of touch must be checked inside the callback, because
some variants use it to decide if they should poll again, and
some do not, such as the ili211x.

If I have misinterpreted your suggestion, could you perhaps
express it in C, so I can understand better?