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

[Ferruh Yigit]

On 10/5/2017 9:43 AM, Tomasz Duszynski wrote:
> On Wed, Oct 04, 2017 at 05:59:11PM +0100, Ferruh Yigit wrote:
>> 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?
>>
> 
> Fair enough. I'll split the driver as suggested. A few specific
> questions about functionality each patch should contain though.
> 
> As for skeleton, I see others just put driver probing here.
> 
> As for Rx/Tx support it seems that there's no common pattern.
> Functionality like starting/stopping device, queues configuration
> and all the other things related to Rx/Tx should be here as well?

As you said there is no common pattern, but I think starting/stopping
device, queues configuration can go into skeleton and mainly Rx/Tx burst
functions can go into Rx/Tx patch.
But please what you think more reasonable matters here.

> 
> What's left are features which go into features-patch.

Yes.
And the .ini file, currently part of doc patch, can be part of this
features patch, it is helps more to see the code add feature and doc
documents it in same patch.

> 
>>>>
>>>>>
>>>>> 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?
> 
> Bus is called Aurora2. That's proprietary SoC interconnect fabric.
> 
>>
>>>>
>>>> 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.
>>
> 
> ACK
> 
>>> Driver was tested on Armada 7k/8k SoCs.
>>
>> Can you please provide link to the HW mentioned in documentation?
>>
> 
> You can find some info here:
> 
> https://www.marvell.com/embedded-processors/armada-70xx/
> https://www.marvell.com/embedded-processors/armada-80xx/

Thanks, would you mind putting these links into driver documentation as
well?

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