| Message ID | 20230707103710.3946651-1-junfeng.guo@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] gve: unify driver name usage | expand |
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>
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.
> -----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!
> -----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
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
> -----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
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
> -----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
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);
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(-)