Re: [PATCH net-next 3/7] net: hns3: add support for forwarding packet to queues of specified TC when flow director rule hit

On Thu, 2020-12-10 at 20:24 +0800, tanhuazhong wrote:

On 2020/12/10 13:40, Saeed Mahameed wrote:

quoted

On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:

quoted

From: Jian Shen <shenjian15@huawei.com>

For some new device, it supports forwarding packet to queues
of specified TC when flow director rule hit. So extend the
command handle to support it.

...

quoted

  static int hclge_config_action(struct hclge_dev *hdev, u8
stage,
  			       struct hclge_fd_rule *rule)
  {
+	struct hclge_vport *vport = hdev->vport;
+	struct hnae3_knic_private_info *kinfo = &vport->nic.kinfo;
  	struct hclge_fd_ad_data ad_data;
  
+	memset(&ad_data, 0, sizeof(struct hclge_fd_ad_data));
  	ad_data.ad_id = rule->location;
  
  	if (rule->action == HCLGE_FD_ACTION_DROP_PACKET) {
  		ad_data.drop_packet = true;
-		ad_data.forward_to_direct_queue = false;
-		ad_data.queue_id = 0;
+	} else if (rule->action == HCLGE_FD_ACTION_SELECT_TC) {
+		ad_data.override_tc = true;
+		ad_data.queue_id =
+			kinfo->tc_info.tqp_offset[rule->tc];
+		ad_data.tc_size =
+			ilog2(kinfo->tc_info.tqp_count[rule->tc]);

In the previous patch you copied this info from mqprio, which is an
egress qdisc feature, this patch is clearly about rx flow director,
I
think the patch is missing some context otherwise it doesn't make
any
sense.

Since tx and rx are in the same tqp, what we do here is to make tx
and 
rx in the same tc when rule is hit.

this needs more clarification, even if tx and rx are the same hw
object, AFAIK there is not correlation between mqprio and tc rx
classifiers. 

quoted

quoted

  	} else {
-		ad_data.drop_packet = false;
  		ad_data.forward_to_direct_queue = true;
  		ad_data.queue_id = rule->queue_id;
  	}

@@ -5937,7 +5950,7 @@ static int hclge_add_fd_entry(struct

hnae3_handle *handle,
  			return -EINVAL;
  		}
  
-		action = HCLGE_FD_ACTION_ACCEPT_PACKET;
+		action = HCLGE_FD_ACTION_SELECT_QUEUE;
  		q_index = ring;
  	}
  
diff --git
a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index b3c1301..a481064 100644

--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h

@@ -572,8 +572,9 @@ enum HCLGE_FD_PACKET_TYPE {
  };
  
  enum HCLGE_FD_ACTION {
-	HCLGE_FD_ACTION_ACCEPT_PACKET,
+	HCLGE_FD_ACTION_SELECT_QUEUE,
  	HCLGE_FD_ACTION_DROP_PACKET,
+	HCLGE_FD_ACTION_SELECT_TC,

what is SELECT_TC ? you never actually write this value
anywhere  in
this patch.

HCLGE_FD_ACTION_SELECT_TC means that the packet will be forwarded
into 
the queue of specified TC when rule is hit.

what is "specified TC" in this context ?

Are we talking about ethtool nfc steering here ? because clearly this
was the purpose of HCLGE_FD_ACTION_ACCEPT_PACKET before it got removed.

the assignment is in the next patch, maybe these two patch should be 
merged for making it more readable.


Thanks.
Huazhong.

quoted

.