[dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver

[Ferruh Yigit]

On 10/4/2017 9:59 AM, Tomasz Duszynski wrote:
> On Wed, Oct 04, 2017 at 01:24:27AM +0100, Ferruh Yigit wrote:
>> On 10/3/2017 12:51 PM, Tomasz Duszynski wrote:
>>> Add support for the Marvell PPv2 (Packet Processor v2) 1/10 Gbps adapter.
>>> Driver is based on external, publicly available, light-weight Marvell
>>> MUSDK library that provides access to network packet processor.
>>>
>>> Driver comes with support for the following features:
>>>
>>> * Speed capabilities
>>> * Link status
>>> * Queue start/stop
>>> * MTU update
>>> * Jumbo frame
>>> * Promiscuous mode
>>> * Allmulticast mode
>>> * Unicast MAC filter
>>> * Multicast MAC filter
>>> * RSS hash
>>> * VLAN filter
>>> * CRC offload
>>> * L3 checksum offload
>>> * L4 checksum offload
>>> * Packet type parsing
>>> * Basic stats
>>> * Stats per queue
>>
>> I have more detailed comments but in high level,
>> what do you think splitting this patch into three patches:
>> - Skeleton
>> - Add Rx/Tx support
>> - Add features, like MTU update or Promiscuous etc.. support
> If it's how submission process works then I think you left me with no
> other option than splitting driver into nice patchset :). 

No, there is no defined submission process.

> On the other
> hand driver is really a wrapper to MUSDK library and thus quite easy to
> follow. What are the benefits of such 3-way split?

To help others review/understand your code. Big code chunks are scary
and I believe most of details gets lost in big code chunks.

When someone from community wants to understand and update/improve/fix
your code, to help them by logically split the code that their focus can
go into more narrow part.

But this also means some effort in your side, so some kind of balance is
required.

I think splitting patch into smaller logical part is helpful for others,
what do you think, is it too much effort?

>>
>>>
>>> Driver was engineered cooperatively by Semihalf and Marvell teams.
>>>
>>> Semihalf:
>>> Jacek Siuda <jck at semihalf.com>
>>> Tomasz Duszynski <tdu at semihalf.com>
>>>
>>> Marvell:
>>> Dmitri Epshtein <dima at marvell.com>
>>> Natalie Samsonov <nsamsono at marvell.com>
>>>
>>> Signed-off-by: Jacek Siuda <jck at semihalf.com>
>>> Signed-off-by: Tomasz Duszynski <tdu at semihalf.com>
>>
>> <...>
>>
>>> +static struct rte_vdev_driver pmd_mrvl_drv = {
>>> +	.probe = rte_pmd_mrvl_probe,
>>> +	.remove = rte_pmd_mrvl_remove,
>>> +};
>>> +
>>> +RTE_PMD_REGISTER_VDEV(net_mrvl, pmd_mrvl_drv);
>>
>> Please help me understand.
>>
>> This driver implemented as virtual driver, because:
>> With the help of custom kernel modules, musdk library already provides
>> userspace datapath support. This PMD is an interface to musdk library.
>> Is this correct?
> That is right. Another reason this NIC is not PCI device.

We support more bus now :). Out of curiosity, which bus is device on?

>>
>> If so, just thinking loud:
>> - Why not implement this PMD directly on top of kernel interface,
>> removing musdk layer completely?
>> - How big problem that this PMD depends on custom kernel code?
> I think the main reason is that MUSDK is already used in different projects.
> Keeping multiple codebases offering similar functionality would be quite
> demanding in terms of extra work needed.
>> - How library and custom kernel code delivered? For which platforms?
> Kernel and library sources are hosted on publicly available repository.

I guess it would be nice to highlight custom kernel with external
patches is required. This is not mentioned in "Prerequisites" section of
the document.

> Driver was tested on Armada 7k/8k SoCs.

Can you please provide link to the HW mentioned in documentation?

>>
>> <....>
>>
> 
> --
> - Tomasz Duszyński
>