linux-kernel - Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt controller.

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: <alpine.DEB.2.11.1510091441200.6097@nanos>
Date:	Fri, 9 Oct 2015 15:47:37 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	"majun (F)" <majun258@...wei.com>
cc:	Catalin.Marinas@....com, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, Will.Deacon@....com,
	mark.rutland@....com, marc.zyngier@....com, jason@...edaemon.net,
	lizefan@...wei.com, huxinwei@...wei.com, dingtianhong@...wei.com,
	zhaojunhua@...ilicon.com, liguozhu@...ilicon.com,
	xuwei5@...ilicon.com, wei.chenwei@...ilicon.com,
	guohanjun@...wei.com, wuyun.wu@...wei.com, guodong.xu@...aro.org,
	haojian.zhuang@...aro.org, zhangfei.gao@...aro.org,
	usman.ahmad@...aro.org, klimov.linux@...il.com
Subject: Re: [PATCH v5 1/3] initialize each mbigen device node as a interrupt
 controller.

On Sun, 4 Oct 2015, majun (F) wrote:
> >> +	info->reg_offset = get_mbigen_vec_reg_addr(info->nid, info->local_pin_offset);
> > 
> > So you fill in a structure with 5 fields and the only information
> > which is ever used is local_pin_offset.
> > 
> > What's the point of this exercise? 
> 
> Besides local_pin_offset , nid, and reg_offset are also useful
> information which will be used in next patch.

This is horrible to review, really.

Split your patches into smaller pieces then. Add the core
functionality and then introduce the other things when you actually
use them.

> > And that msi_irq information comes from where? Nothing in that code
> > initializes it.
> 
> msi_irq is is initialized in next patch.

See above.
 
> > All you ever need from this is local_pin_offset and the base address
> > for that calculation in the eoi callback.
> 
> dev_irq is stored for easily using in next patch when interrupt happened.

Ditto.
 
> >> +
> >> +	/* add this mbigen device into a global list*/
> >> +	spin_lock(&mbigen_device_lock);
> >> +	list_add(&mgn_dev->global_entry, &mbigen_device_list);
> >> +	spin_unlock(&mbigen_device_lock);
> > 
> > And that global list is used whatfor? I can't see anything which makes
> > use of it.
> 
> This global list is used to find out mbigen device when initializing the mbigen
> device as a platform device in next patch.

Sorry, this is unreviewable.
 
> Because there are several mbigen chips in this system, and each mbigen chip also
> contains several mbgien devices.
> 
> I need a list contains all of the mbigen devices which connect to these mbigen
> chips.
> Then, during mbigen chip initializing, we can use this list to find out mbigen devices
> and pass mbigen_device data structure.
> 
> > 
> > That's a complete disaster and I'm not even thinking about looking at
> > the next patch in this series.
> > 
> > Can you please explain in a simple ASCII picture how your irq chip
> > hierarchy looks like and what kind of data you need for each hierarchy
> > level?
> 
> Mbigen chip hardware structure shows as below:
> 
> 		mbigen chip
> |---------------------|-------------------|
> mgn_node0	  mgn_node1		mgn_node2
>  |		 |-------|		|-------|------|
> dev1		dev1    dev2		dev1   dev3   dev4
> 
> 
> 
> Irq chip hierarchy stucture:
> 		
> 	ITS
> 	 |
> 	ITS-pMSI
> 	 | (virq1)
> |--------| -----------------|
> mbigen-device1		mbigen-device2
>  | (virq2)		 | (virq2)
> devices(uart)		device(gmac)

That picture is wrong as it suggests that uart and gmac share the same
virq.
 
> I named virq1 as msi_irq , virq2 as dev-irq and ,virq1 != virq2.
> 
> Each virq2 has a corresponding virq1.

Whatfor?
 
> Mbigen-device is a special hardware.

Everything is special hardware.

> On the one hand, it's a platform device for ITS. We need to
> allocate the msi-irqs for it.(handled in patch 2/3)
> 
> On the other hand, it's a interrupt controller for the devices
> connected to it.(handled in current patch).
>
> To bind these two different irqs, I made a data sutruce named
> mbigen_irq_data which contains some information of this irq,
> including private index, pin_offset, nid, and local_pin_offset.
>
> All these information can help us to find the corresponding reg addr
> and msi_irq quickly.

This is completely wrong. Why would you need two linux virq numbers
for one interrupt?

This needs to be expressed in one hierarchy. mbigen is just a
translator between wired interrupts and MSI, nothing else.

So the hierarchy is:

  mbigen -> ITS-MSI -> ITS -> GIC

No need for extra levels of indirection. Your mbigen irqchip callbacks
are simply doing:

    parent->callback(parent_data);

and you get that for free when using the hierarchy. No need for that
chained interrupt handler either.

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/