Re: drivers/pci/pcie/../pci.h:325:17: sparse: sparse: cast from restricted pci_channel_state_t

[Luc Van Oostenryck]

On Sun, Dec 03, 2023 at 05:59:32PM +0100, Lukas Wunner wrote:
> Hi Dan,
> 
> > > Please advise whether these sparse warnings are false positives which
> > > can be ignored and if they aren't, how to resolve them.  If you happen
> > > to know the rationale for the cast, I'd be grateful if you could shed
> > > some light on it.  Thanks a lot!
> > 
> > The question is more why is pci_channel_state_t declared as __bit_wise.
> > __bit_wise data can only be used through accessor functions, like user
> > pointers have to go through copy_from_user() and endian data has to go
> > through le32_to_cpu() etc.
> 
> __bitwise is not only to ensure endian correctness.  It can be used to
> define data types which are integer-based, but treated as a distinct
> data type by sparse.  A pci_channel_state_t value cannot be assigned
> to an integer variable of a different type, for example.

Correct. It was done on purpose to make a sort of 'strong' enum type,
and thus warn if pci_channel_state_t values are mixed with some other types.
 
> A few arches define arch_xchg() and arch_cmpxchg() as pure macros.
> The sparse warning for pci_channel_state_t does not appear on those
> arches.  (These are:  arc csky riscv x86)
> 
> All other arches use a mix of macros and static inlines to define
> arch_xchg() and arch_cmpxchg().  The static inlines use unsigned long
> as argument and return types and the macros cast the argument type to
> unsigned long.
>
> Why are the casts necessary?  Because there are callers of xchg() and
> cmpxchg() which pass pointers instead of integers as arguments.

Hmmm, I'm missing the precise context but it make me wonder what's happening
on 64-bit archs where enums are usually only 32-bit ...

> Examples include llist_del_all(), __wake_q_add(), mark_oom_victim(),
> fsnotify_attach_connector_to_object().  (Note that NULL is defined as
> "(void *)0".)
> 
> When using xchg() or cmpxchg() with __bitwise arguments (as is done
> by commit 74ff8864cc84 in pci_dev_set_io_state()), the arches which
> define arch_xchg() and arch_cmpxchg() with static inlines see sparse
> warnings because the __bitwise arguments are cast to unsigned long.
> Those arches are:  alpha arm arm64 hexagon loongarch m68k mips openrisc
> parisc powerpc s390 sh sparc xtensa

OK, if the underlying macros do as:
	switch (size) {
	case 1: ...
then things are ok there (but only if the size is given by a sizeof() of
the correct type (not casted to long or something).

> Indeed adding __force to the cast, as you suggest, should avoid the
> issue.  sparse cannot parse the inline assembler, so it does not
> understand that arch_xchg() and arch_cmpxchg() internally perform a
> comparison and/or assignment.  By adding __force, we therefore do not
> lose any validation coverage.

Yes, indeed, using __force inside specific accessors or any low-level macros,
like here these arch_xchg(), is OK. It's done in plenty of similar cases.

> A better approach might be to teach sparse that arch_xchg() and
> arch_cmpxchg() internally perform a comparison and/or assignment.
> Then sparse could validate the argument and return value types.
> 
> builtin.c in the sparse source code already contains functions
> to handle __atomic_exchange() and __atomic_compare_exchange(),
> so I think xchg() and cmpxchg() should be handled there as well.

I don't agree. Sparse shouldn't know about the semantics of functions
in the kernel. Sparse offers some tools (annotations like the __bitwise,
noderef or address_space attributes for __user, __iomem, ... and type
checking stricter than standard/GCC C). By using these annotations, the
kernel can define a stricter semantic, that better suits its needs,
Sparse should know nothing about this, it's not its job.

Best regards,
-- Luc