Re: [dpdk-dev] [PATCH v2 00/56] net: txgbe PMD

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/10/2020 10:45 AM, Jiawen Wu wrote:
> On 10/9/2020 5:47 PM, Ferruh Yigit wrote:
>> On 10/9/2020 4:03 AM, jiawenwu@trustnetic.com wrote:
>>> Hi Ferruh,
>>>
>>> For the syntax/style check issue, should I fix all the errors and warnings or
>> just fix the errors?
>>> It seems to be a lot of warnings.
>>>
>>
>> [Please don't top post, it makes archives un-readable]
>>
>> Please fix all, but beware that there may be false positive in the checkpatch
>> warnings, so you need to process the output first.
>> This is a new PMD, if the syntax is not put correct at first place, very unlikely
>> that it will be fixed later, so lets try to fix them as much as possible.
>>
>> For some drivers, the base code is shared in multiple platforms, like Linux,
>> FreeBSD, Windows etc..., for them we are more flexible and we allow to keep
>> the original syntax of that shared code, *as long as it is consistent within itself*.
>> Do you have similar case in the base folder files?
>>
>> The code for the DPDK should follow the DPDK coding convention [1] and should
>> have as less checkpatch warnings/errors as possible.
>>
>> [1] https://doc.dpdk.org/guides/contributing/coding_style.html
>>
>> Thanks,
>> ferruh
>>
>>
> 
> There are some 'checks' show that macro argument reused will possible take side-effects.
> Like this:
> 
> CHECK:MACRO_ARG_REUSE: Macro argument reuse 'y' - possible side-effects?
> #56: FILE: drivers/net/txgbe/base/txgbe_regs.h:35:
> +#define ROUND_UP(x, y)          (((x) + (y) - 1) / (y) * (y))
> 
> But the example given in the DPDK coding convention is:
> 
> #define MACRO(x, y) do {      \
> 		variable = (x) + (y);   \
> 		(y) += 2;            \
> } while(0)
> 
> It seems to reuse argument, too.
> Should I fix this 'check', or treat it as a false positive?
> 

That is a check, please double check if the usage is safe, and if it is you can 
ignore it.


> 
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Sent: Tuesday, October 6, 2020 7:03 PM
>>> To: Jiawen Wu <jiawenwu@trustnetic.com>; dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH v2 00/56] net: txgbe PMD
>>>
>>> On 10/5/2020 1:08 PM, Jiawen Wu wrote:
>>>> v2: re-order patches and fix some known problems
>>>> v1: introduce txgbe PMD
>>>>
>>>> jiawenwu (56):
>>>>      net/txgbe: add build and doc infrastructure
>>>>      net/txgbe: add ethdev probe and remove
>>>>      net/txgbe: add device init and uninit
>>>>      net/txgbe: add error types and registers
>>>>      net/txgbe: add mac type and bus lan id
>>>>      net/txgbe: add HW infrastructure and dummy function
>>>>      net/txgbe: add EEPROM functions
>>>>      net/txgbe: add HW init and reset operation
>>>>      net/txgbe: add PHY init
>>>>      net/txgbe: add module identify
>>>>      net/txgbe: add PHY reset
>>>>      net/txgbe: add info get operation
>>>>      net/txgbe: add interrupt operation
>>>>      net/txgbe: add device configure operation
>>>>      net/txgbe: add link status change
>>>>      net/txgbe: add multi-speed link setup
>>>>      net/txgbe: add autoc read and write
>>>>      net/txgbe: add MAC address operations
>>>>      net/txgbe: add unicast hash bitmap
>>>>      net/txgbe: add RX and TX init
>>>>      net/txgbe: add RX and TX queues setup and release
>>>>      net/txgbe: add RX and TX start and stop
>>>>      net/txgbe: add packet type
>>>>      net/txgbe: fill simple transmit function
>>>>      net/txgbe: fill transmit function with hardware offload
>>>>      net/txgbe: fill TX prepare funtion
>>>>      net/txgbe: fill receive functions
>>>>      net/txgbe: add device start operation
>>>>      net/txgbe: add RX and TX data path start and stop
>>>>      net/txgbe: add device stop and close operations
>>>>      net/txgbe: support RX interrupt
>>>>      net/txgbe: add RX and TX queue info get
>>>>      net/txgbe: add device stats get
>>>>      net/txgbe: add device xstats get
>>>>      net/txgbe: add queue stats mapping
>>>>      net/txgbe: add VLAN handle support
>>>>      net/txgbe: add SWFW semaphore and lock
>>>>      net/txgbe: add PF module init and uninit for SRIOV
>>>>      net/txgbe: add process mailbox operation
>>>>      net/txgbe: add PF module configure for SRIOV
>>>>      net/txgbe: add VMDq configure
>>>>      net/txgbe: add RSS support
>>>>      net/txgbe: add DCB support
>>>>      net/txgbe: add flow control support
>>>>      net/txgbe: add FC auto negotiation support
>>>>      net/txgbe: add priority flow control support
>>>>      net/txgbe: add device promiscuous and allmulticast mode
>>>>      net/txgbe: add MTU set operation
>>>>      net/txgbe: add FW version get operation
>>>>      net/txgbe: add EEPROM info get operation
>>>>      net/txgbe: add register dump support
>>>>      net/txgbe: support device LED on and off
>>>>      net/txgbe: add mirror rule operations
>>>>      net/txgbe: add PTP support
>>>>      net/txgbe: add DCB info get operation
>>>>      net/txgbe: add Rx and Tx descriptor status
>>>>
>>>
>>> Hi Jiawen,
>>>
>>> Before going into more detailed reviews, the patchset conflicts with some
>> recent changes in the main repo [1], can you please rebase on top of the latest
>> head of the repo?
>>>
>>> Also DPDK syntax/style check scripts are giving errors, can you please fix
>> them too? You should run following to check:
>>> ./devtools/checkpatches.sh -n56
>>> ./devtools/check-git-log.sh -n56
>>> (This one needs codespell package to show spelling errors)
>>>
>>>
>>>
>>> [1] mainly the list is:
>>> 1) PMD close behavior change,
>>>       - .dev_close changes
>>>       - RTE_ETH_DEV_CLOSE_REMOVE flag removed
>>>
>>> 2) Some dev_ops moved to ethdev struct
>>>       - .rx_queue_count
>>>       - .rx_descriptor_done
>>>       - .rx_descriptor_status
>>>       - .tx_descriptor_status
>>>
>>>
>>>
>>>
> 
> 
> 
>