+int pfnmap_track(unsigned long pfn, unsigned long size, pgprot_t *prot)
+{
+ const resource_size_t paddr = (resource_size_t)pfn << PAGE_SHIFT;
+
+ return reserve_pfn_range(paddr, size, prot, 0);
Nitty, but a pattern established by Liam which we've followed consistently
in VMA code is to prefix parameters that might be less than obvious,
especially boolean parameters, with a comment naming the parameter, e.g.:
> > return reserve_pfn_range(paddr, size, prot, /*strict_prot=*/0);
Not sure I like that. But as this parameter goes away patch #8, I'll
leave it as is in this patch and not start a bigger discussion on better
alternatives (don't use these stupid boolean variables ...) ;)
[...]
+
+/**
+ * pfnmap_track - track a pfn range
To risk sounding annoyingly pedantic and giving the kind of review that is
annoying, this really needs to be expanded, I think perhaps this
description is stating the obvious :)
To me the confusing thing is that the 'generic' sounding pfnmap_track() is
actually PAT-specific, so surely the description should give a brief
overview of PAT here, saying it's applicable on x86-64 etc. etc.
I'm not sure there's much use in keeping this generic when it clearly is
not at this point?
Sorry, is your suggestion to document more PAT stuff or what exactly?
As you know, I'm a busy man ... so instructions/suggestions please :)
+ * @pfn: the start of the pfn range
+ * @size: the size of the pfn range
In what units? Given it's a pfn range it's a bit ambiguous as to whether it
should be expressed in pages/bytes.
Agreed. It's bytes. (not my favorite here, but good enough)
--
Cheers,
David / dhildenb
