2016-10-28 17:00:01

by Laura Abbott

Subject: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address

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.

Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
Signed-off-by: Laura Abbott <[email protected]>
---
v2: Fix up one more format string

I put the fixes tag to match with the cleanup that was done.
If there is interest in this fix for pre-4.2 kernels for
stable, I can submit a patch for that as well.
---
drivers/clk/clk-xgene.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
index 3433132..1e67655 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);

@@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
data &= ~pclk->param.reg_csr_mask;
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,
+ pr_debug("%s csr offset 0x%08X mask 0x%08X value 0x%08X\n",
+ clk_hw_get_name(hw),
pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
data);
}
--
2.7.4

2016-10-28 17:07:31

by Loc Ho

Subject: Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address

Hi Laura,

> 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.
>
> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
> Signed-off-by: Laura Abbott <[email protected]>
> ---
> v2: Fix up one more format string
>
> I put the fixes tag to match with the cleanup that was done.
> If there is interest in this fix for pre-4.2 kernels for
> stable, I can submit a patch for that as well.
> ---
> drivers/clk/clk-xgene.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
> index 3433132..1e67655 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);
>
> @@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
> data &= ~pclk->param.reg_csr_mask;
> 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,
> + pr_debug("%s csr offset 0x%08X mask 0x%08X value 0x%08X\n",
> + clk_hw_get_name(hw),
> pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
> data);
> }


The code looks fine to me. May be overly cautious here. Do you have a
board to test this out?

-Loc

2016-10-28 17:09:55

by Laura Abbott

Subject: Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address

On 10/28/2016 10:07 AM, Loc Ho wrote:
> Hi Laura,
>
>> 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.
>>
>> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
>> Signed-off-by: Laura Abbott <[email protected]>
>> ---
>> v2: Fix up one more format string
>>
>> I put the fixes tag to match with the cleanup that was done.
>> If there is interest in this fix for pre-4.2 kernels for
>> stable, I can submit a patch for that as well.
>> ---
>> drivers/clk/clk-xgene.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
>> index 3433132..1e67655 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);
>>
>> @@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
>> data &= ~pclk->param.reg_csr_mask;
>> 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,
>> + pr_debug("%s csr offset 0x%08X mask 0x%08X value 0x%08X\n",
>> + clk_hw_get_name(hw),
>> pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
>> data);
>> }
>
>
> The code looks fine to me. May be overly cautious here. Do you have a
> board to test this out?
>
> -Loc
>

Yes, that was how I caught the problem. There are no errors that I
can see with this patch applied.

Thanks,
Laura

2016-10-28 17:11:31

by Loc Ho

Subject: Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address

Hi,

>>
>>> 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.
>>>
>>> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
>>> Signed-off-by: Laura Abbott <[email protected]>
>>> ---
>>> v2: Fix up one more format string
>>>
>>> I put the fixes tag to match with the cleanup that was done.
>>> If there is interest in this fix for pre-4.2 kernels for
>>> stable, I can submit a patch for that as well.
>>> ---
>>> drivers/clk/clk-xgene.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
>>> index 3433132..1e67655 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);
>>>
>>> @@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
>>> data &= ~pclk->param.reg_csr_mask;
>>> 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,
>>> + pr_debug("%s csr offset 0x%08X mask 0x%08X value
>>> 0x%08X\n",
>>> + clk_hw_get_name(hw),
>>> pclk->param.reg_csr_offset,
>>> pclk->param.reg_csr_mask,
>>> data);
>>> }
>>
>>
>>
>> The code looks fine to me. May be overly cautious here. Do you have a
>> board to test this out?
>>
>> -Loc
>>
>
> Yes, that was how I caught the problem. There are no errors that I
> can see with this patch applied.

Acked-by: Loc Ho <[email protected]>

-Loc

2016-10-28 23:31:05

by Stephen Boyd

Subject: Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address

On 10/28, 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.
>
> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
> Signed-off-by: Laura Abbott <[email protected]>
> ---

Applied to clk-fixes

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