[PATCH] irqchip/bcm-6345-l1: show MMIO address

[PATCH] irqchip/bcm-6345-l1: show MMIO address

[Álvaro Fernández Rojas]

It's safe to show MMIO address.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 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

Re: [PATCH] irqchip/bcm-6345-l1: show MMIO address

[Florian Fainelli]

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 <noltari@gmail.com>

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

Re: [PATCH] irqchip/bcm-6345-l1: show MMIO address

[Marc Zyngier]

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 <noltari@gmail.com>
> 
> 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...

Re: [PATCH] irqchip/bcm-6345-l1: show MMIO address

[Álvaro Fernández Rojas]

El jue, 16 mar 2023 a las 19:13, Florian Fainelli
(<f.fainelli@gmail.com>) 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 <noltari@gmail.com>
>
> 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

Re: [PATCH] irqchip/bcm-6345-l1: show MMIO address

[Florian Fainelli]

On 3/16/23 12:04, Álvaro Fernández Rojas wrote:
> El jue, 16 mar 2023 a las 19:13, Florian Fainelli
> (<f.fainelli@gmail.com>) 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 <noltari@gmail.com>
>>
>> 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

[PATCH v2] irqchip/bcm-6345-l1: request memory region

[Álvaro Fernández Rojas]

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 <noltari@gmail.com>
---
 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

Re: [PATCH] irqchip/bcm-6345-l1: show MMIO address

[Álvaro Fernández Rojas]

El jue, 16 mar 2023 a las 20:10, Florian Fainelli
(<f.fainelli@gmail.com>) escribió:
>
> On 3/16/23 12:04, Álvaro Fernández Rojas wrote:
> > El jue, 16 mar 2023 a las 19:13, Florian Fainelli
> > (<f.fainelli@gmail.com>) 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 <noltari@gmail.com>
> >>
> >> 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

Re: [PATCH v2] irqchip/bcm-6345-l1: request memory region

[Florian Fainelli]

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 <noltari@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
-- 
Florian