Hi Thomas > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Saturday, August 30, 2014 10:43 PM > To: Zhang, Helin > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/3] i40evf: support > I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in DPDK PF host > > Hi Helin, > > The title mention i40evf but the patch is related to PF (for VF > communication). > So I wonder wether it would be clearer to prefix it with i40e? Not sure. > > 2014-08-20 11:33, Helin Zhang: > > To configure VSI queues for VF, Linux PF host supports operation of > > I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES with limited configurations. To > > support more configurations (e.g configurable CRC stripping in VF), a > > new operation of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX has been > > supported in DPDK PF host. > > This patch would be easier to read if you could split it in 3 patches: > 1) renamings and line wrapping rework > 2) introduction of new extended message/operation > 3) crc config > > > - int ret = I40E_SUCCESS; > > - struct i40e_virtchnl_vsi_queue_config_info *qconfig = > > - (struct i40e_virtchnl_vsi_queue_config_info *)msg; > > - int i; > > - struct i40e_virtchnl_queue_pair_info *qpair; > > - > > - if (msg == NULL || msglen <= sizeof(*qconfig) || > > - qconfig->num_queue_pairs > vsi->nb_qps) { > > + struct i40e_virtchnl_vsi_queue_config_info *vc_vqci = > > + (struct i40e_virtchnl_vsi_queue_config_info *)msg; > > + struct i40e_virtchnl_queue_pair_info *vc_qpi; > > + struct i40e_virtchnl_queue_pair_extra_info *vc_qpei = NULL; > > + int i, ret = I40E_SUCCESS; > > + > > + if (msg == NULL || msglen <= sizeof(*vc_vqci) || > > + vc_vqci->num_queue_pairs > vsi->nb_qps) { > > PMD_DRV_LOG(ERR, "vsi_queue_config_info argument wrong\n"); > > ret = I40E_ERR_PARAM; > > goto send_msg; > > } > > > > - qpair = qconfig->qpair; > > - for (i = 0; i < qconfig->num_queue_pairs; i++) { > > - if (qpair[i].rxq.queue_id > vsi->nb_qps - 1 || > > - qpair[i].txq.queue_id > vsi->nb_qps - 1) { > > + vc_qpi = vc_vqci->qpair; > > + if (opcode == I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX) > > + vc_qpei = (struct i40e_virtchnl_queue_pair_extra_info *) > > + (((uint8_t *)vc_qpi) + > > + (sizeof(struct i40e_virtchnl_queue_pair_info) * > > + vc_vqci->num_queue_pairs)); > > + > > + for (i = 0; i < vc_vqci->num_queue_pairs; i++) { > > + if (vc_qpi[i].rxq.queue_id > vsi->nb_qps - 1 || > > + vc_qpi[i].txq.queue_id > vsi->nb_qps - 1) { > > ret = I40E_ERR_PARAM; > > goto send_msg; > > } > > Mixing renaming and new feature makes it difficult to read. > > > case I40E_VIRTCHNL_OP_ENABLE_QUEUES: > > PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received\n"); > > - i40e_pf_host_process_cmd_enable_queues(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_DISABLE_QUEUES: > > PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received\n"); > > - i40e_pf_host_process_cmd_disable_queues(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_ADD_ETHER_ADDRESS: > > PMD_DRV_LOG(INFO, "OP_ADD_ETHER_ADDRESS received\n"); > > - i40e_pf_host_process_cmd_add_ether_address(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_add_ether_address(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_DEL_ETHER_ADDRESS: > > PMD_DRV_LOG(INFO, "OP_DEL_ETHER_ADDRESS received\n"); > > - i40e_pf_host_process_cmd_del_ether_address(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_del_ether_address(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_ADD_VLAN: > > PMD_DRV_LOG(INFO, "OP_ADD_VLAN received\n"); @@ -932,10 > +951,9 @@ > > i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev, > > case I40E_VIRTCHNL_OP_FCOE: > > PMD_DRV_LOG(ERR, "OP_FCOE received, not supported\n"); > > default: > > - PMD_DRV_LOG(ERR, "%u received, not supported\n", > > - opcode); > > - i40e_pf_host_send_msg_to_vf(vf, opcode, > > - I40E_ERR_PARAM, NULL, 0); > > + PMD_DRV_LOG(ERR, "%u received, not supported\n", opcode); > > + i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM, > > + NULL, 0); > > break; > > } > > Line wrapping should go in a cleanup patch (like renaming). > > Thanks > -- > Thomas
Thank you very much! I have reworked the patches and sent out again. Regards, Helin
