Re: [linux-next:master 4897/5417] drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: sparse: incorrect type in argument 1 (different address spaces)

From: "Jan Dąbroś" <jsd@semihalf.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: kernel test robot <lkp@intel.com>,
	kbuild-all@lists.01.org,
	 Linux Memory Management List <linux-mm@kvack.org>,
	Wolfram Sang <wsa-dev@sang-engineering.com>
Subject: Re: [linux-next:master 4897/5417] drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: sparse: incorrect type in argument 1 (different address spaces)
Date: Wed, 16 Feb 2022 15:06:19 +0100	[thread overview]
Message-ID: <CAOtMz3OraM9uyRPNW7++r25LrD5-pV6-mJFg3Ct7NagdTOscwg@mail.gmail.com> (raw)
In-Reply-To: <YgpqrB2mX8gND9SP@smile.fi.intel.com>

pon., 14 lut 2022 o 15:44 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Mon, Feb 14, 2022 at 02:59:34PM +0100, Jan Dąbroś wrote:
> > pon., 14 lut 2022 o 14:28 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > >
> > > On Mon, Feb 14, 2022 at 01:27:35PM +0100, Jan Dąbroś wrote:
> > > > pt., 11 lut 2022 o 22:24 kernel test robot <lkp@intel.com> napisał(a):
> > >
> > > > >    159
> > > > >    160  /* Helper to verify status returned by PSP */
> > > > >    161  static int check_i2c_req_sts(struct psp_i2c_req *req)
> > > > >    162  {
> > > > >    163          int status;
> > > > >    164
> > > > >  > 165          status = readl(&req->hdr.status);
> > > >
> > > > Actually the above error points to something hidden but important -
> > > > for reading from command-response buffer, we shouldn't use __iomem
> > > > specifier (nor readl() family of functions) since this is normal
> > > > memory - however updated by PSP. Thus I will refactor this to use
> > > > 'volatile u32 *' and reading status by de-referencing pointer.
> > >
> > > Not sure volatile is a good idea. Perhaps READ_ONCE() is what you need.
> > > Is this a system memory?
> >
> > Yes, this is system memory.
> >
> > Actually looking at asm-generic/rwonce.h:
> > #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
> > it is more-less based on volatile, so that compiler will not be able
> > to (among others) optimize out such reads of memory which may be
> > changed outside of the scope of "program".
> >
> > I believe that I will get the same outcome from using READ_ONCE and
> > explicit volatile, is the first way preferred in the kernel?
>
> READ_ONCE() may be different on different arches. I believe that's why
> it's preferred.

OK, I see. Let me send a commit with this change this week.

Best Regards,
Jan

>
> --
> With Best Regards,
> Andy Shevchenko
>
>