| Message ID | 20230316180701.783785-1-noltari@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip/bcm-6345-l1: show MMIO address | expand |
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); > } >
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.
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
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.
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
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); }
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(-)