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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

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?


> > -----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
> >
> >
> >
> >