Proposal about reorganize struct irq_data and struct irq_desc

Proposal about reorganize struct irq_data and struct irq_desc

[Jiang Liu]

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.)

With above change applied, struct irq_data only hosts per-chip data, and
struct irq_desc hosts per-irq data. What's your thoughts?
Regards,
Gerry

Re: Proposal about reorganize struct irq_data and struct irq_desc

[Thomas Gleixner]

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;
       ...
};

Thanks,

	tglx

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>