| Message ID | 20200217223651.22688-1-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: fec: Use a proper ID allocation scheme | expand |
From: Fabio Estevam <festevam@gmail.com> Date: Mon, 17 Feb 2020 19:36:51 -0300 > Instead of using such poor mechanism for counting the network interfaces > IDs, use a proper allocation scheme, such as IDR. > > This fixes the network behavior after unbind/bind. What about: 1) unbind fec0 2) unbind fec1 3) bind fec0 It doesn't work even with the IDR scheme.
From: David Miller <davem@davemloft.net> Sent: Tuesday, February 18, 2020 1:49 PM > From: Fabio Estevam <festevam@gmail.com> > Date: Mon, 17 Feb 2020 19:36:51 -0300 > > > Instead of using such poor mechanism for counting the network > > interfaces IDs, use a proper allocation scheme, such as IDR. > > > > This fixes the network behavior after unbind/bind. > > What about: > > 1) unbind fec0 > 2) unbind fec1 > 3) bind fec0 > > It doesn't work even with the IDR scheme. Not only such case, instance#A (maybe fec0 or fec1) depends on instance#B (maybe fec1 or fec0), Unbind instance#B firstly has problem. Bind instance#A firstly also has problem.
Hi Andy, On Tue, Feb 18, 2020 at 3:51 AM Andy Duan <fugang.duan@nxp.com> wrote: > > What about: > > > > 1) unbind fec0 > > 2) unbind fec1 > > 3) bind fec0 > > > > It doesn't work even with the IDR scheme. > > Not only such case, instance#A (maybe fec0 or fec1) depends on instance#B (maybe fec1 or fec0), > Unbind instance#B firstly has problem. > Bind instance#A firstly also has problem. Yes, I do see the error now with the sequence suggested by David. I have also noticed in the fec_main.c comments: /* * The i.MX28 dual fec interfaces are not equal. * Here are the differences: * * - fec0 supports MII & RMII modes while fec1 only supports RMII * - fec0 acts as the 1588 time master while fec1 is slave * - external phys can only be configured by fec0 * * That is to say fec1 can not work independently. It only works * when fec0 is working. The reason behind this design is that the * second interface is added primarily for Switch mode. * * Because of the last point above, both phys are attached on fec0 * mdio interface in board design, and need to be configured by * fec0 mii_bus. */ Should we prevent unbind operation from this driver like this? diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 4432a59904c7..1d348c5d0794 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -3793,6 +3793,7 @@ static struct platform_driver fec_driver = { .name = DRIVER_NAME, .pm = &fec_pm_ops, .of_match_table = fec_dt_ids, + .suppress_bind_attrs = true }, .id_table = fec_devtype, .probe = fec_probe, Please advise. Thanks
From: Fabio Estevam <festevam@gmail.com> Sent: Tuesday, February 18, 2020 9:25 PM > Hi Andy, > > On Tue, Feb 18, 2020 at 3:51 AM Andy Duan <fugang.duan@nxp.com> wrote: > > > > What about: > > > > > > 1) unbind fec0 > > > 2) unbind fec1 > > > 3) bind fec0 > > > > > > It doesn't work even with the IDR scheme. > > > > Not only such case, instance#A (maybe fec0 or fec1) depends on > > instance#B (maybe fec1 or fec0), Unbind instance#B firstly has problem. > > Bind instance#A firstly also has problem. > > Yes, I do see the error now with the sequence suggested by David. > > I have also noticed in the fec_main.c comments: > > /* > * The i.MX28 dual fec interfaces are not equal. > * Here are the differences: > * > * - fec0 supports MII & RMII modes while fec1 only supports RMII > * - fec0 acts as the 1588 time master while fec1 is slave > * - external phys can only be configured by fec0 > * > * That is to say fec1 can not work independently. It only works > * when fec0 is working. The reason behind this design is that the > * second interface is added primarily for Switch mode. > * > * Because of the last point above, both phys are attached on fec0 > * mdio interface in board design, and need to be configured by > * fec0 mii_bus. > */ > > Should we prevent unbind operation from this driver like this? > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 4432a59904c7..1d348c5d0794 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -3793,6 +3793,7 @@ static struct platform_driver fec_driver = { > .name = DRIVER_NAME, > .pm = &fec_pm_ops, > .of_match_table = fec_dt_ids, > + .suppress_bind_attrs = true > }, > .id_table = fec_devtype, > .probe = fec_probe, > > Please advise. > > Thanks For imx6sl/imx8mp/imx8mm/imx8mn, soc only has one instance, bind operation is supported and has no problem. Regards, Andy
Hi Andy, On Tue, Feb 18, 2020 at 10:54 AM Andy Duan <fugang.duan@nxp.com> wrote: > For imx6sl/imx8mp/imx8mm/imx8mn, soc only has one instance, bind operation > is supported and has no problem. This is not true. As per the commit log, here is the result of unbind/bind on a i.mx6qp, which only has a single FEC instance: # echo 2188000.ethernet > /sys/bus/platform/drivers/fec/unbind # echo 2188000.ethernet > /sys/bus/platform/drivers/fec/bind [ 10.756519] pps pps0: new PPS source ptp0 [ 10.792626] libphy: fec_enet_mii_bus: probed [ 10.799330] fec 2188000.ethernet eth0: registered PHC device 1 # udhcpc -i eth0 udhcpc: started, v1.31.1 [ 14.985211] fec 2188000.ethernet eth0: no PHY, assuming direct connection to switch [ 14.993140] libphy: PHY fixed-0:00 not found [ 14.997643] fec 2188000.ethernet eth0: could not attach to PHY After performing unbind/bind operation the network is not functional at all. Don't you agree that unbind/bind is currently broken here even for SoCs with a single FEC? Should we prevent unbind? Or any other suggestion?
From: Fabio Estevam <festevam@gmail.com> Sent: Tuesday, February 18, 2020 10:05 PM > Hi Andy, > > On Tue, Feb 18, 2020 at 10:54 AM Andy Duan <fugang.duan@nxp.com> > wrote: > > > For imx6sl/imx8mp/imx8mm/imx8mn, soc only has one instance, bind > > operation is supported and has no problem. > > This is not true. > > As per the commit log, here is the result of unbind/bind on a i.mx6qp, which > only has a single FEC instance: I mean if apply the patch, it should work for one instance. > > # echo 2188000.ethernet > /sys/bus/platform/drivers/fec/unbind > # echo 2188000.ethernet > /sys/bus/platform/drivers/fec/bind > [ 10.756519] pps pps0: new PPS source ptp0 > [ 10.792626] libphy: fec_enet_mii_bus: probed > [ 10.799330] fec 2188000.ethernet eth0: registered PHC device 1 > # udhcpc -i eth0 > udhcpc: started, v1.31.1 > [ 14.985211] fec 2188000.ethernet eth0: no PHY, assuming direct > connection to switch > [ 14.993140] libphy: PHY fixed-0:00 not found > [ 14.997643] fec 2188000.ethernet eth0: could not attach to PHY > > After performing unbind/bind operation the network is not functional at all. > > Don't you agree that unbind/bind is currently broken here even for SoCs with > a single FEC? > > Should we prevent unbind? Or any other suggestion? Suppose apply the patch, it can work for one instance, but not for two instances. Currently, I agree to prevent unbind operation.
On Tue, Feb 18, 2020 at 11:30 AM Andy Duan <fugang.duan@nxp.com> wrote:
> Currently, I agree to prevent unbind operation.
Ok, thanks for the feedback.
I will submit a patch doing this.
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index f79e57f735b3..0d718545b9a2 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -528,6 +528,7 @@ struct fec_enet_private { struct platform_device *pdev; int dev_id; + struct idr idr; /* Phylib and MDIO interface */ struct mii_bus *mii_bus; diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 4432a59904c7..77b63ecf96b4 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -62,6 +62,7 @@ #include <linux/if_vlan.h> #include <linux/pinctrl/consumer.h> #include <linux/prefetch.h> +#include <linux/idr.h> #include <soc/imx/cpuidle.h> #include <asm/cacheflush.h> @@ -1949,7 +1950,6 @@ static int fec_enet_mii_probe(struct net_device *ndev) char mdio_bus_id[MII_BUS_ID_SIZE]; char phy_name[MII_BUS_ID_SIZE + 3]; int phy_id; - int dev_id = fep->dev_id; if (fep->phy_node) { phy_dev = of_phy_connect(ndev, fep->phy_node, @@ -1964,8 +1964,6 @@ static int fec_enet_mii_probe(struct net_device *ndev) for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { if (!mdiobus_is_registered_device(fep->mii_bus, phy_id)) continue; - if (dev_id--) - continue; strlcpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE); break; } @@ -3406,7 +3404,6 @@ fec_probe(struct platform_device *pdev) struct net_device *ndev; int i, irq, ret = 0; const struct of_device_id *of_id; - static int dev_id; struct device_node *np = pdev->dev.of_node, *phy_node; int num_tx_qs; int num_rx_qs; @@ -3451,7 +3448,13 @@ fec_probe(struct platform_device *pdev) } fep->pdev = pdev; - fep->dev_id = dev_id++; + idr_init(&fep->idr); + + ret = idr_alloc_cyclic(&fep->idr, fep, 0, INT_MAX, GFP_KERNEL); + if (ret < 0) + return ret; + + fep->dev_id = ret; platform_set_drvdata(pdev, ndev); @@ -3632,7 +3635,6 @@ fec_probe(struct platform_device *pdev) of_phy_deregister_fixed_link(np); of_node_put(phy_node); failed_phy: - dev_id--; failed_ioremap: free_netdev(ndev); @@ -3661,6 +3663,7 @@ fec_drv_remove(struct platform_device *pdev) if (of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); of_node_put(fep->phy_node); + idr_destroy(&fep->idr); free_netdev(ndev); clk_disable_unprepare(fep->clk_ahb);
Currently when performing an unbind/bind operation network is no longer functional: # echo 2188000.ethernet > /sys/bus/platform/drivers/fec/unbind # echo 2188000.ethernet > /sys/bus/platform/drivers/fec/bind [ 10.756519] pps pps0: new PPS source ptp0 [ 10.792626] libphy: fec_enet_mii_bus: probed [ 10.799330] fec 2188000.ethernet eth0: registered PHC device 1 # udhcpc -i eth0 udhcpc: started, v1.31.1 [ 14.985211] fec 2188000.ethernet eth0: no PHY, assuming direct connection to switch [ 14.993140] libphy: PHY fixed-0:00 not found [ 14.997643] fec 2188000.ethernet eth0: could not attach to PHY ifconfig: SIOCSIFFLAGS: No such device The reason for the failure is that fec_drv_remove() does not decrement the dev_id variable. Instead of using such poor mechanism for counting the network interfaces IDs, use a proper allocation scheme, such as IDR. This fixes the network behavior after unbind/bind. Tested on a imx6qp-sabresd board. Suggested-by: David Miller <davem@davemloft.net> Signed-off-by: Fabio Estevam <festevam@gmail.com> --- Hi, There was a prior attempt to fix this problem: https://www.spinics.net/lists/netdev/msg616487.html , but it was rejected and David suggested the usage of IDR. drivers/net/ethernet/freescale/fec.h | 1 + drivers/net/ethernet/freescale/fec_main.c | 15 +++++++++------ 2 files changed, 10 insertions(+), 6 deletions(-)