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:   Thu, 10 Feb 2022 09:06:28 +0000
Message-ID: <87zgmz3xbf.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Emil Renner Berthing <kernel@esmil.dk>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Grygorii Strashko <grygorii.strashko@ti.com>,
        Santosh Shilimkar <ssantosh@kernel.org>,
        Kevin Hilman <khilman@kernel.org>,
        Tony Lindgren <tony@atomide.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vladimir Zapolskiy <vz@mleia.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        kernel-team@android.com
Subject: Re: [PATCH 10/10] pinctrl: starfive: Switch to dynamic chip name output
In-Reply-To: <CANBLGcwKeLn7Q1Ra8pCw=cXy=kJeEFRmBjOxjds10+k70LvzXA@mail.gmail.com>
References: <20220209162607.1118325-1-maz@kernel.org>
        <20220209162607.1118325-11-maz@kernel.org>
        <CANBLGcwKeLn7Q1Ra8pCw=cXy=kJeEFRmBjOxjds10+k70LvzXA@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 09 Feb 2022 23:30:55 +0000,
Emil Renner Berthing <kernel@esmil.dk> wrote:
> 
> On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <maz@kernel.org> wrote:
> >
> > Instead of overloading the name field, use the relevant callback to
> > output the device name.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > index 5be9866c2b3c..f29d9ccf858b 100644
> > --- a/drivers/pinctrl/pinctrl-starfive.c
> > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/reset.h>
> > +#include <linux/seq_file.h>
> >  #include <linux/spinlock.h>
> >
> >  #include <linux/pinctrl/pinctrl.h>
> > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> >         return 0;
> >  }
> >
> > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > +{
> > +       struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > +
> > +       seq_printf(p, sfp->gc.label);
> > +}
> > +
> >  static struct irq_chip starfive_irq_chip = {
> >         .irq_ack = starfive_irq_ack,
> >         .irq_mask = starfive_irq_mask,
> >         .irq_mask_ack = starfive_irq_mask_ack,
> >         .irq_unmask = starfive_irq_unmask,
> >         .irq_set_type = starfive_irq_set_type,
> > +       .irq_print_chip = starfive_irq_print_chip,
> >         .flags = IRQCHIP_SET_TYPE_MASKED,
> >  };
> 
> The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> setting the name is considered abuse we can just drop the addition
> above and just delete the two lines below.

Are you sure this never appears? Is there a another irqchip stacked on
top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
where XX is an interrupt number using one of these GPIO pins? Please
run it without this patch, as I just noticed that debugfs blindly
uses the name.

> The gpio framework seems to fill in default handlers in the struct
> above, so unfortunately it can't yet be made const. Is this something
> you intend to fix in the future?

This is next on my list of things to address. The whole 'let's copy a
whole irqchip structure and hijack random pointers' should not have
happened, and it certainly is going to be an interesting ride.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.