RE: [bug report] ASoC: Intel: catpt: Firmware loading and context restore

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

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