Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <ZEf0RYdD5jhE9JEk@nixie71> <20230425164903.610455-1-tomas.mudrunka@gmail.com>
 <ZEnErxfnSn2JOpvm@nixie71>
In-Reply-To: <ZEnErxfnSn2JOpvm@nixie71>
From:   =?UTF-8?B?VG9tw6HFoSBNdWRydcWIa2E=?= <tomas.mudrunka@gmail.com>
Date:   Thu, 27 Apr 2023 10:13:22 +0200
Message-ID: <CAH2-hcLOA0ECg7ubKh_r9BryRuGPjJ1Cv1ErUpMmQ+Yag5qnWA@mail.gmail.com>
Subject: Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver
To:     Jeff LaBundy <jeff@labundy.com>
Cc:     dmitry.torokhov@gmail.com, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello, thanks for your notes.

> This is the right idea. I am sort of splitting hairs here, however I
> think it makes sense to place this read before the IRQ is requested
> and not after.
>
>
> As written, there is room for an ever-so-tiny race condition wherein
> the IRQ is asserted just after it is requested. Before the threaded
> handler has run however, the new read in probe swallows the IRQ status
> before the threaded handler can read it and react to errors.

In fact i believe quite the opposite case to be true.
If i read before registering IRQ there will be ever-so-tiny race condition that
would allow to miss the edge (exactly the bug this patch is fixing,
but limited).

In the case you describe the worst scenario is likely that the interrupt handler
will be called only to re-read status and immediately return on this condition:

if (!status) return IRQ_NONE;

> Also, I think you should at least capture and evaluate lm8333_read8()'s
> return value as is already done for the calls to lm8333_write8().

Well. If you think this will bring any benefits, i might as well just call
lm8333_irq_thread() instead of lm8333_read8()
Would that be acceptable solution?

Tom.