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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR11MB3723743789CBF36E46792158E732A@DM6PR11MB3723.namprd11.prod.outlook.com>
Date: Sat, 8 Jul 2023 02:58:29 +0000
From: "Guo, Junfeng" <junfeng.guo@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "jeroendb@...gle.com"
	<jeroendb@...gle.com>, "pkaligineedi@...gle.com" <pkaligineedi@...gle.com>,
	"shailend@...gle.com" <shailend@...gle.com>, "Wang, Haiyue"
	<haiyue.wang@...el.com>, "awogbemila@...gle.com" <awogbemila@...gle.com>,
	"davem@...emloft.net" <davem@...emloft.net>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "yangchun@...gle.com" <yangchun@...gle.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "csully@...gle.com"
	<csully@...gle.com>
Subject: RE: [PATCH net] gve: unify driver name usage



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Saturday, July 8, 2023 06:20
> To: Guo, Junfeng <junfeng.guo@...el.com>
> Cc: netdev@...r.kernel.org; jeroendb@...gle.com;
> pkaligineedi@...gle.com; shailend@...gle.com; Wang, Haiyue
> <haiyue.wang@...el.com>; awogbemila@...gle.com;
> davem@...emloft.net; pabeni@...hat.com; yangchun@...gle.com;
> edumazet@...gle.com; csully@...gle.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@...gle.com
> > Signed-off-by: Junfeng Guo <junfeng.guo@...el.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