[PATCH net] gve: unify driver name usage

[PATCH net] gve: unify driver name usage

[Junfeng Guo]

Current codebase contained the usage of two different names for this
driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
to use, especially when trying to bind or unbind the driver manually.
The corresponding kernel module is registered with the name of `gve`.
It's more reasonable to align the name of the driver with the module.

Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute Engine Virtual NIC")
Cc: csully@google.com
Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
 drivers/net/ethernet/google/gve/gve.h         |  1 +
 drivers/net/ethernet/google/gve/gve_adminq.c  | 10 +++++-----
 drivers/net/ethernet/google/gve/gve_ethtool.c |  2 +-
 drivers/net/ethernet/google/gve/gve_main.c    | 11 ++++++-----
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 98eb78d98e9f..4b425bf71ede 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -964,5 +964,6 @@ void gve_handle_report_stats(struct gve_priv *priv);
 /* exported by ethtool.c */
 extern const struct ethtool_ops gve_ethtool_ops;
 /* needed by ethtool */
+extern char gve_driver_name[];
 extern const char gve_version_str[];
 #endif /* _GVE_H_ */
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 252974202a3f..ae8f8c935bbe 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -899,7 +899,7 @@ int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
 
 int gve_adminq_report_link_speed(struct gve_priv *priv)
 {
-	union gve_adminq_command gvnic_cmd;
+	union gve_adminq_command gve_cmd;
 	dma_addr_t link_speed_region_bus;
 	__be64 *link_speed_region;
 	int err;
@@ -911,12 +911,12 @@ int gve_adminq_report_link_speed(struct gve_priv *priv)
 	if (!link_speed_region)
 		return -ENOMEM;
 
-	memset(&gvnic_cmd, 0, sizeof(gvnic_cmd));
-	gvnic_cmd.opcode = cpu_to_be32(GVE_ADMINQ_REPORT_LINK_SPEED);
-	gvnic_cmd.report_link_speed.link_speed_address =
+	memset(&gve_cmd, 0, sizeof(gve_cmd));
+	gve_cmd.opcode = cpu_to_be32(GVE_ADMINQ_REPORT_LINK_SPEED);
+	gve_cmd.report_link_speed.link_speed_address =
 		cpu_to_be64(link_speed_region_bus);
 
-	err = gve_adminq_execute_cmd(priv, &gvnic_cmd);
+	err = gve_adminq_execute_cmd(priv, &gve_cmd);
 
 	priv->link_speed = be64_to_cpu(*link_speed_region);
 	dma_free_coherent(&priv->pdev->dev, sizeof(*link_speed_region), link_speed_region,
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 50162ec9424d..233e5946905e 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -15,7 +15,7 @@ static void gve_get_drvinfo(struct net_device *netdev,
 {
 	struct gve_priv *priv = netdev_priv(netdev);
 
-	strscpy(info->driver, "gve", sizeof(info->driver));
+	strscpy(info->driver, gve_driver_name, sizeof(info->driver));
 	strscpy(info->version, gve_version_str, sizeof(info->version));
 	strscpy(info->bus_info, pci_name(priv->pdev), sizeof(info->bus_info));
 }
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 8fb70db63b8b..e6f1711d9be0 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -33,6 +33,7 @@
 #define MIN_TX_TIMEOUT_GAP (1000 * 10)
 #define DQO_TX_MAX	0x3FFFF
 
+char gve_driver_name[] = "gve";
 const char gve_version_str[] = GVE_VERSION;
 static const char gve_version_prefix[] = GVE_VERSION_PREFIX;
 
@@ -2200,7 +2201,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		return err;
 
-	err = pci_request_regions(pdev, "gvnic-cfg");
+	err = pci_request_regions(pdev, gve_driver_name);
 	if (err)
 		goto abort_with_enabled;
 
@@ -2393,8 +2394,8 @@ static const struct pci_device_id gve_id_table[] = {
 	{ }
 };
 
-static struct pci_driver gvnic_driver = {
-	.name		= "gvnic",
+static struct pci_driver gve_driver = {
+	.name		= gve_driver_name,
 	.id_table	= gve_id_table,
 	.probe		= gve_probe,
 	.remove		= gve_remove,
@@ -2405,10 +2406,10 @@ static struct pci_driver gvnic_driver = {
 #endif
 };
 
-module_pci_driver(gvnic_driver);
+module_pci_driver(gve_driver);
 
 MODULE_DEVICE_TABLE(pci, gve_id_table);
 MODULE_AUTHOR("Google, Inc.");
-MODULE_DESCRIPTION("gVNIC Driver");
+MODULE_DESCRIPTION("Google Virtual NIC Driver");
 MODULE_LICENSE("Dual MIT/GPL");
 MODULE_VERSION(GVE_VERSION);
-- 
2.25.1

Re: [PATCH net] gve: unify driver name usage

[Michal Kubiak]

On Fri, Jul 07, 2023 at 06:37:10PM +0800, Junfeng Guo wrote:
> Current codebase contained the usage of two different names for this
> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> to use, especially when trying to bind or unbind the driver manually.
> The corresponding kernel module is registered with the name of `gve`.
> It's more reasonable to align the name of the driver with the module.
> 
> Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute Engine Virtual NIC")
> Cc: csully@google.com
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> ---
>

The patch makes the driver strings much more consistent.
Looks good to me.

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net] gve: unify driver name usage

[Jakub Kicinski]

On Fri,  7 Jul 2023 18:37:10 +0800 Junfeng Guo wrote:
> Current codebase contained the usage of two different names for this
> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> to use, especially when trying to bind or unbind the driver manually.
> The corresponding kernel module is registered with the name of `gve`.
> It's more reasonable to align the name of the driver with the module.
> 
> Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute Engine Virtual NIC")
> Cc: csully@google.com
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>

Google's maintainers definitely need to agree to this, because it's 
a user visible change. It can very well break someone's scripts.

> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 98eb78d98e9f..4b425bf71ede 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -964,5 +964,6 @@ void gve_handle_report_stats(struct gve_priv *priv);
>  /* exported by ethtool.c */
>  extern const struct ethtool_ops gve_ethtool_ops;
>  /* needed by ethtool */
> +extern char gve_driver_name[];
>  extern const char gve_version_str[];
>  #endif /* _GVE_H_ */
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 252974202a3f..ae8f8c935bbe 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -899,7 +899,7 @@ int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
>  
>  int gve_adminq_report_link_speed(struct gve_priv *priv)
>  {
> -	union gve_adminq_command gvnic_cmd;
> +	union gve_adminq_command gve_cmd;
>  	dma_addr_t link_speed_region_bus;
>  	__be64 *link_speed_region;
>  	int err;
> @@ -911,12 +911,12 @@ int gve_adminq_report_link_speed(struct gve_priv *priv)
>  	if (!link_speed_region)
>  		return -ENOMEM;
>  
> -	memset(&gvnic_cmd, 0, sizeof(gvnic_cmd));
> -	gvnic_cmd.opcode = cpu_to_be32(GVE_ADMINQ_REPORT_LINK_SPEED);
> -	gvnic_cmd.report_link_speed.link_speed_address =
> +	memset(&gve_cmd, 0, sizeof(gve_cmd));
> +	gve_cmd.opcode = cpu_to_be32(GVE_ADMINQ_REPORT_LINK_SPEED);
> +	gve_cmd.report_link_speed.link_speed_address =
>  		cpu_to_be64(link_speed_region_bus);
>  
> -	err = gve_adminq_execute_cmd(priv, &gvnic_cmd);
> +	err = gve_adminq_execute_cmd(priv, &gve_cmd);

What's the problem with the variable being called gvnic_cmd ?
Please limit renames, if you want this to be a fix.
-- 
pw-bot: cr

RE: [PATCH net] gve: unify driver name usage

[Guo, Junfeng]

> -----Original Message-----
> From: Kubiak, Michal <michal.kubiak@intel.com>
> Sent: Saturday, July 8, 2023 02:27
> To: Guo, Junfeng <junfeng.guo@intel.com>
> Cc: netdev@vger.kernel.org; jeroendb@google.com;
> pkaligineedi@google.com; shailend@google.com; Wang, Haiyue
> <haiyue.wang@intel.com>; kuba@kernel.org; awogbemila@google.com;
> davem@davemloft.net; pabeni@redhat.com; yangchun@google.com;
> edumazet@google.com; csully@google.com
> Subject: Re: [PATCH net] gve: unify driver name usage
> 
> On Fri, Jul 07, 2023 at 06:37:10PM +0800, Junfeng Guo wrote:
> > Current codebase contained the usage of two different names for this
> > driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> > to use, especially when trying to bind or unbind the driver manually.
> > The corresponding kernel module is registered with the name of `gve`.
> > It's more reasonable to align the name of the driver with the module.
> >
> > Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute
> Engine Virtual NIC")
> > Cc: csully@google.com
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> > ---
> >
> 
> The patch makes the driver strings much more consistent.
> Looks good to me.
> 
> Thanks,
> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Thanks for the review!

RE: [PATCH net] gve: unify driver name usage

[Guo, Junfeng]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Saturday, July 8, 2023 06:20
> To: Guo, Junfeng <junfeng.guo@intel.com>
> Cc: netdev@vger.kernel.org; jeroendb@google.com;
> pkaligineedi@google.com; shailend@google.com; Wang, Haiyue
> <haiyue.wang@intel.com>; awogbemila@google.com;
> davem@davemloft.net; pabeni@redhat.com; yangchun@google.com;
> edumazet@google.com; csully@google.com
> Subject: Re: [PATCH net] gve: unify driver name usage
> 
> On Fri,  7 Jul 2023 18:37:10 +0800 Junfeng Guo wrote:
> > Current codebase contained the usage of two different names for this
> > driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> > to use, especially when trying to bind or unbind the driver manually.
> > The corresponding kernel module is registered with the name of `gve`.
> > It's more reasonable to align the name of the driver with the module.
> >
> > Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute
> Engine Virtual NIC")
> > Cc: csully@google.com
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> 
> Google's maintainers definitely need to agree to this, because it's
> a user visible change. It can very well break someone's scripts.

Sure, exactly!
Google's maintainers are also CC'ed to give decisive comments for this.

This patch is to align the inconsistencies for the driver name and the
kernel module name, which may also help improve some scripts to
avoid using exception mapping with two different names.

> 
> > diff --git a/drivers/net/ethernet/google/gve/gve.h
> b/drivers/net/ethernet/google/gve/gve.h
> > index 98eb78d98e9f..4b425bf71ede 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -964,5 +964,6 @@ void gve_handle_report_stats(struct gve_priv
> *priv);
> >  /* exported by ethtool.c */
> >  extern const struct ethtool_ops gve_ethtool_ops;
> >  /* needed by ethtool */
> > +extern char gve_driver_name[];
> >  extern const char gve_version_str[];
> >  #endif /* _GVE_H_ */
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c
> b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 252974202a3f..ae8f8c935bbe 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> > @@ -899,7 +899,7 @@ int
> gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> >
> >  int gve_adminq_report_link_speed(struct gve_priv *priv)
> >  {
> > -	union gve_adminq_command gvnic_cmd;
> > +	union gve_adminq_command gve_cmd;
> >  	dma_addr_t link_speed_region_bus;
> >  	__be64 *link_speed_region;
> >  	int err;
> > @@ -911,12 +911,12 @@ int gve_adminq_report_link_speed(struct
> gve_priv *priv)
> >  	if (!link_speed_region)
> >  		return -ENOMEM;
> >
> > -	memset(&gvnic_cmd, 0, sizeof(gvnic_cmd));
> > -	gvnic_cmd.opcode =
> cpu_to_be32(GVE_ADMINQ_REPORT_LINK_SPEED);
> > -	gvnic_cmd.report_link_speed.link_speed_address =
> > +	memset(&gve_cmd, 0, sizeof(gve_cmd));
> > +	gve_cmd.opcode =
> cpu_to_be32(GVE_ADMINQ_REPORT_LINK_SPEED);
> > +	gve_cmd.report_link_speed.link_speed_address =
> >  		cpu_to_be64(link_speed_region_bus);
> >
> > -	err = gve_adminq_execute_cmd(priv, &gvnic_cmd);
> > +	err = gve_adminq_execute_cmd(priv, &gve_cmd);
> 
> What's the problem with the variable being called gvnic_cmd ?
> Please limit renames, if you want this to be a fix.

Thanks for the comments!
Yes, this is not very related to the fixing purpose.
Will remove this part in the coming version. Thanks!

> --
> pw-bot: cr

[PATCH net v2] gve: unify driver name usage

[Junfeng Guo]

Current codebase contained the usage of two different names for this
driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
to use, especially when trying to bind or unbind the driver manually.
The corresponding kernel module is registered with the name of `gve`.
It's more reasonable to align the name of the driver with the module.

Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute Engine Virtual NIC")
Cc: csully@google.com
Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
v2:
- Remove un-related variable name chang, just keep it as it is.
---
 drivers/net/ethernet/google/gve/gve.h         |  1 +
 drivers/net/ethernet/google/gve/gve_ethtool.c |  2 +-
 drivers/net/ethernet/google/gve/gve_main.c    | 11 ++++++-----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 98eb78d98e9f..4b425bf71ede 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -964,5 +964,6 @@ void gve_handle_report_stats(struct gve_priv *priv);
 /* exported by ethtool.c */
 extern const struct ethtool_ops gve_ethtool_ops;
 /* needed by ethtool */
+extern char gve_driver_name[];
 extern const char gve_version_str[];
 #endif /* _GVE_H_ */
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 50162ec9424d..233e5946905e 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -15,7 +15,7 @@ static void gve_get_drvinfo(struct net_device *netdev,
 {
 	struct gve_priv *priv = netdev_priv(netdev);
 
-	strscpy(info->driver, "gve", sizeof(info->driver));
+	strscpy(info->driver, gve_driver_name, sizeof(info->driver));
 	strscpy(info->version, gve_version_str, sizeof(info->version));
 	strscpy(info->bus_info, pci_name(priv->pdev), sizeof(info->bus_info));
 }
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 8fb70db63b8b..e6f1711d9be0 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -33,6 +33,7 @@
 #define MIN_TX_TIMEOUT_GAP (1000 * 10)
 #define DQO_TX_MAX	0x3FFFF
 
+char gve_driver_name[] = "gve";
 const char gve_version_str[] = GVE_VERSION;
 static const char gve_version_prefix[] = GVE_VERSION_PREFIX;
 
@@ -2200,7 +2201,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		return err;
 
-	err = pci_request_regions(pdev, "gvnic-cfg");
+	err = pci_request_regions(pdev, gve_driver_name);
 	if (err)
 		goto abort_with_enabled;
 
@@ -2393,8 +2394,8 @@ static const struct pci_device_id gve_id_table[] = {
 	{ }
 };
 
-static struct pci_driver gvnic_driver = {
-	.name		= "gvnic",
+static struct pci_driver gve_driver = {
+	.name		= gve_driver_name,
 	.id_table	= gve_id_table,
 	.probe		= gve_probe,
 	.remove		= gve_remove,
@@ -2405,10 +2406,10 @@ static struct pci_driver gvnic_driver = {
 #endif
 };
 
-module_pci_driver(gvnic_driver);
+module_pci_driver(gve_driver);
 
 MODULE_DEVICE_TABLE(pci, gve_id_table);
 MODULE_AUTHOR("Google, Inc.");
-MODULE_DESCRIPTION("gVNIC Driver");
+MODULE_DESCRIPTION("Google Virtual NIC Driver");
 MODULE_LICENSE("Dual MIT/GPL");
 MODULE_VERSION(GVE_VERSION);
-- 
2.25.1

Re: [PATCH net v2] gve: unify driver name usage

[Andrew Lunn]

On Sat, Jul 08, 2023 at 11:14:51AM +0800, Junfeng Guo wrote:
> Current codebase contained the usage of two different names for this
> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> to use, especially when trying to bind or unbind the driver manually.
> The corresponding kernel module is registered with the name of `gve`.
> It's more reasonable to align the name of the driver with the module.
> 
> Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute Engine Virtual NIC")

You are posting this for net, and have a fixes tag. Please take a look at:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Do you think it fulfils those rules?

   Andrew

Re: [PATCH net v2] gve: unify driver name usage

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat,  8 Jul 2023 11:14:51 +0800 you wrote:
> Current codebase contained the usage of two different names for this
> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> to use, especially when trying to bind or unbind the driver manually.
> The corresponding kernel module is registered with the name of `gve`.
> It's more reasonable to align the name of the driver with the module.
> 
> Fixes: 893ce44df565 ("gve: Add basic driver framework for Compute Engine Virtual NIC")
> Cc: csully@google.com
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> 
> [...]

Here is the summary with links:
  - [net,v2] gve: unify driver name usage
    https://git.kernel.org/netdev/net/c/9d0aba98316d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] gve: unify driver name usage

[Alexander Lobakin]

From: Junfeng Guo <junfeng.guo@intel.com>
Date: Fri,  7 Jul 2023 18:37:10 +0800

> Current codebase contained the usage of two different names for this
> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> to use, especially when trying to bind or unbind the driver manually.
> The corresponding kernel module is registered with the name of `gve`.
> It's more reasonable to align the name of the driver with the module.

[...]

> @@ -2200,7 +2201,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (err)
>  		return err;
>  
> -	err = pci_request_regions(pdev, "gvnic-cfg");
> +	err = pci_request_regions(pdev, gve_driver_name);

I won't repeat others' comments, but will comment on this.
Passing just driver name with no unique identifiers makes it very
confusing to read /proc/iomem et al.
Imagine you have 2 NICs in your system. Then, in /proc/iomem you will have:

gve 0x00001000-0x00002000
gve 0x00004000-0x00005000

Can you say which region belongs to which NIC? Nope.
If you really want to make this more "user friendly", you should make it
possible for users to distinguish different NICs in your system. The
easiest way:

	err = pci_request_regions(pdev, pci_name(pdev));

But you're not limited to this. Just make it unique.

(as a net-next commit obv)

>  	if (err)
>  		goto abort_with_enabled;
>  
[...]

Thanks,
Olek

RE: [PATCH net] gve: unify driver name usage

[Wang, Haiyue]

> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Sent: Tuesday, July 11, 2023 21:14
> To: Guo, Junfeng <junfeng.guo@intel.com>
> Cc: netdev@vger.kernel.org; jeroendb@google.com; pkaligineedi@google.com; shailend@google.com; Wang,
> Haiyue <haiyue.wang@intel.com>; kuba@kernel.org; awogbemila@google.com; davem@davemloft.net;
> pabeni@redhat.com; yangchun@google.com; edumazet@google.com; csully@google.com
> Subject: Re: [PATCH net] gve: unify driver name usage
> 
> From: Junfeng Guo <junfeng.guo@intel.com>
> Date: Fri,  7 Jul 2023 18:37:10 +0800
> 
> > Current codebase contained the usage of two different names for this
> > driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> > to use, especially when trying to bind or unbind the driver manually.
> > The corresponding kernel module is registered with the name of `gve`.
> > It's more reasonable to align the name of the driver with the module.
> 
> [...]
> 
> > @@ -2200,7 +2201,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	if (err)
> >  		return err;
> >
> > -	err = pci_request_regions(pdev, "gvnic-cfg");
> > +	err = pci_request_regions(pdev, gve_driver_name);
> 
> I won't repeat others' comments, but will comment on this.
> Passing just driver name with no unique identifiers makes it very
> confusing to read /proc/iomem et al.
> Imagine you have 2 NICs in your system. Then, in /proc/iomem you will have:
> 
> gve 0x00001000-0x00002000
> gve 0x00004000-0x00005000
> 

Looks like, in real world, it is PCI BAR tree layers, take Intel ice as an example:

err = pcim_iomap_regions(pdev, BIT(ICE_BAR0), dev_driver_string(dev));

3b4000000000-3b7fffffffff : PCI Bus 0000:b7
  3b7ffa000000-3b7ffe4fffff : PCI Bus 0000:b8
    3b7ffa000000-3b7ffbffffff : 0000:b8:00.0   <--- Different NIC, has different BDF
      3b7ffa000000-3b7ffbffffff : ice          <--- The region name is driver name.
    3b7ffc000000-3b7ffdffffff : 0000:b8:00.0
    3b7ffe000000-3b7ffe00ffff : 0000:b8:00.0
    3b7ffe010000-3b7ffe40ffff : 0000:b8:00.0

google/gve/gve_main.c:2203:     err = pci_request_regions(pdev, "gvnic-cfg");
hisilicon/hns3/hns3pf/hclge_main.c:11350:       ret = pci_request_regions(pdev, HCLGE_DRIVER_NAME);
hisilicon/hns3/hns3vf/hclgevf_main.c:2588:      ret = pci_request_regions(pdev, HCLGEVF_DRIVER_NAME);
huawei/hinic/hinic_main.c:1362: err = pci_request_regions(pdev, HINIC_DRV_NAME);
intel/ixgbevf/ixgbevf_main.c:4544:      err = pci_request_regions(pdev, ixgbevf_driver_name);
intel/ixgbevf/ixgbevf_main.c:4546:              dev_err(&pdev->dev, "pci_request_regions failed 0x%x\n", err);
intel/e100.c:2865:      if ((err = pci_request_regions(pdev, DRV_NAME))) {
intel/igbvf/netdev.c:2732:      err = pci_request_regions(pdev, igbvf_driver_name);
intel/iavf/iavf_main.c:4849:    err = pci_request_regions(pdev, iavf_driver_name);
intel/iavf/iavf_main.c:4852:                    "pci_request_regions failed 0x%x\n", err);
jme.c:2939:     rc = pci_request_regions(pdev, DRV_NAME);
marvell/octeontx2/nic/otx2_pf.c:2793:   err = pci_request_regions(pdev, DRV_NAME);
marvell/octeontx2/nic/otx2_vf.c:534:    err = pci_request_regions(pdev, DRV_NAME);
marvell/octeontx2/af/mcs.c:1516:        err = pci_request_regions(pdev, DRV_NAME);
marvell/octeontx2/af/rvu.c:3238:        err = pci_request_regions(pdev, DRV_NAME);
marvell/octeontx2/af/cgx.c:1831:        err = pci_request_regions(pdev, DRV_NAME);


> Can you say which region belongs to which NIC? Nope.
> If you really want to make this more "user friendly", you should make it
> possible for users to distinguish different NICs in your system. The
> easiest way:
> 
> 	err = pci_request_regions(pdev, pci_name(pdev));
> 
> But you're not limited to this. Just make it unique.
> 
> (as a net-next commit obv)
> 
> >  	if (err)
> >  		goto abort_with_enabled;
> >
> [...]
> 
> Thanks,
> Olek

Re: [PATCH net] gve: unify driver name usage

[Alexander Lobakin]

From: Wang, Haiyue <haiyue.wang@intel.com>
Date: Tue, 11 Jul 2023 19:24:36 +0200

>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
>> Sent: Tuesday, July 11, 2023 21:14
>> To: Guo, Junfeng <junfeng.guo@intel.com>
>> Cc: netdev@vger.kernel.org; jeroendb@google.com; pkaligineedi@google.com; shailend@google.com; Wang,
>> Haiyue <haiyue.wang@intel.com>; kuba@kernel.org; awogbemila@google.com; davem@davemloft.net;
>> pabeni@redhat.com; yangchun@google.com; edumazet@google.com; csully@google.com
>> Subject: Re: [PATCH net] gve: unify driver name usage
>>
>> From: Junfeng Guo <junfeng.guo@intel.com>
>> Date: Fri,  7 Jul 2023 18:37:10 +0800
>>
>>> Current codebase contained the usage of two different names for this
>>> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
>>> to use, especially when trying to bind or unbind the driver manually.
>>> The corresponding kernel module is registered with the name of `gve`.
>>> It's more reasonable to align the name of the driver with the module.
>>
>> [...]
>>
>>> @@ -2200,7 +2201,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>  	if (err)
>>>  		return err;
>>>
>>> -	err = pci_request_regions(pdev, "gvnic-cfg");
>>> +	err = pci_request_regions(pdev, gve_driver_name);
>>
>> I won't repeat others' comments, but will comment on this.
>> Passing just driver name with no unique identifiers makes it very
>> confusing to read /proc/iomem et al.
>> Imagine you have 2 NICs in your system. Then, in /proc/iomem you will have:
>>
>> gve 0x00001000-0x00002000
>> gve 0x00004000-0x00005000
>>
> 
> Looks like, in real world, it is PCI BAR tree layers, take Intel ice as an example:
> 
> err = pcim_iomap_regions(pdev, BIT(ICE_BAR0), dev_driver_string(dev));
> 
> 3b4000000000-3b7fffffffff : PCI Bus 0000:b7
>   3b7ffa000000-3b7ffe4fffff : PCI Bus 0000:b8
>     3b7ffa000000-3b7ffbffffff : 0000:b8:00.0   <--- Different NIC, has different BDF
>       3b7ffa000000-3b7ffbffffff : ice          <--- The region name is driver name.

I didn't say Intel drivers do that better ¯\_(ツ)_/¯

Why rely on that the kernel or something else will beautify the output
for you or that the user won't do `grep <drvname> /proc/iomem` or
something else? Or do that just because "look, it's done the same way in
other drivers", which were taken into the tree years ago and sometimes
with no detailed review?
There are efforts[0] time to time[1] to convert precisely what you are
doing into what I'm asking for. Do they exist by mistake or...?

(the second link also shows that even pci_name() is not enough when you
 map several BARs of the same device, but that's not the case this time)

>     3b7ffc000000-3b7ffdffffff : 0000:b8:00.0
>     3b7ffe000000-3b7ffe00ffff : 0000:b8:00.0
>     3b7ffe010000-3b7ffe40ffff : 0000:b8:00.0
> 
> google/gve/gve_main.c:2203:     err = pci_request_regions(pdev, "gvnic-cfg");
> hisilicon/hns3/hns3pf/hclge_main.c:11350:       ret = pci_request_regions(pdev, HCLGE_DRIVER_NAME);
> hisilicon/hns3/hns3vf/hclgevf_main.c:2588:      ret = pci_request_regions(pdev, HCLGEVF_DRIVER_NAME);
> huawei/hinic/hinic_main.c:1362: err = pci_request_regions(pdev, HINIC_DRV_NAME);
> intel/ixgbevf/ixgbevf_main.c:4544:      err = pci_request_regions(pdev, ixgbevf_driver_name);
> intel/ixgbevf/ixgbevf_main.c:4546:              dev_err(&pdev->dev, "pci_request_regions failed 0x%x\n", err);
> intel/e100.c:2865:      if ((err = pci_request_regions(pdev, DRV_NAME))) {
> intel/igbvf/netdev.c:2732:      err = pci_request_regions(pdev, igbvf_driver_name);
> intel/iavf/iavf_main.c:4849:    err = pci_request_regions(pdev, iavf_driver_name);
> intel/iavf/iavf_main.c:4852:                    "pci_request_regions failed 0x%x\n", err);
> jme.c:2939:     rc = pci_request_regions(pdev, DRV_NAME);
> marvell/octeontx2/nic/otx2_pf.c:2793:   err = pci_request_regions(pdev, DRV_NAME);
> marvell/octeontx2/nic/otx2_vf.c:534:    err = pci_request_regions(pdev, DRV_NAME);
> marvell/octeontx2/af/mcs.c:1516:        err = pci_request_regions(pdev, DRV_NAME);
> marvell/octeontx2/af/rvu.c:3238:        err = pci_request_regions(pdev, DRV_NAME);
> marvell/octeontx2/af/cgx.c:1831:        err = pci_request_regions(pdev, DRV_NAME);
> 
> 
>> Can you say which region belongs to which NIC? Nope.
>> If you really want to make this more "user friendly", you should make it
>> possible for users to distinguish different NICs in your system. The
>> easiest way:
>>
>> 	err = pci_request_regions(pdev, pci_name(pdev));
>>
>> But you're not limited to this. Just make it unique.
>>
>> (as a net-next commit obv)
>>
>>>  	if (err)
>>>  		goto abort_with_enabled;
>>>
>> [...]
>>
>> Thanks,
>> Olek

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0af6e21eed2778e68139941389460e2a00d6ef8e
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=35bd8c07db2ce8fd2834ef866240613a4ef982e7

Thanks,
Olek

RE: [PATCH net] gve: unify driver name usage

[Wang, Haiyue]

> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Sent: Thursday, July 13, 2023 00:55
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: Guo, Junfeng <junfeng.guo@intel.com>; netdev@vger.kernel.org; jeroendb@google.com;
> pkaligineedi@google.com; shailend@google.com; kuba@kernel.org; awogbemila@google.com;
> davem@davemloft.net; pabeni@redhat.com; yangchun@google.com; edumazet@google.com; csully@google.com
> Subject: Re: [PATCH net] gve: unify driver name usage
> 
> From: Wang, Haiyue <haiyue.wang@intel.com>
> Date: Tue, 11 Jul 2023 19:24:36 +0200
> 
> >> -----Original Message-----
> >> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> >> Sent: Tuesday, July 11, 2023 21:14
> >> To: Guo, Junfeng <junfeng.guo@intel.com>
> >> Cc: netdev@vger.kernel.org; jeroendb@google.com; pkaligineedi@google.com; shailend@google.com; Wang,
> >> Haiyue <haiyue.wang@intel.com>; kuba@kernel.org; awogbemila@google.com; davem@davemloft.net;
> >> pabeni@redhat.com; yangchun@google.com; edumazet@google.com; csully@google.com
> >> Subject: Re: [PATCH net] gve: unify driver name usage
> >>
> >> From: Junfeng Guo <junfeng.guo@intel.com>
> >> Date: Fri,  7 Jul 2023 18:37:10 +0800
> >>
> >>> Current codebase contained the usage of two different names for this
> >>> driver (i.e., `gvnic` and `gve`), which is quite unfriendly for users
> >>> to use, especially when trying to bind or unbind the driver manually.
> >>> The corresponding kernel module is registered with the name of `gve`.
> >>> It's more reasonable to align the name of the driver with the module.
> >>
> >> [...]
> >>
> >>> @@ -2200,7 +2201,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>>  	if (err)
> >>>  		return err;
> >>>
> >>> -	err = pci_request_regions(pdev, "gvnic-cfg");
> >>> +	err = pci_request_regions(pdev, gve_driver_name);
> >>
> >> I won't repeat others' comments, but will comment on this.
> >> Passing just driver name with no unique identifiers makes it very
> >> confusing to read /proc/iomem et al.
> >> Imagine you have 2 NICs in your system. Then, in /proc/iomem you will have:
> >>
> >> gve 0x00001000-0x00002000
> >> gve 0x00004000-0x00005000
> >>
> >
> > Looks like, in real world, it is PCI BAR tree layers, take Intel ice as an example:
> >
> > err = pcim_iomap_regions(pdev, BIT(ICE_BAR0), dev_driver_string(dev));
> >
> > 3b4000000000-3b7fffffffff : PCI Bus 0000:b7
> >   3b7ffa000000-3b7ffe4fffff : PCI Bus 0000:b8
> >     3b7ffa000000-3b7ffbffffff : 0000:b8:00.0   <--- Different NIC, has different BDF
> >       3b7ffa000000-3b7ffbffffff : ice          <--- The region name is driver name.
> 
> I didn't say Intel drivers do that better ¯\_(ツ)_/¯
> 
> Why rely on that the kernel or something else will beautify the output
> for you or that the user won't do `grep <drvname> /proc/iomem` or
> something else? Or do that just because "look, it's done the same way in
> other drivers", which were taken into the tree years ago and sometimes
> with no detailed review?
> There are efforts[0] time to time[1] to convert precisely what you are
> doing into what I'm asking for. Do they exist by mistake or...?
> 
> (the second link also shows that even pci_name() is not enough when you
>  map several BARs of the same device, but that's not the case this time)
> 

> >>
> >> Thanks,
> >> Olek
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
> next.git/commit/?id=0af6e21eed2778e68139941389460e2a00d6ef8e
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
> next.git/commit/?id=35bd8c07db2ce8fd2834ef866240613a4ef982e7

I see, yes, making sense to beautify the output, and is nice for 'grep ..'
to debug quickly, thanks for sharing more experience. :-)

> Thanks,
> Olek