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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <BYAPR11MB34956FC76E48D37CF146A657F731A@BYAPR11MB3495.namprd11.prod.outlook.com>
Date: Tue, 11 Jul 2023 17:24:36 +0000
From: "Wang, Haiyue" <haiyue.wang@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>, "Guo, Junfeng"
	<junfeng.guo@...el.com>
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>, "kuba@...nel.org"
	<kuba@...nel.org>, "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: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: Tuesday, July 11, 2023 21:14
> 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>; kuba@...nel.org; 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
> 
> From: Junfeng Guo <junfeng.guo@...el.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