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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Mon, Apr 28, 2014 at 5:36 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Vijaya
>
> On 04/28/2014 12:48 PM, Vijay Kilari wrote:
>> On Wed, Apr 16, 2014 at 12:05 AM, Julien Grall <julien.grall@linaro.org> wrote:
>>> 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
>
> No, with your patch, the desc->handler is filled with ops from the
> generic code. You added an indirection to call the specific ops.
>

Do you mean to move gic_host_irq_type & gic_quest_irq_type
to gic-{2/3}.c and register these with generic driver?
So with this gic_lock should also be moved to gic-{2,3}.c

> In any case, you are only partially define the ops here, which is wrong.
> Every callbacks should be defined for consistency.
>
>> 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?
>
> I don't see why you are saying : "The gic_lock protected generic
> functionality".
> I've looked at the code and the gic.lock is only used to protect call to
> gic callback.
> The gic_lock has been designed to protect access to the hardware, *NOT*
> the internal structure.
>
> IHMO, this lock should be moved in each driver and let them decide if
> they need lock to write into the GIC.
>
> With this assumption, you won't need your indirection in the ops handler.
>
> Regards,
>
> --
> Julien Grall