On 3/2/26 06:25, Michael Kelley wrote: > From: Yuvraj Sakshith <[email protected]> Sent: Sunday, March > 1, 2026 7:33 PM >> >> On Fri, Feb 27, 2026 at 09:50:15PM +0100, David Hildenbrand (Arm) wrote: >>> >>> No need for the (). >>> >>> Wondering whether we now also want to do in this patch: >>> >>> >>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >>> index f0042d5743af..d432aadf9d07 100644 >>> --- a/mm/page_reporting.c >>> +++ b/mm/page_reporting.c >>> @@ -11,8 +11,7 @@ >>> #include "page_reporting.h" >>> #include "internal.h" >>> >>> -/* Initialize to an unsupported value */ >>> -unsigned int page_reporting_order = -1; >>> +unsigned int page_reporting_order = PAGE_REPORTING_DEFAULT_ORDER; >>> >>> static int page_order_update_notify(const char *val, const struct >>> kernel_param *kp) >>> { >>> @@ -369,7 +368,7 @@ int page_reporting_register(struct >>> page_reporting_dev_info *prdev) >>> * pageblock_order. >>> */ >>> >>> - if (page_reporting_order == -1) { >>> + if (page_reporting_order == PAGE_REPORTING_DEFAULT_ORDER) { >>> >>> >> >> Sure. Now that I think of it, don’t you think the first nested if() will >> always be false? and can be compressed down to just one if()? > > I don't think what you propose is correct. The purpose of testing > page_reporting_order for -1 is to see if a page reporting order has > been specified on the kernel boot line. If it has been specified, then > the page reporting order specified in the call to page_reporting_register() > [either a specific value or the default] is ignored and the kernel boot > line value prevails. But if page_reporting_order is -1 here, then > no kernel boot line value was specified, and the value passed to > page_reporting_register() should prevail. > > With this in mind, substituting PAGE_REPORTING_DEFAULT_ORDER > for the -1 in the test doesn’t exactly make sense to me. The -1 in the > test doesn't have quite the same meaning as the -1 for > PAGE_REPORTING_DEFAULT_ORDER. You could even use -2 for > the initial value of page_reporting_order, and here in the test, in > order to make that distinction obvious. Or use a separate symbolic > name like PAGE_REPORTING_ORDER_NOT_SET.
I don't really see a difference between "PAGE_REPORTING_DEFAULT_ORDER" and "PAGE_REPORTING_ORDER_NOT_SET" that would warrant a split and adding confusion for the page-reporting drivers. In both cases, we want "no special requirement, just use the default". Maybe we can use a better name to express that. -- Cheers, David
