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

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

On 3/2/26 09:54, Yuvraj Sakshith wrote:
> On Mon, Mar 02, 2026 at 09:09:13AM +0100, David Hildenbrand (Arm) wrote:
>> On 3/2/26 09:00, Yuvraj Sakshith wrote:
>>> Option 1:
>>>
>>> if (page_reporting_order == PAGE_REPORTING_DEFAULT_ORDER) {
>>>         if (page_reporting_order != PAGE_REPORTING_DEFAULT_ORDER
>>>                 && prdev->order <= MAX_PAGE_ORDER) {
>>>                 page_reporting_order = prdev->order;
>>>         } else {
>>>                 page_reporting_order = pageblock_order;
>>>         }
>>> }
>>>
>>> Option 2:
>>>
>>> if (page_reporting_order == PAGE_REPORTING_ORDER_NOT_SET) {
>>>         if (page_reporting_order != PAGE_REPORTING_DEFAULT_ORDER
>>>                 && prdev->order <= MAX_PAGE_ORDER) {
>>>                 page_reporting_order = prdev->order;
>>>         } else {
>>>                 page_reporting_order = pageblock_order;
>>>         }
>>> }
>>>
>>>
>>>
>>> Agreed.
>>>
>>> If we were to read this code without context, wouldn't it be confusing as to
>>> why PAGE_REPORTING_DEFAULT_ORDER is being checked in the first place?
>>
>> I proposed in one of the last mail that
>> "PAGE_REPORTING_USE_DEFAULT_ORDER" could be clearer, stating that it's
>> not really an order just yet. Maybe just using
>> PAGE_REPORTING_ORDER_UNSET might be clearer.
>>
> Ok
>>>
>>> Option 1 checks if page_reporting_order is equal to PAGE_REPORTING_DEFAULT_ORDER
>>> and then immediately checks if its not equal to it. Which is a bit confusing..
>>
>>
>> Because it's wrong? :) We're not supposed to check page_reporting_order
>> a second time. Assume we
>> s/PAGE_REPORTING_ORDER/PAGE_REPORTING_ORDER_UNSET/ and actually check
>> prdev->order:
> Oops, typo :) I meant prdev->order.
>>
>> if (page_reporting_order == PAGE_REPORTING_ORDER_UNSET) {
>> 	if (prdev->order != PAGE_REPORTING_ORDER_UNSET &&
>> 	    prdev->order <= MAX_PAGE_ORDER) {
>> 		page_reporting_order = prdev->order;
>> 	} else {
>> 		page_reporting_order = pageblock_order;
>> 	}
>> }
>>
> Great. Much more clearer on page_reporting.c 's end. 
> 
> Don't you think on the driver's end:
> 
> prdev->order = PAGE_REPORTING_USE_DEFAULT; looks clearer? As compared to:
> prdev->order = PAGE_REPORTING_ORDER_UNSET; ?
> 
> I'm thinking, why would a driver worry about page_reporting_order being set/unset?

Maybe PAGE_REPORTING_ORDER_UNSPECIFIED ?

In any case, we should use a single flag for this. Everything else will
be confusing once drivers could use only one of them.

-- 
Cheers,

David