On Tue, Jan 05, 2016 at 11:08:04AM +0800, Tan, Jianfeng wrote:
>
>
> On 1/4/2016 7:11 PM, Adrien Mazarguil wrote:
> >Hi Jianfeng,
> >
> >I'm only commenting the mlx4/mlx5 bits in this message, see below.
> >
> >On Thu, Dec 31, 2015 at 02:53:15PM +0800, Jianfeng Tan wrote:
> >>Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
> >>---
> >> drivers/net/mlx4/mlx4.c | 27 +++++++++++++++++++++++++++
> >> 1 file changed, 27 insertions(+)
> >>
> >>diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> >>index 207bfe2..85afa32 100644
> >>--- a/drivers/net/mlx4/mlx4.c
> >>+++ b/drivers/net/mlx4/mlx4.c
> >>@@ -2836,6 +2836,8 @@ rxq_cleanup(struct rxq *rxq)
> >> * @param flags
> >> * RX completion flags returned by poll_length_flags().
> >> *
> >>+ * @note: fix mlx4_dev_ptype_info_get() if any change here.
> >>+ *
> >> * @return
> >> * Packet type for struct rte_mbuf.
> >> */
> >>@@ -4268,6 +4270,30 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct
> >>rte_eth_dev_info *info)
> >> priv_unlock(priv);
> >> }
> >>+static int
> >>+mlx4_dev_ptype_info_get(struct rte_eth_dev *dev, uint32_t ptype_mask,
> >>+ uint32_t ptypes[])
Note this line is not properly indented (uint32_t should be aligned like the
rest of the file).
> >>+{
> >>+ int num = 0;
> >>+
> >>+ if ((dev->rx_pkt_burst == mlx4_rx_burst)
> >>+ || (dev->rx_pkt_burst == mlx4_rx_burst_sp)) {
I prefer operators/separators at the end of the previous line, indentation
should be fixed as well.
> >>+ /* refers to rxq_cq_to_pkt_type() */
> >>+ if ((ptype_mask & RTE_PTYPE_L3_MASK) == RTE_PTYPE_L3_MASK) {
> >>+ ptypes[num++] = RTE_PTYPE_L3_IPV4;
> >>+ ptypes[num++] = RTE_PTYPE_L3_IPV6;
> >>+ }
> >>+
> >>+ if ((ptype_mask & RTE_PTYPE_INNER_L3_MASK) ==
> >>RTE_PTYPE_INNER_L3_MASK) {
> >>+ ptypes[num++] = RTE_PTYPE_INNER_L3_IPV4;
> >>+ ptypes[num++] = RTE_PTYPE_INNER_L3_IPV6;
> >>+ }
> >>+ } else
> >>+ num = -ENOTSUP;
> >>+
> >>+ return num;
> >>+}
> >I think checking for mlx4_rx_burst and mlx4_rx_burst_sp is unnecessary at
> >the moment, all RX burst functions do update the packet_type field, no need
> >for extra complexity.
> >
> >Same comment for mlx5.
>
> Hi Mazarguil,
>
> My original thought is that rx_pkt_burst could be also set as
> removed_rx_burst, which does not make sense indeed
> because it's only possible when the device is closed.
Yes, indeed.
> Another consideration is to keep same style with other devices. Each
> kind of device could have several rx burst functions.
> So current implementation can keep extensibility to add new rx burst
> functions. How do you think of it?
OK, that makes sense. Please check my above comments about coding
style/indents (I know I'm annoying).
> >>+
> >> /**
> >> * DPDK callback to get device statistics.
> >> *
> >>@@ -4989,6 +5015,7 @@ static const struct eth_dev_ops mlx4_dev_ops = {
> >> .stats_reset = mlx4_stats_reset,
> >> .queue_stats_mapping_set = NULL,
> >> .dev_infos_get = mlx4_dev_infos_get,
> >>+ .dev_ptypes_info_get = mlx4_dev_ptype_info_get,
> >> .vlan_filter_set = mlx4_vlan_filter_set,
> >> .vlan_tpid_set = NULL,
> >> .vlan_strip_queue_set = NULL,
> >>--
> >>2.1.4
> >>
>
--
Adrien Mazarguil
6WIND
