Skip to contentSkip to conversation

Is iommu_num_pages() broken ?

DISCUSSION Benjamin Herrenschmidt 2 participants 3 messages Aug 5, 2012

lore
Share & tools
lore
#0 lore raw

Hi folks !

I stumbled upon this today:

static inline unsigned long iommu_num_pages(unsigned long addr,
unsigned long len, unsigned long io_page_size)

{
	unsigned long size = (addr & (io_page_size - 1)) + len;

	return DIV_ROUND_UP(size, io_page_size);
}

That doesn't look right to me...

The powerpc iommu code at least uses that with an addr which may not be page aligned (ie, result of sg_virt() which include the offset).

The above code will align the start before adding the len which is wrong and will result in potentially missing a page or am I missing something ?

Shouldn't it be something like

static inline unsigned long iommu_num_pages(unsigned long addr,
unsigned long len, unsigned long io_page_size)

{
	unsigned long start = addr & (io_page_size - 1);
	unsigned long end = addr + len;

	return DIV_ROUND_UP(end - start, io_page_size);
}

?

Cheers,
Ben.

#1 lore raw
Quoted 10 lines · Benjamin Herrenschmidt

static inline unsigned long iommu_num_pages(unsigned long addr,
unsigned long len, unsigned long io_page_size)

{
	unsigned long size = (addr & (io_page_size - 1)) + len;

	return DIV_ROUND_UP(size, io_page_size);
}
Quoted 6 lines

That doesn't look right to me...

The powerpc iommu code at least uses that with an addr which may not be page aligned (ie, result of sg_virt() which include the offset).

The above code will align the start before adding the len which is wrong

addr & (io_page_size - 1) computes the offset into the page pointed to by addr. Looks right to me.

addr & ~(io_page_size - 1) would round addr down to the start of the page.

Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
#2 lore raw
Quoted 5 lines · Andreas Schwab

addr & (io_page_size - 1) computes the offset into the page pointed to by addr. Looks right to me.

addr & ~(io_page_size - 1) would round addr down to the start of the page.

Ah that's right, for some reason I'm so used to seeing ~ before such constructs that my brain must have imagined it :-)

Sorry for the noise. Next time I'll have coffee first then re-check ...

Cheers,
Ben.