[dpdk-dev] [PATCH v4 02/16] net/mrvl: add mrvl net pmd driver skeleton

[Jacek Siuda]

Hi,

The problem is, that both drivers are totally separate entities, linked
into separate libs, but we need to use only one setting in both of them.
So, in order to provide any runtime (or even compile-time) verification, we
would need to create a third entity just to handle a simple assertion - a
bit overkill. At the time the code was written, common configuration option
seemed the most logical. If your long-term goal is to get rid of build-time
configurations, then it puts everything in a different perspective.

What we can do, is to add a run-time driver option and oblige user (in
documentation) to set the option only for the first mrvl vdev. If the
driver doesn't see the option set, it  won't initialize DMA. This, plus
meaningful error handling/logs should take care of any potential
mishandling by user that we were afraid of. I hope that would be more
acceptable.

Best Regards,
Jacek Siuda.




2017-10-12 9:59 GMT+02:00 Vincent JARDIN <vincent.jardin at 6wind.com>:

> +1 with Thomas, see below,
>
> Le 12/10/2017 à 08:51, Tomasz Duszynski a écrit :
>
>> What is MUSDK_DMA_MEMSIZE?
>>> If the value cannot change, it must be a constant in the code.
>>> If it can change, it should be a run-time driver option.
>>>
>> It's up to the user what MUSDK_DMA_MEMSIZE is going to be. Currently it's
>> set to value that should work it all cases.
>>
>> Except that, MUSDK_DMA_MEMSIZE is used as synchronization point for net
>> and crypto (on condition they are used together i.e ipsec-secgw).
>>
>> Suppose we have two different MUSDK_DMA_MEMSIZE defined for net/crypto
>> then
>> dma memsize allocated will depend on driver probing sequence which might
>> confuse user.
>>
> It does not make sense,
> +       /*
> +        * ret == -EEXIST is correct, it means DMA
> +        * has been already initialized (by another PMD).
> +        */
> +       ret = mv_sys_dma_mem_init(RTE_MRVL_MUSDK_DMA_MEMSIZE
>
> int mv_sys_dma_mem_init(u64 size)
> {
>         struct sys_dma  *i_sys_dma;
>         int err;
>
>         if (sys_dma) {
>                 pr_err("Dma object already exits.\n");
>                 return -EEXIST;
>         }
>
> So, I do not understand why you cannot add some checks into the drivers to
> assert that users must have set the same value for both when calling:
>   ret = mv_sys_dma_mem_init(my_best_size);
> maybe, you need to fix and improve musdk first to avoid DPDK from getting
> such compilation issues.
>
> best regards,
>   Vincent
>