| 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
| ||
|
Date: Mon, 4 Jan 2016 10:37:54 +0100 From: Nicolas Ferre <nicolas.ferre@...el.com> To: Milo Kim <milo.kim@...com> CC: Boris Brezillon <boris.brezillon@...e-electrons.com>, <tglx@...utronix.de>, <jason@...edaemon.net>, <marc.zyngier@....com>, <alexandre.belloni@...e-electrons.com>, <ludovic.desroches@...el.com>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 00/19] irqchip: atmel-aic: make unified AIC driver Le 04/01/2016 10:02, Boris Brezillon a écrit : > 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? I was about to ask the same question. I'm really sorry that you spent so much time and effort for something that I'm absolutely against: reworking stable code that had been maturing for years and that is now nicely stable... for no obvious reason... So unless there is a very strong benefit, it's a pretty firm NACK for this series. We will certainly take some time to review the bits that can be taken further to enhance our current drivers. I also advice you to talk to us before starting such a big project and you'll notice that we are open to changes and have many aspect that you can work on with AT91. Bye, >> 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 >> > > > -- Nicolas Ferre -- 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