[linux-next:master 4897/5417] drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: sparse: incorrect type in argument 1 (different address spaces)
8:24 a.m.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: 6d9bd4ad4ca08b1114e814c2c42383b8b13be631
commit: 91560fe37f81ba8145427477a39cea88f4422385 [4897/5417] i2c: designware: Add AMD PSP
I2C bus support
config: i386-allyesconfig
(https://download.01.org/0day-ci/archive/20220212/202202120520.NbWJGvF2-lk...)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-dirty
#
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git fetch --no-tags linux-next master
git checkout 91560fe37f81ba8145427477a39cea88f4422385
# save the config file to linux build tree
mkdir build_dir
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir
ARCH=i386 SHELL=/bin/bash drivers/i2c/busses/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
sparse warnings: (new ones prefixed by >>)
> drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse:
sparse: incorrect type in argument 1 (different address spaces) @@ expected void const
volatile [noderef] __iomem *addr @@ got unsigned int * @@
drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: expected void const
volatile [noderef] __iomem *addr
drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: got unsigned int *
vim +165 drivers/i2c/busses/i2c-designware-amdpsp.c
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);
166
167 switch (status) {
168 case PSP_I2C_REQ_STS_OK:
169 return 0;
170 case PSP_I2C_REQ_STS_BUS_BUSY:
171 return -EBUSY;
172 case PSP_I2C_REQ_STS_INV_PARAM:
173 default:
174 return -EIO;
175 };
176 }
177
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
11:27 p.m.
pt., 11 lut 2022 o 22:24 kernel test robot <lkp(a)intel.com> napisał(a):
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: 6d9bd4ad4ca08b1114e814c2c42383b8b13be631
commit: 91560fe37f81ba8145427477a39cea88f4422385 [4897/5417] i2c: designware: Add AMD PSP
I2C bus support
config: i386-allyesconfig
(https://download.01.org/0day-ci/archive/20220212/202202120520.NbWJGvF2-lk...)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-dirty
#
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git fetch --no-tags linux-next master
git checkout 91560fe37f81ba8145427477a39cea88f4422385
# save the config file to linux build tree
mkdir build_dir
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir
ARCH=i386 SHELL=/bin/bash drivers/i2c/busses/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: sparse: incorrect
type in argument 1 (different address spaces) @@ expected void const volatile
[noderef] __iomem *addr @@ got unsigned int * @@
drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: expected void const
volatile [noderef] __iomem *addr
drivers/i2c/busses/i2c-designware-amdpsp.c:165:25: sparse: got unsigned int *
vim +165 drivers/i2c/busses/i2c-designware-amdpsp.c
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.
Best Regards,
Jan
> 166
> 167 switch (status) {
> 168 case PSP_I2C_REQ_STS_OK:
> 169 return 0;
> 170 case PSP_I2C_REQ_STS_BUS_BUSY:
> 171 return -EBUSY;
> 172 case PSP_I2C_REQ_STS_INV_PARAM:
> 173 default:
> 174 return -EIO;
> 175 };
> 176 }
> 177
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
12:27 a.m.
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(a)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?
--
With Best Regards,
Andy Shevchenko
12:59 a.m.
pon., 14 lut 2022 o 14:28 Andy Shevchenko
<andriy.shevchenko(a)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(a)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?
Best Regards,
Jan
1:43 a.m.
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(a)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(a)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.
--
With Best Regards,
Andy Shevchenko
1:06 a.m.
pon., 14 lut 2022 o 15:44 Andy Shevchenko
<andriy.shevchenko(a)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(a)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(a)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
>
>
216
days inactive
221
days old
5 comments
3 participants
participants (3)
-
Andy Shevchenko -
Jan Dąbroś -
kernel test robot