linux-kernel - Re: [PATCH v2 05/15] x86/dtb: add early parsing of APIC and IO APIC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D35AA2F.3000503@linutronix.de>
Date:	Tue, 18 Jan 2011 15:56:47 +0100
From:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:	Grant Likely <grant.likely@...retlab.ca>
CC:	linux-kernel@...r.kernel.org, sodaville@...utronix.de,
	x86@...nel.org, devicetree-discuss@...ts.ozlabs.org
Subject: Re: [PATCH v2 05/15] x86/dtb: add early parsing of APIC and IO APIC

Grant Likely wrote:
> Hi Sebastian,
Hi Grant,

>> diff --git a/arch/x86/include/asm/prom.h b/arch/x86/include/asm/prom.h
>> index 9076ae4..3bc8ed5 100644
>> --- a/arch/x86/include/asm/prom.h
>> +++ b/arch/x86/include/asm/prom.h
>> @@ -22,10 +22,17 @@
>>  #include <asm/irq_controller.h>
>>  
>>  #ifdef CONFIG_OF
>> +extern int of_ioapic;
>> +extern u64 initial_dtb;
>>  extern void add_dtb(u64 data);
>> +void x86_dtb_find_config(void);
>> +void x86_dtb_get_config(unsigned int unused);
>>  void add_interrupt_host(struct irq_domain *ih);
>>  #else
>>  static inline void add_dtb(u64 data) { }
>> +#define x86_dtb_find_config x86_init_noop
>> +#define x86_dtb_get_config x86_init_uint_noop
>> +#define of_ioapic 0
> 
> Nit: Personally, I prefer static inlines over preprocessor macros, but
> that isn't anything that will block this patch.

Right. x86 does similar for ioapic function callbacks for CONFIG_IOAPIC
case. So I keep this unless x86 folks want it different.

>> +#ifdef CONFIG_X86_IO_APIC
>> +static void __init dtb_add_ioapic(struct device_node *dn)
>> +{
>> +	unsigned int ioapic_id;
>> +	const __be32 *cell;
>> +	int len;
>> +	struct resource r;
>> +	int ret;
>> +
>> +	cell = of_get_property(dn, "id", &len);
>> +	if (!cell) {
>> +		printk(KERN_ERR "Node %s is missing id property.\n",
>> +				dn->full_name);
>> +		return;
>> +	}
>> +	ioapic_id = of_read_ulong(cell, len / 4);
> 
> This looks like poor usage practise for the device tree.  'id' or any
> kind of enumeration property in a device tree node is strongly
> discouraged.  As much as possible, let Linux dynamically enumerate
> devices rather than trying to explicitly specify the numbering.  Since
> you can explicitly describe the relationship between device nodes in
> the tree, you should find that explicitly specifying numbers is almost
> never required.

Good point. I removed the id property and let linux statically enumerate
it. I don't see any relationship and spec. says only to keep it unique.

Sebastian
--
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/