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
