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;
Date:   Wed, 26 Apr 2023 19:27:18 -0500
From:   Jeff LaBundy <jeff@labundy.com>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     Tomas Mudrunka <tomas.mudrunka@gmail.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Fix freeze in lm8333 i2c keyboard driver
Message-ID: <ZEnBZs9VYxriT1Or@nixie71>
References: <20230425130054.591007-1-tomas.mudrunka@gmail.com>
 <ZEf0RYdD5jhE9JEk@nixie71>
 <ZEmwsViIjUVPZ4Cd@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZEmwsViIjUVPZ4Cd@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?xkW0GQJMgJzZfVSYp+thbmzaRD7PZxg8ISkEcLsSvnvQpvdwhxqxCsd110SG?=
 =?us-ascii?Q?sfu4IwYtWRUWucPbJi3kzXVu4Gkdl+CIjBtgCDT2PI6OgHm3vrllI9eVY5pz?=
 =?us-ascii?Q?2rSXnpIgcv02ea/mdr2JLVa7c/tPfjYR6Cv6YdE4/CqPPNCVhq3aaZbx2UFs?=
 =?us-ascii?Q?mfm7Ut8CrAvkuG7D6zmvui3UvGRMkuW8A0E3gpMJJ3mNKPf6h217jBIkunHn?=
 =?us-ascii?Q?6OUU1TiJb2Jn1hIcVoJ7ugFPItVvfWIUXwhQ9/TIlY03h4oAgXTZst6lo+ut?=
 =?us-ascii?Q?RVSml63FioCSbICSsp9GJSZA8Nn5VeR4/x7o1CSXpcy1SgXefdWj6AWZ0Uqa?=
 =?us-ascii?Q?r6qYIJzpXOCgDvFgpyggswCHaphYFw2hlNF5WNfjD/ltbqo0Zf/J5DdCe0Ys?=
 =?us-ascii?Q?CZMCwWl0FjaduIUaFg4krLdQn9dt9MGDsuX1M887q2yNYy6Kj0O1CkGCl2W+?=
 =?us-ascii?Q?yZOBmup78951jWPE1WlnL2KzzwdM4jAw/XN5+0LIEFDWg52nVwvISCGZ5kao?=
 =?us-ascii?Q?XeHVH/jO+S8eyJNpk4+JUUrMSHn9lSVXICj7zJ6Yk+IKcEU+DVRdYnpBZJEr?=
 =?us-ascii?Q?OWXWoG0n339QPJ7tBI6I4D5eyaKCnvSTHgAxOvkq1/LC8gHDnlmTb12XcDAY?=
 =?us-ascii?Q?ERI77nVSVqtEmUeMxmqkC27FQCl8m0k9qGqPH35e3uFuDXd4O62nPA+c/RBl?=
 =?us-ascii?Q?AgMtcW6mI5vJLEM4ui7goa3ySLAPipE5N/kT3WbXbe6DXeWMCbrHyGpQHCdX?=
 =?us-ascii?Q?Vsz0n7Jv+3zjCE8/V/uMTUmGjSIfFYK+a6Xe7lJHmRStmtQEnKc+I/VMAOJL?=
 =?us-ascii?Q?Oz8F6KpWcK/7hEbSIDyxP3CCrJFbAkDueB5+gLuAdXmDsLB6lUypoA0qcWLb?=
 =?us-ascii?Q?xEKDMyGiaAr3MTPP48xhMgfJoXGoQEaMOuEl8m9Q0s2YJOL+D9jA08usLu56?=
 =?us-ascii?Q?5bmMDDroWYSiV3JbxhnNzJf5aHJjMlDwm7dYjxyYJhdR6rUOCC4hFJ8MD1hd?=
 =?us-ascii?Q?RaWwWRO3ZOhP1eKUvEjC9kk78LJVduOK0fe/JbvDwnypV0md5bi8WdMA4ocl?=
 =?us-ascii?Q?PeCYik1QIB5m9c+mBaf78+/wempHiuvuFls9ieD4CQNAs0+0YtHlh0bfjHM1?=
 =?us-ascii?Q?Q7ZqMxZkvPk/OXEe0ti9SSrA110hfSkWJAwX6YRkY4iFIF5c7R3G2pvigGPG?=
 =?us-ascii?Q?N6SVH24wa9M/FD59q6Mbyw99vvpDQ5VgZZwh43VgLd1ltlH0V3Q+lhUkqFZd?=
 =?us-ascii?Q?5XEECeDYRsMyz9/vqjONZs51uNiGMsc+sSb3P6nZakEhW8uC8TjgrR76uiGO?=
 =?us-ascii?Q?AgFpLE9hBYRomWb8qLa+NDNxpQIAB/H/1X8Lt/FXoxcFghdaYoWBL7f5CNO3?=
 =?us-ascii?Q?m3SVikDlOELu/Z8KQMUzuWTATW1Nj7O9BS6jGHKn+cTVQ9xpJbSy3kVY96b9?=
 =?us-ascii?Q?xo50dtaOv0MzW5lFvTblr6/drvEcoU34JmW9gZwT+af/y7wIAFNTmTy64mIq?=
 =?us-ascii?Q?dEH9mnY2HDhnt0Kf0Ivu98wicqbru+LmCTOzDpZBiT43VcRC1ArbwH4Sv0JK?=
 =?us-ascii?Q?ir8yKtmKWiQdthojyIqC671y5XCdtXiQ5DKVcXsU?=
X-OriginatorOrg: labundy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b5e1187f-b5da-473b-585c-08db46b62ad2
X-MS-Exchange-CrossTenant-AuthSource: DM5PR0801MB3767.namprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2023 00:27:22.1512
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 00b69d09-acab-4585-aca7-8fb7c6323e6f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ThjBGyWe2v4jX4+ATKxubooDqXLQ22XgOscAY6s4IcqZss727w20/dqUjH2rM4W3M75+JyM2tu3oJNmFCBMkFg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR08MB8274
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dmitry,

On Wed, Apr 26, 2023 at 04:16:01PM -0700, Dmitry Torokhov wrote:
> Hi Jeff, Tomas,
> 
> On Tue, Apr 25, 2023 at 10:39:49AM -0500, Jeff LaBundy wrote:
> > Hi Tomas,
> > 
> > On Tue, Apr 25, 2023 at 03:00:53PM +0200, Tomas Mudrunka wrote:
> > > LM8333 uses gpio interrupt line which is active-low.
> > > When interrupt is set to FALLING edge and button is pressed
> > > before driver loads, driver will miss the edge and never respond.
> > > To fix this we handle ONESHOT LOW interrupt rather than edge.
> > > 
> > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > > ---
> > >  drivers/input/keyboard/lm8333.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > > index 7457c3220..c5770ebb2 100644
> > > --- a/drivers/input/keyboard/lm8333.c
> > > +++ b/drivers/input/keyboard/lm8333.c
> > > @@ -179,7 +179,7 @@ static int lm8333_probe(struct i2c_client *client)
> > >  	}
> > >  
> > >  	err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > > -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > > +				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > >  				   "lm8333", lm8333);
> > >  	if (err)
> > >  		goto free_mem;
> > 
> > Thanks for the patch, but this is a NAK in my opinion.
> > 
> > First of all, we should not be hard-coding interrupt polarity in the
> > first place; that is an existing piece of technical debt in this driver.
> 
> Yes, I wonder if the original hardware was limited to the edge
> interrupts.
> 
> > 
> > Second, changing from edge-triggered to level-triggered interrupts runs
> > the risk of creating an interrupt storm depending on the time it takes
> > the device to deassert the irq following the I2C read and the point at
> > which the threaded handler returns. Have you measured this?
> 
> IRQF_ONESHOT ensures that the level interrupt is unmasked only when the
> threaded handler returns.

Yes that's correct; what I mean to say is that depending on the nature of
the read-to-clear mechanism in the part, there is a chance that the IRQ
has not been deasserted by the time the threaded handler returns. On some
devices for example, the IRQ is not deasserted until some time after the
read's stop condition.

For these cases, I consider it best practice to measure the I2C and IRQ
lines on a scope and if necessary, add a small delay before the interrupt
handler returns. This is especially true for open-drain interrupts that
may need a few hundred extra us for the pin to rise.

> 
> > 
> > Can we not simply read the interrupt status registers once at start-up
> > to clear any pending status? This is essentially what your change does
> > anyway, albeit indirectly.
> > 
> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy