Prev: linux-next: manual merge of the libata tree with Linus' tree
Next: [ANNOUNCE] Git 1.7.0.3
From: Michael Ellerman on 21 Mar 2010 22:00
On Sun, 2010-03-21 at 18:36 -0700, Yinghai Lu wrote:
> From: Ian Campbell <ian.campbell(a)citrix.com>
....
> To replace the x86 arch_init_chip_data functionality
> irq_to_desc_alloc_node now takes a pointer to a function to allocate
> the chip data. This is necessary to ensure the allocation happens
> under the correct locking at the core level. On PowerPC and SH
> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
> which retains existing chip_data behaviour.
....
>
> -v4: yinghai add irq_to_desc_alloc_node_x...
> so could leave default path not changed...

Apologies for not noticing this sooner, but ..

> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
> return 0;
> }
>
> -int arch_init_chip_data(struct irq_desc *desc, int node)
> +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
> {
> desc->status |= IRQ_NOREQUEST;
> return 0;

This is a bit feral, that is the init_chip_data_fn.

It seems like it only exists to support the following on x86:

> +int arch_init_irq_desc(struct irq_desc *desc, int node,
> + init_chip_data_fn init_chip_data)
> +{
> + if (!init_chip_data)
> + return x86_init_chip_data(desc, node);
> +
> + return init_chip_data(desc, node);
> +}

Which is really just a hack to avoid an if (xen) check isn't it?

It looks to me like this should just be done via a current machine
vector or platform routine, in the same way as powerpc and (I think)
ia64, ie:

> +int arch_init_irq_desc(struct irq_desc *desc, int node)
> +{
> + return current_machine->init_chip_data(desc, node);
> +}

cheers

From: Yinghai Lu on 21 Mar 2010 23:40
On 03/21/2010 06:56 PM, Michael Ellerman wrote:
> On Sun, 2010-03-21 at 18:36 -0700, Yinghai Lu wrote:
>> From: Ian Campbell <ian.campbell(a)citrix.com>
> ...
>> To replace the x86 arch_init_chip_data functionality
>> irq_to_desc_alloc_node now takes a pointer to a function to allocate
>> the chip data. This is necessary to ensure the allocation happens
>> under the correct locking at the core level. On PowerPC and SH
>> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
>> which retains existing chip_data behaviour.
> ...
>>
>> -v4: yinghai add irq_to_desc_alloc_node_x...
>> so could leave default path not changed...
>
> Apologies for not noticing this sooner, but ..
>
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
>> return 0;
>> }
>>
>> -int arch_init_chip_data(struct irq_desc *desc, int node)
>> +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
>> {
>> desc->status |= IRQ_NOREQUEST;
>> return 0;
>
> This is a bit feral, that is the init_chip_data_fn.
>
> It seems like it only exists to support the following on x86:
>
>> +int arch_init_irq_desc(struct irq_desc *desc, int node,
>> + init_chip_data_fn init_chip_data)
>> +{
>> + if (!init_chip_data)
>> + return x86_init_chip_data(desc, node);
>> +
>> + return init_chip_data(desc, node);
>> +}
>
> Which is really just a hack to avoid an if (xen) check isn't it?

arch/powerpc/kernel/irq.c | 2 +-

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 64f6f20..cafd378 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
return 0;
}

-int arch_init_chip_data(struct irq_desc *desc, int node)
+int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
{
desc->status |= IRQ_NOREQUEST;
return 0;

so this patch only change one line with powerpc code.
>
> It looks to me like this should just be done via a current machine
> vector or platform routine, in the same way as powerpc and (I think)
> ia64, ie:
>
>> +int arch_init_irq_desc(struct irq_desc *desc, int node)
>> +{
>> + return current_machine->init_chip_data(desc, node);
>> +}
>
looks like xen in same run time, some irqs need x86_init_chip_data,
and some may need xen_init_chip_data later.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 22 Mar 2010 06:30
On Sun, 21 Mar 2010, Yinghai Lu wrote:

> From: Ian Campbell <ian.campbell(a)citrix.com>
>
> Move arch_init_copy_chip_data and arch_free_chip_data into function
> pointers in struct irq_chip since they operate on irq_desc->chip_data.

Not sure about that. These functions are solely used by x86 and there
is really no need to generalize them. The problem you try to solve is
x86/xen specific and can be solved by x86_platform_ops as well w/o
adding extra function pointers to irq_chip.

> arch_init_chip_data cannot be moved into struct irq_chip because
> irq_desc->chip is not known at the time the irq_desc is setup. Instead
> rename arch_init_chip_data to arch_init_irq_desc for PowerPC, the
> only other user, whose usage better matches the new name.
>
> To replace the x86 arch_init_chip_data functionality
> irq_to_desc_alloc_node now takes a pointer to a function to allocate
> the chip data. This is necessary to ensure the allocation happens
> under the correct locking at the core level. On PowerPC and SH

Err, that argument is totally bogus. The calling convention of
irq_to_desc_alloc_node and arch_init_chip_data/arch_init_irq_desc is
still the same. It does not explain why the heck we need that function
pointer at all.

AFAICT the function pointer to irq_to_desc_alloc_node is completely
pointless. It just solves a Xen/x86 specific problem which can be
solved by using x86_platform_ops and keeps the churn x86 internal.

> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
> which retains existing chip_data behaviour.

> I've retained the chip_data behaviour for uv_irq although it isn't
> clear to me if these interrupt types support migration or how closely
> related to the APIC modes they really are. If it weren't for this the
> x86_{init,copy,free}_chip_data functions could be static to
> io_apic.c.
>
> I've tested by booting on an 64 bit x86 system with sparse IRQ enabled
> and 32 bit without, but it's not clear to me what actions I need to
> take to actually exercise some of these code paths.
>
> -v4: yinghai add irq_to_desc_alloc_node_x...

Aside of the general objection against this, please use descriptive
function names and do not distinguish functions by adding random
characters which tell us absolutely nothing about the purpose.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ian Campbell on 24 Mar 2010 09:40
On Mon, 2010-03-22 at 10:19 +0000, Thomas Gleixner wrote:
> On Sun, 21 Mar 2010, Yinghai Lu wrote:
>
> > From: Ian Campbell <ian.campbell(a)citrix.com>
> >
> > Move arch_init_copy_chip_data and arch_free_chip_data into function
> > pointers in struct irq_chip since they operate on irq_desc->chip_data.
>
> Not sure about that. These functions are solely used by x86 and there
> is really no need to generalize them.

I thought the idea of struct irq_chip was to allow the potential for
multiple IRQ controllers in a system? Given that it seems that struct
irq_desc->chip_data ought to be available for use by whichever struct
irq_chip is managing a given interrupt. At the moment this is not
possible because we step around the abstraction using these arch_*
methods.

Although this might be unusual on x86 I think it is not uncommon in the
embedded world to have an architectural interrupt controller cascading
through to various different IRQ controllers/multiplexors, from random
FPGA based things, to GPIO controllers and things like superio chips
etc.

Currently the set of architectures which typically have this sort of
thing are disjoint from the ones which make use of struct
irq_desc->chip_data but with the growing use of embedded-x86 is this not
something worth considering? (Genuine question, I've been out of the
embedded space for a while now so maybe my experiences are out of date
or I'm overestimating the role of embedded-x86 etc).

Xen is a bit more of a specialised case than the above since it would
like to replace the architectural interrupt handling but I think the
broad requirements on the irq_chip interface are the same. Going forward
it is possible/likely that we would like to be able to make Xen event
channels available via a cascade model as well -- demultiplexing one (or
more?) x86 architectural interrupts into event channels would be part of
running PV Xen drivers on a fully-virtualised (i.e. native) kernel.

> The problem you try to solve is
> x86/xen specific and can be solved by x86_platform_ops as well w/o
> adding extra function pointers to irq_chip.
[...]
> AFAICT the function pointer to irq_to_desc_alloc_node is completely
> pointless. It just solves a Xen/x86 specific problem which can be
> solved by using x86_platform_ops and keeps the churn x86 internal.

I have no problem with that if that is the x86/irq maintainer's
preference, I just thought it would be nicer to solve what I saw as an
oddity in the existing abstraction generically in the core.

> Aside of the general objection against this, please use descriptive
> function names and do not distinguish functions by adding random
> characters which tell us absolutely nothing about the purpose.

I agree on this one. More generally I would say that the number of
existing users of this interface is small enough that _if_ we decide we
need to modify it then we should just bite the bullet and do that
instead of building compatibility layers around stuff. For this reason I
think my original patch was preferable to this version (general
objections not withstanding).

Ian.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 24 Mar 2010 13:50
On Wed, 24 Mar 2010, Ian Campbell wrote:

> On Mon, 2010-03-22 at 10:19 +0000, Thomas Gleixner wrote:
> > On Sun, 21 Mar 2010, Yinghai Lu wrote:
> >
> > > From: Ian Campbell <ian.campbell(a)citrix.com>
> > >
> > > Move arch_init_copy_chip_data and arch_free_chip_data into function
> > > pointers in struct irq_chip since they operate on irq_desc->chip_data.
> >
> > Not sure about that. These functions are solely used by x86 and there
> > is really no need to generalize them.
>
> I thought the idea of struct irq_chip was to allow the potential for
> multiple IRQ controllers in a system? Given that it seems that struct
> irq_desc->chip_data ought to be available for use by whichever struct
> irq_chip is managing a given interrupt. At the moment this is not
> possible because we step around the abstraction using these arch_*
> methods.

Right, but you have exactly _ONE_ irq_chip associated to an irq_desc,
but that same irq_chip can be associated to several irq_descs. So
irq_desc->data is there to provide data for the irq_chip functions
depending on what irq they handle (e.g. base_address ...).

irq_desc->chip_data is set when the irq_chip is assigned to the
irq_desc.

So there is no point in having functions in irq_chip to set
irq_desc->chip_data.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: linux-next: manual merge of the libata tree with Linus' tree
Next: [ANNOUNCE] Git 1.7.0.3