linux-kernel - Re: [PATCH 6/7] [PATCH] MIPS: OCTEON: Add support for OCTEON III 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: <56B4DA3B.40607@caviumnetworks.com>
Date:	Fri, 5 Feb 2016 09:22:03 -0800
From:	David Daney <ddaney@...iumnetworks.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	David Daney <ddaney.cavm@...il.com>, <linux-mips@...ux-mips.org>,
	<ralf@...ux-mips.org>, <linux-kernel@...r.kernel.org>,
	David Daney <david.daney@...ium.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH 6/7] [PATCH] MIPS: OCTEON: Add support for OCTEON III
 interrupt controller.

On 02/05/2016 01:06 AM, Thomas Gleixner wrote:
> On Thu, 4 Feb 2016, David Daney wrote:
>> +static int octeon_irq_ciu_set_type(struct irq_data *data, unsigned int t)
>> +{
>> +	irqd_set_trigger_type(data, t);
>> +
>> +	if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
>> +		irq_set_handler_locked(data, handle_edge_irq);
>> +	else
>> +		irq_set_handler_locked(data, handle_level_irq);
>> +
>> +	return IRQ_SET_MASK_OK;
>
> That doesn't make any sense. First you store the type 't' in irq data, then
> you query irq data for the type and at the end you tell the core to set the
> type in irq data.
>
>       if (t & IRQ_TYPE_EDGE_BOTH)
>       	...
>       else
>          ...
>       return IRQ_SET_MASK_OK;
>
> does the same, right?

Yes, clearly that is better.  I don't know what we were thinking there.


>
>> +int octeon_irq_ciu3_xlat(struct irq_domain *d,
>
> static ?

To be used in follow-on patch for MSI-X irq_chip driver residing in a 
separate file.

The idea was to not be changing the to/from static multiple times as the 
new patches were merged.  If you think it preferable, I can make them 
all static and then remove the static later, when needed.

>
>> +			 struct device_node *node,
>> +			 const u32 *intspec,
>> +			 unsigned int intsize,
>> +			 unsigned long *out_hwirq,
>> +			 unsigned int *out_type)
>> +{
>> +
>> +void octeon_irq_ciu3_enable(struct irq_data *data)
>
> static
>
>> +void octeon_irq_ciu3_disable(struct irq_data *data)
>> +void octeon_irq_ciu3_ack(struct irq_data *data)
>> +void octeon_irq_ciu3_mask(struct irq_data *data)
>> +void octeon_irq_ciu3_mask_ack(struct irq_data *data)
>> +int octeon_irq_ciu3_set_affinity(struct irq_data *data,
>> +				 const struct cpumask *dest, bool force)
>
> ditto

Same, see above.


>
>> +int octeon_irq_ciu3_mapx(struct irq_domain *d, unsigned int virq,
>> +			 irq_hw_number_t hw, struct irq_chip *chip)
>
> ....

Yep.

>
>> +void octeon_ciu3_mbox_send(int cpu, unsigned int mbox)
>> +{
>> +	struct octeon_ciu3_info *ciu3_info;
>> +	unsigned int intsn;
>> +	union cvmx_ciu3_iscx_w1s isc_w1s;
>> +	u64 isc_w1s_addr;
>> +
>> +	if (WARN_ON_ONCE(mbox >= CIU3_MBOX_PER_CORE))
>> +		return;
>> +
>> +	intsn = octeon_irq_ciu3_mbox_intsn_for_cpu(cpu, mbox);
>> +	ciu3_info = per_cpu(octeon_ciu3_info, cpu);
>> +	isc_w1s_addr = ciu3_info->ciu3_addr + CIU3_ISC_W1S(intsn);
>> +
>> +	isc_w1s.u64 = 0;
>> +	isc_w1s.s.raw = 1;
>> +
>> +	cvmx_write_csr(isc_w1s_addr, isc_w1s.u64);
>> +	cvmx_read_csr(isc_w1s_addr);
>> +}
>> +EXPORT_SYMBOL(octeon_ciu3_mbox_send);
>
> Why need modules that function?

Similar to the MSI-X irq_chip thing, but this time for follow-on 
kexec/kdump patches which may be implemented as a module.

I will remove the EXPORT_SYMBOL().

>
> Thanks,
>
> 	tglx
>