All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] clk: xgene: Don't call __pa on ioremaped address
@ 2016-10-27 23:21 Laura Abbott
  2016-10-27 23:24 ` Laura Abbott
  2016-10-28  0:33 ` kbuild test robot
  0 siblings, 2 replies; 5+ messages in thread
From: Laura Abbott @ 2016-10-27 23:21 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Loc Ho
  Cc: Laura Abbott, linux-clk, linux-kernel


ioremaped addresses are not linearly mapped so the physical
address can not be figured out via __pa. More generally, there
is no guarantee that backing value of an ioremapped address
is a physical address at all. The value here is only used
for debugging so just drop the call to __pa on the ioremapped
address.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
Found while working on virt_to_phys debugging for arm64
---
 drivers/clk/clk-xgene.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
index 3433132..4697bf9 100644
--- a/drivers/clk/clk-xgene.c
+++ b/drivers/clk/clk-xgene.c
@@ -243,22 +243,20 @@ static int xgene_clk_enable(struct clk_hw *hw)
 	struct xgene_clk *pclk = to_xgene_clk(hw);
 	unsigned long flags = 0;
 	u32 data;
-	phys_addr_t reg;
 
 	if (pclk->lock)
 		spin_lock_irqsave(pclk->lock, flags);
 
 	if (pclk->param.csr_reg != NULL) {
 		pr_debug("%s clock enabled\n", clk_hw_get_name(hw));
-		reg = __pa(pclk->param.csr_reg);
 		/* First enable the clock */
 		data = xgene_clk_read(pclk->param.csr_reg +
 					pclk->param.reg_clk_offset);
 		data |= pclk->param.reg_clk_mask;
 		xgene_clk_write(data, pclk->param.csr_reg +
 					pclk->param.reg_clk_offset);
-		pr_debug("%s clock PADDR base %pa clk offset 0x%08X mask 0x%08X value 0x%08X\n",
-			clk_hw_get_name(hw), &reg,
+		pr_debug("%s clk offset 0x%08X mask 0x%08X value 0x%08X\n",
+			clk_hw_get_name(hw),
 			pclk->param.reg_clk_offset, pclk->param.reg_clk_mask,
 			data);
 
@@ -269,7 +267,7 @@ static int xgene_clk_enable(struct clk_hw *hw)
 		xgene_clk_write(data, pclk->param.csr_reg +
 					pclk->param.reg_csr_offset);
 		pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
-			clk_hw_get_name(hw), &reg,
+			clk_hw_get_name(hw),
 			pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
 			data);
 	}
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: xgene: Don't call __pa on ioremaped address
  2016-10-27 23:21 [PATCH] clk: xgene: Don't call __pa on ioremaped address Laura Abbott
@ 2016-10-27 23:24 ` Laura Abbott
  2016-10-27 23:33   ` Stephen Boyd
  2016-10-28  0:33 ` kbuild test robot
  1 sibling, 1 reply; 5+ messages in thread
From: Laura Abbott @ 2016-10-27 23:24 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Loc Ho; +Cc: linux-clk, linux-kernel

On 10/27/2016 04:21 PM, Laura Abbott wrote:
> ioremaped addresses are not linearly mapped so the physical
> address can not be figured out via __pa. More generally, there
> is no guarantee that backing value of an ioremapped address
> is a physical address at all. The value here is only used
> for debugging so just drop the call to __pa on the ioremapped
> address.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Found while working on virt_to_phys debugging for arm64
> ---
>  drivers/clk/clk-xgene.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
> index 3433132..4697bf9 100644
> --- a/drivers/clk/clk-xgene.c
> +++ b/drivers/clk/clk-xgene.c
> @@ -243,22 +243,20 @@ static int xgene_clk_enable(struct clk_hw *hw)
>  	struct xgene_clk *pclk = to_xgene_clk(hw);
>  	unsigned long flags = 0;
>  	u32 data;
> -	phys_addr_t reg;
>
>  	if (pclk->lock)
>  		spin_lock_irqsave(pclk->lock, flags);
>
>  	if (pclk->param.csr_reg != NULL) {
>  		pr_debug("%s clock enabled\n", clk_hw_get_name(hw));
> -		reg = __pa(pclk->param.csr_reg);
>  		/* First enable the clock */
>  		data = xgene_clk_read(pclk->param.csr_reg +
>  					pclk->param.reg_clk_offset);
>  		data |= pclk->param.reg_clk_mask;
>  		xgene_clk_write(data, pclk->param.csr_reg +
>  					pclk->param.reg_clk_offset);
> -		pr_debug("%s clock PADDR base %pa clk offset 0x%08X mask 0x%08X value 0x%08X\n",
> -			clk_hw_get_name(hw), &reg,
> +		pr_debug("%s clk offset 0x%08X mask 0x%08X value 0x%08X\n",
> +			clk_hw_get_name(hw),
>  			pclk->param.reg_clk_offset, pclk->param.reg_clk_mask,
>  			data);
>
> @@ -269,7 +267,7 @@ static int xgene_clk_enable(struct clk_hw *hw)
>  		xgene_clk_write(data, pclk->param.csr_reg +
>  					pclk->param.reg_csr_offset);
>  		pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",

Bah, this got fixed locally but didn't get amended. I'll remove
the format here in a v2 assuming there are no other objections.

> -			clk_hw_get_name(hw), &reg,
> +			clk_hw_get_name(hw),
>  			pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
>  			data);
>  	}
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: xgene: Don't call __pa on ioremaped address
  2016-10-27 23:24 ` Laura Abbott
@ 2016-10-27 23:33   ` Stephen Boyd
  2016-10-28  4:57     ` Loc Ho
  0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2016-10-27 23:33 UTC (permalink / raw)
  To: Laura Abbott; +Cc: Michael Turquette, Loc Ho, linux-clk, linux-kernel

On 10/27, Laura Abbott wrote:
> On 10/27/2016 04:21 PM, Laura Abbott wrote:
> >
> >@@ -269,7 +267,7 @@ static int xgene_clk_enable(struct clk_hw *hw)
> > 		xgene_clk_write(data, pclk->param.csr_reg +
> > 					pclk->param.reg_csr_offset);
> > 		pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
> 
> Bah, this got fixed locally but didn't get amended. I'll remove
> the format here in a v2 assuming there are no other objections.
> 

Include a Fixes: tag too? It would be nice if sparse complained
about __pa used on an __iomem pointer too.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: xgene: Don't call __pa on ioremaped address
  2016-10-27 23:21 [PATCH] clk: xgene: Don't call __pa on ioremaped address Laura Abbott
  2016-10-27 23:24 ` Laura Abbott
@ 2016-10-28  0:33 ` kbuild test robot
  1 sibling, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2016-10-28  0:33 UTC (permalink / raw)
  To: Laura Abbott
  Cc: kbuild-all, Michael Turquette, Stephen Boyd, Loc Ho,
	Laura Abbott, linux-clk, linux-kernel

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

Hi Laura,

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v4.9-rc2 next-20161027]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Laura-Abbott/clk-xgene-Don-t-call-__pa-on-ioremaped-address/20161028-072455
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
                    from include/linux/list.h:8,
                    from include/linux/module.h:9,
                    from drivers/clk/clk-xgene.c:23:
   drivers/clk/clk-xgene.c: In function 'xgene_clk_enable':
>> drivers/clk/clk-xgene.c:489:12: warning: format '%p' expects argument of type 'void *', but argument 4 has type 'u32 {aka unsigned int}' [-Wformat=]
      pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
               ^
   include/linux/printk.h:261:21: note: in definition of macro 'pr_fmt'
    #define pr_fmt(fmt) fmt
                        ^~~
   include/linux/printk.h:309:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~~
>> drivers/clk/clk-xgene.c:489:3: note: in expansion of macro 'pr_debug'
      pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
      ^~~~~~~~
>> drivers/clk/clk-xgene.c:489:12: warning: format '%X' expects a matching 'unsigned int' argument [-Wformat=]
      pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
               ^
   include/linux/printk.h:261:21: note: in definition of macro 'pr_fmt'
    #define pr_fmt(fmt) fmt
                        ^~~
   include/linux/printk.h:309:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~~
>> drivers/clk/clk-xgene.c:489:3: note: in expansion of macro 'pr_debug'
      pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
      ^~~~~~~~

vim +489 drivers/clk/clk-xgene.c

308964ca Loc Ho       2013-06-26  473  		data = xgene_clk_read(pclk->param.csr_reg +
308964ca Loc Ho       2013-06-26  474  					pclk->param.reg_clk_offset);
308964ca Loc Ho       2013-06-26  475  		data |= pclk->param.reg_clk_mask;
308964ca Loc Ho       2013-06-26  476  		xgene_clk_write(data, pclk->param.csr_reg +
308964ca Loc Ho       2013-06-26  477  					pclk->param.reg_clk_offset);
1ee1b519 Laura Abbott 2016-10-27  478  		pr_debug("%s clk offset 0x%08X mask 0x%08X value 0x%08X\n",
1ee1b519 Laura Abbott 2016-10-27  479  			clk_hw_get_name(hw),
308964ca Loc Ho       2013-06-26  480  			pclk->param.reg_clk_offset, pclk->param.reg_clk_mask,
308964ca Loc Ho       2013-06-26  481  			data);
308964ca Loc Ho       2013-06-26  482  
308964ca Loc Ho       2013-06-26  483  		/* Second enable the CSR */
308964ca Loc Ho       2013-06-26  484  		data = xgene_clk_read(pclk->param.csr_reg +
308964ca Loc Ho       2013-06-26  485  					pclk->param.reg_csr_offset);
308964ca Loc Ho       2013-06-26  486  		data &= ~pclk->param.reg_csr_mask;
308964ca Loc Ho       2013-06-26  487  		xgene_clk_write(data, pclk->param.csr_reg +
308964ca Loc Ho       2013-06-26  488  					pclk->param.reg_csr_offset);
6ae5fd38 Stephen Boyd 2015-05-01 @489  		pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
1ee1b519 Laura Abbott 2016-10-27  490  			clk_hw_get_name(hw),
308964ca Loc Ho       2013-06-26  491  			pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
308964ca Loc Ho       2013-06-26  492  			data);
308964ca Loc Ho       2013-06-26  493  	}
308964ca Loc Ho       2013-06-26  494  
308964ca Loc Ho       2013-06-26  495  	if (pclk->lock)
308964ca Loc Ho       2013-06-26  496  		spin_unlock_irqrestore(pclk->lock, flags);
308964ca Loc Ho       2013-06-26  497  

:::::: The code at line 489 was first introduced by commit
:::::: 6ae5fd381251993a650a7fad51bd9318c19a6c80 clk: xgene: Silence sparse warnings

:::::: TO: Stephen Boyd <sboyd@codeaurora.org>
:::::: CC: Stephen Boyd <sboyd@codeaurora.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: xgene: Don't call __pa on ioremaped address
  2016-10-27 23:33   ` Stephen Boyd
@ 2016-10-28  4:57     ` Loc Ho
  0 siblings, 0 replies; 5+ messages in thread
From: Loc Ho @ 2016-10-28  4:57 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Laura Abbott, Michael Turquette, linux-clk, Linux Kernel Mailing List

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

Hi Stephen,

On Thu, Oct 27, 2016 at 4:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 10/27, Laura Abbott wrote:
> > On 10/27/2016 04:21 PM, Laura Abbott wrote:
> > >
> > >@@ -269,7 +267,7 @@ static int xgene_clk_enable(struct clk_hw *hw)
> > >             xgene_clk_write(data, pclk->param.csr_reg +
> > >                                     pclk->param.reg_csr_offset);
> > >             pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X
> mask 0x%08X value 0x%08X\n",
> >
> > Bah, this got fixed locally but didn't get amended. I'll remove
> > the format here in a v2 assuming there are no other objections.
> >
>
> Include a Fixes: tag too? It would be nice if sparse complained
> about __pa used on an __iomem pointer too.
>

​The change looks fine after you fix the pr_debug mentioned above. Will you
be generate another version?

-Loc​

[-- Attachment #2: Type: text/html, Size: 1642 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-10-28  4:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-27 23:21 [PATCH] clk: xgene: Don't call __pa on ioremaped address Laura Abbott
2016-10-27 23:24 ` Laura Abbott
2016-10-27 23:33   ` Stephen Boyd
2016-10-28  4:57     ` Loc Ho
2016-10-28  0:33 ` kbuild test robot

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.