| Message ID | 20220902215737.981341-6-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dpaa: Cleanups in preparation for phylink conversion (part 2) | expand |
Hi Sean, On Sat, Sep 3, 2022 at 12:00 AM Sean Anderson <sean.anderson@seco.com> wrote: > 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 <sean.anderson@seco.com> > Acked-by: Camelia Groza <camelia.groza@nxp.com> Thanks for your patch, which is now commit 262f2b782e255b79 ("net: fman: Map the base address once") in v6.1-rc1. > --- 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"... 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? 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. What's the real purpose of dpaa_eth_show_addr()? Perhaps it should be removed? > else > return sprintf(buf, "none"); > } 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
On 10/17/22 11:15 AM, Geert Uytterhoeven wrote: > Hi Sean, > > On Sat, Sep 3, 2022 at 12:00 AM Sean Anderson <sean.anderson@seco.com> wrote: >> 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 <sean.anderson@seco.com> >> Acked-by: Camelia Groza <camelia.groza@nxp.com> > > Thanks for your patch, which is now commit 262f2b782e255b79 > ("net: fman: Map the base address once") in v6.1-rc1. > >> --- 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? > 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? > 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. > What's the real purpose of dpaa_eth_show_addr()? I have no idea. This is a question for Madalin. > Perhaps it should be removed? That would be reasonable IMO. --Sean >> else >> return sprintf(buf, "none"); >> } > > 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 >
Hi Sean, On Mon, Oct 17, 2022 at 5:34 PM Sean Anderson <sean.anderson@seco.com> wrote: > On 10/17/22 11:15 AM, Geert Uytterhoeven wrote: > > On Sat, Sep 3, 2022 at 12:00 AM Sean Anderson <sean.anderson@seco.com> wrote: > >> 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 <sean.anderson@seco.com> > >> Acked-by: Camelia Groza <camelia.groza@nxp.com> > > > > Thanks for your patch, which is now commit 262f2b782e255b79 > > ("net: fman: Map the base address once") in v6.1-rc1. > > > >> --- 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... > > 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. > > 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. > > > What's the real purpose of dpaa_eth_show_addr()? > > I have no idea. This is a question for Madalin. > > > 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
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c index e974d90f15e3..02b588c46fcf 100644 --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c @@ -218,8 +218,8 @@ static int dpaa_netdev_init(struct net_device *net_dev, net_dev->netdev_ops = dpaa_ops; mac_addr = priv->mac_dev->addr; - net_dev->mem_start = priv->mac_dev->res->start; - net_dev->mem_end = priv->mac_dev->res->end; + net_dev->mem_start = (unsigned long)priv->mac_dev->vaddr; + net_dev->mem_end = (unsigned long)priv->mac_dev->vaddr_end; net_dev->min_mtu = ETH_MIN_MTU; net_dev->max_mtu = dpaa_get_max_mtu(); diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth_sysfs.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth_sysfs.c index 4fee74c024bd..258eb6c8f4c0 100644 --- 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); else return sprintf(buf, "none"); } diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c index 7afedd4995c9..62af81c0c942 100644 --- a/drivers/net/ethernet/freescale/fman/mac.c +++ b/drivers/net/ethernet/freescale/fman/mac.c @@ -28,7 +28,6 @@ MODULE_LICENSE("Dual BSD/GPL"); MODULE_DESCRIPTION("FSL FMan MAC API based driver"); struct mac_priv_s { - void __iomem *vaddr; u8 cell_index; struct fman *fman; /* List of multicast addresses */ @@ -63,12 +62,7 @@ int set_fman_mac_params(struct mac_device *mac_dev, { struct mac_priv_s *priv = mac_dev->priv; - params->base_addr = (typeof(params->base_addr)) - devm_ioremap(mac_dev->dev, mac_dev->res->start, - resource_size(mac_dev->res)); - if (!params->base_addr) - return -ENOMEM; - + params->base_addr = mac_dev->vaddr; memcpy(¶ms->addr, mac_dev->addr, sizeof(mac_dev->addr)); params->max_speed = priv->max_speed; params->phy_if = mac_dev->phy_if; @@ -305,7 +299,7 @@ static int mac_probe(struct platform_device *_of_dev) struct device_node *mac_node, *dev_node; struct mac_device *mac_dev; struct platform_device *of_dev; - struct resource res; + struct resource *res; struct mac_priv_s *priv; u32 val; u8 fman_id; @@ -368,30 +362,25 @@ static int mac_probe(struct platform_device *_of_dev) of_node_put(dev_node); /* Get the address of the memory mapped registers */ - err = of_address_to_resource(mac_node, 0, &res); - if (err < 0) { - dev_err(dev, "of_address_to_resource(%pOF) = %d\n", - mac_node, err); - goto _return_of_node_put; + res = platform_get_mem_or_io(_of_dev, 0); + if (!res) { + dev_err(dev, "could not get registers\n"); + return -EINVAL; } - mac_dev->res = __devm_request_region(dev, - fman_get_mem_region(priv->fman), - res.start, resource_size(&res), - "mac"); - if (!mac_dev->res) { - dev_err(dev, "__devm_request_mem_region(mac) failed\n"); - err = -EBUSY; + err = devm_request_resource(dev, fman_get_mem_region(priv->fman), res); + if (err) { + dev_err_probe(dev, err, "could not request resource\n"); goto _return_of_node_put; } - priv->vaddr = devm_ioremap(dev, mac_dev->res->start, - resource_size(mac_dev->res)); - if (!priv->vaddr) { + mac_dev->vaddr = devm_ioremap(dev, res->start, resource_size(res)); + if (!mac_dev->vaddr) { dev_err(dev, "devm_ioremap() failed\n"); err = -EIO; goto _return_of_node_put; } + mac_dev->vaddr_end = mac_dev->vaddr + resource_size(res); if (!of_device_is_available(mac_node)) { err = -ENODEV; diff --git a/drivers/net/ethernet/freescale/fman/mac.h b/drivers/net/ethernet/freescale/fman/mac.h index da410a7d00c9..7aa71b05bd3e 100644 --- a/drivers/net/ethernet/freescale/fman/mac.h +++ b/drivers/net/ethernet/freescale/fman/mac.h @@ -19,8 +19,9 @@ struct fman_mac; struct mac_priv_s; struct mac_device { + void __iomem *vaddr; + void __iomem *vaddr_end; struct device *dev; - struct resource *res; u8 addr[ETH_ALEN]; struct fman_port *port[2]; u32 if_support;