| Message ID | 1978999632.15003049.1719232814587.JavaMail.zimbra@univ-grenoble-alpes.fr |
|---|---|
| State | Changes Requested |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Improve networking support on FriendlyElec Nanopi R5C | expand |
Hi Etienne, On 6/24/24 2:40 PM, ETIENNE DUBLE wrote: > This commit implements the .bind member function, > and gives a different name to interfaces: > "RTL8169#0", "RTL8169#1", etc. > > This was tested on a FriendlyElec Nanopi R5C board, > which has two RTL-8125B interfaces managed by this > driver. Since they were given the same name, it > was previously not possible to select the 2nd one > using ethact or ethprime environment variables. > > Signed-off-by: Etienne Dublé <etienne.duble@imag.fr> > --- > > drivers/net/rtl8169.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c > index 93e83661ce..b30d51731f 100644 > --- a/drivers/net/rtl8169.c > +++ b/drivers/net/rtl8169.c > @@ -1091,6 +1091,16 @@ static int rtl8169_eth_probe(struct udevice *dev) > return 0; > } > > +static int rtl8169_eth_bind(struct udevice *dev) > +{ > + static int card_number; > + char name[16]; > + > + sprintf(name, "RTL8169#%u", card_number++); > + > + return device_set_name(dev, name); > +} > + I don't think we can guarantee bind order so this may not be stable over time. I'm wondering if there isn't a way to use the "ethernet" (ethernet0, ethernet1) alias in DT instead? Cheers, Quentin
Hi Quentin, Thanks for reviewing my patches. Le 24/06/2024 à 15:08, Quentin Schulz a écrit : > [...] > >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c >> index 93e83661ce..b30d51731f 100644 >> --- a/drivers/net/rtl8169.c >> +++ b/drivers/net/rtl8169.c >> @@ -1091,6 +1091,16 @@ static int rtl8169_eth_probe(struct udevice *dev) >> return 0; >> } >> +static int rtl8169_eth_bind(struct udevice *dev) >> +{ >> + static int card_number; >> + char name[16]; >> + >> + sprintf(name, "RTL8169#%u", card_number++); >> + >> + return device_set_name(dev, name); >> +} >> + > > I don't think we can guarantee bind order so this may not be stable > over time. > > I'm wondering if there isn't a way to use the "ethernet" (ethernet0, > ethernet1) alias in DT instead? Actually the ethernet interfaces are not declared in the device tree, only PCI buses are (at least on this Nanopi board). The ethernet interfaces are only detected when running "pci enum". Another option may be to name them "rtl8169@<hexa>", with "<hexa>" reflecting the PCI region address (so it is unique and stable). What do you think? Cheers, Etienne
Hi Etienne, On 6/25/24 8:38 AM, Etienne Dublé wrote: > Hi Quentin, > > Thanks for reviewing my patches. > > Le 24/06/2024 à 15:08, Quentin Schulz a écrit : >> [...] >> >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c >>> index 93e83661ce..b30d51731f 100644 >>> --- a/drivers/net/rtl8169.c >>> +++ b/drivers/net/rtl8169.c >>> @@ -1091,6 +1091,16 @@ static int rtl8169_eth_probe(struct udevice *dev) >>> return 0; >>> } >>> +static int rtl8169_eth_bind(struct udevice *dev) >>> +{ >>> + static int card_number; >>> + char name[16]; >>> + >>> + sprintf(name, "RTL8169#%u", card_number++); >>> + >>> + return device_set_name(dev, name); >>> +} >>> + >> >> I don't think we can guarantee bind order so this may not be stable >> over time. >> >> I'm wondering if there isn't a way to use the "ethernet" (ethernet0, >> ethernet1) alias in DT instead? > > Actually the ethernet interfaces are not declared in the device tree, > only PCI buses are (at least on this Nanopi board). > The ethernet interfaces are only detected when running "pci enum". > Ah shoot. > Another option may be to name them "rtl8169@<hexa>", with "<hexa>" > reflecting the PCI region address (so it is unique and stable). What do > you think? > I guess that's one way, I'm also wondering how systemd renames those to be unique but stable on the same machine, maybe we could take some inspiration from them for that? FYI, for NVMEs I also have /dev/disk/by-path/platform-fe190000.pcie-pci-0004:41:00.0-nvme-1 for example. platform-fe19000.pcie- being the pcie controller at address fe19000 on the platform root bus, no clue about what's after that though. I also assume the N in nvme-N isn't necessarily stable over time but whatever's before should be? Maybe there's something like ports/addresses on the PCIe bus that will never change or rely on probe order/timings to keep a stable naming? Sorry, don't know much about PCIe so cannot suggest anything meaningful :/ Cheers, Quentin
Hello, On 2024-06-25 12:03, Quentin Schulz wrote: > On 6/25/24 8:38 AM, Etienne Dublé wrote: >> Thanks for reviewing my patches. >> >> Le 24/06/2024 à 15:08, Quentin Schulz a écrit : >>> [...] >>> >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c >>>> index 93e83661ce..b30d51731f 100644 >>>> --- a/drivers/net/rtl8169.c >>>> +++ b/drivers/net/rtl8169.c >>>> @@ -1091,6 +1091,16 @@ static int rtl8169_eth_probe(struct udevice >>>> *dev) >>>> return 0; >>>> } >>>> +static int rtl8169_eth_bind(struct udevice *dev) >>>> +{ >>>> + static int card_number; >>>> + char name[16]; >>>> + >>>> + sprintf(name, "RTL8169#%u", card_number++); >>>> + >>>> + return device_set_name(dev, name); >>>> +} >>>> + >>> >>> I don't think we can guarantee bind order so this may not be stable >>> over time. >>> >>> I'm wondering if there isn't a way to use the "ethernet" (ethernet0, >>> ethernet1) alias in DT instead? >> >> Actually the ethernet interfaces are not declared in the device tree, >> only PCI buses are (at least on this Nanopi board). >> The ethernet interfaces are only detected when running "pci enum". >> > > Ah shoot. > >> Another option may be to name them "rtl8169@<hexa>", with "<hexa>" >> reflecting the PCI region address (so it is unique and stable). What >> do you think? >> > > I guess that's one way, I'm also wondering how systemd renames those > to be unique but stable on the same machine, maybe we could take some > inspiration from them for that? Systemd also allows renaming of network interfaces using some rules predefined by the user, so there's also the possibility to rename the interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules that would be based on the PCI region address. > FYI, for NVMEs I also have > /dev/disk/by-path/platform-fe190000.pcie-pci-0004:41:00.0-nvme-1 for > example. platform-fe19000.pcie- being the pcie controller at address > fe19000 on the platform root bus, no clue about what's after that > though. I also assume the N in nvme-N isn't necessarily stable over > time but whatever's before should be? Maybe there's something like > ports/addresses on the PCIe bus that will never change or rely on > probe order/timings to keep a stable naming? > Sorry, don't know much about PCIe so cannot suggest anything meaningful > :/
Hello, Le 25/06/2024 à 12:25, Dragan Simic a écrit : >>> Another option may be to name them "rtl8169@<hexa>", with "<hexa>" >>> reflecting the PCI region address (so it is unique and stable). What >>> do you think? >> >> I guess that's one way, I'm also wondering how systemd renames those >> to be unique but stable on the same machine, maybe we could take some >> inspiration from them for that? > > Systemd also allows renaming of network interfaces using some rules > predefined by the user, so there's also the possibility to rename the > interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules > that would be based on the PCI region address. OK but the renaming occurs in the rtl8169 driver, that is used by several boards, whereas the PCI region addresses come from the device-tree, so they differ from board to board. Regards, Etienne
Hi Etienne, On 6/25/24 1:42 PM, Etienne Dublé wrote: > > Hello, > > Le 25/06/2024 à 12:25, Dragan Simic a écrit : >>>> Another option may be to name them "rtl8169@<hexa>", with "<hexa>" >>>> reflecting the PCI region address (so it is unique and stable). What >>>> do you think? >>> >>> I guess that's one way, I'm also wondering how systemd renames those >>> to be unique but stable on the same machine, maybe we could take some >>> inspiration from them for that? >> >> Systemd also allows renaming of network interfaces using some rules >> predefined by the user, so there's also the possibility to rename the >> interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules >> that would be based on the PCI region address. > > OK but the renaming occurs in the rtl8169 driver, that is used by > several boards, whereas the PCI region addresses come from the > device-tree, so they differ from board to board. > I'm of the opinion that we only care about stability for the same product, not for different products with the same Ethernet PHY on different SoCs/products. Cheers, Quentin
Hi Quentin, Le 27/06/2024 à 14:27, Quentin Schulz a écrit : > Hi Etienne, > > On 6/25/24 1:42 PM, Etienne Dublé wrote: >> >> Hello, >> >> Le 25/06/2024 à 12:25, Dragan Simic a écrit : >>>>> Another option may be to name them "rtl8169@<hexa>", with "<hexa>" >>>>> reflecting the PCI region address (so it is unique and stable). >>>>> What do you think? >>>> >>>> I guess that's one way, I'm also wondering how systemd renames those >>>> to be unique but stable on the same machine, maybe we could take some >>>> inspiration from them for that? >>> >>> Systemd also allows renaming of network interfaces using some rules >>> predefined by the user, so there's also the possibility to rename the >>> interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules >>> that would be based on the PCI region address. >> >> OK but the renaming occurs in the rtl8169 driver, that is used by >> several boards, whereas the PCI region addresses come from the >> device-tree, so they differ from board to board. >> > > I'm of the opinion that we only care about stability for the same > product, not for different products with the same Ethernet PHY on > different SoCs/products. I agree, but my comment was more about how u-boot code is structured (network interface naming is done in the driver, and the driver code should probably not include board-specific code, so we are quite limited). In theory we could just name the interfaces ethernet0, ethernet1, etc., by using the PCI region address as the sort key, and maybe this is what Dragan was suggesting. But in practice when driver->bind() is called we don't know if more interfaces will be discovered next; and renaming the 1st interface when the 2nd one is discovered with a lower address is probably not the kind of code u-boot code experts would like... Cheers, Etienne Dublé
Hello, On 2024-06-28 08:51, Etienne Dublé wrote: > Le 27/06/2024 à 14:27, Quentin Schulz a écrit : >> On 6/25/24 1:42 PM, Etienne Dublé wrote: >> Le 25/06/2024 à 12:25, Dragan Simic a écrit : >> Another option may be to name them "rtl8169@<hexa>", with "<hexa>" >> reflecting the PCI region address (so it is unique and stable). What >> do you think? >> >> I guess that's one way, I'm also wondering how systemd renames those >> >> to be unique but stable on the same machine, maybe we could take >> some >> inspiration from them for that? > > Systemd also allows renaming of network interfaces using some rules > predefined by the user, so there's also the possibility to rename the > interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules > that would be based on the PCI region address. > > OK but the renaming occurs in the rtl8169 driver, that is used by > several boards, whereas the PCI region addresses come from the > device-tree, so they differ from board to board. > > I'm of the opinion that we only care about stability for the same > product, not for different products with the same Ethernet PHY on > different SoCs/products. > > I agree, but my comment was more about how u-boot code is structured > (network interface naming is done in the driver, and the driver code > should probably not include board-specific code, so we are quite > limited). > In theory we could just name the interfaces ethernet0, ethernet1, > etc., by using the PCI region address as the sort key, and maybe this > is what Dragan was suggesting. But in practice when driver->bind() is > called we don't know if more interfaces will be discovered next; and > renaming the 1st interface when the 2nd one is discovered with a lower > address is probably not the kind of code u-boot code experts would > like... Good point, we don't know how many Ethernet interfaces will be there in total. I'll see to go through the source code, to see what options are available for stable interface naming.
diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c index 93e83661ce..b30d51731f 100644 --- a/drivers/net/rtl8169.c +++ b/drivers/net/rtl8169.c @@ -1091,6 +1091,16 @@ static int rtl8169_eth_probe(struct udevice *dev) return 0; } +static int rtl8169_eth_bind(struct udevice *dev) +{ + static int card_number; + char name[16]; + + sprintf(name, "RTL8169#%u", card_number++); + + return device_set_name(dev, name); +} + static const struct eth_ops rtl8169_eth_ops = { .start = rtl8169_eth_start, .send = rtl8169_eth_send, @@ -1108,6 +1118,7 @@ U_BOOT_DRIVER(eth_rtl8169) = { .name = "eth_rtl8169", .id = UCLASS_ETH, .of_match = rtl8169_eth_ids, + .bind = rtl8169_eth_bind, .probe = rtl8169_eth_probe, .ops = &rtl8169_eth_ops, .priv_auto = sizeof(struct rtl8169_private),
This commit implements the .bind member function, and gives a different name to interfaces: "RTL8169#0", "RTL8169#1", etc. This was tested on a FriendlyElec Nanopi R5C board, which has two RTL-8125B interfaces managed by this driver. Since they were given the same name, it was previously not possible to select the 2nd one using ethact or ethprime environment variables. Signed-off-by: Etienne Dublé <etienne.duble@imag.fr> --- drivers/net/rtl8169.c | 11 +++++++++++ 1 file changed, 11 insertions(+)