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]

Date:	Sun, 11 Oct 2015 19:58:36 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Oleksij Rempel <linux@...pel-privat.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

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

> +	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?

Thanks,

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