Re: [PATCH v2] doc: add new driver guidelines

[fengchengwen <fengchengwen@huawei.com>]

Hi Ferruh,

Thanks for the explanation.

In the next new version:
Acked-by: Chengwen Feng <fengchengwen@huawei.com>

Thanks

On 2024/9/6 16:27, Ferruh Yigit wrote:
> On 9/6/2024 9:05 AM, fengchengwen wrote:
>> On 2024/8/15 3:08, Stephen Hemminger wrote:
>>> From: Nandini Persad <nandinipersad361@gmail.com>
>>>
>>> This document was created to assist contributors in creating DPDK drivers
>>> and provides suggestions and guidelines on how to upstream effectively.
>>>
>>> Co-authored-by: Ferruh Yigit <ferruh.yigit@amd.com>
>>> Co-authored-by: Thomas Monjalon <thomas@monjalon.net>
>>> Signed-off-by: Nandini Persad <nandinipersad361@gmail.com>
>>> Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>>
>>> v2 - review feedback
>>>    - add co-author and reviewed-by
>>>
>>>  doc/guides/contributing/index.rst      |   1 +
>>>  doc/guides/contributing/new_driver.rst | 202 +++++++++++++++++++++++++
>>>  2 files changed, 203 insertions(+)
>>>  create mode 100644 doc/guides/contributing/new_driver.rst
>>>
>>
>> ...
>>
>>> +
>>> +Finalizing
>>> +----------
>>> +
>>> +Once the driver has been upstreamed, the author has
>>> +a responsibility to the community to maintain it.
>>> +
>>> +This includes the public test report. Authors must send a public
>>> +test report after the first upstreaming of the PMD. The same
>>> +public test procedure may be reproduced regularly per release.
>>> +
>>> +After the PMD is upstreamed, the author should send a patch
>>> +to update the website with the name of the new PMD and supported devices
>>> +via the DPDK mailing list..
>>
>> .. -> .
>>
>>> +
>>> +For more information about the role of maintainers, see :doc:`patches`.
>>> +
>>> +
>>> +
>>> +Splitting into Patches
>>> +----------------------
>>> +
>>
>> ...
>>
>>> +
>>> +
>>> +The following order in the patch series is as suggested below.
>>> +
>>> +The first patch should have the driver's skeleton which should include:
>>> +
>>> +* Maintainer's file update
>>> +* Driver documentation
>>> +* Document must have links to official product documentation web page
>>> +* The  new document should be added into the index (`doc/guides/index.rst`)
>>
>> The  new -> The new
>>
>> ...
>>
>>> +
>>> +Additional Suggestions
>>> +----------------------
>>> +
>>> +* We recommend using DPDK macros instead of inventing new ones in the PMD.
>>> +* Do not include unused headers. Use the ./devtools/process-iwyu.py tool.
>>> +* Do not disable compiler warnings in the build file.
>>> +* Do not use #ifdef with driver-defined macros, instead prefer runtime configuration.
>>> +* Document device parameters in the driver guide.
>>> +* Make device operations struct 'const'.
>>> +* Use dynamic logging.
>>> +* Do not use DPDK version checks in the upstream code.
>>
>> Could you explain it (DPDK version check) ?
>>
> 
> It refers usage of 'RTE_VERSION_NUM' macro. This may be required for out
> of tree drivers, as they may be supporting multiple DPDK version.
> 
> Not sure adding too much details for sure, what about following update:
> `* Do not use DPDK version checks (via RTE_VERSION_NUM) in the upstream
> code.`

Got it, thanks

> 
> 
>>> +* Be sure to have SPDX license tags and copyright notice on each side.
>>> +  Use ./devtools/check-spdx-tag.sh
>>> +* Run the Coccinelle scripts ./devtools/cocci.sh which check for common cleanups such as
>>> +  useless null checks before calling free routines.
>>> +
>>> +Dependencies
>>> +------------
>>> +
>>> +At times, drivers may have dependencies to external software.
>>> +For driver dependencies, same DPDK rules for dependencies applies.
>>> +Dependencies should be publicly and freely available,
>>> +or this is a blocker for upstreaming the driver.
>>
>> Could you explain it (what's the blocker) ?
>>
> 
> It is trying to say, this prevents upstreaming, wording can be updated
> to clarify, what about following:
> 
> `Dependencies should be publicly and freely available to be able to
> upstream the driver.`

+1

> 
> 
>>> +
>>> +
>>> +.. _tool_list:
>>> +
>>> +Test Tools
>>> +----------
>>> +
>>> +Build and check the driver's documentation. Make sure there are no
>>> +warnings and driver shows up in the relevant index page.
>>> +
>>> +Be sure to run the following test tools per patch in a patch series:
>>> +
>>> +* checkpatches.sh
>>> +* check-git-log.sh
>>> +* check-meson.py
>>> +* check-doc-vs-code.sh
>>>
>>
>> Some drivers already provide private APIs, I think we should add note
>> for "not add private APIs, prefer to extend the corresponding framework API" for new drivers.
>>
> 
> Ack.
> What about adding this to "Additional Suggestions", like following:
> `Do not introduce public APIs directly from the driver.`

+1

> 
> .
>