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

[Yankejian (Hackim Yim)]

On 2016/3/3 21:39, Andy Shevchenko wrote:
> 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;

Hi, Andy,
Thanks for your suggestion. I think it should be return ret. the other case will be return the value from hardware.
it can tell callers if the operation is successful or not.

Best Regards

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

Thanks. I will fix it in next submit

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

Thanks. I will fix it in next submit

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

to keep the unifying style, i will fix it in next submit.

>> +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() ?

ok, thanks.i will fix it in next submit.



>> +		skb->data[5] += 0x1f;
> This has to be explained.
>
>> +	}
>> +
>> +	frame_size &= ~1ul;
> And how 1ul is different to plain 1 here?

the bit width must be 32bit

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

the caller, who generates the frame, ensure that it will not be overwritten. it is only one caller in the system.

>> +}
>> +