Message-ID: <CABjd4YyXwznntcLVcYL6qx16YEwv4_VWzrXrE7_QHmQxiE0pXQ@mail.gmail.com>
Date: Tue, 20 May 2025 21:39:15 +0400
From: Alexey Charkov <alchark@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: kernel test robot <lkp@...el.com>, Krzysztof Kozlowski <krzk@...nel.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Wim Van Sebroeck <wim@...ux-watchdog.org>, oe-kbuild-all@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for
 watchdog functionality

On Mon, May 19, 2025 at 5:34 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 5/19/25 04:34, Alexey Charkov wrote:
> > On Sun, May 18, 2025 at 5:24 AM kernel test robot <lkp@...el.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@...el.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.

TIL: auxiliary bus :)

Thanks for the pointer Guenter, it does indeed look like a more
appropriate choice. I'll try and port the driver to use that instead,
and resubmit.

> 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.

I guess it resolves itself with the auxiliary driver approach, as I
can then just upcast the auxiliary device pointer to the parent's
enclosing private struct, which can then contain both a timer read
function and the specific pointers to the two registers the watchdog
needs. No need then for the child to do its arbitrary offsets into the
parent's iomem region - just use what's given directly. It's still
going to be iomem pointer access, but on the other hand putting a
layer of indirection (i.e. regmap) into the system timer code sounds a
bit scary to me.

Best regards,
Alexey

Powered by blists - more mailing lists