Messages in this thread | | | Date | Thu, 10 Aug 2017 10:35:58 -0600 | From | Tycho Andersen <> | Subject | Re: [PATCH v5 08/10] arm64/mm: Add support for XPFO to swiotlb |
| |
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
| |