All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	kernel test robot <lkp@intel.com>,
	kbuild-all@lists.01.org, linux-kernel@vger.kernel.org,
	Sudeep Holla <sudeep.holla@arm.com>
Subject: Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression
Date: Wed, 6 Jan 2021 16:13:53 +0000	[thread overview]
Message-ID: <20210106161353.GC3579531@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20210106155214.GA30892@arm.com>

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.

WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro@zeniv.linux.org.uk>
To: kbuild-all@lists.01.org
Subject: Re: arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression
Date: Wed, 06 Jan 2021 16:13:53 +0000	[thread overview]
Message-ID: <20210106161353.GC3579531@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20210106155214.GA30892@arm.com>

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

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.

  reply	other threads:[~2021-01-06 16:15 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-17 21:00 arch/arm64/kernel/topology.c:367:22: sparse: sparse: dereference of noderef expression kernel test robot
2020-12-17 21:00 ` kernel test robot
2020-12-18 10:44 ` Catalin Marinas
2020-12-18 10:44   ` Catalin Marinas
2021-01-06 15:07   ` Ionela Voinescu
2021-01-06 15:07     ` Ionela Voinescu
2021-01-06 15:21     ` Catalin Marinas
2021-01-06 15:21       ` Catalin Marinas
2021-01-06 15:52       ` Ionela Voinescu
2021-01-06 15:52         ` Ionela Voinescu
2021-01-06 16:13         ` Al Viro [this message]
2021-01-06 16:13           ` Al Viro
2021-01-06 16:47           ` Ionela Voinescu
2021-01-06 16:47             ` Ionela Voinescu
2021-01-06 17:47     ` Al Viro
2021-01-06 17:47       ` Al Viro
2021-01-06 20:12       ` Ionela Voinescu
2021-01-06 20:12         ` Ionela Voinescu
2021-01-06 20:46         ` Al Viro
2021-01-06 20:46           ` Al Viro
2021-01-06  5:50 kernel test robot
2021-01-06  5:50 ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210106161353.GC3579531@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=catalin.marinas@arm.com \
    --cc=ionela.voinescu@arm.com \
    --cc=kbuild-all@lists.01.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=sudeep.holla@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.