From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v3 07/16] xen/arm: segregate and split GIC
	low level functionality
Date: Mon, 28 Apr 2014 13:06:24 +0100
Message-ID: <535E4440.7010305@linaro.org>
References: <1397560675-29861-1-git-send-email-vijay.kilari@gmail.com>
	<1397560675-29861-8-git-send-email-vijay.kilari@gmail.com>
	<534D7BDC.1010308@linaro.org>
	<CALicx6uQo4jg1fCjZhz7Pq4W6eGTq_CfE_u=usjAKgW3CsD9Cw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CALicx6uQo4jg1fCjZhz7Pq4W6eGTq_CfE_u=usjAKgW3CsD9Cw@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Vijay Kilari <vijay.kilari@gmail.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>, Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>, xen-devel@lists.xen.org, Stefano Stabellini <stefano.stabellini@citrix.com>
List-Id: xen-devel@lists.xenproject.org

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.

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