Re: [PATCH RFC 09/15] arm_mpam: Fix MSC MMIO window size to use resource_size() instead of end - start

[Yin Li <yin.li@oss.qualcomm.com>]

On 9/9/2026 6:11 PM, Ben Horgan wrote:
> Hi Yin,
> 
> On 09/09/2026 10:25, Yin Li wrote:
>>
>>
>> On 9/4/2026 11:05 AM, Yin Li wrote:
>>>
>>>
>>> On 9/3/2026 9:23 PM, Ben Horgan wrote:
>>>> Hi Yin,
>>>>
>>>> On 03/09/2026 11:20, Ben Horgan wrote:
>>>>> Hi Yin,
>>>>>
>>>>> On 11/08/2026 14:30, Yin Li wrote:
>>>>>> struct resource uses an inclusive end address, so the correct size is
>>>>>> end - start + 1. The previous calculation of end - start was off by one,
>>>>>> resulting in a mapped window one byte smaller than the actual resource.
>>>>>> Use resource_size() which correctly computes end - start + 1.
>>>>>>
>>>>>> Signed-off-by: Yin Li <yin.li@oss.qualcomm.com>
>>>>>
>>>>> I just got a kernel ci report for this one which asks for tags:
>>>>>
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202609030809.ObirDhR3- lkp@intel.com/
>>>>>
>>>>> It doesn't look to give a useful fixes tag though. I'd go with this as that's where the error was
>>>>> introduced.
>>>>>
>>>>> Fixes: f04046f2577a ("arm_mpam: Add probe/remove for mpam msc driver and kbuild boiler plate")
>>>>>
>>>>> Looks good to me.
>>>>>
>>>>> Reviewed-by: Ben Horgan <ben.horgan@arm.com>
>>>>
>>>> Scratch that. As Ilpo points out,[1], there is no functional bug but just some misleading naming
>>>> which never the less would be good to fix. This does require > in the warnings becoming >= though
>>>> and there would be no need for fixes tag. Do you agree with this analysis?
>>>>
>>>> Thanks,
>>>>
>>>> Ben
>>>>
>>>> [1]
>>>> https://lore.kernel.org/ lkml/03055fbc-281f-4ed9-9282-4853d560e17f@arm.com/T/
>>>> #mdb57d40c888ff4ce656a9d9a00ecf5d99466530c
>>>>
>>>>
>>>
>>> Hi Ben,
>>>
>>> Thanks, and thanks to Ilpo for the detailed analysis.
>>>
>>> Agreed — the current code is functionally correct because the off-by-one
>>> in "end - start" is cancelled out by the ">" checks, since
>>> mapped_hwpage_sz effectively holds the last mapped byte rather than the
>>> size. So there's no functional bug and no Fixes tag is needed.
>>>
>>> I'll update the patch to switch to resource_size() and change the
>>> corresponding ">" checks to ">=" together, so the naming becomes
>>> accurate while keeping the behaviour unchanged. I'll also reword the
>>> commit message to describe this as a naming/readability cleanup rather
>>> than a bugfix.
>>>
>>>
>>
>> Hi Ben,
>>
>> I looked into this more closely, and I think the situation is a bit
>> different from previous analysis — could you and Ilpo double-check?
>>
>> All three bounds checks have the access width included on the left-hand
>> side, e.g.:
>>
>>        WARN_ON_ONCE(reg + sizeof(u32) > msc->mapped_hwpage_sz);
>>
>> Here "reg + sizeof(u32)" is the one-past-the-end offset of the write, so
>> a legal access needs "reg + 4 <= size", i.e. the out-of-bounds condition
>> is correctly "> size".
>>
>> Take a 0x1000-sized window (valid offsets 0x000..0xFFF):
>>
>>      - With the old value, mapped_hwpage_sz = end - start = 0xFFF (size - 1).
>>      A write at reg = 0xFFC touches bytes 0xFFC..0xFFF — exactly the last
>>      4 bytes, which is legal. But the check computes 0xFFC + 4 = 0x1000 >
>>      0xFFF → true, so it falsely warns on a valid access. That's a real
>>      off-by-one.
>>
>>      - With resource_size() = 0x1000 (true size), the same access gives
>>      0x1000 > 0x1000 → false, so it correctly passes, while an access at
>>      reg = 0xFFD (0x1001 > 0x1000 → true) is correctly rejected.
>>
>> So resource_size() fixes a genuine off-by-one here, and the ">" checks
>> should stay as ">". Changing them to ">=" would break the reg = 0xFFC
>> case (0x1000 >= 0x1000 → true) and wrongly reject a legal access.
>>
>> So for the next version I plan to keep the resource_size() change, keep
>> the ">" checks unchanged, and describe it as an off-by-one fix rather
>> than a naming cleanup. Does this match what you and Ilpo see?
> 
> Yes. Thank you very much for taking the time and looking into this more closely.
> It looks like I introduced the functional bug whilst upstreaming James' patches.
>

Hi Ben,

Glad we got to the bottom of it. For the next version I'll treat it as a 
proper off-by-one fix: keep the ">" checks unchanged, use 
resource_size(), and add the tags:

Fixes: f04046f2577a ("arm_mpam: Add probe/remove for mpam msc driver and 
kbuild boiler plate")
Reported-by: kernel test robot <lkp@intel.com>
Closes: 
https://lore.kernel.org/oe-kbuild-all/202609030809.ObirDhR3-lkp@intel.com/

Along with your Reviewed-by.

Thanks,
Yin

> Ben
> 
>>
>> Best regards,
>> Yin Li
>>
>>
>>
>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Ben
>>>>>
>>>>>> ---
>>>>>>    drivers/resctrl/mpam_devices.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/ mpam_devices.c
>>>>>> index 1e082fb60e30..5d1854d97371 100644
>>>>>> --- a/drivers/resctrl/mpam_devices.c
>>>>>> +++ b/drivers/resctrl/mpam_devices.c
>>>>>> @@ -2296,7 +2296,7 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>>>>>>                dev_err_once(dev, "Failed to map MSC base address\n");
>>>>>>                return ERR_CAST(io);
>>>>>>            }
>>>>>> -        msc->mapped_hwpage_sz = msc_res->end - msc_res->start;
>>>>>> +        msc->mapped_hwpage_sz = resource_size(msc_res);
>>>>>>            msc->mapped_hwpage = io;
>>>>>>        } else {
>>>>>>            return ERR_PTR(-EINVAL);
>>>>>>
>>>>>
>>>>
>>>
>>
> 

-- 
Thx and BRs,
Yin