On Wed, Oct 09, 2024 at 02:06:27PM +0000, Arnd Bergmann wrote:
This is clearly a good idea, and I'm happy to take that through
the asm-generic tree if there are no complaints.
Do you have any other patches that depend on it?
Well, I have new code that would benefit from these helpers, but just
open coding it for now and then doing a swipe to clean that up later
together with the existing open coded versions is easy enough.
-/*
- * Change "struct page" to physical address.
- */
-static inline phys_addr_t page_to_phys(struct page *page)
-{
- unsigned long pfn = page_to_pfn(page);
-
- WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && !pfn_valid(pfn));
-
- return PFN_PHYS(pfn);
-}
This part is technically a change in behavior, not sure how
much anyone cares.
Well, the only other comment to the patch so far mentioned it.
It also feels like a useful check, but I'm a bit worried about
it triggering in various new places. Although that's just with
CONFIG_DEBUG_VIRTUAL and probably points to real bugs, so maybe
adding it everywhere is a good idea.
+#define page_to_phys(page) __pfn_to_phys(page_to_pfn(page))
+#define phys_to_page(phys) pfn_to_page(__phys_to_pfn(phys))
I think we should try to have a little fewer nested macros
to evaluate here, right now this ends up expanding
__pfn_to_phys, PFN_PHYS, PAGE_SHIFT, CONFIG_PAGE_SHIFT,
page_to_pfn and __page_to_pfn. While the behavior is fine,
modern gcc versions list all of those in an warning message
if someone passes the wrong arguments.
Changing the two macros above into inline functions
would help as well, but may cause other problems.
Doing them as inlines seems useful to me, let me throw that at
the buildbot and see if anything explodes.
On a related note, it would be even better if we could come
up with a generic definition for either __pa/__va or
virt_to_phys/phys_to_virt. Most architectures define one
of the two pairs in terms of the other, which leads to
confusion with header include order.
Agreed, but that's a separate project.
