Re: [PATCH net-next v5 05/14] net: fman: Map the base address once

Hi Sean,

On Mon, Oct 17, 2022 at 5:34 PM Sean Anderson [off-list ref] wrote:

On 10/17/22 11:15 AM, Geert Uytterhoeven wrote:

quoted

On Sat, Sep 3, 2022 at 12:00 AM Sean Anderson [off-list ref] wrote:

quoted

We don't need to remap the base address from the resource twice (once in
mac_probe() and again in set_fman_mac_params()). We still need the
resource to get the end address, but we can use a single function call
to get both at once.

While we're at it, use platform_get_mem_or_io and devm_request_resource
to map the resource. I think this is the more "correct" way to do things
here, since we use the pdev resource, instead of creating a new one.
It's still a bit tricky, since we need to ensure that the resource is a
child of the fman region when it gets requested.

Signed-off-by: Sean Anderson <redacted>
Acked-by: Camelia Groza <redacted>

Thanks for your patch, which is now commit 262f2b782e255b79
("net: fman: Map the base address once") in v6.1-rc1.

quoted

--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth_sysfs.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth_sysfs.c

@@ -18,7 +18,7 @@ static ssize_t dpaa_eth_show_addr(struct device *dev,

        if (mac_dev)
                return sprintf(buf, "%llx",
-                               (unsigned long long)mac_dev->res->start);
+                               (unsigned long long)mac_dev->vaddr);

On 32-bit:

    warning: cast from pointer to integer of different size
[-Wpointer-to-int-cast]

Obviously you should cast to "uintptr_t" or "unsigned long" instead,
and change the "%llx" to "%p" or "%lx"...

Isn't there a %px for this purpose?

Yes there is.  But if it makes sense to use that depends on the
still to be answered questions at the bottom...

quoted

However, taking a closer look:
  1. The old code exposed a physical address to user space, the new
     code exposes the mapped virtual address.
     Is that change intentional?

No, this is not intentional. So to make this backwards-compatible, I
suppose I need a virt_to_phys?

I think virt_to_phys() will work only on real memory, not on MMIO,
so you may need to reintroduce the resource again.

quoted

  2. Virtual addresses are useless in user space.
     Moreover, addresses printed by "%p" are obfuscated, as this is
     considered a security issue. Likewise for working around this by
     casting to an integer.

Yes, you're right that this probably shouldn't be exposed to userspace.

quoted

What's the real purpose of dpaa_eth_show_addr()?

I have no idea. This is a question for Madalin.

quoted

Perhaps it should be removed?

That would be reasonable IMO.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds