diff mbox series

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

Message ID 20230411011354.2619359-3-pavan.kumar.linga@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Introduce Intel IDPF driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pavan Kumar Linga April 11, 2023, 1:13 a.m. UTC
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

Comments

Leon Romanovsky April 11, 2023, 12:36 p.m. UTC | #1
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
>
Emil Tantilov April 12, 2023, 11:10 p.m. UTC | #2
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@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?

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
>>
Leon Romanovsky April 13, 2023, 6:03 a.m. UTC | #3
On Wed, Apr 12, 2023 at 04:10:18PM -0700, Tantilov, Emil S wrote:
> 
> 
> 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@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_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.

I don't think so, as PCI probe/remove has very clear lifetime model and
doesn't need garbage collection memory logic. In general, it is better
to avoid devm_*() APIs as they hide error unwind flows.

Thanks
Emil Tantilov April 13, 2023, 6:58 p.m. UTC | #4
On 4/12/2023 11:03 PM, Leon Romanovsky wrote:
> On Wed, Apr 12, 2023 at 04:10:18PM -0700, Tantilov, Emil S wrote:
>>
>>
>> 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@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_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.
> 
> I don't think so, as PCI probe/remove has very clear lifetime model and
> doesn't need garbage collection memory logic. In general, it is better
> to avoid devm_*() APIs as they hide error unwind flows.

We'll remove it in v3.

Thanks,
Emil

> Thanks
Emil Tantilov April 20, 2023, 6:13 p.m. UTC | #5
On 4/12/2023 4:10 PM, Tantilov, Emil S wrote:
> 
> 
> 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@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_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.

On second look, the reason PCI ids are added in the last patch is 
because none of the modules from the previous patches would result in a 
functional driver.

Thanks,
Emil
Leon Romanovsky April 20, 2023, 6:20 p.m. UTC | #6
On Thu, Apr 20, 2023 at 11:13:09AM -0700, Tantilov, Emil S wrote:
> 
> 
> On 4/12/2023 4:10 PM, Tantilov, Emil S wrote:
> > 
> > 
> > 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@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_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.
> 
> On second look, the reason PCI ids are added in the last patch is because
> none of the modules from the previous patches would result in a functional
> driver.

And yet patches should be split to meaningful and logical chunks. If
you add pci_device_id, please add relevant device at the same patch.

Thanks

> 
> Thanks,
> Emil
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index c18c3b373846..8c250326985f 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -373,4 +373,15 @@  config IGC
 	  To compile this driver as a module, choose M here. The module
 	  will be called igc.
 
+config IDPF
+	tristate "Intel(R) Infrastructure Data Path Function Support"
+	depends on PCI_MSI
+	select DIMLIB
+	help
+	  This driver supports Intel(R) Infrastructure Processing Unit (IPU)
+	  devices.
+
+	  To compile this driver as a module, choose M here. The module
+	  will be called idpf.
+
 endif # NET_VENDOR_INTEL
diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
index 3075290063f6..70cc2ae1cb97 100644
--- a/drivers/net/ethernet/intel/Makefile
+++ b/drivers/net/ethernet/intel/Makefile
@@ -16,3 +16,4 @@  obj-$(CONFIG_IXGB) += ixgb/
 obj-$(CONFIG_IAVF) += iavf/
 obj-$(CONFIG_FM10K) += fm10k/
 obj-$(CONFIG_ICE) += ice/
+obj-$(CONFIG_IDPF) += idpf/
diff --git a/drivers/net/ethernet/intel/idpf/Makefile b/drivers/net/ethernet/intel/idpf/Makefile
new file mode 100644
index 000000000000..d26521c987e1
--- /dev/null
+++ b/drivers/net/ethernet/intel/idpf/Makefile
@@ -0,0 +1,10 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+# Copyright (C) 2023 Intel Corporation
+
+# Makefile for Infrastructure Data Path Function Linux Driver
+
+obj-$(CONFIG_IDPF) += idpf.o
+
+idpf-y := \
+	idpf_lib.o		\
+	idpf_main.o
diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
new file mode 100644
index 000000000000..7494f9d29970
--- /dev/null
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -0,0 +1,27 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2023 Intel Corporation */
+
+#ifndef _IDPF_H_
+#define _IDPF_H_
+
+#include <linux/aer.h>
+#include <linux/etherdevice.h>
+#include <linux/pci.h>
+
+#include "idpf_controlq.h"
+
+/* available message levels */
+#define IDPF_AVAIL_NETIF_M (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK)
+
+struct idpf_adapter {
+	struct pci_dev *pdev;
+	u32 msg_enable;
+	struct idpf_hw hw;
+};
+
+int idpf_pf_probe(struct pci_dev *pdev, struct idpf_adapter *adapter);
+int idpf_vf_probe(struct pci_dev *pdev, struct idpf_adapter *adapter);
+int idpf_probe_common(struct pci_dev *pdev, struct idpf_adapter *adapter);
+void idpf_remove_common(struct pci_dev *pdev);
+
+#endif /* !_IDPF_H_ */
diff --git a/drivers/net/ethernet/intel/idpf/idpf_controlq.h b/drivers/net/ethernet/intel/idpf/idpf_controlq.h
new file mode 100644
index 000000000000..383089c91675
--- /dev/null
+++ b/drivers/net/ethernet/intel/idpf/idpf_controlq.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2023 Intel Corporation */
+
+#ifndef _IDPF_CONTROLQ_H_
+#define _IDPF_CONTROLQ_H_
+
+struct idpf_hw {
+	void __iomem *hw_addr;
+	resource_size_t hw_addr_len;
+
+	void *back;
+};
+
+#endif /* _IDPF_CONTROLQ_H_ */
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
new file mode 100644
index 000000000000..bf25574c75e4
--- /dev/null
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -0,0 +1,96 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Intel Corporation */
+
+#include "idpf.h"
+
+/**
+ * idpf_cfg_hw - Initialize HW struct
+ * @adapter: adapter to setup hw struct for
+ *
+ * Returns 0 on success, negative on failure
+ */
+static int idpf_cfg_hw(struct idpf_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct idpf_hw *hw = &adapter->hw;
+
+	hw->hw_addr = pcim_iomap_table(pdev)[0];
+	if (!hw->hw_addr) {
+		pci_err(pdev, "failed to allocate PCI iomap table\n");
+
+		return -ENOMEM;
+	}
+
+	hw->back = adapter;
+
+	return 0;
+}
+
+/**
+ * idpf_probe_common - Device initialization routine
+ * @pdev: PCI device information struct
+ * @adapter: driver specific private structure
+ *
+ * Returns 0 on success, negative on failure
+ */
+int idpf_probe_common(struct pci_dev *pdev, struct idpf_adapter *adapter)
+{
+	struct device *dev = &pdev->dev;
+	int err;
+
+	adapter->pdev = pdev;
+
+	err = pcim_enable_device(pdev);
+	if (err)
+		return err;
+
+	err = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
+	if (err) {
+		pci_err(pdev, "pcim_iomap_regions failed %pe\n", ERR_PTR(err));
+
+		return err;
+	}
+
+	/* set up for high or low dma */
+	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	if (err) {
+		pci_err(pdev, "DMA configuration failed: %pe\n", ERR_PTR(err));
+
+		return err;
+	}
+
+	pci_enable_pcie_error_reporting(pdev);
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, adapter);
+
+	/* setup msglvl */
+	adapter->msg_enable = netif_msg_init(-1, IDPF_AVAIL_NETIF_M);
+
+	err = idpf_cfg_hw(adapter);
+	if (err) {
+		dev_err(dev, "Failed to configure HW structure for adapter: %d\n",
+			err);
+		goto err_cfg_hw;
+	}
+
+	return 0;
+
+err_cfg_hw:
+	pci_disable_pcie_error_reporting(pdev);
+
+	return err;
+}
+
+/**
+ * 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)
+		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)
+		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);
+	if (!adapter)
+		return -ENOMEM;
+
+	return idpf_probe_common(pdev, adapter);
+}
+
+/* idpf_pci_tbl - PCI Dev idpf ID Table
+ */
+static const struct pci_device_id idpf_pci_tbl[] = {
+	{ /* Sentinel */ }
+};
+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);