On 2020-10-10 10:15 PM, Dan Carpenter wrote:
> Hello Cezary Rojewski,
>
> The patch a9aa6fb3eb6c: "ASoC: Intel: catpt: Firmware loading and
> context restore" from Sep 29, 2020, leads to the following static
> checker warning:
>
> sound/soc/intel/catpt/loader.c:654 catpt_first_boot_firmware()
> warn: consider using resource_size() here
>
> sound/soc/intel/catpt/loader.c
> 638 int catpt_first_boot_firmware(struct catpt_dev *cdev)
> 639 {
> 640 struct resource *res;
> 641 int ret;
> 642
> 643 ret = catpt_boot_firmware(cdev, false);
> 644 if (ret) {
> 645 dev_err(cdev->dev, "basefw boot failed: %d\n", ret);
> 646 return ret;
> 647 }
> 648
> 649 /* restrict FW Core dump area */
> 650 __request_region(&cdev->dram, 0, 0x200, NULL, 0);
> 651 /* restrict entire area following BASE_FW - highest offset in DRAM */
> 652 for (res = cdev->dram.child; res->sibling; res = res->sibling)
> 653 ;
> 654 __request_region(&cdev->dram, res->end + 1,
> 655 cdev->dram.end - res->end, NULL, 0);
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> It's been years since I have seen one of these warnings. Back in the
> day we used have have a lot of off by one warnings because resource_size()
> is supposed to be calculated as "end - start + 1". But here we are
> calculating "dram.end - res->end" so I'm not sure if the math is correct
> or not. This is very new code so hopefully you know the answer off the
> top of your head?
>
>
Hello Dan,
Thanks for your report. However, I do not see any problem with above
code.
Let me elaborate the context so both parties are aware of what's going
on:
Host (kernel driver) is tasked with reserving entire region of DRAM past
the module (aka resource) with highest offset. Offset is unknown
upfront. It needs to be searched for. So, after all modules have their
adequate DRAM space assigned, we search for the very last module. Its
'end + 1' marks the beginning of DSP-reserved region, that is one that
DSP firmware will be making use of internally. Host needs to reserve it
early so further reservations (done e.g.: on audio stream open) won't
target said region. 'dram.end' on the other hand marks the end of the
entire block - past that point IRAM begins. Area in-between is what we
need to flag as already in-use.
Some basic math/example:
dram.start = 0x0
dram.end = 0xAFFFF
resource_size(dram) = 0xAFFFF - 0x0 + 1 = 0xB0000
found 'res':
res.end = 0x9FFFF
area to be reserved:
target.start = res.end + 1 = 0xA0000
target.end = dram.end = 0xAFFFF
resource_size(target) = 0xAFFFF - 0xA0000 + 1 = 0x10000
and so:
dram.end - res.end = 0xAFFFF - 0x9FFFF = 0x10000
what equals resource_size(target).
Please, let me know if I'm missing something here.
Thanks,
Czarek
