Re: [PATCH net] net: hns: fix the bug about loopback

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, 2016-03-03 at 20:02 +0800, Kejian Yan wrote:
> It will always be passed if the soc is tested the loopback cases.
> This
> patch will fix this bug.

Few style related comments.

> @@ -686,6 +690,10 @@ static int hns_ae_config_loopback(struct
> hnae_handle *handle,
>  	default:
>  		ret = -EINVAL;
>  	}
> +
> +	if (!ret)
> +		hns_dsaf_set_inner_lb(mac_cb->dsaf_dev, mac_cb-
> >mac_id, en);
> +
>  	return ret;

if (ret)
 return ret;

whatever();
return 0;

>  }

--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> @@ -230,6 +230,30 @@ static void hns_dsaf_mix_def_qid_cfg(struct
> dsaf_device *dsaf_dev)
>  	}
>  }
>  
> +static void hns_dsaf_inner_qid_cfg(struct dsaf_device *dsaf_dev)
> +{
> +	u16 max_q_per_vf, max_vfn;
> +	u32 q_id = 0, q_num_per_port;
> +	u32 mac_id;
> +
> +	if (AE_IS_VER1(dsaf_dev->dsaf_ver))
> +		return;
> +
> +	hns_rcb_get_queue_mode(dsaf_dev->dsaf_mode,
> +			       HNS_DSAF_COMM_SERVICE_NW_IDX,
> +			       &max_vfn, &max_q_per_vf);
> +	q_num_per_port = max_vfn * max_q_per_vf;
> +
> +	for (mac_id = 0, q_id = 0; mac_id < DSAF_SERVICE_NW_NUM; 

q_id is already assigned to 0. Get rid of either.

> mac_id++) {
> +		dsaf_set_dev_field(dsaf_dev,
> +				   DSAFV2_SERDES_LBK_0_REG + 0x4 *
> mac_id,
> +				   DSAFV2_SERDES_LBK_QID_M,
> +				   DSAFV2_SERDES_LBK_QID_S,
> +				   q_id);
> +		q_id += q_num_per_port;
> +	}
> +}

 
> +void hns_dsaf_set_inner_lb(struct dsaf_device *dsaf_dev, u32 mac_id,
> u32 en)
> +{
> +	if (AE_IS_VER1(dsaf_dev->dsaf_ver) ||
> +	    dsaf_dev->mac_cb[mac_id].mac_type == HNAE_PORT_DEBUG)
> +		return;
> +
> +	dsaf_set_dev_bit(dsaf_dev, DSAFV2_SERDES_LBK_0_REG + 0x4 *
> mac_id,

0x4 -> 4 (it's about register width, right?)

> +			 DSAFV2_SERDES_LBK_EN_B, !!en);
> +}

>  #define PPEV2_CFG_RSS_TBL_4N3_S	24
>  #define PPEV2_CFG_RSS_TBL_4N3_M	(((1UL << 5) - 1) <<
> PPEV2_CFG_RSS_TBL_4N3_S)
>  
> +#define DSAFV2_SERDES_LBK_EN_B  8
> +#define DSAFV2_SERDES_LBK_QID_S 0
> +#define DSAFV2_SERDES_LBK_QID_M \
> +	(((1UL << DSAFV2_SERDES_LBK_EN_B) - 1) <<
> DSAFV2_SERDES_LBK_QID_S)

Why not like above?

#define DSAFV2_SERDES_LBK_QID_M	(((1UL << 8) - 1)
<< DSAFV2_SERDES_LBK_QID_S)

> +static void __lb_fill_txt_skb_buff(struct net_device *ndev,
> +				   struct sk_buff *skb)
> +{
> +	struct hns_nic_priv *priv = netdev_priv(ndev);
> +	struct hnae_handle *h = priv->ae_handle;
> +	u32 frame_size;
> +
> +	frame_size = skb->len;
> +	memset(skb->data, 0xFF, frame_size);
> +
> +	if ((!AE_IS_VER1(priv->enet_ver)) &&
> +	    (h->port_type == HNAE_PORT_SERVICE)) {
> +		memcpy(skb->data, ndev->dev_addr, 6);

ether_addr_copy() ?

> +		skb->data[5] += 0x1f;

This has to be explained.

> +	}
> +
> +	frame_size &= ~1ul;

And how 1ul is different to plain 1 here?

> +	memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 -
> 1);
> +	memset(&skb->data[frame_size / 2 + 10], 0xBE, frame_size / 2
> - 11);
> +	memset(&skb->data[frame_size / 2 + 12], 0xAF, frame_size / 2
> - 13);

Magic numbers have to be explained.
Also, what is the logic to overwrite the data if frame_size is big
enough?

> +}
> +

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy