Re: Proposal about reorganize struct irq_data and struct irq_desc

[Jiang Liu]

On 2015/1/20 17:31, Thomas Gleixner wrote:
> On Mon, 19 Jan 2015, Jiang Liu wrote:
> 
>> Hi Thomas and Marc,
>> 	During working on the generic MSI support, I have some proposal
>> about reorganizing struct irq_data and struct irq_desc. The proposed
>> changes are:
>> 1) Add a pointer "struct irq_desc *" to struct irq_data, so we could
>>    quickly get struct irq_desc from struct irq_data.
>> 2) Move "node" from struct irq_data into struct irq_desc, NUMA info
>>    should be per-irq instead of per-chip.
>> 3) Move "affinity" from struct irq_data into struct irq_desc, NUMA info
>>    should be per-irq instead of per-chip.
>> 4) Move "msi_desc" from struct irq_data into struct irq_desc. (Not sure
>>    whether we should do this. Theoretically we should use
>>    irq_data->handler_data to store msi_desc.)
> 
> msi_desc belongs to the msi chip, while handler_data is common data.
> 
> I had a look at the usage sites of handler_data. Most of them use it
> in combination with chained handlers. Some sites use it instead of
> chip data and only a few use it for some random other stuff, where the
> x86 sites (hpet, ht, iommu ...) will go away with the irqdomain
> conversion.
> 
> So in the long run we should provide:
> 
>    irq_set_chained_handler_and_data(irq, handler, handler_data)
> 
> convert everything over and finally remove the direct accessor to
> handler_data.
> 
> msi_desc in a hierarchical implementation should actually be in
> chip_data, but we probably need to keep the msi_desc pointer for
> backward compability reasons.
> 
>> With above change applied, struct irq_data only hosts per-chip data, and
>> struct irq_desc hosts per-irq data. What's your thoughts?
> 
> I'm not so happy with exposing irqdesc to random code again. I went a
> great way to hide it from abuse.
> 
> So I'd rather like to see something like this:
> 
> struct irq_common_data {
> 	unsigned int		state_use_accessors;
> 	unsigned int		node;
> 	void			*handler_data;
> 	cpumask_var_t		affinity;
> };
> 
> struct irq_data {
> 	u32			mask;
> 	unsigned int		irq;
> 	unsigned long		hwirq;
> 	struct irq_chip		*chip;
> 	struct irq_domain	*domain;
> #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> 	struct irq_data		*parent_data;
> #endif
> 	void			*chip_data;
> 	struct msi_desc		*msi_desc;
> 	struct irq_common_data	*common_data;
> };
> 
> struct irq_desc {
>        struct irq_data		irq_data;
>        struct common_irq_data	common_data;
>        ...
> };
Great, will go this step by step:)

> 
> Thanks,
> 
> 	tglx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/