| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87tud64yqa.wl-maz@kernel.org>
Date: Thu, 10 Feb 2022 13:50:37 +0000
From: Marc Zyngier <maz@...nel.org>
To: Emil Renner Berthing <kernel@...il.dk>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Matthias Brugger <matthias.bgg@...il.com>,
Grygorii Strashko <grygorii.strashko@...com>,
Santosh Shilimkar <ssantosh@...nel.org>,
Kevin Hilman <khilman@...nel.org>,
Tony Lindgren <tony@...mide.com>,
Thomas Gleixner <tglx@...utronix.de>,
Vladimir Zapolskiy <vz@...ia.com>,
Andrew Lunn <andrew@...n.ch>,
Gregory Clement <gregory.clement@...tlin.com>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
kernel-team@...roid.com
Subject: Re: [PATCH 10/10] pinctrl: starfive: Switch to dynamic chip name output
On Thu, 10 Feb 2022 13:44:12 +0000,
Emil Renner Berthing <kernel@...il.dk> wrote:
>
> On Thu, 10 Feb 2022 at 14:32, Marc Zyngier <maz@...nel.org> wrote:
> > On Thu, 10 Feb 2022 12:59:59 +0000,
> > Emil Renner Berthing <kernel@...il.dk> wrote:
> > >
> > > On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <maz@...nel.org> wrote:
> > > > On Wed, 09 Feb 2022 23:30:55 +0000,
> > > > Emil Renner Berthing <kernel@...il.dk> wrote:
> > > > >
> > > > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <maz@...nel.org> wrote:
> > > > > >
> > > > > > Instead of overloading the name field, use the relevant callback to
> > > > > > output the device name.
> > > > > >
> > > > > > Signed-off-by: Marc Zyngier <maz@...nel.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.
> > >
> > > 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.
> >
> > But this patch does not deal with the parent interrupt. It deals with
> > the irqchip that is used for the 'children interrupt'. Output
> > interrupts for a chained handler are never shown, as they don't really
> > make much sense on their own (you'd only see the sum of the input
> > interrupts).
>
> I see. Sorry for the confusion.
>
> > >
> > > root@...ionfive~# 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
> > ^^^^^^^^^^^^^^^^
> > This is what this patch deals with. Going with your suggestion of
> > dropping this output (or to hardcode it to something else) would be a
> > userspace visible change, and we can't do that.
>
> Gotcha. The SoC has been out in very few numbers for less than a year
> and the driver only entered mainline in 5.17-rc1, so I doubt anyone
> has had time to write scripts that check for this, but I'll let it be
> up to you.
Ah, I should have checked that. In which case, would you be OK if I
simply pushed the removal of this label as a fix for 5.17, and just
have it to say "Star5 GPIO", for example, without any indication of
the device (which appears in debugfs anyway as part of the irqdomain)?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists