linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andre Przywara <andre.przywara@arm.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: "Maxime Ripard" <mripard@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Jernej Skrabec" <jernej.skrabec@siol.net>,
	"Samuel Holland" <samuel@sholland.org>,
	"Icenowy Zheng" <icenowy@aosc.io>,
	"Rob Herring" <robh@kernel.org>,
	"Clément Péron" <peron.clem@gmail.com>,
	"Shuosheng Huang" <huangshuosheng@allwinnertech.com>,
	"Yangtao Li" <tiny.windzz@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com,
	"Lee Jones" <lee.jones@linaro.org>,
	linux-input@vger.kernel.org, "Mark Brown" <broonie@kernel.org>
Subject: Re: [PATCH v5 05/20] Input: axp20x-pek: Bail out if AXP has no interrupt line connected
Date: Thu, 28 Jan 2021 11:11:28 +0000	[thread overview]
Message-ID: <20210128104627.76b35a5c@slackpad.fritz.box> (raw)
In-Reply-To: <YBHCF2tWIX4MeMia@google.com>

On Wed, 27 Jan 2021 11:42:15 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

Hi Dmitry,

thanks for your feedback!

> On Wed, Jan 27, 2021 at 05:24:45PM +0000, Andre Przywara wrote:
> > On at least one board (Orangepi Zero2) the AXP305 PMIC does not have its
> > interrupt line connected to the CPU (mostly because the H616 SoC does
> > not feature an NMI pin anymore).
> > After allowing the AXP driver to proceed without an "interrupts"
> > property [1], the axp20x-pek driver crashes with a NULL pointer
> > dereference (see below).
> > 
> > Check for the regmap_irqc member to be not NULL before proceeding with
> > probe. This gets normally filled by the call to regmap_add_irq_chip(),
> > which we allow to skip now, when the DT node lacks an interrupt
> > property.  
> 
> No, the driver is not the right place to patch this; regmap should be
> fixed so it does not crash instead.

I am not sure this is the right approach, those regmap functions look
more like an internal interface to me, with lots of wrapper functions
happily dereferencing pointers and reaching into structs. Moving
NULL checks into those does not sound like the right thing. CC:ing Mark
for more opinions on this.

A more general solution would be to not instantiate this driver here
at all, when we don't have an interrupt line.
However at the moment the AXP MFD driver uses a const struct to hold
all MFD cells, so there is no easy way of omitting the power key
device dynamically. And even then it would hard code the requirement
for an interrupt into the MFD driver, when this could be considered an
implementation detail of the axp20x-pek driver.

That's why I came up with this patch here, which was the easiest and
cleanest: This driver *requires* a valid regmap_irqc, so it should
verify this at probe time, kind of like a normal driver would bail out
if no IRQ line could be reserved.

Let me know what you think!

Cheers,
Andre

  reply	other threads:[~2021-01-28 11:13 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210127172500.13356-1-andre.przywara@arm.com>
2021-01-27 17:24 ` [PATCH v5 05/20] Input: axp20x-pek: Bail out if AXP has no interrupt line connected Andre Przywara
2021-01-27 19:42   ` Dmitry Torokhov
2021-01-28 11:11     ` Andre Przywara [this message]
2021-01-28 11:36       ` Mark Brown
2021-01-28 12:31         ` Andre Przywara
2021-01-28 15:05           ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210128104627.76b35a5c@slackpad.fritz.box \
    --to=andre.przywara@arm.com \
    --cc=broonie@kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=huangshuosheng@allwinnertech.com \
    --cc=icenowy@aosc.io \
    --cc=jernej.skrabec@siol.net \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=mripard@kernel.org \
    --cc=peron.clem@gmail.com \
    --cc=robh@kernel.org \
    --cc=samuel@sholland.org \
    --cc=tiny.windzz@gmail.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).