arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   74f602dc96dd854c7b2034947798c1e2a6b84066
commit: 68c5debcc06d6d24f15dbf978780fc5efc147d5e arm64: implement CPPC FFH support using AMUs
date:   5 weeks ago
config: arm64-randconfig-s032-20201217 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-184-g1b896707-dirty
        # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68c5debcc06d6d24f15dbf978780fc5efc147d5e
        git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
        git fetch --no-tags linus master
        git checkout 68c5debcc06d6d24f15dbf978780fc5efc147d5e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

vim +367 arch/arm64/kernel/topology.c

   362	
   363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
   364	{
   365		int ret = -EOPNOTSUPP;
   366	
 > 367		switch ((u64)reg->address) {
   368		case 0x0:
   369			ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
   370			break;
   371		case 0x1:
   372			ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
   373			break;
   374		}
   375	
   376		if (!ret) {
   377			*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
   378					    reg->bit_offset);
   379			*val >>= reg->bit_offset;
   380		}
   381	
   382		return ret;
   383	}
   384	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33134 bytes --]

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Catalin Marinas]

On Fri, Dec 18, 2020 at 05:00:16AM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   74f602dc96dd854c7b2034947798c1e2a6b84066
> commit: 68c5debcc06d6d24f15dbf978780fc5efc147d5e arm64: implement CPPC FFH support using AMUs
> date:   5 weeks ago
> config: arm64-randconfig-s032-20201217 (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 9.3.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # apt-get install sparse
>         # sparse version: v0.6.3-184-g1b896707-dirty
>         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68c5debcc06d6d24f15dbf978780fc5efc147d5e
>         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>         git fetch --no-tags linus master
>         git checkout 68c5debcc06d6d24f15dbf978780fc5efc147d5e
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> "sparse warnings: (new ones prefixed by >>)"
> >> arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression
> 
> vim +367 arch/arm64/kernel/topology.c
> 
>    362	
>    363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
>    364	{
>    365		int ret = -EOPNOTSUPP;
>    366	
>  > 367		switch ((u64)reg->address) {

That's not a dereference but I guess sparse complains of dropping the
__iomem. We could change the cast to (__force u64) to silence sparse.

Thanks for the report.

-- 
Catalin

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Ionela Voinescu]

Hi,

On Friday 18 Dec 2020 at 10:44:10 (+0000), Catalin Marinas wrote:
> On Fri, Dec 18, 2020 at 05:00:16AM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   74f602dc96dd854c7b2034947798c1e2a6b84066
> > commit: 68c5debcc06d6d24f15dbf978780fc5efc147d5e arm64: implement CPPC FFH support using AMUs
> > date:   5 weeks ago
> > config: arm64-randconfig-s032-20201217 (attached as .config)
> > compiler: aarch64-linux-gcc (GCC) 9.3.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # apt-get install sparse
> >         # sparse version: v0.6.3-184-g1b896707-dirty
> >         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68c5debcc06d6d24f15dbf978780fc5efc147d5e
> >         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >         git fetch --no-tags linus master
> >         git checkout 68c5debcc06d6d24f15dbf978780fc5efc147d5e
> >         # save the attached .config to linux build tree
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > 
> > "sparse warnings: (new ones prefixed by >>)"
> > >> arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression
> > 
> > vim +367 arch/arm64/kernel/topology.c
> > 
> >    362	
> >    363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> >    364	{
> >    365		int ret = -EOPNOTSUPP;
> >    366	
> >  > 367		switch ((u64)reg->address) {
> 
> That's not a dereference but I guess sparse complains of dropping the
> __iomem. We could change the cast to (__force u64) to silence sparse.
> 
> Thanks for the report.
> 

Nothing I've tried seemed to silence sparse here, including casting to
(__force u64). I think all error checks in the kernel for __iomem
addresses result in the same warning, or at least the ones in
cppc_acpi.c, which I've checked at the time. I'm not sure if this is
something that should be improved in sparse or that can be made better
in the kernel. I'll take another look.

Thank you,
Ionela.

> -- 
> Catalin

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Catalin Marinas]

On Wed, Jan 06, 2021 at 03:07:24PM +0000, Ionela Voinescu wrote:
> On Friday 18 Dec 2020 at 10:44:10 (+0000), Catalin Marinas wrote:
> > On Fri, Dec 18, 2020 at 05:00:16AM +0800, kernel test robot wrote:
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head:   74f602dc96dd854c7b2034947798c1e2a6b84066
> > > commit: 68c5debcc06d6d24f15dbf978780fc5efc147d5e arm64: implement CPPC FFH support using AMUs
> > > date:   5 weeks ago
> > > config: arm64-randconfig-s032-20201217 (attached as .config)
> > > compiler: aarch64-linux-gcc (GCC) 9.3.0
> > > reproduce:
> > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # apt-get install sparse
> > >         # sparse version: v0.6.3-184-g1b896707-dirty
> > >         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68c5debcc06d6d24f15dbf978780fc5efc147d5e
> > >         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > >         git fetch --no-tags linus master
> > >         git checkout 68c5debcc06d6d24f15dbf978780fc5efc147d5e
> > >         # save the attached .config to linux build tree
> > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 
> > > 
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > 
> > > 
> > > "sparse warnings: (new ones prefixed by >>)"
> > > >> arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression
> > > 
> > > vim +367 arch/arm64/kernel/topology.c
> > > 
> > >    362	
> > >    363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> > >    364	{
> > >    365		int ret = -EOPNOTSUPP;
> > >    366	
> > >  > 367		switch ((u64)reg->address) {
> > 
> > That's not a dereference but I guess sparse complains of dropping the
> > __iomem. We could change the cast to (__force u64) to silence sparse.
> > 
> > Thanks for the report.
> > 
> 
> Nothing I've tried seemed to silence sparse here, including casting to
> (__force u64).

Would it work if we changed the case lines to (u64 __iomem)0x0?

-- 
Catalin

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Ionela Voinescu]

On Wednesday 06 Jan 2021 at 15:21:19 (+0000), Catalin Marinas wrote:
> On Wed, Jan 06, 2021 at 03:07:24PM +0000, Ionela Voinescu wrote:
> > On Friday 18 Dec 2020 at 10:44:10 (+0000), Catalin Marinas wrote:
> > > On Fri, Dec 18, 2020 at 05:00:16AM +0800, kernel test robot wrote:
> > > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > head:   74f602dc96dd854c7b2034947798c1e2a6b84066
> > > > commit: 68c5debcc06d6d24f15dbf978780fc5efc147d5e arm64: implement CPPC FFH support using AMUs
> > > > date:   5 weeks ago
> > > > config: arm64-randconfig-s032-20201217 (attached as .config)
> > > > compiler: aarch64-linux-gcc (GCC) 9.3.0
> > > > reproduce:
> > > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         # apt-get install sparse
> > > >         # sparse version: v0.6.3-184-g1b896707-dirty
> > > >         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68c5debcc06d6d24f15dbf978780fc5efc147d5e
> > > >         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > >         git fetch --no-tags linus master
> > > >         git checkout 68c5debcc06d6d24f15dbf978780fc5efc147d5e
> > > >         # save the attached .config to linux build tree
> > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 
> > > > 
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > 
> > > > 
> > > > "sparse warnings: (new ones prefixed by >>)"
> > > > >> arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression
> > > > 
> > > > vim +367 arch/arm64/kernel/topology.c
> > > > 
> > > >    362	
> > > >    363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> > > >    364	{
> > > >    365		int ret = -EOPNOTSUPP;
> > > >    366	
> > > >  > 367		switch ((u64)reg->address) {
> > > 
> > > That's not a dereference but I guess sparse complains of dropping the
> > > __iomem. We could change the cast to (__force u64) to silence sparse.
> > > 
> > > Thanks for the report.
> > > 
> > 
> > Nothing I've tried seemed to silence sparse here, including casting to
> > (__force u64).
> 
> Would it work if we changed the case lines to (u64 __iomem)0x0?
> 

No, it does not. We still get the same warning on the switch line even
if there is no cast. Same if we directly check for:

if (reg->address == (u64 __iomem)0x0)

Ionela.

> -- 
> Catalin

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Al Viro]

On Wed, Jan 06, 2021 at 03:52:14PM +0000, Ionela Voinescu wrote:
> > > > > vim +367 arch/arm64/kernel/topology.c
> > > > > 
> > > > >    362	
> > > > >    363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> > > > >    364	{
> > > > >    365		int ret = -EOPNOTSUPP;
> > > > >    366	
> > > > >  > 367		switch ((u64)reg->address) {
> > > > 
> > > > That's not a dereference but I guess sparse complains of dropping the
> > > > __iomem. We could change the cast to (__force u64) to silence sparse.
> > > > 
> > > > Thanks for the report.
> > > > 
> > > 
> > > Nothing I've tried seemed to silence sparse here, including casting to
> > > (__force u64).
> > 
> > Would it work if we changed the case lines to (u64 __iomem)0x0?
> > 
> 
> No, it does not. We still get the same warning on the switch line even
> if there is no cast. Same if we directly check for:
> 
> if (reg->address == (u64 __iomem)0x0)

Folks, could you stop with the voodoo?  This u64 __iomem address thing is completely
wrong.  What it says is "address of that field shall be an iomem pointer",
which makes no sense whatsoever.

Just what had been intended?  __iomem is a qualifier of the same sort
as const or volatile - this mess makes as much sense as
struct cpc_reg {
        u8 descriptor;
        u16 length;
        u8 space_id;   
        u8 bit_width;   
        u8 bit_offset;
        u8 access_width;
        u64 const address;
} __packed;

Which would *NOT* be read as "reg->address is a numeric representation of
address of something unmodifiable" - it would be "the value stored in
reg->address can not be modified".

This annotation says "reg->address (somehow) lives in iomem", resulting in
"so why the hell are you trying to read it by plain dereferencing of
reg + field offset?" from sparse.

Get rid of this misannotation and don't breed force-cast to confuse
everything hard enough to STFU.

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Ionela Voinescu]

Hi,

On Wednesday 06 Jan 2021 at 16:13:53 (+0000), Al Viro wrote:
> On Wed, Jan 06, 2021 at 03:52:14PM +0000, Ionela Voinescu wrote:
> > > > > > vim +367 arch/arm64/kernel/topology.c
> > > > > > 
> > > > > >    362	
> > > > > >    363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> > > > > >    364	{
> > > > > >    365		int ret = -EOPNOTSUPP;
> > > > > >    366	
> > > > > >  > 367		switch ((u64)reg->address) {
> > > > > 
> > > > > That's not a dereference but I guess sparse complains of dropping the
> > > > > __iomem. We could change the cast to (__force u64) to silence sparse.
> > > > > 
> > > > > Thanks for the report.
> > > > > 
> > > > 
> > > > Nothing I've tried seemed to silence sparse here, including casting to
> > > > (__force u64).
> > > 
> > > Would it work if we changed the case lines to (u64 __iomem)0x0?
> > > 
> > 
> > No, it does not. We still get the same warning on the switch line even
> > if there is no cast. Same if we directly check for:
> > 
> > if (reg->address == (u64 __iomem)0x0)
> 
> Folks, could you stop with the voodoo?  This u64 __iomem address thing is completely
> wrong.  What it says is "address of that field shall be an iomem pointer",
> which makes no sense whatsoever.
> 
> Just what had been intended?  __iomem is a qualifier of the same sort
> as const or volatile - this mess makes as much sense as
> struct cpc_reg {
>         u8 descriptor;
>         u16 length;
>         u8 space_id;   
>         u8 bit_width;   
>         u8 bit_offset;
>         u8 access_width;
>         u64 const address;
> } __packed;
> 
> Which would *NOT* be read as "reg->address is a numeric representation of
> address of something unmodifiable" - it would be "the value stored in
> reg->address can not be modified".
> 
> This annotation says "reg->address (somehow) lives in iomem", resulting in
> "so why the hell are you trying to read it by plain dereferencing of
> reg + field offset?" from sparse.
> 
> Get rid of this misannotation and don't breed force-cast to confuse
> everything hard enough to STFU.

Thanks, it makes sense, and removing the attribute solves the other
similar warnings in cppc_acpi. I'll double check and submit a patch for
that.

Thanks,
Ionela.

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Al Viro]

On Wed, Jan 06, 2021 at 03:07:24PM +0000, Ionela Voinescu wrote:

> > >  > 367		switch ((u64)reg->address) {
> > 
> > That's not a dereference but I guess sparse complains of dropping the
> > __iomem. We could change the cast to (__force u64) to silence sparse.

Oh, yes, it is - that of &reg->address, to fetch the value you are
casting to u64.  And nonsense in declaration of struct cpc_reg says
that its 'address' field somehow manages to be located in iomem,
regardless of where the entire structure is stored.

Qualifiers apply to lvalues - it's "how can that object be accessed".
They don't say anything with the values _stored_ in that object.
It is possible to have them applied to individual fields of a structure;
for some qualifiers that might be legitimate - e.g. you could do
struct foo {
	char *s;
	volatile int x;
} *p;
telling the compiler that p->x is to be treated as volatile (make no
assumptions about the value not being changed behind your back, etc.),
while p->s is not.

However, for __iomem (or __user, etc.) that makes no sense whatsoever;
you are saying "this field lives in iomem, no matter where the entire
structure is located".

To quote C99 6.3.2.1[2]:
	Except when it is the operand of the sizeof operator, the unary & operator, the ++
operator, the -- operator, or the left operand of the . operator or an assignment operator,
an lvalue that does not have array type is converted to the value stored in the designated
object (and is no longer an lvalue). If the lvalue has qualified type, the value has the
unqualified version of the type of the lvalue; otherwise, the value has the type of the
lvalue. If the lvalue has an incomplete type and does not have array type, the behavior is
undefined.

	IOW, in the example above, as lvalue p->x will have "volatile int"
for type; using it as argument of cast operator will convert it (_before_
doing the cast) to whatever integer that had been found stored
in that field and the type of that will be "int", not "volatile int".
As soon as you fetch the value stored in object, qualifiers are gone.

	The syntax is somewhat unfortunate - it's easy to confuse
qualified pointer to type with pointer to qualified type.
	const int *r
means "r is an unqualified pointer to const int"; the value stored in r may
be modified, but the value stored in *r may not.
	int * const r
means "r is a const pointer to int"; the value stored in r may not be modified,
but the value stored in *r may.

	You often run into something like
struct foo {
	...
	u64 __iomem *some_reg;
	...
} *p;
and, unlike the mess in struct cpc_reg declaration, here p->some_reg is *NOT*
__iomem-qualified.  It's a perfectly normal field of a structure somewhere
in kernel memory, it can be fetched from, stored into, etc.  The contents
of that field is a pointer to __iomem u64.  It can be passed to e.g.
readq(), but trying to directly fetch *(p->some_reg) will barf.
	In such cases the limitations apply not to how we can access the
field itself, but to what we can do with the value we find in that
field.

	At a guess, the intent of that (mis)annotation had been
"this field contains a 64bit unsigned integer that happens to contain
an address of something in iomem".  But qualifiers are useless for
that - once you've fetched that value, all you have is plain u64.
Nor would they be carried through the arithmetics, etc.

	It might be possible to cook something more useful by a bit
of creative misuse of __bitwise, but I hadn't looked through the
places where that field is used.

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Ionela Voinescu]

On Wednesday 06 Jan 2021 at 17:47:58 (+0000), Al Viro wrote:
> On Wed, Jan 06, 2021 at 03:07:24PM +0000, Ionela Voinescu wrote:
> 
> > > >  > 367		switch ((u64)reg->address) {
> > > 
> > > That's not a dereference but I guess sparse complains of dropping the
> > > __iomem. We could change the cast to (__force u64) to silence sparse.
> 
> Oh, yes, it is - that of &reg->address, to fetch the value you are
> casting to u64.  And nonsense in declaration of struct cpc_reg says
> that its 'address' field somehow manages to be located in iomem,
> regardless of where the entire structure is stored.
> 
> Qualifiers apply to lvalues - it's "how can that object be accessed".
> They don't say anything with the values _stored_ in that object.
> It is possible to have them applied to individual fields of a structure;
> for some qualifiers that might be legitimate - e.g. you could do
> struct foo {
> 	char *s;
> 	volatile int x;
> } *p;
> telling the compiler that p->x is to be treated as volatile (make no
> assumptions about the value not being changed behind your back, etc.),
> while p->s is not.
> 
> However, for __iomem (or __user, etc.) that makes no sense whatsoever;
> you are saying "this field lives in iomem, no matter where the entire
> structure is located".
> 
> To quote C99 6.3.2.1[2]:
> 	Except when it is the operand of the sizeof operator, the unary & operator, the ++
> operator, the -- operator, or the left operand of the . operator or an assignment operator,
> an lvalue that does not have array type is converted to the value stored in the designated
> object (and is no longer an lvalue). If the lvalue has qualified type, the value has the
> unqualified version of the type of the lvalue; otherwise, the value has the type of the
> lvalue. If the lvalue has an incomplete type and does not have array type, the behavior is
> undefined.
> 
> 	IOW, in the example above, as lvalue p->x will have "volatile int"
> for type; using it as argument of cast operator will convert it (_before_
> doing the cast) to whatever integer that had been found stored
> in that field and the type of that will be "int", not "volatile int".
> As soon as you fetch the value stored in object, qualifiers are gone.
> 
> 	The syntax is somewhat unfortunate - it's easy to confuse
> qualified pointer to type with pointer to qualified type.
> 	const int *r
> means "r is an unqualified pointer to const int"; the value stored in r may
> be modified, but the value stored in *r may not.
> 	int * const r
> means "r is a const pointer to int"; the value stored in r may not be modified,
> but the value stored in *r may.
> 
> 	You often run into something like
> struct foo {
> 	...
> 	u64 __iomem *some_reg;
> 	...
> } *p;
> and, unlike the mess in struct cpc_reg declaration, here p->some_reg is *NOT*
> __iomem-qualified.  It's a perfectly normal field of a structure somewhere
> in kernel memory, it can be fetched from, stored into, etc.  The contents
> of that field is a pointer to __iomem u64.  It can be passed to e.g.
> readq(), but trying to directly fetch *(p->some_reg) will barf.
> 	In such cases the limitations apply not to how we can access the
> field itself, but to what we can do with the value we find in that
> field.
> 
> 	At a guess, the intent of that (mis)annotation had been
> "this field contains a 64bit unsigned integer that happens to contain
> an address of something in iomem".  But qualifiers are useless for
> that - once you've fetched that value, all you have is plain u64.
> Nor would they be carried through the arithmetics, etc.
>

This could have been the intention, as that value is used as an offset
in the PCC virtual space (although it does not make complete sense even
in this case). Otherwise it's used as a pysical address offset for
system memory, and given as pysical address argument to ioremap :).

In any case, thank you for the detailed explanation. After your first
email I was thinking that it does not make sense to have the __iomem
annotation for address in cpc_reg, especially given its uses in
cppc_acpi.c and for the code that implements the ffh functions, but I
think only after this email it really  sunk in how wrong that
annotation really was.

Initially I though it always only makes sense to have a __iomem pointer.
That is, it only makes sense to have a pointer with a cookie attached
specifying that it addresses a device memory space that should only be
accessed using special functions.

But then you've got something like this in drivers/input/serio/apbps2.c:
struct apbps2_regs {
	u32 __iomem data;	/* 0x00 */
	u32 __iomem status;	/* 0x04 */
	u32 __iomem ctrl;	/* 0x08 */
	u32 __iomem reload;	/* 0x0c */
};
struct apbps2_priv {
	struct serio		*io;
	struct apbps2_regs	*regs;
};
[..] (followed by)
ioread32be(&priv->regs->status)

which I think is correct despite contradicting my assumption, but it's
the only example I've found in the kernel.

Many thanks,
Ionela.

> 	It might be possible to cook something more useful by a bit
> of creative misuse of __bitwise, but I hadn't looked through the
> places where that field is used.

Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[Al Viro]

On Wed, Jan 06, 2021 at 08:12:27PM +0000, Ionela Voinescu wrote:

> Initially I though it always only makes sense to have a __iomem pointer.
> That is, it only makes sense to have a pointer with a cookie attached
> specifying that it addresses a device memory space that should only be
> accessed using special functions.
> 
> But then you've got something like this in drivers/input/serio/apbps2.c:
> struct apbps2_regs {
> 	u32 __iomem data;	/* 0x00 */
> 	u32 __iomem status;	/* 0x04 */
> 	u32 __iomem ctrl;	/* 0x08 */
> 	u32 __iomem reload;	/* 0x0c */
> };
> struct apbps2_priv {
> 	struct serio		*io;
> 	struct apbps2_regs	*regs;
> };
> [..] (followed by)
> ioread32be(&priv->regs->status)
> 
> which I think is correct despite contradicting my assumption, but it's
> the only example I've found in the kernel.

Frankly, I would rather turn that into
	struct apbps2_regs	__iomem *regs;
and striped the individual field qualifiers...

arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
commit: 68c5debcc06d6d24f15dbf978780fc5efc147d5e arm64: implement CPPC FFH support using AMUs
date:   8 weeks ago
config: arm64-randconfig-s032-20210106 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-208-g46a52ca4-dirty
        # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68c5debcc06d6d24f15dbf978780fc5efc147d5e
        git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
        git fetch --no-tags linus master
        git checkout 68c5debcc06d6d24f15dbf978780fc5efc147d5e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression

vim +367 arch/arm64/kernel/topology.c

   362	
   363	int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
   364	{
   365		int ret = -EOPNOTSUPP;
   366	
 > 367		switch ((u64)reg->address) {
   368		case 0x0:
   369			ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
   370			break;
   371		case 0x1:
   372			ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
   373			break;
   374		}
   375	
   376		if (!ret) {
   377			*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
   378					    reg->bit_offset);
   379			*val >>= reg->bit_offset;
   380		}
   381	
   382		return ret;
   383	}
   384	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25778 bytes --]