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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
