[PATCH] drivers/net: remove alias for virtual devices

[PATCH] drivers/net: remove alias for virtual devices

[Ferruh Yigit]

Virtual devices are probed/matched based on name, and this name is user
facing value, since device name is provided by user as eal '--vdev'
parameter, like:
`dpdk-testpmd --vdev net_null0`.

The current name format is 'net_<pmd_name>', but previously it was
'eth_<pmd_name>', and an alias to legacy naming format was introduced
for backward compatibility.
Commit 9fa80cb26bd0 ("net: register aliases for renamed vdev drivers")

Since new device name format is around for 6 years, removing alias for
legacy naming.

Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
Alias for device name is used by other device abstraction layers too,
('crypto', 'baseband', 'raw'), since I am not aware of their maturity
level, leaving them out in this patch.
---
 drivers/net/af_packet/rte_eth_af_packet.c | 1 -
 drivers/net/bonding/rte_eth_bond_pmd.c    | 1 -
 drivers/net/mvpp2/mrvl_ethdev.c           | 1 -
 drivers/net/null/rte_eth_null.c           | 1 -
 drivers/net/octeontx/octeontx_ethdev.c    | 1 -
 drivers/net/pcap/pcap_ethdev.c            | 1 -
 drivers/net/ring/rte_eth_ring.c           | 1 -
 drivers/net/tap/rte_eth_tap.c             | 1 -
 drivers/net/vdev_netvsc/vdev_netvsc.c     | 1 -
 drivers/net/vhost/rte_eth_vhost.c         | 1 -
 drivers/net/virtio/virtio_user_ethdev.c   | 1 -
 11 files changed, 11 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 1396f32c3dcc..46baf108aa9b 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -1128,7 +1128,6 @@ static struct rte_vdev_driver pmd_af_packet_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_af_packet, pmd_af_packet_drv);
-RTE_PMD_REGISTER_ALIAS(net_af_packet, eth_af_packet);
 RTE_PMD_REGISTER_PARAM_STRING(net_af_packet,
 	"iface=<string> "
 	"qpairs=<int> "
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 3191158ca785..ad54e11f80f4 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3912,7 +3912,6 @@ struct rte_vdev_driver pmd_bond_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_bonding, pmd_bond_drv);
-RTE_PMD_REGISTER_ALIAS(net_bonding, eth_bond);
 
 RTE_PMD_REGISTER_PARAM_STRING(net_bonding,
 	"slave=<ifc> "
diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index 735efb6cfc06..590e5e89220a 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -3314,5 +3314,4 @@ static struct rte_vdev_driver pmd_mrvl_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_mvpp2, pmd_mrvl_drv);
-RTE_PMD_REGISTER_ALIAS(net_mvpp2, eth_mvpp2);
 RTE_LOG_REGISTER_DEFAULT(mrvl_logtype, NOTICE);
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index bb89c1abc4a2..2536d4b8f278 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -746,7 +746,6 @@ static struct rte_vdev_driver pmd_null_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_null, pmd_null_drv);
-RTE_PMD_REGISTER_ALIAS(net_null, eth_null);
 RTE_PMD_REGISTER_PARAM_STRING(net_null,
 	"size=<int> "
 	"copy=<int> "
diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index 290e562126a4..743ac408cba7 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -1885,5 +1885,4 @@ static struct rte_vdev_driver octeontx_pmd_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(OCTEONTX_PMD, octeontx_pmd_drv);
-RTE_PMD_REGISTER_ALIAS(OCTEONTX_PMD, eth_octeontx);
 RTE_PMD_REGISTER_PARAM_STRING(OCTEONTX_PMD, "nr_port=<int> ");
diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
index ec29fd6bc53c..8cc49e14ca8d 100644
--- a/drivers/net/pcap/pcap_ethdev.c
+++ b/drivers/net/pcap/pcap_ethdev.c
@@ -1643,7 +1643,6 @@ static struct rte_vdev_driver pmd_pcap_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_pcap, pmd_pcap_drv);
-RTE_PMD_REGISTER_ALIAS(net_pcap, eth_pcap);
 RTE_PMD_REGISTER_PARAM_STRING(net_pcap,
 	ETH_PCAP_RX_PCAP_ARG "=<string> "
 	ETH_PCAP_TX_PCAP_ARG "=<string> "
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index cfb81da5fe16..8ccca9db1935 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -772,6 +772,5 @@ static struct rte_vdev_driver pmd_ring_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_ring, pmd_ring_drv);
-RTE_PMD_REGISTER_ALIAS(net_ring, eth_ring);
 RTE_PMD_REGISTER_PARAM_STRING(net_ring,
 	ETH_RING_NUMA_NODE_ACTION_ARG "=name:node:action(ATTACH|CREATE)");
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 9e1032fe7269..b05ff227d381 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2638,7 +2638,6 @@ static struct rte_vdev_driver pmd_tap_drv = {
 
 RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
 RTE_PMD_REGISTER_VDEV(net_tun, pmd_tun_drv);
-RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
 RTE_PMD_REGISTER_PARAM_STRING(net_tun,
 			      ETH_TAP_IFACE_ARG "=<string> ");
 RTE_PMD_REGISTER_PARAM_STRING(net_tap,
diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index 25871951685c..89ac58e153c0 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -764,7 +764,6 @@ static struct rte_vdev_driver vdev_netvsc_vdev = {
 };
 
 RTE_PMD_REGISTER_VDEV(VDEV_NETVSC_DRIVER, vdev_netvsc_vdev);
-RTE_PMD_REGISTER_ALIAS(VDEV_NETVSC_DRIVER, eth_vdev_netvsc);
 RTE_PMD_REGISTER_PARAM_STRING(net_vdev_netvsc,
 			      VDEV_NETVSC_ARG_IFACE "=<string> "
 			      VDEV_NETVSC_ARG_MAC "=<string> "
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 7e512d94bf99..8eb937f91785 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1777,7 +1777,6 @@ static struct rte_vdev_driver pmd_vhost_drv = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_vhost, pmd_vhost_drv);
-RTE_PMD_REGISTER_ALIAS(net_vhost, eth_vhost);
 RTE_PMD_REGISTER_PARAM_STRING(net_vhost,
 	"iface=<ifc> "
 	"queues=<int> "
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index a7d7063c2a88..8f93ed01e45a 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -775,7 +775,6 @@ static struct rte_vdev_driver virtio_user_driver = {
 };
 
 RTE_PMD_REGISTER_VDEV(net_virtio_user, virtio_user_driver);
-RTE_PMD_REGISTER_ALIAS(net_virtio_user, virtio_user);
 RTE_PMD_REGISTER_PARAM_STRING(net_virtio_user,
 	"path=<path> "
 	"mac=<mac addr> "
-- 
2.25.1

Re: [PATCH] drivers/net: remove alias for virtual devices

[Thomas Monjalon]

I think this patch requires a techboard vote
as it is dropping some user-facing naming.


21/09/2022 15:34, Ferruh Yigit:
> Virtual devices are probed/matched based on name, and this name is user
> facing value, since device name is provided by user as eal '--vdev'
> parameter, like:
> `dpdk-testpmd --vdev net_null0`.
> 
> The current name format is 'net_<pmd_name>', but previously it was
> 'eth_<pmd_name>', and an alias to legacy naming format was introduced
> for backward compatibility.
> Commit 9fa80cb26bd0 ("net: register aliases for renamed vdev drivers")
> 
> Since new device name format is around for 6 years, removing alias for
> legacy naming.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
> Alias for device name is used by other device abstraction layers too,
> ('crypto', 'baseband', 'raw'), since I am not aware of their maturity
> level, leaving them out in this patch.

Re: [PATCH] drivers/net: remove alias for virtual devices

[Ferruh Yigit]

On 9/21/2022 3:26 PM, Thomas Monjalon wrote:

> 
> 21/09/2022 15:34, Ferruh Yigit:
>> Virtual devices are probed/matched based on name, and this name is user
>> facing value, since device name is provided by user as eal '--vdev'
>> parameter, like:
>> `dpdk-testpmd --vdev net_null0`.
>>
>> The current name format is 'net_<pmd_name>', but previously it was
>> 'eth_<pmd_name>', and an alias to legacy naming format was introduced
>> for backward compatibility.
>> Commit 9fa80cb26bd0 ("net: register aliases for renamed vdev drivers")
>>
>> Since new device name format is around for 6 years, removing alias for
>> legacy naming.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
>> ---
>> Alias for device name is used by other device abstraction layers too,
>> ('crypto', 'baseband', 'raw'), since I am not aware of their maturity
>> level, leaving them out in this patch.
> 
> I think this patch requires a techboard vote
> as it is dropping some user-facing naming.
> 

ack, can you please take this to techboard agenda.

Re: [PATCH] drivers/net: remove alias for virtual devices

[Bruce Richardson]

On Wed, Sep 21, 2022 at 04:26:11PM +0200, Thomas Monjalon wrote:
> I think this patch requires a techboard vote
> as it is dropping some user-facing naming.
> 
+1
I personally am a little uncertain about dropping this support. It could be
widely used, still, and removing it doesn't save us a huge amount.

What would be better might be to:
* automatically add the eth-to-net aliases at a higher level so it is all
  in one place, rather than having it in each driver.
* once centralised, we can add a warning on use of the eth_ aliases to
  inform users that they should use "net" instead.
> 
> 21/09/2022 15:34, Ferruh Yigit:
> > Virtual devices are probed/matched based on name, and this name is user
> > facing value, since device name is provided by user as eal '--vdev'
> > parameter, like:
> > `dpdk-testpmd --vdev net_null0`.
> > 
> > The current name format is 'net_<pmd_name>', but previously it was
> > 'eth_<pmd_name>', and an alias to legacy naming format was introduced
> > for backward compatibility.
> > Commit 9fa80cb26bd0 ("net: register aliases for renamed vdev drivers")
> > 
> > Since new device name format is around for 6 years, removing alias for
> > legacy naming.
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> > ---
> > Alias for device name is used by other device abstraction layers too,
> > ('crypto', 'baseband', 'raw'), since I am not aware of their maturity
> > level, leaving them out in this patch.
> 
> 
> 
> 

Re: [PATCH] drivers/net: remove alias for virtual devices

[Bruce Richardson]

On Wed, Sep 21, 2022 at 04:26:11PM +0200, Thomas Monjalon wrote:
> I think this patch requires a techboard vote
> as it is dropping some user-facing naming.
> 
I think a better solution is to centralize the use of aliases. I've just
posted a patch to this thread to add the aliasing for net drivers to the
vdev bus driver. While it does imply a certain amount of abstraction
"leakage", it does make the use of aliases consistent and saves it being
spread across multiple driver files.

Thoughts?

/Bruce

Re: [PATCH] drivers/net: remove alias for virtual devices

[Andrew Rybchenko]

On 10/19/22 16:13, Bruce Richardson wrote:
> On Wed, Sep 21, 2022 at 04:26:11PM +0200, Thomas Monjalon wrote:
>> I think this patch requires a techboard vote
>> as it is dropping some user-facing naming.
>>
> I think a better solution is to centralize the use of aliases. I've just
> posted a patch to this thread to add the aliasing for net drivers to the
> vdev bus driver. While it does imply a certain amount of abstraction
> "leakage", it does make the use of aliases consistent and saves it being
> spread across multiple driver files.
> 
> Thoughts?
> 
> /Bruce

Removal is declined by techboard. So, I'm marking the patch as
Rejected.

However, moving allies to common code, as suggested by Bruce,
should be considered.

[PATCH] bus/vdev: automatically add eth alias for net drivers

[Bruce Richardson]

For historical reasons, a number of net vdev drivers also add a driver
alias using the "eth_" prefix. Since this is done on a per-driver basis,
the use of the alias in inconsistent and is spread across multiple
files. We can remove the per-driver aliases by just adding the alias
automatically at the vdev bus level.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/vdev/vdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index f5b43f1930..bfd7ce60c1 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
 void
 rte_vdev_register(struct rte_vdev_driver *driver)
 {
+	/* For net driver vdevs, add an automatic alias using "eth" prefix */
+	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
+		char *alias = strdup(driver->driver.name);
+		memcpy(alias, "eth_", 4);
+		driver->driver.alias = alias;
+	}
 	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
 }
 
-- 
2.34.1

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[Bruce Richardson]

On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> For historical reasons, a number of net vdev drivers also add a driver
> alias using the "eth_" prefix. Since this is done on a per-driver basis,
> the use of the alias in inconsistent and is spread across multiple
> files. We can remove the per-driver aliases by just adding the alias
> automatically at the vdev bus level.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index f5b43f1930..bfd7ce60c1 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
>  void
>  rte_vdev_register(struct rte_vdev_driver *driver)
>  {
> +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> +		char *alias = strdup(driver->driver.name);
> +		memcpy(alias, "eth_", 4);
> +		driver->driver.alias = alias;
> +	}
>  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
>  }
>  

As a self-review comment, I realise this solution has got an issue that it
leaks memory if drivers are constantly being registered or unregistered. I
find it hard to see situations where this can occur, but it is a potential
issue.

A second solution that does not have this problem is to move the aliasing
to EAL, as below:

index fb5d0a293b..37b86914a0 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -226,9 +226,14 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev)
        da->name[i] = '\0';
        if (bus == NULL) {
                bus = rte_bus_find_by_device_name(da->name);
+               if (bus == NULL && strncmp(da->name, "eth_", 4) == 0) {
+                       RTE_LOG(INFO, EAL, "failed to parse device \"%s\"...\n", da->name);
+                       memcpy(da->name, "net_", 4);
+                       RTE_LOG(INFO, EAL, "... trying device \"%s\"\n", da->name);
+                       bus = rte_bus_find_by_device_name(da->name);
+               }
                if (bus == NULL) {
                       RTE_LOG(ERR, EAL, "failed to parse device \"%s\"\n",
                               da->name);

While this doesn't have a memory freeing issue, it's downside is obviously
that we have further abstraction leakage, from the vdev bus for net drivers
all the way to EAL. Again, since the code delta is fairly small, this may
be acceptable, so opinions welcome.

/Bruce

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[Thomas Monjalon]

19/10/2022 15:20, Bruce Richardson:
> On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> > For historical reasons, a number of net vdev drivers also add a driver
> > alias using the "eth_" prefix. Since this is done on a per-driver basis,
> > the use of the alias in inconsistent and is spread across multiple
> > files. We can remove the per-driver aliases by just adding the alias
> > automatically at the vdev bus level.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/bus/vdev/vdev.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > index f5b43f1930..bfd7ce60c1 100644
> > --- a/drivers/bus/vdev/vdev.c
> > +++ b/drivers/bus/vdev/vdev.c
> > @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
> >  void
> >  rte_vdev_register(struct rte_vdev_driver *driver)
> >  {
> > +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> > +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> > +		char *alias = strdup(driver->driver.name);
> > +		memcpy(alias, "eth_", 4);
> > +		driver->driver.alias = alias;
> > +	}
> >  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
> >  }
> >  
> 
> As a self-review comment, I realise this solution has got an issue that it
> leaks memory if drivers are constantly being registered or unregistered. I
> find it hard to see situations where this can occur, but it is a potential
> issue.
> 
> A second solution that does not have this problem is to move the aliasing
> to EAL, as below:

Honestly I think the status quo is OK:
We have some aliases in some PMD for some historical reason
and everybody looks OK with that. Isn't it?

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[Bruce Richardson]

On Thu, Oct 20, 2022 at 10:23:27AM +0200, Thomas Monjalon wrote:
> 19/10/2022 15:20, Bruce Richardson:
> > On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> > > For historical reasons, a number of net vdev drivers also add a driver
> > > alias using the "eth_" prefix. Since this is done on a per-driver basis,
> > > the use of the alias in inconsistent and is spread across multiple
> > > files. We can remove the per-driver aliases by just adding the alias
> > > automatically at the vdev bus level.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  drivers/bus/vdev/vdev.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > > index f5b43f1930..bfd7ce60c1 100644
> > > --- a/drivers/bus/vdev/vdev.c
> > > +++ b/drivers/bus/vdev/vdev.c
> > > @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
> > >  void
> > >  rte_vdev_register(struct rte_vdev_driver *driver)
> > >  {
> > > +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> > > +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> > > +		char *alias = strdup(driver->driver.name);
> > > +		memcpy(alias, "eth_", 4);
> > > +		driver->driver.alias = alias;
> > > +	}
> > >  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
> > >  }
> > >  
> > 
> > As a self-review comment, I realise this solution has got an issue that it
> > leaks memory if drivers are constantly being registered or unregistered. I
> > find it hard to see situations where this can occur, but it is a potential
> > issue.
> > 
> > A second solution that does not have this problem is to move the aliasing
> > to EAL, as below:
> 
> Honestly I think the status quo is OK:
> We have some aliases in some PMD for some historical reason
> and everybody looks OK with that. Isn't it?
> 

Well, the inconsistency bugs me a little, but if others feel the status quo
is ok, I'm ok with that.

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[Ferruh Yigit]

On 10/20/2022 9:48 AM, Bruce Richardson wrote:
> On Thu, Oct 20, 2022 at 10:23:27AM +0200, Thomas Monjalon wrote:
>> 19/10/2022 15:20, Bruce Richardson:
>>> On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
>>>> For historical reasons, a number of net vdev drivers also add a driver
>>>> alias using the "eth_" prefix. Since this is done on a per-driver basis,
>>>> the use of the alias in inconsistent and is spread across multiple
>>>> files. We can remove the per-driver aliases by just adding the alias
>>>> automatically at the vdev bus level.
>>>>
>>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>> ---
>>>>   drivers/bus/vdev/vdev.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
>>>> index f5b43f1930..bfd7ce60c1 100644
>>>> --- a/drivers/bus/vdev/vdev.c
>>>> +++ b/drivers/bus/vdev/vdev.c
>>>> @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
>>>>   void
>>>>   rte_vdev_register(struct rte_vdev_driver *driver)
>>>>   {
>>>> +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
>>>> +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
>>>> +		char *alias = strdup(driver->driver.name);
>>>> +		memcpy(alias, "eth_", 4);
>>>> +		driver->driver.alias = alias;
>>>> +	}
>>>>   	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
>>>>   }
>>>>   
>>>
>>> As a self-review comment, I realise this solution has got an issue that it
>>> leaks memory if drivers are constantly being registered or unregistered. I
>>> find it hard to see situations where this can occur, but it is a potential
>>> issue.
>>>
>>> A second solution that does not have this problem is to move the aliasing
>>> to EAL, as below:
>>
>> Honestly I think the status quo is OK:
>> We have some aliases in some PMD for some historical reason
>> and everybody looks OK with that. Isn't it?
>>
> 
> Well, the inconsistency bugs me a little, but if others feel the status quo
> is ok, I'm ok with that.

In my perspective this is for cleanup, and new PMDs keep adding alias 
because they are copying from existing drivers.
Except from above there is no harm to have alias.

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[David Marchand]

On Thu, Oct 20, 2022 at 1:52 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >> Honestly I think the status quo is OK:
> >> We have some aliases in some PMD for some historical reason
> >> and everybody looks OK with that. Isn't it?
> >>
> >
> > Well, the inconsistency bugs me a little, but if others feel the status quo
> > is ok, I'm ok with that.
>
> In my perspective this is for cleanup, and new PMDs keep adding alias
> because they are copying from existing drivers.
> Except from above there is no harm to have alias.

Do we have a "valid" case of adding new aliases?
I don't think it is the case, so we can warn of new aliases
introduction in checkpatches.sh.

At worse, if a valid case is identified later, checkpatches.sh is only
a warning in patchwork and maintainers will manually review this
warning.


-- 
David Marchand

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[Ferruh Yigit]

On 10/27/2022 8:58 AM, David Marchand wrote:
> On Thu, Oct 20, 2022 at 1:52 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>> Honestly I think the status quo is OK:
>>>> We have some aliases in some PMD for some historical reason
>>>> and everybody looks OK with that. Isn't it?
>>>>
>>>
>>> Well, the inconsistency bugs me a little, but if others feel the status quo
>>> is ok, I'm ok with that.
>>
>> In my perspective this is for cleanup, and new PMDs keep adding alias
>> because they are copying from existing drivers.
>> Except from above there is no harm to have alias.
> 
> Do we have a "valid" case of adding new aliases?
> I don't think it is the case, so we can warn of new aliases
> introduction in checkpatches.sh.
> 

I commented a few of them to drop alias.
checkpatch can be an option, but my intention was to drop old code to 
reduce noise, not to add more :)

OK to keep the alias if removing it will cause more trouble.

> At worse, if a valid case is identified later, checkpatches.sh is only
> a warning in patchwork and maintainers will manually review this
> warning.
> 
> 

Re: [PATCH] bus/vdev: automatically add eth alias for net drivers

[Bruce Richardson]

On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> For historical reasons, a number of net vdev drivers also add a driver
> alias using the "eth_" prefix. Since this is done on a per-driver basis,
> the use of the alias in inconsistent and is spread across multiple
> files. We can remove the per-driver aliases by just adding the alias
> automatically at the vdev bus level.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index f5b43f1930..bfd7ce60c1 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
>  void
>  rte_vdev_register(struct rte_vdev_driver *driver)
>  {
> +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> +		char *alias = strdup(driver->driver.name);
> +		memcpy(alias, "eth_", 4);
> +		driver->driver.alias = alias;
> +	}
>  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
>  }
>  
Just to close this off...

Following the discussion in the thread, it seems a fix is not needed/wanted
here, so dropping patch and marked "rejected" in patchwork.