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

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

[Dan Carpenter]

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?

   656  
   657          ret = catpt_ipc_get_mixer_stream_info(cdev, &cdev->mixer);
   658          if (ret)
   659                  return CATPT_IPC_ERROR(ret);
   660  
   661          ret = catpt_arm_stream_templates(cdev);
   662          if (ret) {
   663                  dev_err(cdev->dev, "arm templates failed: %d\n", ret);
   664                  return ret;
   665          }
   666  
   667          /* update dram pg for scratch and restricted regions */
   668          catpt_dsp_update_srampge(cdev, &cdev->dram, cdev->spec->dram_mask);
   669  
   670          return 0;
   671  }

regards,
dan carpenter

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

[Rojewski, Cezary]

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