Le 24/09/2024 à 16:10, Vincenzo Frascino a écrit :
On 23/09/2024 17:54, Arnd Bergmann wrote:
On Mon, Sep 23, 2024, at 14:19, Vincenzo Frascino wrote:
The VDSO implementation includes headers from outside of the
vdso/ namespace.
Introduce vdso/page.h to make sure that the generic library
uses only the allowed namespace.
Cc: Andy Lutomirski <luto@xxxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Jason A. Donenfeld <Jason@xxxxxxxxx>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
Thanks for the new version. This looks all good, just some
very minor ideas for how to possibly improve the new version:
Thanks Arnd.
+/* PAGE_SHIFT determines the page size */
+#define PAGE_SHIFT CONFIG_PAGE_SHIFT
+
+#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
+
+#if defined(CONFIG_PHYS_ADDR_T_64BIT) && !defined(CONFIG_X86_64)
+#define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
+#else
+#define PAGE_MASK (~(PAGE_SIZE-1))
+#endif
I would open-code the CONFIG_PAGE_SHIFT in PAGE_SIZE
and PAGE_MASK, just to avoid the extra indirection in the
preprocessor. This mainly has the benefit of slightly
shorter compiler warnings when all the macros get
traced back but can also slightly improve compile speed
in case this is used in deeply nested macros.
I will fix it in the next iteration.
Without a comment, the special case for CONFIG_X86_64
not very clear, and probably not needed. If you are
worried about introducing an architecture specific
regression, I would suggest instead explaining the
possible issue in the patch description but using the
more generic and simpler #ifdef check.
If I do not add the #ifdef, it does not build. But you are right, I should have
put a comment in the commit message.
Regression below:
drivers/gpu/drm/i915/gt/intel_gt_print.h:29:36: error: format ‘%lx’ expects
argument of type ‘long unsigned int’, but argument 6 has type ‘u32’ {aka
‘unsigned int’} [-Werror=format=]
29 | drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
| ^~~~~~~~
include/drm/drm_print.h:424:39: note: in definition of macro ‘drm_dev_dbg’
424 | __drm_dev_dbg(NULL, dev, cat, fmt, ##__VA_ARGS__)
| ^~~
include/drm/drm_print.h:524:33: note: in expansion of macro ‘drm_dbg_driver’
524 | #define drm_dbg(drm, fmt, ...) drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~
linux/drivers/gpu/drm/i915/gt/intel_gt_print.h:29:9: note: in expansion of macro
‘drm_dbg’
29 | drm_dbg(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id,
##__VA_ARGS__)
| ^~~~~~~
drivers/gpu/drm/i915/gt/intel_gt.c:310:25: note: in expansion of macro ‘gt_dbg’
310 | gt_dbg(gt, "Unexpected fault\n"
| ^~~~~~
I am open to alternative suggestions.
'fault' is an 'u32' and 'mask' should be agnostic so the format should
be %x not %lx I think:
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index a6c69a706fd7..352ef5e1c615 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -308,7 +308,7 @@ static void gen6_check_faults(struct intel_gt *gt)
fault = GEN6_RING_FAULT_REG_READ(engine);
if (fault & RING_FAULT_VALID) {
gt_dbg(gt, "Unexpected fault\n"
- "\tAddr: 0x%08lx\n"
+ "\tAddr: 0x%08x\n"
"\tAddress space: %s\n"
"\tSource ID: %d\n"
"\tType: %d\n",
Christophe
