From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v6 09/31] xen/arm: ITS: Add APIs to add and
 assign device
Date: Wed, 9 Sep 2015 14:28:02 +0100
Message-ID: <1441805282.24450.324.camel@citrix.com>
References: <1441019208-2764-1-git-send-email-vijay.kilari@gmail.com>
	<1441019208-2764-10-git-send-email-vijay.kilari@gmail.com>
	<55E884BD.5080503@citrix.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: <55E884BD.5080503@citrix.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: Julien Grall <julien.grall@citrix.com>, vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org
Cc: Prasun.Kapoor@caviumnetworks.com, Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>, manish.jaggi@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

On Thu, 2015-09-03 at 18:34 +0100, Julien Grall wrote:
> @@ -522,6 +535,205 @@ static void its_lpi_free(struct its_device *dev)
> >      xfree(dev->event_map.lpi_map);
> >  }
> >  
> > +static void its_discard_lpis(struct its_device *dev, u32 ids)
> > +{
> > +    int i;
> > +
> 
> I would have expected a function more complex than that. If you discard
> the LPIs, you also need to free the MSI desc and potentially reset the
> IRQ desc.
> 
> Otherwise you will left the irq_desc in an unknown state for the next
> one.

But discard != free? (or at least "discard" has a specific meaning in its).

If this function is supposed to free everything then I would agree, and
also add that the function is therefore badly (or at least confusingly)
named.

If it is just supposed to discard (==clear any h/w pending state of an LPI
mapped by a given event on a given device) then I think it is correct,
isn't it?

> > +    xfree(dev->event_map.col_map);
> > +    xfree(dev);
> > +}
> > +
> > +static struct its_device *its_alloc_device(u32 devid, u32 nr_ites,
> > +                                           struct dt_device_node
> > *dt_its)
> > +{
> > +    struct its_device *dev;
> > +    paddr_t *itt;
> 
> Why paddr_t? You only allocate it and pass directly to the hardware.

paddr_t seems correct, it the fact that it is a paddr_t * (i.e. a pointer)
which seems odd to me. I think you commented the same thing on dev
->itt_addr which is where this ends up assigned.

Ian.