[PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality

[Guenter Roeck]

On 5/19/25 04:34, Alexey Charkov wrote:
> On Sun, May 18, 2025 at 5:24 AM kernel test robot <lkp at intel.com> wrote:
>>
>> Hi Alexey,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
>> base:   92a09c47464d040866cf2b4cd052bc60555185fb
>> patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-3-2197a1b062bd%40gmail.com
>> patch subject: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
>> config: loongarch-randconfig-r123-20250517 (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/config)
>> compiler: loongarch64-linux-gcc (GCC) 14.2.0
>> reproduce: (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp at intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202505180911.hDevFA1N-lkp@intel.com/
>>
>> sparse warnings: (new ones prefixed by >>)
>>>> drivers/clocksource/timer-vt8500.c:201:51: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *platform_data @@     got void [noderef] __iomem *static [assigned] [toplevel] regbase @@
>>     drivers/clocksource/timer-vt8500.c:201:51: sparse:     expected void *platform_data
>>     drivers/clocksource/timer-vt8500.c:201:51: sparse:     got void [noderef] __iomem *static [assigned] [toplevel] regbase
>>
>> vim +201 drivers/clocksource/timer-vt8500.c
>>
>>     175
>>     176  /*
>>     177   * This probe gets called after the timer is already up and running. This will create
>>     178   * the watchdog device as a child since the registers are shared.
>>     179   */
>>     180  static int vt8500_timer_probe(struct platform_device *pdev)
>>     181  {
>>     182          struct platform_device *vt8500_watchdog_device;
>>     183          struct device *dev = &pdev->dev;
>>     184          int ret;
>>     185
>>     186          if (!sys_timer_ch) {
>>     187                  dev_info(dev, "Not enabling watchdog: only one irq was given");
>>     188                  return 0;
>>     189          }
>>     190
>>     191          if (!regbase)
>>     192                  return dev_err_probe(dev, -ENOMEM,
>>     193                          "Timer not initialized, cannot create watchdog");
>>     194
>>     195          vt8500_watchdog_device = platform_device_alloc("vt8500-wdt", -1);
>>     196          if (!vt8500_watchdog_device)
>>     197                  return dev_err_probe(dev, -ENOMEM,
>>     198                          "Failed to allocate vt8500-wdt");
>>     199
>>     200          /* Pass the base address as platform data and nothing else */
>>   > 201          vt8500_watchdog_device->dev.platform_data = regbase;
> 
> Frankly, given that this driver only applies to VT8500 (which is ARM
> based), the warning appears a bit overzealous. After all, on ARM MMIO
> addresses are in the same physical address space as normal memory
> addresses, and furthermore this platform_data is never dereferenced
> directly anyway.

Guess we'll need AI compilers in the future to help them know that.
I for my part would argue that "this warning can be ignored" is the
source of many problems flying under the radar.

> 
> I could silence the warning either by more aggressive casting or by
> wrapping the pointer into some struct, but both of those sound a bit
> overreaching. Would appreciate guidance from the list on how to best
> approach this.
> 

First of all, I am quite sure that using platform drivers for this is the
wrong approach to start with. This seems to be a perfect candidate for
an auxiliary driver.

Second, I do consider passing an iomem pointer as platform data to be
inherently unsafe. I would very much prefer either passing a regmap
pointer or, if that doesn't work, a data structure.

Guenter