Re: [Xen-devel] [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Apr 16, 2014 at 12:05 AM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijaya,
>
> Thank you for the patch.
>
>>      /* Enable routing */
>> -    GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
>> +    gic_hw_ops->gic_irq_ops->enable(desc);
>
> This is not the right way to use gic_irq_ops. You should directly
> assigned this structure to desc->handler.
>
 I think desc->handler is already filled with ops of below struct
which is used

static hw_irq_controller gic_host_irq_type = {
    .typename = "gic",
    .startup = gic_irq_startup,
    .shutdown = gic_irq_shutdown,
    .enable = gic_irq_enable,
    .disable = gic_irq_disable,
    .ack = gic_irq_ack,
    .end = gic_host_irq_end,
    .set_affinity = gic_irq_set_affinity,
};

> I guess you don't do it because you have the desc->status and gic_lock
> locking which is common.
>
> IHMO, you have to let the GICv{2,3} drivers handling their own lock for
> the hardware access.

The existing lock gic_lock protects both generic & hw specific functionality.
So I could not see need for another level of locking. Anything specific that
you see it is missing?

Regards
Vijay

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel