Re: [PATCH v5 08/10] arm64/mm: Add support for XPFO to swiotlb

Tycho Andersen
2017-08-10T16:37:48Z

Hi Konrad,

Thanks for taking a look!

On Thu, Aug 10, 2017 at 09:11:12AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 09, 2017 at 02:07:53PM -0600, Tycho Andersen wrote:
> > +
> > +inline void xpfo_dma_map_unmap_area(bool map, const void *addr, size_t size,
>
> And inline? You sure about that? It is quite a lot of code to duplicate
> in all of those call-sites.
>
> > + int dir)
>
> Not enum dma_data_direction ?

I'll fix both of these, thanks.

> > +{
> > + unsigned long flags;
> > + struct page *page = virt_to_page(addr);
> > +
> > + /*
> > + * +2 here because we really want
> > + * ceil(size / PAGE_SIZE), not floor(), and one extra in case things are
> > + * not page aligned
> > + */
> > + int i, possible_pages = size / PAGE_SIZE + 2;
>
> Could you use the PAGE_SHIFT macro instead? Or PFN_UP ?
>
> And there is also the PAGE_ALIGN macro...
>
> > + void *buf[possible_pages];
>
> What if you just did 'void *buf[possible_pages] = { };'
>
> Wouldn't that eliminate the need for the memset?

gcc doesn't seem to like that:

arch/arm64//mm/xpfo.c: In function ‘xpfo_dma_map_unmap_area’:
arch/arm64//mm/xpfo.c:80:2: error: variable-sized object may not be initialized
void *buf[possible_pages] = {};
^~~~

I thought about putting this on the heap, but there's no real way to return
errors here if e.g. the kmalloc fails. I'm open to suggestions though, because
this is ugly.

> > +
> > + memset(buf, 0, sizeof(void *) * possible_pages);
> > +
> > + local_irq_save(flags);
>
> ?? Why?

I'm afraid I don't really know. I'll drop it for the next version, thanks!

Tycho