Re: [PATCH net-next v2 02/15] idpf: add module register and probe functionality

[Leon Romanovsky <leon@kernel.org>]

On Mon, Apr 10, 2023 at 06:13:41PM -0700, Pavan Kumar Linga wrote:
> From: Phani Burra <phani.r.burra@intel.com>
> 
> Add the required support to register IDPF PCI driver, as well as
> probe and remove call backs. Enable the PCI device and request
> the kernel to reserve the memory resources that will be used by the
> driver. Finally map the BAR0 address space.
> 
> PCI IDs table is intentionally left blank to prevent the kernel from
> probing the device with the incomplete driver. It will be added
> in the last patch of the series.
> 
> Signed-off-by: Phani Burra <phani.r.burra@intel.com>
> Co-developed-by: Alan Brady <alan.brady@intel.com>
> Signed-off-by: Alan Brady <alan.brady@intel.com>
> Co-developed-by: Madhu Chittim <madhu.chittim@intel.com>
> Signed-off-by: Madhu Chittim <madhu.chittim@intel.com>
> Co-developed-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
> Signed-off-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
> Co-developed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
>  drivers/net/ethernet/intel/Kconfig            | 11 +++
>  drivers/net/ethernet/intel/Makefile           |  1 +
>  drivers/net/ethernet/intel/idpf/Makefile      | 10 ++
>  drivers/net/ethernet/intel/idpf/idpf.h        | 27 ++++++
>  .../net/ethernet/intel/idpf/idpf_controlq.h   | 14 +++
>  drivers/net/ethernet/intel/idpf/idpf_lib.c    | 96 +++++++++++++++++++
>  drivers/net/ethernet/intel/idpf/idpf_main.c   | 70 ++++++++++++++
>  7 files changed, 229 insertions(+)
>  create mode 100644 drivers/net/ethernet/intel/idpf/Makefile
>  create mode 100644 drivers/net/ethernet/intel/idpf/idpf.h
>  create mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.h
>  create mode 100644 drivers/net/ethernet/intel/idpf/idpf_lib.c
>  create mode 100644 drivers/net/ethernet/intel/idpf/idpf_main.c

<...>

> +/**
> + * idpf_remove_common - Device removal routine
> + * @pdev: PCI device information struct
> + */
> +void idpf_remove_common(struct pci_dev *pdev)
> +{
> +	struct idpf_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	if (!adapter)

How is it possible to have adapter be NULL here?

> +		return;
> +
> +	pci_disable_pcie_error_reporting(pdev);
> +}
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> new file mode 100644
> index 000000000000..617df9b924fa
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2023 Intel Corporation */
> +
> +#include "idpf.h"
> +
> +#define DRV_SUMMARY	"Infrastructure Data Path Function Linux Driver"
> +
> +MODULE_DESCRIPTION(DRV_SUMMARY);
> +MODULE_LICENSE("GPL");
> +
> +/**
> + * idpf_remove - Device removal routine
> + * @pdev: PCI device information struct
> + */
> +static void idpf_remove(struct pci_dev *pdev)
> +{
> +	struct idpf_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	if (!adapter)

Ditto

> +		return;
> +
> +	idpf_remove_common(pdev);
> +	pci_set_drvdata(pdev, NULL);
> +}
> +
> +/**
> + * idpf_shutdown - PCI callback for shutting down device
> + * @pdev: PCI device information struct
> + */
> +static void idpf_shutdown(struct pci_dev *pdev)
> +{
> +	idpf_remove(pdev);
> +
> +	if (system_state == SYSTEM_POWER_OFF)
> +		pci_set_power_state(pdev, PCI_D3hot);
> +}
> +
> +/**
> + * idpf_probe - Device initialization routine
> + * @pdev: PCI device information struct
> + * @ent: entry in idpf_pci_tbl
> + *
> + * Returns 0 on success, negative on failure
> + */
> +static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	struct idpf_adapter *adapter;
> +
> +	adapter = devm_kzalloc(&pdev->dev, sizeof(*adapter), GFP_KERNEL);

Why devm_kzalloc() and not kzalloc?

> +	if (!adapter)
> +		return -ENOMEM;
> +
> +	return idpf_probe_common(pdev, adapter);

There is no need in idpf_probe_common/idpf_remove_common functions and
they better be embedded here. They called only once and just obfuscate
the code.

> +}
> +
> +/* idpf_pci_tbl - PCI Dev idpf ID Table
> + */
> +static const struct pci_device_id idpf_pci_tbl[] = {
> +	{ /* Sentinel */ }

What does it mean empty pci_device_id table?

> +};
> +MODULE_DEVICE_TABLE(pci, idpf_pci_tbl);
> +
> +static struct pci_driver idpf_driver = {
> +	.name			= KBUILD_MODNAME,
> +	.id_table		= idpf_pci_tbl,
> +	.probe			= idpf_probe,
> +	.remove			= idpf_remove,
> +	.shutdown		= idpf_shutdown,
> +};
> +module_pci_driver(idpf_driver);
> -- 
> 2.37.3
>