| 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: <861qr1fs55.wl-maz@kernel.org>
Date: Fri, 21 Oct 2022 10:22:14 +0100
From: Marc Zyngier <maz@...nel.org>
To: Firas Ashkar <firas.ashkar@...oirfairelinux.com>
Cc: alex@...riz.org.uk, tglx@...utronix.de,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
On Thu, 20 Oct 2022 15:13:51 +0100,
Firas Ashkar <firas.ashkar@...oirfairelinux.com> wrote:
>
> 1. add TS-7800v1 fpga based irq controller driver, and
> 2. add related memory and irq resources
>
> By default only mapped FPGA interrupts will be chained/multiplexed, these
> chained interrupts will then be available to other device drivers to
> request via the corresponding Linux IRQs.
>
> $ cat /proc/cpuinfo
> processor : 0
> model name : Feroceon rev 0 (v5l)
> BogoMIPS : 333.33
> Features : swp half thumb fastmult edsp
> CPU implementer : 0x41
> CPU architecture: 5TEJ
> CPU variant : 0x0
> CPU part : 0x926
> CPU revision : 0
>
> Hardware : Technologic Systems TS-78xx SBC
> Revision : 0000
> Serial : 0000000000000000
> $
>
> $ cat /proc/interrupts
> CPU0
> 1: 902 orion_irq Level orion_tick
> 4: 795 orion_irq Level ttyS0
> 13: 0 orion_irq Level ehci_hcd:usb2
> 18: 0 orion_irq Level ehci_hcd:usb1
> 22: 69 orion_irq Level eth0
> 23: 171 orion_irq Level orion-mdio
> 29: 0 orion_irq Level mv_crypto
> 31: 2 orion_irq Level mv_xor.0
> 65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
> Err: 0
> $
>
> $ uname -a
> Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022 armv5tel
> GNU/Linux
> $
>
> Signed-off-by: Firas Ashkar <firas.ashkar@...oirfairelinux.com>
> ---
>
> Notes:
> Changes in V2
> * limit the commit message
Well, there is still a lot of work to do. Most of this commit message
could be reduced to a single paragraph:
"Add support for FooBar IRQ controller usually found on Zorglub
platform."
The rest is plain obvious.
> * format comments in source code
> * use raw spin locks to protect mask/unmask ops
> * use 'handle_edge_irq' and 'irq_ack' logic
> * remove 'irq_domain_xlate_onecell'
> * remove unnecessary status flags
> * use 'builtin_platform_driver' helper routine
>
> :100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-orion5x/ts78xx-fpga.h
> :100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-orion5x/ts78xx-setup.c
> :100644 100644 7ef9f5e696d3 d184fb435c5d M drivers/irqchip/Kconfig
> :100644 100644 87b49a10962c b022eece2042 M drivers/irqchip/Makefile
> :000000 100644 000000000000 e975607fa4d5 A drivers/irqchip/irq-ts7800.c
> arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
> arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
> drivers/irqchip/Kconfig | 12 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-ts7800.c | 347 +++++++++++++++++++++++++++
> 5 files changed, 416 insertions(+)
>
> diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-orion5x/ts78xx-fpga.h
> index 2f4fe3ca5c1a..ed8378893208 100644
> --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> @@ -32,6 +32,7 @@ struct fpga_devices {
> struct fpga_device ts_rtc;
> struct fpga_device ts_nand;
> struct fpga_device ts_rng;
> + struct fpga_device ts_irqc;
> };
>
> struct ts78xx_fpga_data {
> diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-orion5x/ts78xx-setup.c
> index af810e7ccd79..d319a68b7160 100644
> --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
> platform_device_del(&ts78xx_ts_rng_device);
> }
>
> +/*****************************************************************************
> + * fpga irq controller
> + ****************************************************************************/
[...]
Sorry, but I have to ask: what part of "we're not taking any
additional non-DT changes to these obsolete setups" did I fail to
accurately communicate?
Until this board is entirely converted to DT, I'm not taking any
irqchip changes other than the most obvious bug fixes.
[...]
> +static void ts7800_irq_mask(struct irq_data *d)
> +{
> + struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
> + u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> + u32 mask_bits = 1 << d->hwirq;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&data->lock, flags);
> + writel(fpga_mask_reg & ~mask_bits, data->base + IRQ_MASK_REG);
> + writel(fpga_mask_reg & ~mask_bits, data->bridge + IRQ_MASK_REG);
> + raw_spin_unlock_irqrestore(&data->lock, flags);
OMFG. What do you expect this protects? Same question applies to all
the instances where you take this pointless lock.
[...]
> +static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
> +{
> + struct ts7800_irq_data *data = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> + u32 status = readl(data->bridge);
> +
> + chained_irq_enter(chip, desc);
> +
> + if (unlikely(status == 0)) {
> + handle_bad_irq(desc);
> + goto out;
> + }
> +
> + do {
> + unsigned int bit = __ffs(status);
> + int irq = irq_find_mapping(data->domain, bit);
> +
> + if (irq && (mask_bits & BIT(bit)))
> + generic_handle_irq(irq);
Again, this is not appropriate. I've pointed you to the right API in
my previous review of this patch.
[...]
> +static struct platform_driver ts7800_ic_driver = {
> + .probe = ts7800_ic_probe,
> + .remove = ts7800_ic_remove,
> + .id_table = ts7800v1_ic_ids,
> + .driver = {
> + .name = DRIVER_NAME,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> +};
> +builtin_platform_driver(ts7800_ic_driver);
Again, this isn't appropriate either, and I pointed it out last
time. We have specific helpers for irqchip, and using them isn't
optional. But of course, you'll need to move to DT for that.
Anyway, this is the last time I review this patch until you convert
the corresponding platform to DT.
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists