| 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: <20160104100238.2b40f736@bbrezillon> Date: Mon, 4 Jan 2016 10:02:38 +0100 From: Boris Brezillon <boris.brezillon@...e-electrons.com> To: Milo Kim <milo.kim@...com> Cc: <tglx@...utronix.de>, <jason@...edaemon.net>, <marc.zyngier@....com>, <alexandre.belloni@...e-electrons.com>, <ludovic.desroches@...el.com>, <nicolas.ferre@...el.com>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Hi Milo, On Mon, 4 Jan 2016 13:28:24 +0900 Milo Kim <milo.kim@...com> wrote: > This patch-set provides unified Atmel AIC (Advanced Interrupt Controller) > driver. Currently, there are two AIC drivers, AIC and AIC5. > Each driver consists of chip specific part (irq-atmel-aic.o or > irq-atmel-aic5.o) and shared code (irq-atmel-aic-common.o). > But consolidated AIC driver is just one file driver which supports both > IRQ chip systems. Sorry, but what's the real motivation behind this rework? > > How to handle two IRQ chips in one driver > ----------------------------------------- > Structure aic_reg_offset is used for device configuration. > AIC5 IRQ chip uses SSR (Source Select Register) to select IRQ number. > On the other hand, AIC IRQ chip has simple register access. > To support both IRQ chips, aic_is_ssr_used() helper is used. > > Patches > ------- > 1 ~ 5: fix IRQ priority issue, clean up RTC/RTT fixup code and etc. As explained in my review, those irq fixup are essential, and cannot remove them. > 6 ~ 19: create unified IRQ chip operation with aic_reg_offset data. I started to review those patches, but honestly I don't see the point of this rework, since you're trying to merge drivers for 2 IPs that are completely different from a functional POV (except for a few tiny things like priority or irq type definition). Before reviewing the remaining patches, I'd like to know more about your real motivations for pushing those changes? > > Target boards > ------------- > Tested with two boards. > * Arietta G25 (SoC: AT91SAM9G25) > * Xplained board (SoC: SAMA5D3) > > Number of driver files > ---------------------- > AIC: 3 (irq-atmel-aic.c, irq-atmel-aic-common.c and h) > AIC5: 3 (irq-atmel-aic5.c, irq-atmel-aic-common.c and h) > Consolidated AIC: 1 (irq-aic.c) > > Code size > --------- > AIC (irq-atmel-aic.o and irq-atmel-aic-common.o) > text data bss dec hex filename > 5137 196 4 5337 14d9 drivers/irqchip/built-in.o > > AIC5 (irq-atmel-aic5.o and irq-atmel-aic-common.o) > text data bss dec hex filename > 5548 196 4 5748 1674 drivers/irqchip/built-in.o > > Consolidated AIC (irq-aic.o) > text data bss dec hex filename > 4841 196 8 5045 13b5 drivers/irqchip/built-in.o > > Lines of code > ------------- > AIC: 597 > AIC5: 688 > Consolidated AIC: 609 Please, redo the same thing, but after keeping the IRQ fixup stuff, and I'm pretty sure the text section of the AIC/AIC5 and the consolidated version will be much closer. Not to mention that you're adding a bunch of if () statements in the critical IRQ path, which is never a good think for latency concern. Best Regards, Boris > > Milo Kim (19): > irqchip: atmel-aic: fix wrong bit operation for IRQ priority > irqchip: atmel-aic: clean up RTC interrupt code > irqchip: atmel-aic: clean up RTT interrupt code > irqchip: atmel-aic: replace magic numbers with named constant > irqchip: atmel-aic: use simple constant to get number of interrupts > per chip > irqchip: atmel-aic: introduce register data structure > irqchip: atmel-aic: make common IRQ domain translate function > irqchip: atmel-aic: add common mask and unmask functions > irqchip: atmel-aic: add common retrigger function > irqchip: atmel-aic: add common set_type function > irqchip: atmel-aic: add common PM IRQ chip operation > irqchip: atmel-aic: use EOI register data in aic_reg_data > irqchip: atmel-aic: clean up irq_chip_generic > irqchip: atmel-aic: add common HW init function > irqchip: atmel-aic: add common interrupt handler > irqchip: atmel-aic: get total number of IRQs from device node > irqchip: atmel-aic: use unified IRQ chip initialization function > irqchip: atmel-aic: use unified AIC driver > irqchip: atmel-aic: rename AIC driver and fix Kconfig > > arch/arm/mach-at91/Kconfig | 2 +- > drivers/irqchip/Kconfig | 7 - > drivers/irqchip/Makefile | 3 +- > drivers/irqchip/irq-aic.c | 609 +++++++++++++++++++++++++++++++++ > drivers/irqchip/irq-atmel-aic-common.c | 280 --------------- > drivers/irqchip/irq-atmel-aic-common.h | 41 --- > drivers/irqchip/irq-atmel-aic.c | 276 --------------- > drivers/irqchip/irq-atmel-aic5.c | 367 -------------------- > 8 files changed, 611 insertions(+), 974 deletions(-) > create mode 100644 drivers/irqchip/irq-aic.c > delete mode 100644 drivers/irqchip/irq-atmel-aic-common.c > delete mode 100644 drivers/irqchip/irq-atmel-aic-common.h > delete mode 100644 drivers/irqchip/irq-atmel-aic.c > delete mode 100644 drivers/irqchip/irq-atmel-aic5.c > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists