[dpdk-dev] [PATCH 2/9] pci: register all pdev as pci drivers

[David Marchand]

On Mon, Feb 8, 2016 at 12:03 PM, Jan Viktorin <viktorin at rehivetech.com> wrote:
> On Fri, 29 Jan 2016 15:08:29 +0100
> David Marchand <david.marchand at 6wind.com> wrote:
>
>> pdev drivers are actually pci drivers.
>> Wrappers for ethdev and crypto pci drivers, that assume a 1 to 1
>> association between pci device and upper device, have been added so that
>> current drivers are not impacted.
>
> It took me a while to find out what's going on in this patch. I could
> see several not-so-related changes down the e-mail. I'd suggest to split
> it this way:
>
> 1) separate coding style fixes
> 2) rename init/uninit to probe/remove (preserve the 'static' there)
> 3) remove rte_eth_driver_register invocations
> 4) separate VDEV and PDEV for cryptodev
> 5) play with the NEXT_ABI (remove those 'static' keywords?)
>
> A more detailed commit log would help too. But this would
> be automatically solved by the separation, I think.

Yes, I rushed for this patchset to still be in the proposal window ...
Sorry, I will split for next version.


> See comments below...

Globally, all my answers are "Yes, will do and it will be easier with
smaller patches".
Just added some comments where appropriate.


>> diff --git a/drivers/crypto/qat/rte_qat_cryptodev.c b/drivers/crypto/qat/rte_qat_cryptodev.c
>> index e500c1e..6853aee 100644
>> --- a/drivers/crypto/qat/rte_qat_cryptodev.c
>> +++ b/drivers/crypto/qat/rte_qat_cryptodev.c
>> @@ -113,10 +113,12 @@ crypto_qat_dev_init(__attribute__((unused)) struct rte_cryptodev_driver *crypto_
>>  }
>>
>>  static struct rte_cryptodev_driver rte_qat_pmd = {
>> -     {
>> +     .pci_drv = {
>
> I believe that you are making the driver independent on the exact
> location of the pci_drv member in the rte_cryptodev_drivers struct. Is
> it true? Why is that important?

Yes, plus all other drivers are doing this, there were little
exception to this convention, so I just aligned here.


>>               .name = "rte_qat_pmd",
>>               .id_table = pci_id_qat_map,
>>               .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
>> +             .devinit = rte_cryptodev_pci_probe,
>> +             .devuninit = rte_cryptodev_pci_remove,
>>       },
>>       .cryptodev_init = crypto_qat_dev_init,
>>       .dev_private_size = sizeof(struct qat_pmd_private),
>> @@ -126,7 +128,8 @@ static int
>>  rte_qat_pmd_init(const char *name __rte_unused, const char *params __rte_unused)
>>  {
>>       PMD_INIT_FUNC_TRACE();
>> -     return rte_cryptodev_pmd_driver_register(&rte_qat_pmd, PMD_PDEV);
>> +     rte_eal_pci_register(&rte_qat_pmd.pci_drv);
>> +     return 0;
>
> So, I finally discovered that this change somehow separates the PCI
> (PDEV) and VDEV initialization. Is that correct?

Yes.
I will separate crypto updates from netdev updates since crypto
framework has a slight different way of initialising.


>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index 756b234..17e4f4d 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -239,9 +239,9 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>>       return 0;
>>  }
>>
>> -static int
>> -rte_eth_dev_init(struct rte_pci_driver *pci_drv,
>> -              struct rte_pci_device *pci_dev)
>> +int
>> +rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
>> +                   struct rte_pci_device *pci_dev)
>
> As I've suggested at the beginning, what about "first just rename then
> make it public (non-static)"? I don't see the connection between the
> rename and the NEXT_ABI conditional.

I don't think we need NEXT_ABI checks here.
I am not breaking anything here, just adding a new symbol.

I will introduce this new symbol, then convert all existing
pdev/eth_driver to pdev/pci_driver in a second patch.


-- 
David Marchand