Re: [PATCH 10/10] pinctrl: starfive: Switch to dynamic chip name output

[Emil Renner Berthing]

On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> On Wed, 09 Feb 2022 23:30:55 +0000,
> Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> >
> > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > Instead of overloading the name field, use the relevant callback to
> > > output the device name.
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> > >  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.

Yes, the old gpio driver this derives from used to set
    sfp->gc.irq.parent_handler = NULL
and then register its own irq handler, then the parent would show up
in /proc/interrupts. But after switching to letting the gpio framework
register the handler it stopped showing up.

root@visionfive~# cat /proc/interrupts
           CPU0       CPU1
  5:       5035       4907  RISC-V INTC   5 Edge      riscv-timer
  6:        960          0  SiFive PLIC   4 Edge      dw-mci
  7:       4384          0  SiFive PLIC   5 Edge      dw-mci
  8:        562          0  SiFive PLIC   6 Edge      eth0
 10:          1          0  SiFive PLIC   7 Edge      eth0
 11:          0          0  SiFive PLIC   2 Edge      dw_axi_dmac_platform
 15:       2690          0  SiFive PLIC  44 Edge      xhci-hcd:usb1
 17:          0          0  SiFive PLIC  43 Edge      104c0000.usb
 18:          0          0  SiFive PLIC   1 Edge      dw_axi_dmac_platform
 20:        234          0  SiFive PLIC  96 Edge      118b0000.i2c
 21:          0          0  SiFive PLIC  97 Edge      118c0000.i2c
 22:          7          0  SiFive PLIC  98 Edge      118d0000.trng
 28:          0          0  SiFive PLIC 101 Edge      sf_lcdc
 29:          0          0  SiFive PLIC 103 Edge      sf_vpp1
 30:          0          0  SiFive PLIC  70 Edge      12410000.spi
 31:        205          0  SiFive PLIC  73 Edge      ttyS0
 32:          0          0  SiFive PLIC  74 Edge      12450000.i2c
 33:          0          0  SiFive PLIC  80 Edge      12480000.watchdog
 34:         28          0  SiFive PLIC 122 Edge      124a0000.tmon
 37:          0          0  11910000.pinctrl  35 Edge      gpiomon
IPI0:        52        133  Rescheduling interrupts
IPI1:      2591       6924  Function call interrupts
IPI2:         0          0  CPU stop interrupts
IPI3:         0          0  IRQ work interrupts
IPI4:         0          0  Timer broadcast interrupts
root@visionfive~# cat /sys/kernel/debug/irq/irqs/26
handler:  starfive_gpio_irq_handler
device:   (null)
status:   0x00010c00
            _IRQ_NOPROBE
            _IRQ_NOREQUEST
            _IRQ_NOTHREAD
istate:   0x00000000
ddepth:   0
wdepth:   0
dstate:   0x02401200
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
            IRQD_AFFINITY_SET
            IRQD_DEFAULT_TRIGGER_SET
node:     0
affinity: 0-1
domain:  :soc:interrupt-controller@c000000
 hwirq:   0x20
 chip:    SiFive PLIC
  flags:   0x0
root@visionfive~# cat /sys/kernel/debug/irq/irqs/37
handler:  handle_edge_irq
device:   (null)
status:   0x00000403
            _IRQ_NOPROBE
istate:   0x00000020
            IRQS_ONESHOT
ddepth:   0
wdepth:   0
dstate:   0x00400203
            IRQ_TYPE_EDGE_RISING
            IRQ_TYPE_EDGE_FALLING
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
node:     0
affinity: 0-1
domain:  :soc:pinctrl@11910000
 hwirq:   0x23
 chip:    11910000.pinctrl
  flags:   0x1
             IRQCHIP_SET_TYPE_MASKED

> > 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.

Sounds great, thanks.

/Emil