| 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
| ||
|
Message-ID: <b6ed7b0b-9262-3578-1d88-4c848d1aea82@intel.com>
Date: Wed, 12 Apr 2023 16:10:18 -0700
From: "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To: Leon Romanovsky <leon@...nel.org>,
Pavan Kumar Linga <pavan.kumar.linga@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
<joshua.a.hay@...el.com>, <sridhar.samudrala@...el.com>,
<jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
<willemb@...gle.com>, <decot@...gle.com>, <pabeni@...hat.com>,
<kuba@...nel.org>, <edumazet@...gle.com>, <davem@...emloft.net>,
Phani Burra <phani.r.burra@...el.com>,
Alan Brady <alan.brady@...el.com>,
Madhu Chittim <madhu.chittim@...el.com>,
Shailendra Bhatnagar <shailendra.bhatnagar@...el.com>
Subject: Re: [PATCH net-next v2 02/15] idpf: add module register and probe
functionality
On 4/11/2023 5:36 AM, Leon Romanovsky wrote:
> On Mon, Apr 10, 2023 at 06:13:41PM -0700, Pavan Kumar Linga wrote:
>> From: Phani Burra <phani.r.burra@...el.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@...el.com>
>> Co-developed-by: Alan Brady <alan.brady@...el.com>
>> Signed-off-by: Alan Brady <alan.brady@...el.com>
>> Co-developed-by: Madhu Chittim <madhu.chittim@...el.com>
>> Signed-off-by: Madhu Chittim <madhu.chittim@...el.com>
>> Co-developed-by: Shailendra Bhatnagar <shailendra.bhatnagar@...el.com>
>> Signed-off-by: Shailendra Bhatnagar <shailendra.bhatnagar@...el.com>
>> Co-developed-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
>> Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
>> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
>> Reviewed-by: Willem de Bruijn <willemb@...gle.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?
I think it is just defensive check since remove can be called on error
code path, but in general you're right and we likely don't need it. Will
remove in v3.
>
>> + 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
Will remove in v3.
>> + 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?
It provides managed memory allocation on probe, which seems to be the
preferred method in that case.
>> + 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.
Will remove in v3.
>> +}
>> +
>> +/* 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?
Device ID's are added later, but it does make sense to be in this patch
instead. Will fix in v3.
>> +};
>> +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
>>
Powered by blists - more mailing lists