linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [renesas-drivers:sh-pfc-for-v5.10 5/8] drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y
@ 2020-09-23 11:47 kernel test robot
  2020-09-24  8:01 ` Geert Uytterhoeven
  0 siblings, 1 reply; 3+ messages in thread
From: kernel test robot @ 2020-09-23 11:47 UTC (permalink / raw)
  To: Kuninori Morimoto; +Cc: kbuild-all, linux-renesas-soc, Geert Uytterhoeven

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

Hi Kuninori,

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

# 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
4e53b5004745ef drivers/pinctrl/pinctrl-rzn1.c Phil Edworthy 2018-09-26  179  	 * the status_protect register to itself. It is unlocked by writing the
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);
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);
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

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [renesas-drivers:sh-pfc-for-v5.10 5/8] drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y
  2020-09-23 11:47 [renesas-drivers:sh-pfc-for-v5.10 5/8] drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y kernel test robot
@ 2020-09-24  8:01 ` Geert Uytterhoeven
  2020-09-24 19:17   ` Luc Van Oostenryck
  0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2020-09-24  8:01 UTC (permalink / raw)
  To: kernel test robot
  Cc: Kuninori Morimoto, kbuild-all, Linux-Renesas, Gareth Williams,
	Luc Van Oostenryck

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [renesas-drivers:sh-pfc-for-v5.10 5/8] drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y
  2020-09-24  8:01 ` Geert Uytterhoeven
@ 2020-09-24 19:17   ` Luc Van Oostenryck
  0 siblings, 0 replies; 3+ messages in thread
From: Luc Van Oostenryck @ 2020-09-24 19:17 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: kernel test robot, Kuninori Morimoto, kbuild-all, Linux-Renesas,
	Gareth Williams

Hi,

On Thu, Sep 24, 2020 at 10:01:57AM +0200, Geert Uytterhoeven wrote:
> Hi Kernel test robot,
> 
> CC Gareth, Luc,
> 
> > 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).
> 
> > 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?

Not really, at least no kind of annotation to suppress the warning
(and I don't think it would be a good idea to have one). But of
course, since the warning is about the possible error of mixing
a bitwise and with a logical not, you can rewrite the expression as:
	u32 val = ipctl->lev1_protect_phys | ((value & LOCK_LEVEL1) == 0);
or, better;
	u32 val = ipctl->lev1_protect_phys | ((value & LOCK_LEVEL1) ? 0  1);

But I understand quite well why people would prefer the simpler, more
idiomatic !(value & LOCK_LEVEL1).

Best regards,
-- Luc

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-24 19:17 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 11:47 [renesas-drivers:sh-pfc-for-v5.10 5/8] drivers/pinctrl/sh-pfc/pinctrl-rzn1.c:183:52: sparse: sparse: dubious: x | !y kernel test robot
2020-09-24  8:01 ` Geert Uytterhoeven
2020-09-24 19:17   ` Luc Van Oostenryck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).