[PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[krytarowski]

From: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>

This change enables drivers needing custom pci (de)initialization functions
through the standard callback overloading.

For example:

 /*
  * virtual function driver struct
  */
 static struct eth_driver rte_DRIVER_pmd = {
 	{
 		.name = "rte_DRIVER_pmd",
 		.id_table = pci_id_DRIVER_map,
 		.drv_flags = RTE_PCI_DRV_NEED_MAPPING,
		.devinit = DRIVER_pci_devinit,
 	},
 	.eth_dev_init = eth_DRIVER_dev_init,
 	.dev_private_size = sizeof(struct DRIVER),
 };

Use-case is as follows: NIC offers several pci virtual functions, while
one of them is to be treated as port, we need to configure the rest in a
specific way for particular device for full interface (port) functionality.

Signed-off-by: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
---
 lib/librte_ether/rte_ethdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 756b234..ac4aeab 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -351,8 +351,10 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
 void
 rte_eth_driver_register(struct eth_driver *eth_drv)
 {
-	eth_drv->pci_drv.devinit = rte_eth_dev_init;
-	eth_drv->pci_drv.devuninit = rte_eth_dev_uninit;
+	if (!eth_drv->pci_drv.devinit)
+		eth_drv->pci_drv.devinit = rte_eth_dev_init;
+	if (!eth_drv->pci_drv.devuninit)
+		eth_drv->pci_drv.devuninit = rte_eth_dev_uninit;
 	rte_eal_pci_register(&eth_drv->pci_drv);
 }
 
-- 
1.9.1

[PATCH 2/2] ethdev: Export rte_eth_dev_create_unique_device_name() to public API

[krytarowski]

From: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>

Once pci_drv.devinit is overloaded, it's a function used in the original
rte_eth_dev_init(), still reusable in altered versions.

Signed-off-by: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
---
 lib/librte_ether/rte_ethdev.c |  2 +-
 lib/librte_ether/rte_ethdev.h | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index ac4aeab..7f5e741 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -214,7 +214,7 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
 	return eth_dev;
 }
 
-static int
+int
 rte_eth_dev_create_unique_device_name(char *name, size_t size,
 		struct rte_pci_device *pci_dev)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 8710dd7..b19db9d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -3880,6 +3880,24 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *eth_dev, const char *name,
 			 uint16_t queue_id, size_t size,
 			 unsigned align, int socket_id);
 
+/**
+ * Create unique device name
+ *
+ * @param name
+ *   The port identifier of the Ethernet device.
+ * @param size
+ *   Maximum string length of the generated name
+ * @param pci_dev
+ *   PCI device pointer
+ *
+ * @return
+ *   - 0: Success.
+ *   - <0: Error during generatin
+ *   - -EINVAL: Invalid input parameters.
+ */
+int rte_eth_dev_create_unique_device_name(char *name, size_t size,
+					  struct rte_pci_device *pci_dev);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.9.1

Re: [PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[David Marchand]

Hello,

On Tue, Feb 2, 2016 at 3:27 PM,  <krytarowski@caviumnetworks.com> wrote:
> From: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
>
> This change enables drivers needing custom pci (de)initialization functions
> through the standard callback overloading.
>
> For example:
>
>  /*
>   * virtual function driver struct
>   */
>  static struct eth_driver rte_DRIVER_pmd = {
>         {
>                 .name = "rte_DRIVER_pmd",
>                 .id_table = pci_id_DRIVER_map,
>                 .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
>                 .devinit = DRIVER_pci_devinit,
>         },
>         .eth_dev_init = eth_DRIVER_dev_init,
>         .dev_private_size = sizeof(struct DRIVER),
>  };
>
> Use-case is as follows: NIC offers several pci virtual functions, while
> one of them is to be treated as port, we need to configure the rest in a
> specific way for particular device for full interface (port) functionality.

Mmm, why don't you register a custom pci driver rather than a eth_driver ?
And do your custom things in its devinit function ?


-- 
David Marchand

Re: [PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[Kamil Rytarowski]

W dniu 03.02.2016 o 09:47, David Marchand pisze:
> Hello,
Hello,

> On Tue, Feb 2, 2016 at 3:27 PM,  <krytarowski@caviumnetworks.com> wrote:
>> From: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
>>
>> This change enables drivers needing custom pci (de)initialization functions
>> through the standard callback overloading.
>>
>> For example:
>>
>>   /*
>>    * virtual function driver struct
>>    */
>>   static struct eth_driver rte_DRIVER_pmd = {
>>          {
>>                  .name = "rte_DRIVER_pmd",
>>                  .id_table = pci_id_DRIVER_map,
>>                  .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
>>                  .devinit = DRIVER_pci_devinit,
>>          },
>>          .eth_dev_init = eth_DRIVER_dev_init,
>>          .dev_private_size = sizeof(struct DRIVER),
>>   };
>>
>> Use-case is as follows: NIC offers several pci virtual functions, while
>> one of them is to be treated as port, we need to configure the rest in a
>> specific way for particular device for full interface (port) functionality.
> Mmm, why don't you register a custom pci driver rather than a eth_driver ?
Both Virtual Functions are of eth_driver type and they share the same 
PCI ID.

They are like 2 (or more) NIC devices working in tandem (it's not 
bonding), with a possibility to join into a single one and offer 
additional functionality. In that case one is master and the other is 
donor of functionality, like additional queue sets.

> And do your custom things in its devinit function ?
>
>
Yes, I'm doing custom things.

I'm requesting from PF the mode of the device to be initialized. This 
part is handled dynamically and depends of the current configuration in PF.

In my use-case there are two device types: primary (master) and 
secondary (slave). For the primary VF I'm creating a DPDK port normally, 
for secondary I retain configured PCI device for further reuse (and 
there is no port created).

I cannot overload functions called after .devinit() as the secondary 
devices will be wrongly treated like a DPDK port.


My use-case doesn't need modified API to be functional.

The missing handling of overloaded .devinit and .devuninit looks like a 
bug - there is API for it, but it will keep overwriting the pointers 
with local functions (rte_eth_dev_init(), rte_eth_dev_uninit()).

Re: [PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[David Marchand]

On Wed, Feb 3, 2016 at 12:39 PM, Kamil Rytarowski
<krytarowski@caviumnetworks.com> wrote:
> W dniu 03.02.2016 o 09:47, David Marchand pisze:
>> And do your custom things in its devinit function ?
>
> I'm requesting from PF the mode of the device to be initialized. This part
> is handled dynamically and depends of the current configuration in PF.
>
> In my use-case there are two device types: primary (master) and secondary
> (slave). For the primary VF I'm creating a DPDK port normally, for secondary
> I retain configured PCI device for further reuse (and there is no port
> created).

Well, again, if you don't want to associate a port to this pci
resource, why are you registering a eth_driver ?
A eth_driver driver supposes you have a 1 - 1 relation between ethdev
and pci resource.

For your case, register a pci driver, then in your pci probing
function (.devinit), depending on what you want to do, you can either
do nothing (?) or create one or more ethdevs (see mlx* and cxgbe
drivers).



-- 
David Marchand

Re: [PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[Kamil Rytarowski]

W dniu 03.02.2016 o 15:08, David Marchand pisze:
> On Wed, Feb 3, 2016 at 12:39 PM, Kamil Rytarowski
> <krytarowski@caviumnetworks.com> wrote:
>> W dniu 03.02.2016 o 09:47, David Marchand pisze:
>>> And do your custom things in its devinit function ?
>> I'm requesting from PF the mode of the device to be initialized. This part
>> is handled dynamically and depends of the current configuration in PF.
>>
>> In my use-case there are two device types: primary (master) and secondary
>> (slave). For the primary VF I'm creating a DPDK port normally, for secondary
>> I retain configured PCI device for further reuse (and there is no port
>> created).
> Well, again, if you don't want to associate a port to this pci
> resource, why are you registering a eth_driver ?
> A eth_driver driver supposes you have a 1 - 1 relation between ethdev
> and pci resource.
In my use-case one DPDK port optionally manages more than single PCI 
resource, and these PCI resources compose single interface.

Another example of overloaded .devinit is in app/test/test_pci.c:

/*
  * PCI test
  * ========
  *
  * - Register a driver with a ``devinit()`` function.
  *
  * - Dump all PCI devices.
  *
  * - Check that the ``devinit()`` function is called at least once.
  */

With the current implementation it won't work, as .devinit callback will 
be overwritten by the internal function.

> For your case, register a pci driver, then in your pci probing
> function (.devinit), depending on what you want to do, you can either
> do nothing (?) or create one or more ethdevs (see mlx* and cxgbe
> drivers).
>
>
>

This is what I'm doing right now.

I need to initialize PCI bars and interrupts (resources) - all having 
the same PCI ID and their functionality depending upon PF configuration. 
Depending on this state, I'm making further decisions in 
DRIVER_devinit() and whether to make from it a port or a resource to 
reuse by a master port.

Re: [PATCH 1/2] ethdev: Allow to overload pci_drv.devinit and pci_drv.devuninit

[Kamil Rytarowski]

W dniu 03.02.2016 o 16:49, Kamil Rytarowski pisze:
>
>
> W dniu 03.02.2016 o 15:08, David Marchand pisze:
>> On Wed, Feb 3, 2016 at 12:39 PM, Kamil Rytarowski
>> <krytarowski@caviumnetworks.com> wrote:
>>> W dniu 03.02.2016 o 09:47, David Marchand pisze:
>>>> And do your custom things in its devinit function ?
>>> I'm requesting from PF the mode of the device to be initialized. 
>>> This part
>>> is handled dynamically and depends of the current configuration in PF.
>>>
>>> In my use-case there are two device types: primary (master) and 
>>> secondary
>>> (slave). For the primary VF I'm creating a DPDK port normally, for 
>>> secondary
>>> I retain configured PCI device for further reuse (and there is no port
>>> created).
>> Well, again, if you don't want to associate a port to this pci
>> resource, why are you registering a eth_driver ?
>> A eth_driver driver supposes you have a 1 - 1 relation between ethdev
>> and pci resource.
> In my use-case one DPDK port optionally manages more than single PCI 
> resource, and these PCI resources compose single interface.
>
> Another example of overloaded .devinit is in app/test/test_pci.c:
>
> /*
>  * PCI test
>  * ========
>  *
>  * - Register a driver with a ``devinit()`` function.
>  *
>  * - Dump all PCI devices.
>  *
>  * - Check that the ``devinit()`` function is called at least once.
>  */
>
> With the current implementation it won't work, as .devinit callback 
> will be overwritten by the internal function.
>
>> For your case, register a pci driver, then in your pci probing
>> function (.devinit), depending on what you want to do, you can either
>> do nothing (?) or create one or more ethdevs (see mlx* and cxgbe
>> drivers).
>>
>>
>>
>
> This is what I'm doing right now.
>
> I need to initialize PCI bars and interrupts (resources) - all having 
> the same PCI ID and their functionality depending upon PF 
> configuration. Depending on this state, I'm making further decisions 
> in DRIVER_devinit() and whether to make from it a port or a resource 
> to reuse by a master port.

After sending the mail I noted that I can work with the .init callback 
and I may be wrong.

I will have a closer look and I will check whether it will work for me.

Thank you,

Re: [PATCH 2/2] ethdev: Export rte_eth_dev_create_unique_device_name() to public API

[Panu Matilainen]

On 02/02/2016 04:27 PM, krytarowski@caviumnetworks.com wrote:
> From: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
>
> Once pci_drv.devinit is overloaded, it's a function used in the original
> rte_eth_dev_init(), still reusable in altered versions.
>
> Signed-off-by: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
> ---
>   lib/librte_ether/rte_ethdev.c |  2 +-
>   lib/librte_ether/rte_ethdev.h | 18 ++++++++++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ac4aeab..7f5e741 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -214,7 +214,7 @@ rte_eth_dev_allocate(const char *name, enum rte_eth_dev_type type)
>   	return eth_dev;
>   }
>
> -static int
> +int
>   rte_eth_dev_create_unique_device_name(char *name, size_t size,
>   		struct rte_pci_device *pci_dev)
>   {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 8710dd7..b19db9d 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -3880,6 +3880,24 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *eth_dev, const char *name,
>   			 uint16_t queue_id, size_t size,
>   			 unsigned align, int socket_id);
>
> +/**
> + * Create unique device name
> + *
> + * @param name
> + *   The port identifier of the Ethernet device.
> + * @param size
> + *   Maximum string length of the generated name
> + * @param pci_dev
> + *   PCI device pointer
> + *
> + * @return
> + *   - 0: Success.
> + *   - <0: Error during generatin
> + *   - -EINVAL: Invalid input parameters.
> + */
> +int rte_eth_dev_create_unique_device_name(char *name, size_t size,
> +					  struct rte_pci_device *pci_dev);
> +
>   #ifdef __cplusplus
>   }
>   #endif
>

To really export it, you'll need to add it to rte_ether_version.map as well.

	- Panu -

Re: [PATCH 2/2] ethdev: Export rte_eth_dev_create_unique_device_name() to public API

[David Marchand]

On Thu, Feb 11, 2016 at 5:56 PM, Panu Matilainen <pmatilai@redhat.com> wrote:
> On 02/02/2016 04:27 PM, krytarowski@caviumnetworks.com wrote:
>>
>> From: Kamil Rytarowski <Kamil.Rytarowski@caviumnetworks.com>
>> +int rte_eth_dev_create_unique_device_name(char *name, size_t size,
>> +                                         struct rte_pci_device *pci_dev);
> To really export it, you'll need to add it to rte_ether_version.map as well.

I already submitted something equivalent, and anyway, this belongs to
eal, not ethdev.
http://dpdk.org/ml/archives/dev/2016-January/032394.html


-- 
David Marchand