Re: [PATCH v1 4/4] page_reporting: change PAGE_REPORTING_DEFAULT_ORDER to -1

["David Hildenbrand (Arm)" <david@kernel.org>]

On 3/2/26 06:25, Michael Kelley wrote:
> From: Yuvraj Sakshith <yuvraj.sakshith@oss.qualcomm.com> 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