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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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