| Message ID | 20230329144548.66708-2-louis.peens@corigine.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Small series of enhancements | expand |
On Wed, Mar 29, 2023 at 04:45:47PM +0200, Louis Peens wrote: > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > `dev_port` is used to differentiate devices that share the same > function, which is the case in most of NFP NICs. And how did it work without dev_port? I have no idea what does it mean "different devices that share the same function". Thanks > > In some customized scenario, `dev_port` is used to rename netdev > instead of `phys_port_name`, which requires to initialize it > correctly to get expected netdev name. > > Example rules using `dev_port`: > > SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", ATTR{dev_port}=="0", NAME:="ens8np0" > SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", ATTR{dev_port}=="1", NAME:="ens8np1" > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com> > Acked-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Louis Peens <louis.peens@corigine.com> > --- > drivers/net/ethernet/netronome/nfp/nfp_port.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c > index 4f2308570dcf..54640bcb70fb 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_port.c > +++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c > @@ -189,6 +189,7 @@ int nfp_port_init_phy_port(struct nfp_pf *pf, struct nfp_app *app, > > port->eth_port = &pf->eth_tbl->ports[id]; > port->eth_id = pf->eth_tbl->ports[id].index; > + port->netdev->dev_port = id; > if (pf->mac_stats_mem) > port->eth_stats = > pf->mac_stats_mem + port->eth_id * NFP_MAC_STATS_SIZE; > -- > 2.34.1 >
On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote: > In some customized scenario, `dev_port` is used to rename netdev > instead of `phys_port_name`, which requires to initialize it > correctly to get expected netdev name. What do you mean by "which requires to initialize it correctly to get expected netdev name." ?
On Wed, 29 Mar 2023 21:49:59 +0300, Leon Romanovsky wrote: > On Wed, Mar 29, 2023 at 04:45:47PM +0200, Louis Peens wrote: > > From: Yinjun Zhang <yinjun.zhang@corigine.com> > > > > `dev_port` is used to differentiate devices that share the same > > function, which is the case in most of NFP NICs. > > And how did it work without dev_port? No functionality fault, just cannot rename netdev as expected when the udev rules use `dev_port` attribute as the example below. > I have no idea what does it mean "different devices that share the same > function". That's how it's commented for the `dev_port` field of struct netdev: * @dev_port: Used to differentiate devices that share * the same function I think it's used when single pci function instantiates more than one netdev. Ref: 3f85944fe207 ("net: Add sysfs file for port number") > > Thanks > > > > > In some customized scenario, `dev_port` is used to rename netdev > > instead of `phys_port_name`, which requires to initialize it > > correctly to get expected netdev name. > > > > Example rules using `dev_port`: > > > > SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", > ATTR{dev_port}=="0", NAME:="ens8np0" > > SUBSYSTEM=="net", ACTION=="add", KERNELS=="0000:e1:00.0", > ATTR{dev_port}=="1", NAME:="ens8np1" > >
On Wed, 29 Mar 2023 12:22:27 -0700, Jakub Kicinski wrote: > On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote: > > In some customized scenario, `dev_port` is used to rename netdev > > instead of `phys_port_name`, which requires to initialize it > > correctly to get expected netdev name. > > What do you mean by "which requires to initialize it correctly to get > expected netdev name." ? I mean it cannot be renamed by udev rules as expected if `dev_port` is not correctly initialized, because the second port doesn't match 'ATTR{dev_port}=="1"'.
On Thu, 30 Mar 2023 01:40:30 +0000 Yinjun Zhang wrote: > On Wed, 29 Mar 2023 12:22:27 -0700, Jakub Kicinski wrote: > > On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote: > > > In some customized scenario, `dev_port` is used to rename netdev > > > instead of `phys_port_name`, which requires to initialize it > > > correctly to get expected netdev name. > > > > What do you mean by "which requires to initialize it correctly to get > > expected netdev name." ? > > I mean it cannot be renamed by udev rules as expected if `dev_port` > is not correctly initialized, because the second port doesn't match > 'ATTR{dev_port}=="1"'. Yes, but phys_port_name is still there, and can be used, right? So why add another attr?
On Wed, 29 Mar 2023 19:38:31 -0700, Jakub Kicinski wrote: > On Thu, 30 Mar 2023 01:40:30 +0000 Yinjun Zhang wrote: > > On Wed, 29 Mar 2023 12:22:27 -0700, Jakub Kicinski wrote: > > > On Wed, 29 Mar 2023 16:45:47 +0200 Louis Peens wrote: > > > > In some customized scenario, `dev_port` is used to rename netdev > > > > instead of `phys_port_name`, which requires to initialize it > > > > correctly to get expected netdev name. > > > > > > What do you mean by "which requires to initialize it correctly to get > > > expected netdev name." ? > > > > I mean it cannot be renamed by udev rules as expected if `dev_port` > > is not correctly initialized, because the second port doesn't match > > 'ATTR{dev_port}=="1"'. > > Yes, but phys_port_name is still there, and can be used, right? > So why add another attr? Yes, phys_port_name is still there. But some users prefer to use dev_port. I don't add this new attr, it's already existed since 3f85944fe207 ("net: Add sysfs file for port number"). I just make the attr's value correct.
On Thu, 30 Mar 2023 02:52:22 +0000 Yinjun Zhang wrote: > > Yes, but phys_port_name is still there, and can be used, right? > > So why add another attr? > > Yes, phys_port_name is still there. But some users prefer to use dev_port. > I don't add this new attr, it's already existed since > 3f85944fe207 ("net: Add sysfs file for port number"). > I just make the attr's value correct. You're using a different ID than phys_port_name, as far as I can tell :( When the port is not split will id == label, always?
On Wed, 29 Mar 2023 20:10:29 -0700, Jakub Kicinski wrote: > On Thu, 30 Mar 2023 02:52:22 +0000 Yinjun Zhang wrote: > > > Yes, but phys_port_name is still there, and can be used, right? > > > So why add another attr? > > > > Yes, phys_port_name is still there. But some users prefer to use dev_port. > > I don't add this new attr, it's already existed since > > 3f85944fe207 ("net: Add sysfs file for port number"). > > I just make the attr's value correct. > > You're using a different ID than phys_port_name, as far as I can tell :( > When the port is not split will id == label, always? You got the point. We create netdevs according to the port sequence in eth_table from management firmware. I think M-FW will make sure the sequence matches the port label id.
On Thu, 30 Mar 2023 03:27:56 +0000 Yinjun Zhang wrote: > > > Yes, phys_port_name is still there. But some users prefer to use dev_port. > > > I don't add this new attr, it's already existed since > > > 3f85944fe207 ("net: Add sysfs file for port number"). > > > I just make the attr's value correct. > > > > You're using a different ID than phys_port_name, as far as I can tell :( > > When the port is not split will id == label, always? > > You got the point. We create netdevs according to the port sequence in > eth_table from management firmware. I think M-FW will make sure > the sequence matches the port label id. Alright, then, with an improved commit message this patch should be fine.
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_port.c b/drivers/net/ethernet/netronome/nfp/nfp_port.c index 4f2308570dcf..54640bcb70fb 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_port.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_port.c @@ -189,6 +189,7 @@ int nfp_port_init_phy_port(struct nfp_pf *pf, struct nfp_app *app, port->eth_port = &pf->eth_tbl->ports[id]; port->eth_id = pf->eth_tbl->ports[id].index; + port->netdev->dev_port = id; if (pf->mac_stats_mem) port->eth_stats = pf->mac_stats_mem + port->eth_id * NFP_MAC_STATS_SIZE;