linux-kernel - Re: [PATCH v6 1/2] ARM: irqchip: mxs: prepare driver for HW with different offsets

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: <561B7D60.4040808@rempel-privat.de>
Date:	Mon, 12 Oct 2015 11:29:04 +0200
From:	Oleksij Rempel <linux@...pel-privat.de>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	linux-kernel@...r.kernel.org, marc.zyngier@....com,
	jason@...edaemon.net
Subject: Re: [PATCH v6 1/2] ARM: irqchip: mxs: prepare driver for HW with
 different offsets

Am 11.10.2015 um 19:58 schrieb Thomas Gleixner:
> Oleksij,
> 
> On Sat, 10 Oct 2015, Oleksij Rempel wrote:
> 
> The proper subject line starts with:
> 
> irqchip/mxs: 
> 
>> Some HW has similar functionality but different register offsets.
>> Make sure we can change offsets dynamically.
> 
> The patch does way more than that. I told you in V2 already:
> 
>>> You forgot to mention the other preparatory changes.
> 
> There is still nothing in the changelog.
> 
>> +static void __init icoll_add_domain(struct device_node *np,
>> +			  int num)
>> +{
>> +	icoll_domain = irq_domain_add_linear(np, num,
>> +					     &icoll_irq_domain_ops, NULL);
>> +
>> +	if (!icoll_domain)
>> +		panic("%s: unable add irq domain", np->full_name);
> 
> This splitout should be a separate patch with an explanation why you
> add

I assume i can remove this both lines. irq_set_default_host is an
artefact from old code.

>> +	irq_set_default_host(icoll_domain);
> 
> and
> 
>> +	set_handle_irq(icoll_handle_irq);
> 
> The latter is already done via the DT_MACHINE_START magic. So you
> should it remove there, because otherwise that call is just
> pointless. See the implementation of set_handle_irq.
> 
>> +}
>> +
>> +static void __iomem * __init icoll_init_iobase(struct device_node *np)
>> +{
>> +	void __iomem *icoll_base;
>> +
>> +	icoll_base = of_io_request_and_map(np, 0, np->name);
>> +	if (!icoll_base)
>> +		panic("%s: unable to map resource", np->full_name);
>> +	return icoll_base;
>> +}
> 
> The panic() is actually a bug fix, because the old code had a
> WARN_ON() and then happily dereferenced the NULL pointer. So this
> wants to go into a separate patch as well.
> 
>> +	icoll_add_domain(np, ICOLL_NUM_IRQS);
>>  
>> -	icoll_domain = irq_domain_add_linear(np, ICOLL_NUM_IRQS,
>> -					     &icoll_irq_domain_ops, NULL);
>>  	return icoll_domain ? 0 : -ENODEV;
> 
> In case of !icoll_domain this return is not reached as you paniced
> already. So why would we still check icoll_domain?

I'm paniced on !icoll_base, icoll_domain will give only warning.
http://lxr.free-electrons.com/source/kernel/irq/irqdomain.c#L52

Or do i miss some thing?

> 
> Thanks,
> 
> 	tglx
> 


-- 
Regards,
Oleksij


Download attachment "signature.asc" of type "application/pgp-signature" (214 bytes)