[PATCH net 0/2] net: dsa: bcm_sf2: Couple fixes

[PATCH net 0/2] net: dsa: bcm_sf2: Couple fixes

[Florian Fainelli]

Hi David,

Here are a couple of fixes for bcm_sf2, please queue these up for -stable
as well, thank you very much!

Florian Fainelli (2):
  net: dsa: bcm_sf2: Do not clobber b53_switch_ops
  net: dsa: bcm_sf2: Utilize nested MDIO read/write

 drivers/net/dsa/bcm_sf2.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.9.3

[PATCH net 1/2] net: dsa: bcm_sf2: Do not clobber b53_switch_ops

[Florian Fainelli]

We make the bcm_sf2 driver override ds->ops which points to
b53_switch_ops since b53_switch_alloc() did the assignent. This is all
well and good until a second b53 switch comes in, and ends up using the
bcm_sf2 operations. Make a proper local copy, substitute the ds->ops
pointer and then override the operations.

Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 9ec33b51a0ed..2f9f910c0e40 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -982,6 +982,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 	const char *reg_names[BCM_SF2_REGS_NUM] = BCM_SF2_REGS_NAME;
 	struct device_node *dn = pdev->dev.of_node;
 	struct b53_platform_data *pdata;
+	struct dsa_switch_ops *ops;
 	struct bcm_sf2_priv *priv;
 	struct b53_device *dev;
 	struct dsa_switch *ds;
@@ -995,6 +996,10 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	ops = devm_kzalloc(&pdev->dev, sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return -ENOMEM;
+
 	dev = b53_switch_alloc(&pdev->dev, &bcm_sf2_io_ops, priv);
 	if (!dev)
 		return -ENOMEM;
@@ -1014,6 +1019,8 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 	ds = dev->ds;
 
 	/* Override the parts that are non-standard wrt. normal b53 devices */
+	memcpy(ops, ds->ops, sizeof(*ops));
+	ds->ops = ops;
 	ds->ops->get_tag_protocol = bcm_sf2_sw_get_tag_protocol;
 	ds->ops->setup = bcm_sf2_sw_setup;
 	ds->ops->get_phy_flags = bcm_sf2_sw_get_phy_flags;
-- 
2.9.3

Re: [PATCH net 1/2] net: dsa: bcm_sf2: Do not clobber b53_switch_ops

[Andrew Lunn]

On Sat, Jan 07, 2017 at 09:01:56PM -0800, Florian Fainelli wrote:
> We make the bcm_sf2 driver override ds->ops which points to
> b53_switch_ops since b53_switch_alloc() did the assignent. This is all
> well and good until a second b53 switch comes in, and ends up using the
> bcm_sf2 operations. Make a proper local copy, substitute the ds->ops
> pointer and then override the operations.
> 
> Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Hi Florian

There is a general trend of making ops structures const. It closes off
kernel exploits. This coping and then modifying prevents us making
ds->ops a pointer to a const.

You are already using b53_common.c as a library. Could you go further
with the concept, and export the ops you need for SF2, and have SF2
define its own ops structure? We can then swap to const ops dsa wide.

Thanks
	Andrew

Re: [PATCH net 1/2] net: dsa: bcm_sf2: Do not clobber b53_switch_ops

[Florian Fainelli]

Le 01/08/17 à 09:41, Andrew Lunn a écrit :
> On Sat, Jan 07, 2017 at 09:01:56PM -0800, Florian Fainelli wrote:
>> We make the bcm_sf2 driver override ds->ops which points to
>> b53_switch_ops since b53_switch_alloc() did the assignent. This is all
>> well and good until a second b53 switch comes in, and ends up using the
>> bcm_sf2 operations. Make a proper local copy, substitute the ds->ops
>> pointer and then override the operations.
>>
>> Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Hi Florian

Hi Andrew,

> 
> There is a general trend of making ops structures const. It closes off
> kernel exploits. This coping and then modifying prevents us making
> ds->ops a pointer to a const.

Agreed, and this was my initial approach, but I also wanted a minimal
fix for David to pull into "net" while we can properly resolve this for
"net-next" see below.

> 
> You are already using b53_common.c as a library. Could you go further
> with the concept, and export the ops you need for SF2, and have SF2
> define its own ops structure? We can then swap to const ops dsa wide.

Making the ops const was my initial approach but there are several
challenges to making it possible right now which I will address against
net-next:

- register/unregister_switch_driver actually do modify dsa_switch_ops
while updating the list pointer, so we need to encapsulate
dsa_switch_ops into a dsa_switch_driver plus a list member

- as you pointed out, b53 needs to export the operations to other
drivers that are going to make use of them

Thanks for your comments!
-- 
Florian

Re: [PATCH net 1/2] net: dsa: bcm_sf2: Do not clobber b53_switch_ops

[Andrew Lunn]

> Agreed, and this was my initial approach, but I also wanted a minimal
> fix for David to pull into "net" while we can properly resolve this for
> "net-next" see below.

O.K, so in that case, this is fine.

> Making the ops const was my initial approach but there are several
> challenges to making it possible right now which I will address against
> net-next:
> 
> - register/unregister_switch_driver actually do modify dsa_switch_ops
> while updating the list pointer, so we need to encapsulate
> dsa_switch_ops into a dsa_switch_driver plus a list member

O.K, this is dsa v1. I had v2 in mind. Yes, the list needs
abstracting.

	Thanks
		Andrew

Re: [PATCH net 1/2] net: dsa: bcm_sf2: Do not clobber b53_switch_ops

[Andrew Lunn]

On Sat, Jan 07, 2017 at 09:01:56PM -0800, Florian Fainelli wrote:
> We make the bcm_sf2 driver override ds->ops which points to
> b53_switch_ops since b53_switch_alloc() did the assignent. This is all
> well and good until a second b53 switch comes in, and ends up using the
> bcm_sf2 operations. Make a proper local copy, substitute the ds->ops
> pointer and then override the operations.
> 
> Fixes: f458995b9ad8 ("net: dsa: bcm_sf2: Utilize core B53 driver when possible")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net 2/2] net: dsa: bcm_sf2: Utilize nested MDIO read/write

[Florian Fainelli]

We are implementing a MDIO bus which is behind another one, so use the
nested version of the accessors to get lockdep annotations correct.

Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 2f9f910c0e40..2ce7ae97ac91 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -393,7 +393,7 @@ static int bcm_sf2_sw_mdio_read(struct mii_bus *bus, int addr, int regnum)
 	if (addr == BRCM_PSEUDO_PHY_ADDR && priv->indir_phy_mask & BIT(addr))
 		return bcm_sf2_sw_indir_rw(priv, 1, addr, regnum, 0);
 	else
-		return mdiobus_read(priv->master_mii_bus, addr, regnum);
+		return mdiobus_read_nested(priv->master_mii_bus, addr, regnum);
 }
 
 static int bcm_sf2_sw_mdio_write(struct mii_bus *bus, int addr, int regnum,
@@ -407,7 +407,7 @@ static int bcm_sf2_sw_mdio_write(struct mii_bus *bus, int addr, int regnum,
 	if (addr == BRCM_PSEUDO_PHY_ADDR && priv->indir_phy_mask & BIT(addr))
 		bcm_sf2_sw_indir_rw(priv, 0, addr, regnum, val);
 	else
-		mdiobus_write(priv->master_mii_bus, addr, regnum, val);
+		mdiobus_write_nested(priv->master_mii_bus, addr, regnum, val);
 
 	return 0;
 }
-- 
2.9.3

Re: [PATCH net 2/2] net: dsa: bcm_sf2: Utilize nested MDIO read/write

[Andrew Lunn]

On Sat, Jan 07, 2017 at 09:01:57PM -0800, Florian Fainelli wrote:
> We are implementing a MDIO bus which is behind another one, so use the
> nested version of the accessors to get lockdep annotations correct.
> 
> Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net 0/2] net: dsa: bcm_sf2: Couple fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Sat,  7 Jan 2017 21:01:55 -0800

> Here are a couple of fixes for bcm_sf2, please queue these up for
> -stable as well, thank you very much!

Series applied and queued up for -stable, thanks.