[dpdk-dev] [PATCH 08/12] pmd/mlx4: add dev_ptype_info_get implementation

[Adrien Mazarguil]

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