netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] felix: Fix initialization of ioremap resources
@ 2020-05-22  8:54 Claudiu Manoil
  2020-05-22  9:25 ` Vladimir Oltean
  2020-05-22 21:26 ` David Miller
  0 siblings, 2 replies; 3+ messages in thread
From: Claudiu Manoil @ 2020-05-22  8:54 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, vladimir.oltean

The caller of devm_ioremap_resource(), either accidentally
or by wrong assumption, is writing back derived resource data
to global static resource initialization tables that should
have been constant.  Meaning that after it computes the final
physical start address it saves the address for no reason
in the static tables.  This doesn't affect the first driver
probing after reboot, but it breaks consecutive driver reloads
(i.e. driver unbind & bind) because the initialization tables
no longer have the correct initial values.  So the next probe()
will map the device registers to wrong physical addresses,
causing ARM SError async exceptions.
This patch fixes all of the above.

Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family")

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         | 23 +++++++++++------------
 drivers/net/dsa/ocelot/felix.h         |  6 +++---
 drivers/net/dsa/ocelot/felix_vsc9959.c | 22 ++++++++++------------
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index e2c6bf0..e8aae64 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -388,6 +388,7 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
 	struct ocelot *ocelot = &felix->ocelot;
 	phy_interface_t *port_phy_modes;
 	resource_size_t switch_base;
+	struct resource res;
 	int port, i, err;
 
 	ocelot->num_phys_ports = num_phys_ports;
@@ -422,17 +423,16 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
 
 	for (i = 0; i < TARGET_MAX; i++) {
 		struct regmap *target;
-		struct resource *res;
 
 		if (!felix->info->target_io_res[i].name)
 			continue;
 
-		res = &felix->info->target_io_res[i];
-		res->flags = IORESOURCE_MEM;
-		res->start += switch_base;
-		res->end += switch_base;
+		memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
+		res.flags = IORESOURCE_MEM;
+		res.start += switch_base;
+		res.end += switch_base;
 
-		target = ocelot_regmap_init(ocelot, res);
+		target = ocelot_regmap_init(ocelot, &res);
 		if (IS_ERR(target)) {
 			dev_err(ocelot->dev,
 				"Failed to map device memory space\n");
@@ -453,7 +453,6 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
 	for (port = 0; port < num_phys_ports; port++) {
 		struct ocelot_port *ocelot_port;
 		void __iomem *port_regs;
-		struct resource *res;
 
 		ocelot_port = devm_kzalloc(ocelot->dev,
 					   sizeof(struct ocelot_port),
@@ -465,12 +464,12 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
 			return -ENOMEM;
 		}
 
-		res = &felix->info->port_io_res[port];
-		res->flags = IORESOURCE_MEM;
-		res->start += switch_base;
-		res->end += switch_base;
+		memcpy(&res, &felix->info->port_io_res[port], sizeof(res));
+		res.flags = IORESOURCE_MEM;
+		res.start += switch_base;
+		res.end += switch_base;
 
-		port_regs = devm_ioremap_resource(ocelot->dev, res);
+		port_regs = devm_ioremap_resource(ocelot->dev, &res);
 		if (IS_ERR(port_regs)) {
 			dev_err(ocelot->dev,
 				"failed to map registers for port %d\n", port);
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index 9af1065..730a8a9 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -8,9 +8,9 @@
 
 /* Platform-specific information */
 struct felix_info {
-	struct resource			*target_io_res;
-	struct resource			*port_io_res;
-	struct resource			*imdio_res;
+	const struct resource		*target_io_res;
+	const struct resource		*port_io_res;
+	const struct resource		*imdio_res;
 	const struct reg_field		*regfields;
 	const u32 *const		*map;
 	const struct ocelot_ops		*ops;
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 8bf395f..5211f05 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -333,10 +333,8 @@ static const u32 *vsc9959_regmap[] = {
 	[GCB]	= vsc9959_gcb_regmap,
 };
 
-/* Addresses are relative to the PCI device's base address and
- * will be fixed up at ioremap time.
- */
-static struct resource vsc9959_target_io_res[] = {
+/* Addresses are relative to the PCI device's base address */
+static const struct resource vsc9959_target_io_res[] = {
 	[ANA] = {
 		.start	= 0x0280000,
 		.end	= 0x028ffff,
@@ -379,7 +377,7 @@ static struct resource vsc9959_target_io_res[] = {
 	},
 };
 
-static struct resource vsc9959_port_io_res[] = {
+static const struct resource vsc9959_port_io_res[] = {
 	{
 		.start	= 0x0100000,
 		.end	= 0x010ffff,
@@ -415,7 +413,7 @@ static struct resource vsc9959_port_io_res[] = {
 /* Port MAC 0 Internal MDIO bus through which the SerDes acting as an
  * SGMII/QSGMII MAC PCS can be found.
  */
-static struct resource vsc9959_imdio_res = {
+static const struct resource vsc9959_imdio_res = {
 	.start		= 0x8030,
 	.end		= 0x8040,
 	.name		= "imdio",
@@ -1111,7 +1109,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	struct device *dev = ocelot->dev;
 	resource_size_t imdio_base;
 	void __iomem *imdio_regs;
-	struct resource *res;
+	struct resource res;
 	struct enetc_hw *hw;
 	struct mii_bus *bus;
 	int port;
@@ -1128,12 +1126,12 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	imdio_base = pci_resource_start(felix->pdev,
 					felix->info->imdio_pci_bar);
 
-	res = felix->info->imdio_res;
-	res->flags = IORESOURCE_MEM;
-	res->start += imdio_base;
-	res->end += imdio_base;
+	memcpy(&res, felix->info->imdio_res, sizeof(res));
+	res.flags = IORESOURCE_MEM;
+	res.start += imdio_base;
+	res.end += imdio_base;
 
-	imdio_regs = devm_ioremap_resource(dev, res);
+	imdio_regs = devm_ioremap_resource(dev, &res);
 	if (IS_ERR(imdio_regs)) {
 		dev_err(dev, "failed to map internal MDIO registers\n");
 		return PTR_ERR(imdio_regs);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net] felix: Fix initialization of ioremap resources
  2020-05-22  8:54 [PATCH net] felix: Fix initialization of ioremap resources Claudiu Manoil
@ 2020-05-22  9:25 ` Vladimir Oltean
  2020-05-22 21:26 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: Vladimir Oltean @ 2020-05-22  9:25 UTC (permalink / raw)
  To: Claudiu Manoil; +Cc: netdev, David S . Miller, Vladimir Oltean

On Fri, 22 May 2020 at 11:55, Claudiu Manoil <claudiu.manoil@nxp.com> wrote:
>
> The caller of devm_ioremap_resource(), either accidentally
> or by wrong assumption, is writing back derived resource data
> to global static resource initialization tables that should
> have been constant.  Meaning that after it computes the final
> physical start address it saves the address for no reason
> in the static tables.  This doesn't affect the first driver
> probing after reboot, but it breaks consecutive driver reloads
> (i.e. driver unbind & bind) because the initialization tables
> no longer have the correct initial values.  So the next probe()
> will map the device registers to wrong physical addresses,
> causing ARM SError async exceptions.
> This patch fixes all of the above.
>
> Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family")
>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
> ---

Excellent!

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

# echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind
[   30.935525] mscc_felix 0000:00:00.5 swp0: Link is Up - 1Gbps/Full -
flow control off
[   30.943932] mscc_felix 0000:00:00.5 swp0: Link is Down
[   31.137763] mscc_felix 0000:00:00.5: Link is Down
[   31.155225] DSA: tree 0 torn down
# echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/bind
[   34.712125] libphy: VSC9959 internal MDIO bus: probed
[   34.721083] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 0
[   34.731611] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 1
[   34.741964] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 2
[   34.752351] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 3
[   35.022739] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY
[0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[   35.177379] mscc_felix 0000:00:00.5 swp0: configuring for
inband/qsgmii link mode
[   35.202391] 8021q: adding VLAN 0 to HW filter on device swp0
[   35.262619] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY
[0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[   35.408862] mscc_felix 0000:00:00.5 swp1: configuring for
inband/qsgmii link mode
[   35.445788] 8021q: adding VLAN 0 to HW filter on device swp1
[   35.506616] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY
[0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[   35.652391] mscc_felix 0000:00:00.5 swp2: configuring for
inband/qsgmii link mode
[   35.693938] 8021q: adding VLAN 0 to HW filter on device swp2
[   35.754626] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY
[0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
[   35.768684] mscc_felix 0000:00:00.5: configuring for fixed/internal link mode
[   35.776304] mscc_felix 0000:00:00.5: Link is Up - 2.5Gbps/Full -
flow control off
[   35.783967] DSA: tree 0 setup

>  drivers/net/dsa/ocelot/felix.c         | 23 +++++++++++------------
>  drivers/net/dsa/ocelot/felix.h         |  6 +++---
>  drivers/net/dsa/ocelot/felix_vsc9959.c | 22 ++++++++++------------
>  3 files changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index e2c6bf0..e8aae64 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -388,6 +388,7 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
>         struct ocelot *ocelot = &felix->ocelot;
>         phy_interface_t *port_phy_modes;
>         resource_size_t switch_base;
> +       struct resource res;
>         int port, i, err;
>
>         ocelot->num_phys_ports = num_phys_ports;
> @@ -422,17 +423,16 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
>
>         for (i = 0; i < TARGET_MAX; i++) {
>                 struct regmap *target;
> -               struct resource *res;
>
>                 if (!felix->info->target_io_res[i].name)
>                         continue;
>
> -               res = &felix->info->target_io_res[i];
> -               res->flags = IORESOURCE_MEM;
> -               res->start += switch_base;
> -               res->end += switch_base;
> +               memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
> +               res.flags = IORESOURCE_MEM;
> +               res.start += switch_base;
> +               res.end += switch_base;
>
> -               target = ocelot_regmap_init(ocelot, res);
> +               target = ocelot_regmap_init(ocelot, &res);
>                 if (IS_ERR(target)) {
>                         dev_err(ocelot->dev,
>                                 "Failed to map device memory space\n");
> @@ -453,7 +453,6 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
>         for (port = 0; port < num_phys_ports; port++) {
>                 struct ocelot_port *ocelot_port;
>                 void __iomem *port_regs;
> -               struct resource *res;
>
>                 ocelot_port = devm_kzalloc(ocelot->dev,
>                                            sizeof(struct ocelot_port),
> @@ -465,12 +464,12 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)
>                         return -ENOMEM;
>                 }
>
> -               res = &felix->info->port_io_res[port];
> -               res->flags = IORESOURCE_MEM;
> -               res->start += switch_base;
> -               res->end += switch_base;
> +               memcpy(&res, &felix->info->port_io_res[port], sizeof(res));
> +               res.flags = IORESOURCE_MEM;
> +               res.start += switch_base;
> +               res.end += switch_base;
>
> -               port_regs = devm_ioremap_resource(ocelot->dev, res);
> +               port_regs = devm_ioremap_resource(ocelot->dev, &res);
>                 if (IS_ERR(port_regs)) {
>                         dev_err(ocelot->dev,
>                                 "failed to map registers for port %d\n", port);
> diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
> index 9af1065..730a8a9 100644
> --- a/drivers/net/dsa/ocelot/felix.h
> +++ b/drivers/net/dsa/ocelot/felix.h
> @@ -8,9 +8,9 @@
>
>  /* Platform-specific information */
>  struct felix_info {
> -       struct resource                 *target_io_res;
> -       struct resource                 *port_io_res;
> -       struct resource                 *imdio_res;
> +       const struct resource           *target_io_res;
> +       const struct resource           *port_io_res;
> +       const struct resource           *imdio_res;
>         const struct reg_field          *regfields;
>         const u32 *const                *map;
>         const struct ocelot_ops         *ops;
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 8bf395f..5211f05 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -333,10 +333,8 @@ static const u32 *vsc9959_regmap[] = {
>         [GCB]   = vsc9959_gcb_regmap,
>  };
>
> -/* Addresses are relative to the PCI device's base address and
> - * will be fixed up at ioremap time.
> - */
> -static struct resource vsc9959_target_io_res[] = {
> +/* Addresses are relative to the PCI device's base address */
> +static const struct resource vsc9959_target_io_res[] = {
>         [ANA] = {
>                 .start  = 0x0280000,
>                 .end    = 0x028ffff,
> @@ -379,7 +377,7 @@ static struct resource vsc9959_target_io_res[] = {
>         },
>  };
>
> -static struct resource vsc9959_port_io_res[] = {
> +static const struct resource vsc9959_port_io_res[] = {
>         {
>                 .start  = 0x0100000,
>                 .end    = 0x010ffff,
> @@ -415,7 +413,7 @@ static struct resource vsc9959_port_io_res[] = {
>  /* Port MAC 0 Internal MDIO bus through which the SerDes acting as an
>   * SGMII/QSGMII MAC PCS can be found.
>   */
> -static struct resource vsc9959_imdio_res = {
> +static const struct resource vsc9959_imdio_res = {
>         .start          = 0x8030,
>         .end            = 0x8040,
>         .name           = "imdio",
> @@ -1111,7 +1109,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>         struct device *dev = ocelot->dev;
>         resource_size_t imdio_base;
>         void __iomem *imdio_regs;
> -       struct resource *res;
> +       struct resource res;
>         struct enetc_hw *hw;
>         struct mii_bus *bus;
>         int port;
> @@ -1128,12 +1126,12 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>         imdio_base = pci_resource_start(felix->pdev,
>                                         felix->info->imdio_pci_bar);
>
> -       res = felix->info->imdio_res;
> -       res->flags = IORESOURCE_MEM;
> -       res->start += imdio_base;
> -       res->end += imdio_base;
> +       memcpy(&res, felix->info->imdio_res, sizeof(res));
> +       res.flags = IORESOURCE_MEM;
> +       res.start += imdio_base;
> +       res.end += imdio_base;
>
> -       imdio_regs = devm_ioremap_resource(dev, res);
> +       imdio_regs = devm_ioremap_resource(dev, &res);
>         if (IS_ERR(imdio_regs)) {
>                 dev_err(dev, "failed to map internal MDIO registers\n");
>                 return PTR_ERR(imdio_regs);
> --
> 2.7.4
>

Thanks,
-Vladimir

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net] felix: Fix initialization of ioremap resources
  2020-05-22  8:54 [PATCH net] felix: Fix initialization of ioremap resources Claudiu Manoil
  2020-05-22  9:25 ` Vladimir Oltean
@ 2020-05-22 21:26 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2020-05-22 21:26 UTC (permalink / raw)
  To: claudiu.manoil; +Cc: netdev, vladimir.oltean

From: Claudiu Manoil <claudiu.manoil@nxp.com>
Date: Fri, 22 May 2020 11:54:34 +0300

> The caller of devm_ioremap_resource(), either accidentally
> or by wrong assumption, is writing back derived resource data
> to global static resource initialization tables that should
> have been constant.  Meaning that after it computes the final
> physical start address it saves the address for no reason
> in the static tables.  This doesn't affect the first driver
> probing after reboot, but it breaks consecutive driver reloads
> (i.e. driver unbind & bind) because the initialization tables
> no longer have the correct initial values.  So the next probe()
> will map the device registers to wrong physical addresses,
> causing ARM SError async exceptions.
> This patch fixes all of the above.
> 
> Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family")
> 
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Applied and queued up for -stable, thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-22 21:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-22  8:54 [PATCH net] felix: Fix initialization of ioremap resources Claudiu Manoil
2020-05-22  9:25 ` Vladimir Oltean
2020-05-22 21:26 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).