Re: [Xen-devel] [PATCH v6 09/31] xen/arm: ITS: Add APIs to add and assign device

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.

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