Re: [renesas-drivers:sh-pfc-for-v5.10 5/8] drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Kernel test robot,

CC Gareth, Luc,

On Wed, Sep 23, 2020 at 1:48 PM kernel test robot <lkp@intel.com> wrote:
> First bad commit (maybe != root cause):
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git sh-pfc-for-v5.10
> head:   46d5baf0885ac8e1a799bdc4dc396dab403ccdf9
> commit: 8449bfa9e6a9f7ec9b9037a2e61784140b673823 [5/8] pinctrl: sh-pfc: Collect Renesas related CONFIGs in one place
> config: c6x-randconfig-s031-20200923 (attached as .config)
> compiler: c6x-elf-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.2-201-g24bdaac6-dirty
>         git checkout 8449bfa9e6a9f7ec9b9037a2e61784140b673823
>         # 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=c6x
>
> 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 >>)
>
> >> drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y
>    drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:189:52: sparse: sparse: dubious: x | !y

I believe the code is correct (see below).

> # https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?id=8449bfa9e6a9f7ec9b9037a2e61784140b673823
> git remote add renesas-drivers https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
> git fetch --no-tags renesas-drivers sh-pfc-for-v5.10
> git checkout 8449bfa9e6a9f7ec9b9037a2e61784140b673823
> vim +183 drivers/pinctrl/sh-pfc/pinctrl-rzn1.c
>
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  174
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  175  static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  176  {
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  177     /*
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  178      * The pinmux configuration is locked by writing the physical address of

The comments seem to be wrong, though: "unlocked"

> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  179      * the status_protect register to itself. It is unlocked by writing the

"locked".
Also, the function seems to be used in the opposite way than the
variables are named, which confuses the reader:

                rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1); /*
unlock level 1 before use */
                writel(l1, &ipctl->lev1->conf[pin]);
                rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0); /* lock level
1 again */

But the actual operation is correct, and matches the hardware manual ;-)

> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  180      * address | 1.
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  181      */
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  182     if (lock & LOCK_LEVEL1) {
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26 @183             u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);

IMHO this is correct: ipctl->lev1_protect_phys is to be ORed with 0 or 1,
depending on whether LOCK_LEVEL1 is set or nor.

Is there a way to inform sparse this code is correct?

> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  184
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  185             writel(val, &ipctl->lev1->status_protect);
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  186     }
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  187
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  188     if (lock & LOCK_LEVEL2) {
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  189             u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);

Likewise.

> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  190
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  191             writel(val, &ipctl->lev2->status_protect);
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  192     }
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  193  }
> 4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  194
>
> :::::: The code at line 183 was first introduced by commit
> :::::: 4e53b5004745ef26a37bca4933b2d3ea71313f2a pinctrl: renesas: Renesas RZ/N1 pinctrl driver
>
> :::::: TO: Phil Edworthy <phil.edworthy@renesas.com>
> :::::: CC: Geert Uytterhoeven <geert+renesas@glider.be>
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org



-- 
Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds