| Message ID | 20200923142528.303730-1-s.riedmueller@phytec.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: fec: Keep device numbering consistent with datasheet | expand |
On Wed, Sep 23, 2020 at 04:25:28PM +0200, Stefan Riedmueller wrote: > From: Christian Hemp <c.hemp@phytec.de> > > Make use of device tree alias for device enumeration to keep the device > order consistent with the naming in the datasheet. > > Otherwise for the i.MX 6UL/ULL the ENET1 interface is enumerated as eth1 > and ENET2 as eth0. What is wrong with that? Ethernet interface names have never been guaranteed to be stable. You have to be careful here. Have you reviewed all the DTS files to make sure none already have aliases? You could be breaking boards which currently 'work' because the alias is ignored. udev is actually a better solution for giving the interfaces dependable names. Andrew
From: Stefan Riedmueller <s.riedmueller@phytec.de> Date: Wed, 23 Sep 2020 16:25:28 +0200 > From: Christian Hemp <c.hemp@phytec.de> > > Make use of device tree alias for device enumeration to keep the device > order consistent with the naming in the datasheet. > > Otherwise for the i.MX 6UL/ULL the ENET1 interface is enumerated as eth1 > and ENET2 as eth0. > > Signed-off-by: Christian Hemp <c.hemp@phytec.de> > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> Device naming and ordering for networking devices was never, ever, guaranteed. Use udev or similar. > @@ -3691,6 +3692,10 @@ fec_probe(struct platform_device *pdev) > > ndev->max_mtu = PKT_MAXBUF_SIZE - ETH_HLEN - ETH_FCS_LEN; > > + eth_id = of_alias_get_id(pdev->dev.of_node, "ethernet"); > + if (eth_id >= 0) > + sprintf(ndev->name, "eth%d", eth_id); You can't ever just write into ndev->name, what if another networking device is already using that name? This change is incorrect on many levels.
From: David Miller <davem@davemloft.net> Sent: Thursday, September 24, 2020 4:32 AM > From: Stefan Riedmueller <s.riedmueller@phytec.de> > Date: Wed, 23 Sep 2020 16:25:28 +0200 > > > From: Christian Hemp <c.hemp@phytec.de> > > > > Make use of device tree alias for device enumeration to keep the > > device order consistent with the naming in the datasheet. > > > > Otherwise for the i.MX 6UL/ULL the ENET1 interface is enumerated as > > eth1 and ENET2 as eth0. > > > > Signed-off-by: Christian Hemp <c.hemp@phytec.de> > > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> > > Device naming and ordering for networking devices was never, ever, > guaranteed. > > Use udev or similar. > > > @@ -3691,6 +3692,10 @@ fec_probe(struct platform_device *pdev) > > > > ndev->max_mtu = PKT_MAXBUF_SIZE - ETH_HLEN - ETH_FCS_LEN; > > > > + eth_id = of_alias_get_id(pdev->dev.of_node, "ethernet"); > > + if (eth_id >= 0) > > + sprintf(ndev->name, "eth%d", eth_id); > > You can't ever just write into ndev->name, what if another networking device is > already using that name? > > This change is incorrect on many levels. David is correct. For example, imx8DXL has ethernet0 is EQOS TSN, ethernet1 is FEC. EQOS TSN is andother driver and is registered early, the dev->name is eth0. So the patch will bring conflict in such case. Andy
Hi Andy, David and Andrew, first of all, thanks for your review. I really appreciate it! On 24.09.20 08:36, Andy Duan wrote: > From: David Miller <davem@davemloft.net> Sent: Thursday, September 24, 2020 4:32 AM >> From: Stefan Riedmueller <s.riedmueller@phytec.de> >> Date: Wed, 23 Sep 2020 16:25:28 +0200 >> >>> From: Christian Hemp <c.hemp@phytec.de> >>> >>> Make use of device tree alias for device enumeration to keep the >>> device order consistent with the naming in the datasheet. >>> >>> Otherwise for the i.MX 6UL/ULL the ENET1 interface is enumerated as >>> eth1 and ENET2 as eth0. >>> >>> Signed-off-by: Christian Hemp <c.hemp@phytec.de> >>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de> >> >> Device naming and ordering for networking devices was never, ever, >> guaranteed. >> >> Use udev or similar. >> >>> @@ -3691,6 +3692,10 @@ fec_probe(struct platform_device *pdev) >>> >>> ndev->max_mtu = PKT_MAXBUF_SIZE - ETH_HLEN - ETH_FCS_LEN; >>> >>> + eth_id = of_alias_get_id(pdev->dev.of_node, "ethernet"); >>> + if (eth_id >= 0) >>> + sprintf(ndev->name, "eth%d", eth_id); >> >> You can't ever just write into ndev->name, what if another networking device is >> already using that name? >> >> This change is incorrect on many levels. > > David is correct. > > For example, imx8DXL has ethernet0 is EQOS TSN, ethernet1 is FEC. > EQOS TSN is andother driver and is registered early, the dev->name is eth0. > So the patch will bring conflict in such case. I was not aware of that conflict, but now that you mention it it makes total sense. I wanted to make life a little easier for myself but underestimated the global context. I will try to find a solution with udev or something similar. So please drop this patch and sorry for the noise. Stefan > > Andy >
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index fb37816a74db..97dd41bed70a 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -3504,6 +3504,7 @@ fec_probe(struct platform_device *pdev) char irq_name[8]; int irq_cnt; struct fec_devinfo *dev_info; + int eth_id; fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs); @@ -3691,6 +3692,10 @@ fec_probe(struct platform_device *pdev) ndev->max_mtu = PKT_MAXBUF_SIZE - ETH_HLEN - ETH_FCS_LEN; + eth_id = of_alias_get_id(pdev->dev.of_node, "ethernet"); + if (eth_id >= 0) + sprintf(ndev->name, "eth%d", eth_id); + ret = register_netdev(ndev); if (ret) goto failed_register;