It's safe to show MMIO address.
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
---
drivers/irqchip/irq-bcm6345-l1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
index 6899e37810a8..55a2d9b31597 100644
--- a/drivers/irqchip/irq-bcm6345-l1.c
+++ b/drivers/irqchip/irq-bcm6345-l1.c
@@ -335,7 +335,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
for_each_cpu(idx, &intc->cpumask) {
struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
- pr_info(" CPU%u at MMIO 0x%p (irq = %d)\n", idx,
+ pr_info(" CPU%u at MMIO 0x%px (irq = %d)\n", idx,
cpu->map_base, cpu->parent_irq);
}
--
2.30.2
On 3/16/23 11:07, Álvaro Fernández Rojas wrote:
> It's safe to show MMIO address.
>
> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
This is going to be the kernel virtual address, and while on MIPS it is
easy to resolve to the physical address because these platforms map
registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel
virtual addresses are pretty meaningless unless what you want to debug
is how ioremap() mapped the address.
I would rather do the following change:
diff --git a/drivers/irqchip/irq-bcm6345-l1.c
b/drivers/irqchip/irq-bcm6345-l1.c
index 1bd0621c4ce2..832957d363a4 100644
--- a/drivers/irqchip/irq-bcm6345-l1.c
+++ b/drivers/irqchip/irq-bcm6345-l1.c
@@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
device_node *dn,
if (!cpu->map_base)
return -ENOMEM;
+ request_mem_region(res.start, sz, res.name);
+
for (i = 0; i < n_words; i++) {
cpu->enable_cache[i] = 0;
__raw_writel(0, cpu->map_base + reg_enable(intc, i));
such that this shows up in /proc/iomem. WDYT?
> ---
> drivers/irqchip/irq-bcm6345-l1.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
> index 6899e37810a8..55a2d9b31597 100644
> --- a/drivers/irqchip/irq-bcm6345-l1.c
> +++ b/drivers/irqchip/irq-bcm6345-l1.c
> @@ -335,7 +335,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
> for_each_cpu(idx, &intc->cpumask) {
> struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
>
> - pr_info(" CPU%u at MMIO 0x%p (irq = %d)\n", idx,
> + pr_info(" CPU%u at MMIO 0x%px (irq = %d)\n", idx,
> cpu->map_base, cpu->parent_irq);
> }
>
--
Florian
On 2023-03-16 18:13, Florian Fainelli wrote:
> On 3/16/23 11:07, Álvaro Fernández Rojas wrote:
>> It's safe to show MMIO address.
>>
>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>
> This is going to be the kernel virtual address, and while on MIPS it
> is easy to resolve to the physical address because these platforms map
> registers through KSEG0/1, on other platforms like ARM/ARM64 the
> kernel virtual addresses are pretty meaningless unless what you want
> to debug is how ioremap() mapped the address.
>
> I would rather do the following change:
>
> diff --git a/drivers/irqchip/irq-bcm6345-l1.c
> b/drivers/irqchip/irq-bcm6345-l1.c
> index 1bd0621c4ce2..832957d363a4 100644
> --- a/drivers/irqchip/irq-bcm6345-l1.c
> +++ b/drivers/irqchip/irq-bcm6345-l1.c
> @@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
> device_node *dn,
> if (!cpu->map_base)
> return -ENOMEM;
>
> + request_mem_region(res.start, sz, res.name);
> +
> for (i = 0; i < n_words; i++) {
> cpu->enable_cache[i] = 0;
> __raw_writel(0, cpu->map_base + reg_enable(intc, i));
>
> such that this shows up in /proc/iomem. WDYT?
That's certainly much more useful in general.
Also, the current pr_info() is probably pretty useless, given
that the OP was trying to circumvent the obfuscation. Either
printing the PA or removing the message altogether would be
good.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
El jue, 16 mar 2023 a las 19:13, Florian Fainelli
(<[email protected]>) escribió:
>
> On 3/16/23 11:07, Álvaro Fernández Rojas wrote:
> > It's safe to show MMIO address.
> >
> > Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>
> This is going to be the kernel virtual address, and while on MIPS it is
> easy to resolve to the physical address because these platforms map
> registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel
> virtual addresses are pretty meaningless unless what you want to debug
> is how ioremap() mapped the address.
>
> I would rather do the following change:
>
> diff --git a/drivers/irqchip/irq-bcm6345-l1.c
> b/drivers/irqchip/irq-bcm6345-l1.c
> index 1bd0621c4ce2..832957d363a4 100644
> --- a/drivers/irqchip/irq-bcm6345-l1.c
> +++ b/drivers/irqchip/irq-bcm6345-l1.c
> @@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
> device_node *dn,
> if (!cpu->map_base)
> return -ENOMEM;
>
> + request_mem_region(res.start, sz, res.name);
> +
> for (i = 0; i < n_words; i++) {
> cpu->enable_cache[i] = 0;
> __raw_writel(0, cpu->map_base + reg_enable(intc, i));
>
> such that this shows up in /proc/iomem. WDYT?
I tried doing it that way, but it still shows (ptrval):
[ 0.000000] irq_bcm6345_l1: registered BCM6345 L1 intc (IRQs: 32)
[ 0.000000] irq_bcm6345_l1: CPU0 at MMIO 0x(ptrval) (irq = 2)
I checked /proc/iomem and it's shown:
root@OpenWrt:/# cat /proc/iomem
00000000-03ffffff : System RAM
00010000-0068e96f : Kernel code
0068e970-008834ff : Kernel data
01610000-016458e7 : Kernel bss
08000000-0800ffff : BCM6348 PCI IO space
1e000000-1fffffff : 1e000000.nor nor@1e000000
30000000-37ffffff : pci@fffe1000
30000000-3000ffff : 0000:00:01.0
30000000-3000ffff : ath9k
fffe0004-fffe0007 : fffe0004.clock-controller clock-controller@fffe0004
fffe000c-fffe0013 : interrupt-controller@fffe000c
fffe0034-fffe0037 : fffe0034.reset-controller reset-controller@fffe0034
fffe005c-fffe0067 : fffe005c.watchdog watchdog@fffe005c
fffe0100-fffe0117 : fffe0100.serial serial@fffe0100
fffe1000-fffe11ff : fffe1000.pci pci
fffe1300-fffe13ff : fffe1300.usb usb@fffe1300
fffe1400-fffe14ff : fffe1400.usb usb@fffe1400
fffe1500-fffe1537 : fffe1500.usb-phy usb-phy@fffe1500
Any idea why this could be hapenning?
>
> > ---
> > drivers/irqchip/irq-bcm6345-l1.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
> > index 6899e37810a8..55a2d9b31597 100644
> > --- a/drivers/irqchip/irq-bcm6345-l1.c
> > +++ b/drivers/irqchip/irq-bcm6345-l1.c
> > @@ -335,7 +335,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
> > for_each_cpu(idx, &intc->cpumask) {
> > struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
> >
> > - pr_info(" CPU%u at MMIO 0x%p (irq = %d)\n", idx,
> > + pr_info(" CPU%u at MMIO 0x%px (irq = %d)\n", idx,
> > cpu->map_base, cpu->parent_irq);
> > }
> >
>
> --
> Florian
>
Álvaro
On 3/16/23 12:04, Álvaro Fernández Rojas wrote:
> El jue, 16 mar 2023 a las 19:13, Florian Fainelli
> (<[email protected]>) escribió:
>>
>> On 3/16/23 11:07, Álvaro Fernández Rojas wrote:
>>> It's safe to show MMIO address.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>>
>> This is going to be the kernel virtual address, and while on MIPS it is
>> easy to resolve to the physical address because these platforms map
>> registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel
>> virtual addresses are pretty meaningless unless what you want to debug
>> is how ioremap() mapped the address.
>>
>> I would rather do the following change:
>>
>> diff --git a/drivers/irqchip/irq-bcm6345-l1.c
>> b/drivers/irqchip/irq-bcm6345-l1.c
>> index 1bd0621c4ce2..832957d363a4 100644
>> --- a/drivers/irqchip/irq-bcm6345-l1.c
>> +++ b/drivers/irqchip/irq-bcm6345-l1.c
>> @@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
>> device_node *dn,
>> if (!cpu->map_base)
>> return -ENOMEM;
>>
>> + request_mem_region(res.start, sz, res.name);
>> +
>> for (i = 0; i < n_words; i++) {
>> cpu->enable_cache[i] = 0;
>> __raw_writel(0, cpu->map_base + reg_enable(intc, i));
>>
>> such that this shows up in /proc/iomem. WDYT?
>
> I tried doing it that way, but it still shows (ptrval):
> [ 0.000000] irq_bcm6345_l1: registered BCM6345 L1 intc (IRQs: 32)
> [ 0.000000] irq_bcm6345_l1: CPU0 at MMIO 0x(ptrval) (irq = 2)
Well yes, if you don't remove the pr_info() you are still going to be
printing it, and because map_base is the return of ioremap() which is a
kernel virtual address, it is still hashed, also see Marc's message that
came in. I guess I should have been way more explicit and also provide a
tentative patch that also took out the pr_info().
>
> I checked /proc/iomem and it's shown:
> root@OpenWrt:/# cat /proc/iomem
> 00000000-03ffffff : System RAM
> 00010000-0068e96f : Kernel code
> 0068e970-008834ff : Kernel data
> 01610000-016458e7 : Kernel bss
> 08000000-0800ffff : BCM6348 PCI IO space
> 1e000000-1fffffff : 1e000000.nor nor@1e000000
> 30000000-37ffffff : pci@fffe1000
> 30000000-3000ffff : 0000:00:01.0
> 30000000-3000ffff : ath9k
> fffe0004-fffe0007 : fffe0004.clock-controller clock-controller@fffe0004
> fffe000c-fffe0013 : interrupt-controller@fffe000c
> fffe0034-fffe0037 : fffe0034.reset-controller reset-controller@fffe0034
> fffe005c-fffe0067 : fffe005c.watchdog watchdog@fffe005c
> fffe0100-fffe0117 : fffe0100.serial serial@fffe0100
> fffe1000-fffe11ff : fffe1000.pci pci
> fffe1300-fffe13ff : fffe1300.usb usb@fffe1300
> fffe1400-fffe14ff : fffe1400.usb usb@fffe1400
> fffe1500-fffe1537 : fffe1500.usb-phy usb-phy@fffe1500
>
> Any idea why this could be hapenning?
We now have the desired resource listed using its physical address:
fffe000c-fffe0013 : interrupt-controller@fffe000c
There could be a variety of improvements to how request_mem_region() is
called if you want to provide a break down of each resource on a per-CPU
basis.
--
Florian
Request memory region in order to display it in /proc/iomem.
Also stop printing the MMIO address since it just displays (ptrval).
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
---
v2: request memory region and stop displaying MMIO address.
drivers/irqchip/irq-bcm6345-l1.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
index 6899e37810a8..fa113cb2529a 100644
--- a/drivers/irqchip/irq-bcm6345-l1.c
+++ b/drivers/irqchip/irq-bcm6345-l1.c
@@ -257,6 +257,9 @@ static int __init bcm6345_l1_init_one(struct device_node *dn,
if (!cpu->map_base)
return -ENOMEM;
+ if (!request_mem_region(res.start, sz, res.name))
+ pr_err("failed to request intc memory");
+
for (i = 0; i < n_words; i++) {
cpu->enable_cache[i] = 0;
__raw_writel(0, cpu->map_base + reg_enable(intc, i));
@@ -335,8 +338,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
for_each_cpu(idx, &intc->cpumask) {
struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
- pr_info(" CPU%u at MMIO 0x%p (irq = %d)\n", idx,
- cpu->map_base, cpu->parent_irq);
+ pr_info(" CPU%u (irq = %d)\n", idx, cpu->parent_irq);
}
return 0;
--
2.30.2
El jue, 16 mar 2023 a las 20:10, Florian Fainelli
(<[email protected]>) escribió:
>
> On 3/16/23 12:04, Álvaro Fernández Rojas wrote:
> > El jue, 16 mar 2023 a las 19:13, Florian Fainelli
> > (<[email protected]>) escribió:
> >>
> >> On 3/16/23 11:07, Álvaro Fernández Rojas wrote:
> >>> It's safe to show MMIO address.
> >>>
> >>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> >>
> >> This is going to be the kernel virtual address, and while on MIPS it is
> >> easy to resolve to the physical address because these platforms map
> >> registers through KSEG0/1, on other platforms like ARM/ARM64 the kernel
> >> virtual addresses are pretty meaningless unless what you want to debug
> >> is how ioremap() mapped the address.
> >>
> >> I would rather do the following change:
> >>
> >> diff --git a/drivers/irqchip/irq-bcm6345-l1.c
> >> b/drivers/irqchip/irq-bcm6345-l1.c
> >> index 1bd0621c4ce2..832957d363a4 100644
> >> --- a/drivers/irqchip/irq-bcm6345-l1.c
> >> +++ b/drivers/irqchip/irq-bcm6345-l1.c
> >> @@ -261,6 +261,8 @@ static int __init bcm6345_l1_init_one(struct
> >> device_node *dn,
> >> if (!cpu->map_base)
> >> return -ENOMEM;
> >>
> >> + request_mem_region(res.start, sz, res.name);
> >> +
> >> for (i = 0; i < n_words; i++) {
> >> cpu->enable_cache[i] = 0;
> >> __raw_writel(0, cpu->map_base + reg_enable(intc, i));
> >>
> >> such that this shows up in /proc/iomem. WDYT?
> >
> > I tried doing it that way, but it still shows (ptrval):
> > [ 0.000000] irq_bcm6345_l1: registered BCM6345 L1 intc (IRQs: 32)
> > [ 0.000000] irq_bcm6345_l1: CPU0 at MMIO 0x(ptrval) (irq = 2)
>
> Well yes, if you don't remove the pr_info() you are still going to be
> printing it, and because map_base is the return of ioremap() which is a
> kernel virtual address, it is still hashed, also see Marc's message that
> came in. I guess I should have been way more explicit and also provide a
> tentative patch that also took out the pr_info().
Ah, sorry for that, I didn't get your point...
However, I'd rather keep the pr_info, so I just removed the MMIO address in v2.
>
> >
> > I checked /proc/iomem and it's shown:
> > root@OpenWrt:/# cat /proc/iomem
> > 00000000-03ffffff : System RAM
> > 00010000-0068e96f : Kernel code
> > 0068e970-008834ff : Kernel data
> > 01610000-016458e7 : Kernel bss
> > 08000000-0800ffff : BCM6348 PCI IO space
> > 1e000000-1fffffff : 1e000000.nor nor@1e000000
> > 30000000-37ffffff : pci@fffe1000
> > 30000000-3000ffff : 0000:00:01.0
> > 30000000-3000ffff : ath9k
> > fffe0004-fffe0007 : fffe0004.clock-controller clock-controller@fffe0004
> > fffe000c-fffe0013 : interrupt-controller@fffe000c
> > fffe0034-fffe0037 : fffe0034.reset-controller reset-controller@fffe0034
> > fffe005c-fffe0067 : fffe005c.watchdog watchdog@fffe005c
> > fffe0100-fffe0117 : fffe0100.serial serial@fffe0100
> > fffe1000-fffe11ff : fffe1000.pci pci
> > fffe1300-fffe13ff : fffe1300.usb usb@fffe1300
> > fffe1400-fffe14ff : fffe1400.usb usb@fffe1400
> > fffe1500-fffe1537 : fffe1500.usb-phy usb-phy@fffe1500
> >
> > Any idea why this could be hapenning?
>
> We now have the desired resource listed using its physical address:
>
> fffe000c-fffe0013 : interrupt-controller@fffe000c
>
> There could be a variety of improvements to how request_mem_region() is
> called if you want to provide a break down of each resource on a per-CPU
> basis.
> --
> Florian
>
Álvaro
On 3/16/23 12:28, Álvaro Fernández Rojas wrote:
> Request memory region in order to display it in /proc/iomem.
> Also stop printing the MMIO address since it just displays (ptrval).
>
> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
Thanks!
--
Florian
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: 23c7ff129cf33dee5f1f4fd9fa729ab440e8f1c5
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/23c7ff129cf33dee5f1f4fd9fa729ab440e8f1c5
Author: Álvaro Fernández Rojas <[email protected]>
AuthorDate: Thu, 16 Mar 2023 20:28:33 +01:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Sat, 08 Apr 2023 11:07:43 +01:00
irqchip/bcm-6345-l1: Request memory region
Request memory region in order to display it in /proc/iomem.
Also stop printing the MMIO address since it just displays (ptrval).
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-bcm6345-l1.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
index 6899e37..fa113cb 100644
--- a/drivers/irqchip/irq-bcm6345-l1.c
+++ b/drivers/irqchip/irq-bcm6345-l1.c
@@ -257,6 +257,9 @@ static int __init bcm6345_l1_init_one(struct device_node *dn,
if (!cpu->map_base)
return -ENOMEM;
+ if (!request_mem_region(res.start, sz, res.name))
+ pr_err("failed to request intc memory");
+
for (i = 0; i < n_words; i++) {
cpu->enable_cache[i] = 0;
__raw_writel(0, cpu->map_base + reg_enable(intc, i));
@@ -335,8 +338,7 @@ static int __init bcm6345_l1_of_init(struct device_node *dn,
for_each_cpu(idx, &intc->cpumask) {
struct bcm6345_l1_cpu *cpu = intc->cpus[idx];
- pr_info(" CPU%u at MMIO 0x%p (irq = %d)\n", idx,
- cpu->map_base, cpu->parent_irq);
+ pr_info(" CPU%u (irq = %d)\n", idx, cpu->parent_irq);
}
return 0;