| 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: <4826FAC8.4010604@seznam.cz>
Date: Sun, 11 May 2008 15:55:20 +0200
From: Michal Simek <monstr@...nam.cz>
To: Thomas Gleixner <tglx@...utronix.de>
CC: linux-kernel@...r.kernel.org, arnd@...db.de,
linux-arch@...r.kernel.org, stephen.neuendorffer@...inx.com,
John.Linn@...inx.com, john.williams@...alogix.com, matthew@....cx,
will.newton@...il.com, drepper@...hat.com,
microblaze-uclinux@...e.uq.edu.au, grant.likely@...retlab.ca,
Michal Simek <monstr@...str.eu>
Subject: Re: [PATCH 08/56] microblaze_v2: Interrupt handling, timer support,
supported function
Hi Thomas,
comments are below.
> Michal,
>
> On Sun, 4 May 2008, monstr@...nam.cz wrote:
>> From: Michal Simek <monstr@...str.eu>
>
>> +/* NOTE
>> + * self-modified part of code for improvement of interrupt controller
>> + * save instruction in interrupt rutine
>> + */
>> +void function_hack(const int *arr_fce, const unsigned int base)
>> +{
>> + unsigned int flags = 0;
>
> no need to initialize flags
>
>> + unsigned int j, i;
>> + unsigned int *addr = NULL;
>
> Just a coding style nit:
>
> unsigned int i, j, flags, *addr = NULL;
>
> saves 2 lines of code :)
Saving lines are good. :-)
>> + local_irq_save(flags);
>> + __disable_icache();
>> +
>> + /* zero terminated array */
>> + for (j = 0; arr_fce[j] != 0; j++) {
>> + /* get start address of function */
>> + addr = (unsigned int *) arr_fce[j];
>> + pr_debug("%s: func(%d) at 0x%x\n",
>> + __func__, j, (unsigned int) addr);
>> + for (i = 0;; i++) {
>> + pr_debug("%s: instruction code at %d: 0x%x\n",
>> + __func__, i, addr[i]);
>> + if (addr[i] == 0xb0001234) {
>
> Please replace all those hex constants with proper named defines so
> the code becomes readable
I added some macros which I hope clear this code.
>> + /* detecting of lwi or swi instruction */
>> + if ((addr[i+1] & 0xec00ff00) == 0xe800ff00) {
>> + pr_debug("%s: curr instr, "
>> + "(%d):0x%x, "
>> + "next(%d):0x%x\n",
>> + __func__, i, addr[i], i+1,
>> + addr[i+1]);
>> + addr[i] = 0xb0000000 + (base >> 16);
>> + addr[i+1] = (addr[i+1] & 0xffff00ff) +
>> + (base & 0xffff);
>> + __invalidate_icache(addr[i]);
>> + __invalidate_icache(addr[i+1]);
>> + pr_debug("%s: hack instr, "
>> + "(%d):0x%x, "
>> + "next(%d):0x%x\n",
>> + __func__, i, addr[i], i+1,
>> + addr[i+1]);
>
> Please split this modification code out into a separate function. The
> same code is used below.
Add Macro.
>> + } else /* detection addik for ack */
>> + if ((addr[i+1] & 0xfc00ff00) == 0x3000ff00) {
>> + pr_debug("%s: curr instr, "
>> + "(%d):0x%x, "
>> + "next(%d):0x%x\n",
>> + __func__, i, addr[i], i+1,
>> + addr[i+1]);
>> + addr[i] = 0xb0000000 + (base >> 16);
>> + addr[i+1] = (addr[i+1] & 0xffff00ff) +
>> + (base & 0xffff);
>> + __invalidate_icache(addr[i]);
>> + __invalidate_icache(addr[i+1]);
>> + pr_debug("%s: hack instr, "
>> + "(%d):0x%x, "
>> + "next(%d):0x%x\n",
>> + __func__, i, addr[i], i+1,
>> + addr[i+1]);
>> + }
>
>> +#include <linux/init.h>
>> +#include <linux/irq.h>
>> +#include <linux/autoconf.h>
>> +#include <asm/page.h>
>> +#include <asm/io.h>
>> +
>> +#include <asm/prom.h>
>> +#include <asm/irq.h>
>> +
>> +#ifdef CONFIG_HACK
>> +#include <asm/hack.h>
>> +#else
>> +static unsigned int intc_baseaddr;
>> +#endif
>> +
>> +unsigned int NR_IRQ;
>
> Please use lower case for variables
fixed
>> +/* No one else should require these constants, so define them locally here. */
>> +#define ISR 0x00 /* Interrupt Status Register */
>> +#define IPR 0x04 /* Interrupt Pending Register */
>> +#define IER 0x08 /* Interrupt Enable Register */
>> +#define IAR 0x0c /* Interrupt Acknowledge Register */
>> +#define SIE 0x10 /* Set Interrupt Enable bits */
>> +#define CIE 0x14 /* Clear Interrupt Enable bits */
>> +#define IVR 0x18 /* Interrupt Vector Register */
>> +#define MER 0x1c /* Master Enable Register */
>> +
>> +#define MER_ME (1<<0)
>> +#define MER_HIE (1<<1)
>>
>>
>>
>> +static void intc_enable(unsigned int irq)
>> +{
>> + unsigned int mask = (0x00000001 << (irq & 31));
>> + pr_debug("enable: %d\n", irq);
>> +#ifdef CONFIG_HACK
>
> Please use a sensible name for that config switch and the constants
> (e.g. HACK_BASE_ADDR)
SELFMOD
>> + iowrite32(mask, HACK_BASE_ADDR + SIE);
>> +#else
>> + iowrite32(mask, intc_baseaddr + SIE);
>> +#endif
>
> Please create a macro for that instead of uglyfying each function with
> ifdeffery
>
> #ifdef CONFIG_XXXX
> # define INTC_BASE XXX_BASE_ADDR
> #else
> # define INTC_BASE intc_baseaddr
> #endif
>
> So now your functions reads simple:
>
> unsigned int mask = (0x00000001 << (irq & 31));
>
> iowrite32(mask, INTC_BASE_ADDR + SIE);
Fixed intc and timer functions
>> +}
>> +
>> +static void intc_disable(unsigned int irq)
>> +{
>> + unsigned long mask = (0x00000001 << (irq & 31));
>> + pr_debug("disable: %d\n", irq);
>> +#ifdef CONFIG_HACK
>> + iowrite32(mask, HACK_BASE_ADDR + CIE);
>> +#else
>> + iowrite32(mask, intc_baseaddr + CIE);
>> +#endif
>> +}
>> +
>> +static void intc_disable_and_ack(unsigned int irq)
>> +{
>> + unsigned long mask = (0x00000001 << (irq & 31));
>> + pr_debug("disable_and_ack: %d\n", irq);
>> +#ifdef CONFIG_HACK
>> + iowrite32(mask, HACK_BASE_ADDR + CIE);
>> + /* ack edge triggered intr */
>> + if (!(irq_desc[irq].status & IRQ_LEVEL))
>> + iowrite32(mask, HACK_BASE_ADDR + IAR);
>
> Please do not do that. Create two irq_chips one for level and one for
> edge with different implementations of the functions so you don't have
> to check for level/edge in the functions itself.
>
>> +
>> + for (i = 0; i < NR_IRQ; ++i) {
>> + irq_desc[i].chip = &intc_dev;
>> +
>> + if (handle & (0x00000001 << i))
>> + irq_desc[i].status &= ~IRQ_LEVEL;
>> + else
>> + irq_desc[i].status |= IRQ_LEVEL;
>> + }
>
> With two irq chips this would be:
>
> for (i = 0; i < NR_IRQ; ++i) {
> if (handle & (0x00000001 << i)) {
> set_irq_chip_and_handler(irq, &intc_edge, handle_edge_irq);
> irq_desc[i].status &= ~IRQ_LEVEL;
> } else {
> set_irq_chip_and_handler(irq, &intc_level, handle_level_irq);
> irq_desc[i].status |= IRQ_LEVEL;
> }
> }
Two chips - two levels.
>> +}
>> diff --git a/arch/microblaze/kernel/irq.c b/arch/microblaze/kernel/irq.c
>> new file mode 100644
>> index 0000000..4cd30aa
>> --- /dev/null
>> +++ b/arch/microblaze/kernel/irq.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * arch/microblaze/kernel/process.c
>
> Please remove those useless (and in this case even wrong) file names.
Removing from all Microblaze code.
>> +void do_IRQ(struct pt_regs *regs)
>> +{
>> + unsigned int irq;
>> +
>> + irq_enter();
>> + set_irq_regs(regs);
>> + irq = get_irq(regs);
>> + BUG_ON(irq == -1U);
>> + __do_IRQ(irq);
>
> Please do not use __do_IRQ() use the handlers which provide per
> interrupt type optimized handlers.
>
> struct irq_desc *desc = irq_desc + irq;
>
> desc->handle(irq);
It is a little bit different but with sililar style. You can check it in next
set of patches.
>> + irq_exit();
>> +}
>
>> +
>> +/*unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
>> +{
>> + printk ("ERROR %s\n",__FUNCTION__);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_of_parse_and_map);*/
>
> Please remove unused code
This function is implemented above file.
>> +static void timer_ack(void)
>> +{
>> +#ifdef CONFIG_HACK
>> + iowrite32(ioread32(HACK_BASE_ADDR + TCSR0), HACK_BASE_ADDR + TCSR0);
>> +#else
>> + iowrite32(ioread32(timer_baseaddr + TCSR0), timer_baseaddr + TCSR0);
>> +#endif
>
> Same thing as the interrupt one
<snip>
>> +}
>> +
>> +irqreturn_t timer_interrupt(int irq, void *dev_id)
>> +{
>> +#ifdef CONFIG_HEART_BEAT
>> + heartbeat();
>> +#endif
>> + timer_ack();
>> +
>> + write_seqlock(&xtime_lock);
>> +
>> + do_timer(1);
>
> Can you please move a new architecture to clockevents / clocksource
> right from the beginning ? No need to invent another incompatible set
> of time(r) related functions.
I move whole code to GENERIC_TIME. Did you meant any others changes?
>> + update_process_times(user_mode(get_irq_regs()));
>> + profile_tick(CPU_PROFILING);
>> +
>> + write_sequnlock(&xtime_lock);
>> +
>> + return IRQ_HANDLED;
>> +}
>
>> +/*
>> + * HACK_BASE_ADDR is constant address for hack function.
>> + * do not change this value - it is hardcoded in hack function
>> + * arch/microblaze/kernel/hack.c:function_hack()
>
> Grr. Please use defines for everything and do not hardcode stuff here
> and there and make a define depend on some hardcoded value in a c
> function.
BARRIER_BASE_ADDR
>> + */
>> +
>> +#define HACK_BASE_ADDR 0x1234ff00
>
>
>> +#ifndef _ASM_MICROBLAZE_IRQ_H
>> +#define _ASM_MICROBLAZE_IRQ_H
>> +
>> +#include <linux/seq_file.h>
>> +#define NR_IRQS 32
>
> Eeew.
>
>> +#include <linux/irq.h>
>
> asm/irq.h is included from linux/irq.h not the other way round
fixed
>> +extern unsigned int NR_IRQ;
>> +extern void ledoff(void);
>
> Is ledoff an interrupt function ?
I think no - removed.
>> +#define NO_IRQ (-1)
>> +
>> +static inline int irq_canonicalize(int irq)
>> +{
>> + return (irq);
>> +}
>
> Why is this needed ? Any users ?
is used in serial_core.c
684 new_serial.irq = irq_canonicalize(new_serial.irq);
>> +struct pt_regs;
>> +extern void do_IRQ(struct pt_regs *regs);
>> +extern void __init init_IRQ(void);
>
>> +int show_interrupts(struct seq_file *p, void *v);
>
> Already defined in include/linux/interrupt.h
fixed.
> Thanks,
> tglx
>
Thanks for your review,
Michal Simek
--
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