Re: [PATCH net-next 3/7] net: hns3: add support for forwarding packet to queues of specified TC when flow director rule hit
On 2020/12/11 4:46, Saeed Mahameed wrote:
On Thu, 2020-12-10 at 20:24 +0800, tanhuazhong wrote:
On 2020/12/10 13:40, Saeed Mahameed wrote:
On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
From: Jian Shen
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.
...
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 = >nic.kinfo;
struct hclge_fd_ad_data ad_data;
+ memset(_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.
Could comment below make the code more readable?
"Since tx and rx are in the same tqp, if hit rule, forward the packet to
the rx queues pair with specified TC"
} 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 ?
TC specified by 'tc flower'
Are we talking about ethtool nfc steering here ? because clearly this
was the purpose of HCLGE_FD_ACTION_ACCEPT_PACKET before it got removed.
In fact, HCLGE_FD_ACTION_ACCEPT_PACKET is splitted in to
HCLGE_FD_ACTION_SELECT_QUEUE and HCLGE_FD_ACTION_SELECT_TC.
HCLGE_FD_ACTION_SELECT_QUEUE is configured by 'ethtool -U' (nfc
steering) or aRFS.
HCLGE_FD_ACTION_SELECT_TC is configured by 'tc flower' so far.
the assignment is in the next patch, maybe these two patch should be
merged for making it more readable.
Thanks.
Huazhong.
.
.
Re: [PATCH net-next 2/7] net: hns3: add support for tc mqprio offload
On 2020/12/11 4:24, Saeed Mahameed wrote:
On Thu, 2020-12-10 at 20:27 +0800, tanhuazhong wrote:
On 2020/12/10 12:50, Saeed Mahameed wrote:
On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
From: Jian Shen
Currently, the HNS3 driver only supports offload for tc number
and prio_tc. This patch adds support for other qopts, including
queues count and offset for each tc.
When enable tc mqprio offload, it's not allowed to change
queue numbers by ethtool. For hardware limitation, the queue
number of each tc should be power of 2.
For the queues is not assigned to each tc by average, so it's
should return vport->alloc_tqps for hclge_get_max_channels().
The commit message needs some improvements, it is not really clear
what
the last two sentences are about.
The hclge_get_max_channels() returns the max queue number of each TC
for
user can set by command ethool -L. In previous implement, the queues
are
assigned to each TC by average, so we return it by vport-:
alloc_tqps / num_tc. And now we can assign differrent queue number
for
each TC, so it shouldn't be divided by num_tc.
What do you mean by "queues assigned to each tc by average" ?
In previous implement the number of queue in each TC is same, now, we
allow that the number of queue in each TC is different.
[...]
+ }
+ if (hdev->vport[0].alloc_tqps < queue_sum) {
can't you just allocate new tqps according to the new mqprio input
like
other drivers do ? how the user allocates those tqps ?
maybe the name of 'alloc_tqps' is a little bit misleading, the
meaning
of this field is the total number of the available tqps in this
vport.
from your driver code it seems alloc_tqps is number of rings allocated
via ethool -L.
My point is, it seems like in this patch you demand for the queues to
be preallocated, but what other drivers do on setup tc, they just
duplicate what ever number of queues was configured prior to setup tc,
num_tc times.
The maximum number of queues is defined by 'alloc_tqps', not
preallocated queues. The behavior on setup tc of HNS3 is same as other
driver.
'alloc_tqps' in HNS3 has the same means as 'num_queue_pairs' in i40e below
if (vsi->num_queue_pairs <
(mqprio_qopt->qopt.offset[i] + mqprio_qopt->qopt.count[i])) {
return -EINVAL;
}
+ dev_err(>pdev->dev,
+ "qopt queue count sum should be less than
%u\n",
+ hdev->vport[0].alloc_tqps);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void hclge_sync_mqprio_qopt(struct hnae3_tc_info
*tc_info,
+ struct tc_mqprio_qopt_offload
*mqprio_qopt)
+{
+ int i;
+
+ memset(tc_info, 0, sizeof(*tc_info));
+ tc_info->num_tc = mqprio_qopt->qopt.num_tc;
+ memcpy(tc_info->prio_tc, mqprio_qopt->qopt.prio_tc_map,
+ sizeof_field(struct hnae3_tc_info, prio_tc));
+ memcpy(tc_info->tqp_count, mqprio_qopt->qopt.count,
+ sizeof_field(struct hnae3_tc_info, tqp_count));
+ memcpy(tc_info->tqp_offset, mqprio_qopt->qopt.offset,
+ sizeof_field(struct hnae3_tc_info, tqp_offset));
+
isn't it much easier to just store a copy of tc_mqprio_qopt in you
tc_info and then just:
tc_info->qopt = mqprio->qopt;
[...]
The tc_mqprio_qopt_offload still contains a lot of opt hns3 driver
does
not use yet, even if the hns3 use all the opt, I still think it is
better to create our own struct, if struct tc_mqprio_qopt_offload
changes in the future, we can limit the change to the
tc_mqprio_qopt_offload convertion.
ok.
Thanks.
Huazhong.
.
Re: [PATCH net-next 2/7] net: hns3: add support for tc mqprio offload
On 2020/12/10 12:50, Saeed Mahameed wrote:
On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
From: Jian Shen
Currently, the HNS3 driver only supports offload for tc number
and prio_tc. This patch adds support for other qopts, including
queues count and offset for each tc.
When enable tc mqprio offload, it's not allowed to change
queue numbers by ethtool. For hardware limitation, the queue
number of each tc should be power of 2.
For the queues is not assigned to each tc by average, so it's
should return vport->alloc_tqps for hclge_get_max_channels().
The commit message needs some improvements, it is not really clear what
the last two sentences are about.
The hclge_get_max_channels() returns the max queue number of each TC for
user can set by command ethool -L. In previous implement, the queues are
assigned to each TC by average, so we return it by vport-:
alloc_tqps / num_tc. And now we can assign differrent queue number for
each TC, so it shouldn't be divided by num_tc.
Signed-off-by: Jian Shen
Signed-off-by: Huazhong Tan
---
...
+ if (kinfo->tc_info.mqprio_active) {
+ dev_err(>dev,
why not use netdev_err() and friends ?
anyway I see your driver is using dev_err(>dev, ...)
intensively,
maybe submit a follow up patch to fix all your prints ?
yes, will fix it with another patch.
...]
+static int hclge_mqprio_qopt_check(struct hclge_dev *hdev,
+ struct tc_mqprio_qopt_offload
*mqprio_qopt)
+{
+ u16 queue_sum = 0;
+ int ret;
+ int i;
+
+ if (!mqprio_qopt->qopt.num_tc) {
+ mqprio_qopt->qopt.num_tc = 1;
+ return 0;
+ }
+
+ ret = hclge_dcb_common_validate(hdev, mqprio_qopt->qopt.num_tc,
+ mqprio_qopt->qopt.prio_tc_map);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < mqprio_qopt->qopt.num_tc; i++) {
+ if (!is_power_of_2(mqprio_qopt->qopt.count[i])) {
+ dev_err(>pdev->dev,
+ "qopt queue count must be power of
2\n");
+ return -EINVAL;
+ }
+
+ if (mqprio_qopt->qopt.count[i] > hdev->rss_size_max) {
+ dev_err(>pdev->dev,
+ "qopt queue count should be no more
than %u\n",
+ hdev->rss_size_max);
+ return -EINVAL;
+ }
+
+ if (mqprio_qopt->qopt.offset[i] != queue_sum) {
+ dev_err(>pdev->dev,
+ "qopt queue offset must start from 0,
and being continuous\n");
+ return -EINVAL;
+ }
+
+ if (mqprio_qopt->min_rate[i] || mqprio_qopt-
max_rate[i]) {
+ dev_err(>pdev->dev,
+ "qopt tx_rate is not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ queue_sum = mqprio_qopt->qopt.offset[i];
+ queue_sum += mqprio_qopt->qopt.count[i];
it will make more sense if you moved this queue summing outside of the
loop
this queue_sum is used in this loop:
if (mqprio_qopt->qopt.offset[i] != queue_sum) {
...
+ }
+ if (hdev->vport[0].alloc_tqps < queue_sum) {
can't you just allocate new tqps according to the new mqprio input like
other drivers do ? how the user allocates those tqps ?
maybe the name of 'alloc_tqps' is a little bit misleading, the meaning
of this field is the total number of the available tqps in this vport.
+ dev_err(>pdev->dev,
+ "qopt queue count sum should be less than
%u\n",
+ hdev->vport[0].alloc_tqps);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void hclge_sync_mqprio_qopt(struct hnae3_tc_info *tc_info,
+ struct tc_mqprio_qopt_offload
*mqprio_qopt)
+{
+ int i;
+
+ memset(tc_info, 0, sizeof(*tc_info));
+ tc_info->num_tc = mqprio_qopt->qopt.num_tc;
+ memcpy(tc_info->prio_tc, mqprio_qopt->qopt.prio_tc_map,
+ sizeof_field(struct hnae3_tc_info, prio_tc));
+ memcpy(tc_info->tqp_count, mqprio_qopt->qopt.count,
+ sizeof_field(struct hnae3_tc_info, tqp_count));
+ memcpy(tc_info->tqp_offset, mqprio_qopt->qopt.offset,
+ sizeof_field(struct hnae3_tc_info, tqp_offset));
+
isn't it much easier to just store a copy of tc_mqprio_qopt in you
tc_info and then just:
tc_info->qopt = mqprio->qopt;
[...]
The tc_mqprio_qopt_offload still contains a lot of opt hns3 driver does
not use yet, even if the hns3 use all the opt, I still think it is
better to create our own struct, if struct tc_mqprio_qopt_offload
changes in the future, we can limit the change to the
tc_mqprio_qopt_offload convertion.
- hclge_tm_schd_info_update(hdev, tc);
-
Re: [PATCH net-next 3/7] net: hns3: add support for forwarding packet to queues of specified TC when flow director rule hit
On 2020/12/10 13:40, Saeed Mahameed wrote:
On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
From: Jian Shen
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.
...
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 = >nic.kinfo;
struct hclge_fd_ad_data ad_data;
+ memset(_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.
} 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.
the assignment is in the next patch, maybe these two patch should be
merged for making it more readable.
Thanks.
Huazhong.
.
Re: [PATCH net-next 3/3] net: hns3: refine the VLAN tag handle for port based VLAN
On 2020/12/5 10:22, Jakub Kicinski wrote: On Thu, 3 Dec 2020 20:18:56 +0800 Huazhong Tan wrote: tranmist Please spell check the commit messages and comments. will fix spelling mistakes, thanks .
Re: [PATCH net-next 2/3] net: hns3: add priv flags support to switch limit promisc mode
On 2020/12/5 10:24, Jakub Kicinski wrote:
On Thu, 3 Dec 2020 20:18:55 +0800 Huazhong Tan wrote:
@@ -224,6 +224,7 @@ static int hclge_map_unmap_ring_to_vf_vector(struct
hclge_vport *vport, bool en,
static int hclge_set_vf_promisc_mode(struct hclge_vport *vport,
struct hclge_mbx_vf_to_pf_cmd *req)
{
+ struct hnae3_handle *handle = >nic;
bool en_bc = req->msg.en_bc ? true : false;
bool en_uc = req->msg.en_uc ? true : false;
bool en_mc = req->msg.en_mc ? true : false;
Please order variable lines longest to shortest.
will fix it, thanks.
@@ -1154,6 +1158,8 @@ static int hclgevf_cmd_set_promisc_mode(struct
hclgevf_dev *hdev,
send_msg.en_bc = en_bc_pmc ? 1 : 0;
send_msg.en_uc = en_uc_pmc ? 1 : 0;
send_msg.en_mc = en_mc_pmc ? 1 : 0;
+ send_msg.en_limit_promisc =
+ test_bit(HNAE3_PFLAG_LIMIT_PROMISC_ENABLE, >priv_flags) ? 1 : 0;
The continuation line should be indented more than the first line.
yes, will fix it.
I suggest you rename HNAE3_PFLAG_LIMIT_PROMISC_ENABLE to
HNAE3_PFLAG_LIMIT_PROMISC, the _ENABLE doesn't add much
to the meaning. That way the lines will get shorter.
ok, thanks.
.
Re: [PATCH net-next 1/7] net: hns3: add support for RX completion checksum
On 2020/11/28 4:52, Jakub Kicinski wrote: On Fri, 27 Nov 2020 16:47:16 +0800 Huazhong Tan wrote: In some cases (for example ip fragment), hardware will calculate the checksum of whole packet in RX, and setup the HNS3_RXD_L2_CSUM_B flag in the descriptor, so add support to utilize this checksum. Signed-off-by: Huazhong Tan drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:2810:14: warning: incorrect type in assignment (different base types) drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:2810:14:expected restricted __sum16 [usertype] csum drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:2810:14:got unsigned int drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:2812:14: warning: invalid assignment: |= drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:2812:14:left side has type restricted __sum16 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:2812:14:right side has type unsigned int Will fix it, thanks. .
Re: [PATCH net-next 5/7] net: hns3: add more info to hns3_dbg_bd_info()
On 2020/11/28 4:53, Jakub Kicinski wrote: On Fri, 27 Nov 2020 16:47:20 +0800 Huazhong Tan wrote: Since TX hardware checksum and RX completion checksum have been supported now, so add related information in hns3_dbg_bd_info(). Signed-off-by: Huazhong Tan drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:266:22: warning: incorrect type in assignment (different base types) drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:266:22:expected restricted __sum16 [usertype] csum drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:266:22:got unsigned int drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:268:22: warning: invalid assignment: |= drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:268:22:left side has type restricted __sum16 drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c:268:22:right side has type unsigned int Will fix it, thanks. .
Re: [RFC V2 net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
On 2020/11/20 23:25, Andrew Lunn wrote:
@@ -310,6 +334,13 @@ int ethnl_set_coalesce(struct sk_buff *skb, struct
genl_info *info)
ret = dev->ethtool_ops->set_coalesce(dev, );
if (ret < 0)
goto out_ops;
+
+ if (ops->set_ext_coalesce) {
+ ret = ops->set_ext_coalesce(dev, _coalesce);
+ if (ret < 0)
+ goto out_ops;
+ }
+
The problem here is, if ops->set_ext_coalesce() fails, you need to
undo what dev->ethtool_ops->set_coalesce() did. From the users
perspective, this should be atomic. It does everything, or it does
nothing and returns an error code.
And that is not easy given this structure of two op calls.
Andrew
yes, i will try what Michal suggested in V1.
Regards.
Huazhong.
.
Re: [RFC net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
On 2020/11/21 5:22, Michal Kubecek wrote:
On Fri, Nov 20, 2020 at 02:39:38PM +0100, Andrew Lunn wrote:
On Fri, Nov 20, 2020 at 08:23:22AM +0100, Michal Kubecek wrote:
On Fri, Nov 20, 2020 at 10:59:59AM +0800, tanhuazhong wrote:
On 2020/11/20 6:02, Michal Kubecek wrote:
We could use a similar approach as struct ethtool_link_ksettings, e.g.
struct kernel_ethtool_coalesce {
struct ethtool_coalesce base;
/* new members which are not part of UAPI */
}
get_coalesce() and set_coalesce() would get pointer to struct
kernel_ethtool_coalesce and ioctl code would be modified to only touch
the base (legacy?) part.
> While already changing the ops arguments, we could also add extack
pointer, either as a separate argument or as struct member (I slightly
prefer the former).
If changing the ops arguments, each driver who implement
set_coalesce/get_coalesce of ethtool_ops need to be updated. Is it
acceptable adding two new ops to get/set ext_coalesce info (like
ecc31c60240b ("ethtool: Add link extended state") does)? Maybe i can send V2
in this way, and then could you help to see which one is more suitable?
If it were just this one case, adding an extra op would be perfectly
fine. But from long term point of view, we should expect extending also
other existing ethtool requests and going this way for all of them would
essentially double the number of callbacks in struct ethtool_ops.
coccinella might be useful here.
I played with spatch a bit and it with the spatch and patch below, I got
only three build failures (with allmodconfig) that would need to be
addressed manually - these drivers call the set_coalesce() callback on
device initialization.
I also tried to make the structure argument const in ->set_coalesce()
but that was more tricky as adjusting other functions that the structure
is passed to required either running the spatch three times or repeating
the same two rules three times in the spatch (or perhaps there is
a cleaner way but I'm missing relevant knowledge of coccinelle). Then
there was one more problem in i40e driver which modifies the structure
before passing it on to its helpers. It could be worked around but I'm
not sure if constifying the argument is worth these extra complications.
Michal
will implement it like this in V3.
Regards,
Huazhong.
Re: [RFC net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
On 2020/11/20 6:02, Michal Kubecek wrote:
On Thu, Nov 19, 2020 at 04:56:42PM +0800, tanhuazhong wrote:
On 2020/11/19 12:15, Andrew Lunn wrote:
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9ca87bc..afd8de2 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -433,6 +433,7 @@ struct ethtool_modinfo {
* a TX interrupt, when the packet rate is above @pkt_rate_high.
* @rate_sample_interval: How often to do adaptive coalescing packet rate
* sampling, measured in seconds. Must not be zero.
+ * @use_dim: Use DIM for IRQ coalescing, if adaptive coalescing is enabled.
*
* Each pair of (usecs, max_frames) fields specifies that interrupts
* should be coalesced until
@@ -483,6 +484,7 @@ struct ethtool_coalesce {
__u32 tx_coalesce_usecs_high;
__u32 tx_max_coalesced_frames_high;
__u32 rate_sample_interval;
+ __u32 use_dim;
};
You cannot do this.
static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
void __user *useraddr)
{
struct ethtool_coalesce coalesce;
int ret;
if (!dev->ethtool_ops->set_coalesce)
return -EOPNOTSUPP;
if (copy_from_user(, useraddr, sizeof(coalesce)))
return -EFAULT;
An old ethtool binary is not going to set this extra last byte to
anything meaningful. You cannot tell if you have an old or new user
space, so you have no idea if it put anything into use_dim, or if it
is random junk.
Even worse, as there is no indication of data length, ETHTOOL_GCOALESCE
ioctl request from old ethtool on new kernel would result in kernel
writing past the end of userspace buffer.
You have to leave the IOCTL interface unchanged, and limit this new
feature to the netlink API.
Hi, Andrew.
thanks for pointing out this problem, i will fix it.
without callling set_coalesce/set_coalesce of ethtool_ops, do you have any
suggestion for writing/reading this new attribute to/from the driver? add a
new field in net_device or a new callback function in ethtool_ops seems not
good.
We could use a similar approach as struct ethtool_link_ksettings, e.g.
struct kernel_ethtool_coalesce {
struct ethtool_coalesce base;
/* new members which are not part of UAPI */
}
get_coalesce() and set_coalesce() would get pointer to struct
kernel_ethtool_coalesce and ioctl code would be modified to only touch
the base (legacy?) part.
> While already changing the ops arguments, we could also add extack
pointer, either as a separate argument or as struct member (I slightly
prefer the former).
Hi, Michal.
If changing the ops arguments, each driver who implement
set_coalesce/get_coalesce of ethtool_ops need to be updated. Is it
acceptable adding two new ops to get/set ext_coalesce info (like
ecc31c60240b ("ethtool: Add link extended state") does)? Maybe i can
send V2 in this way, and then could you help to see which one is more
suitable?
BtW, please don't forget to update the message descriptions in
Documentation/networking/ethtool-netlink.rst
Michal
.
Re: [RFC net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
On 2020/11/20 6:02, Michal Kubecek wrote:
On Thu, Nov 19, 2020 at 04:56:42PM +0800, tanhuazhong wrote:
On 2020/11/19 12:15, Andrew Lunn wrote:
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9ca87bc..afd8de2 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -433,6 +433,7 @@ struct ethtool_modinfo {
* a TX interrupt, when the packet rate is above @pkt_rate_high.
* @rate_sample_interval: How often to do adaptive coalescing packet rate
* sampling, measured in seconds. Must not be zero.
+ * @use_dim: Use DIM for IRQ coalescing, if adaptive coalescing is enabled.
*
* Each pair of (usecs, max_frames) fields specifies that interrupts
* should be coalesced until
@@ -483,6 +484,7 @@ struct ethtool_coalesce {
__u32 tx_coalesce_usecs_high;
__u32 tx_max_coalesced_frames_high;
__u32 rate_sample_interval;
+ __u32 use_dim;
};
You cannot do this.
static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
void __user *useraddr)
{
struct ethtool_coalesce coalesce;
int ret;
if (!dev->ethtool_ops->set_coalesce)
return -EOPNOTSUPP;
if (copy_from_user(, useraddr, sizeof(coalesce)))
return -EFAULT;
An old ethtool binary is not going to set this extra last byte to
anything meaningful. You cannot tell if you have an old or new user
space, so you have no idea if it put anything into use_dim, or if it
is random junk.
Even worse, as there is no indication of data length, ETHTOOL_GCOALESCE
ioctl request from old ethtool on new kernel would result in kernel
writing past the end of userspace buffer.
You have to leave the IOCTL interface unchanged, and limit this new
feature to the netlink API.
Hi, Andrew.
thanks for pointing out this problem, i will fix it.
without callling set_coalesce/set_coalesce of ethtool_ops, do you have any
suggestion for writing/reading this new attribute to/from the driver? add a
new field in net_device or a new callback function in ethtool_ops seems not
good.
We could use a similar approach as struct ethtool_link_ksettings, e.g.
struct kernel_ethtool_coalesce {
struct ethtool_coalesce base;
/* new members which are not part of UAPI */
}
get_coalesce() and set_coalesce() would get pointer to struct
kernel_ethtool_coalesce and ioctl code would be modified to only touch
the base (legacy?) part.
Thanks for your suggestion. i will implement it in V2.
While already changing the ops arguments, we could also add extack
pointer, either as a separate argument or as struct member (I slightly
prefer the former).
BtW, please don't forget to update the message descriptions in
Documentation/networking/ethtool-netlink.rst
Michal
will update the doc in V2.
Thanks.
Huazhong.
.
Re: [RFC net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
On 2020/11/19 12:15, Andrew Lunn wrote:
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9ca87bc..afd8de2 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -433,6 +433,7 @@ struct ethtool_modinfo {
*a TX interrupt, when the packet rate is above @pkt_rate_high.
* @rate_sample_interval: How often to do adaptive coalescing packet rate
*sampling, measured in seconds. Must not be zero.
+ * @use_dim: Use DIM for IRQ coalescing, if adaptive coalescing is enabled.
*
* Each pair of (usecs, max_frames) fields specifies that interrupts
* should be coalesced until
@@ -483,6 +484,7 @@ struct ethtool_coalesce {
__u32 tx_coalesce_usecs_high;
__u32 tx_max_coalesced_frames_high;
__u32 rate_sample_interval;
+ __u32 use_dim;
};
You cannot do this.
static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
void __user *useraddr)
{
struct ethtool_coalesce coalesce;
int ret;
if (!dev->ethtool_ops->set_coalesce)
return -EOPNOTSUPP;
if (copy_from_user(, useraddr, sizeof(coalesce)))
return -EFAULT;
An old ethtool binary is not going to set this extra last byte to
anything meaningful. You cannot tell if you have an old or new user
space, so you have no idea if it put anything into use_dim, or if it
is random junk.
You have to leave the IOCTL interface unchanged, and limit this new
feature to the netlink API.
Hi, Andrew.
thanks for pointing out this problem, i will fix it.
without callling set_coalesce/set_coalesce of ethtool_ops, do you have
any suggestion for writing/reading this new attribute to/from the
driver? add a new field in net_device or a new callback function in
ethtool_ops seems not good.
diff --git a/include/uapi/linux/ethtool_netlink.h
b/include/uapi/linux/ethtool_netlink.h
index e2bf36e..e3458d9 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -366,6 +366,7 @@ enum {
ETHTOOL_A_COALESCE_TX_USECS_HIGH, /* u32 */
ETHTOOL_A_COALESCE_TX_MAX_FRAMES_HIGH, /* u32 */
ETHTOOL_A_COALESCE_RATE_SAMPLE_INTERVAL,/* u32 */
+ ETHTOOL_A_COALESCE_USE_DIM, /* u8 */
This appears to be a boolean? So /* flag */ would be better. Or do you
think there is scope for a few different algorithms, and an enum would
be better. If so, you should add the enum with the two current
options.
Andrew
ok, boolean seems enough.
Thanks.
Huazhong.
.
Re: [PATCH V3 net-next 06/10] net: hns3: add ethtool priv-flag for DIM
On 2020/11/15 2:54, Jakub Kicinski wrote: On Thu, 12 Nov 2020 11:33:14 +0800 Huazhong Tan wrote: Add a control private flag in ethtool for enable/disable DIM feature. Signed-off-by: Huazhong Tan Please work on a common ethtool API for the configuration instead of using private flags. Private flags were overused because the old IOCTL-based ethtool was hard to extend, but we have a netlink API now. For example here you're making a choice between device and DIM implementation of IRQ coalescing. You can add a new netlink attribute to the ETHTOOL_MSG_COALESCE_GET/ETHTOOL_MSG_COALESCE_SET commands which controls the type of adaptive coalescing (if enabled). The device's implementation of IRQ coalescing will be removed, if DIM works ok for a long time. So could this private flag for DIM be uptreamed as a transition scheme? And adding a new netlink attrtibute to controls the type of adaptive coalescing seems useless for other drivers. One question I don't think we have a strong answer for is how to handle this extension from ethtool_ops point of view. Should we add a new "extended" op which drivers may start implementing? Or separate the structure passed in to the ops from the one used as uAPI? Thoughts anyone? Huazhong Tan, since the DIM and EQ/CQ patches may require more infrastructure work feel free to repost the first 4 patches separately, I can apply those as is. ok, thanks. .
Re: [PATCH V2 net-next 11/11] net: hns3: add debugfs support for interrupt coalesce
On 2020/11/12 0:15, Jakub Kicinski wrote: On Wed, 11 Nov 2020 11:16:37 +0800 tanhuazhong wrote: On 2020/11/11 9:28, Jakub Kicinski wrote: On Mon, 9 Nov 2020 11:22:39 +0800 Huazhong Tan wrote: Since user may need to check the current configuration of the interrupt coalesce, so add debugfs support for query this info, which includes DIM profile, coalesce configuration of both software and hardware. Signed-off-by: Huazhong Tan Please create a file per vector so that users can just read it instead of dumping the info into the logs. This patch should be removed from this series right now. Since this new read method needs some adaptations and verifications, and there maybe another better ways to dump this info. Even better we should put as much of this information as possible into the ethtool API. dim state is hardly hardware-specific. Should the ethtool API used to dump the hardware info? Could you provide some hints to do it? Not necessarily hardware info but if there is a use case for inspecting DIM state we can extend coalesce_fill_reply() in net/ethtool/coalesce.c to report it. ok, thanks. .
Re: [PATCH V2 net-next 11/11] net: hns3: add debugfs support for interrupt coalesce
On 2020/11/11 9:28, Jakub Kicinski wrote: On Mon, 9 Nov 2020 11:22:39 +0800 Huazhong Tan wrote: Since user may need to check the current configuration of the interrupt coalesce, so add debugfs support for query this info, which includes DIM profile, coalesce configuration of both software and hardware. Signed-off-by: Huazhong Tan Please create a file per vector so that users can just read it instead of dumping the info into the logs. This patch should be removed from this series right now. Since this new read method needs some adaptations and verifications, and there maybe another better ways to dump this info. Even better we should put as much of this information as possible into the ethtool API. dim state is hardly hardware-specific. Should the ethtool API used to dump the hardware info? Could you provide some hints to do it? .
Re: [PATCH V2 net-next 09/11] net: hns3: add support for EQ/CQ mode configuration
On 2020/11/11 9:25, Jakub Kicinski wrote: On Mon, 9 Nov 2020 11:22:37 +0800 Huazhong Tan wrote: For device whose version is above V3(include V3), the GL can select EQ or CQ mode, so adds support for it. In CQ mode, the coalesced timer will restart upon new completion, while in EQ mode, the timer will not restart. Signed-off-by: Huazhong Tan Let's see if I understand - in CQ mode the timer is restarted very time new frame gets received/transmitted? IOW for a continuous stream of frames it will only generate an interrupt once it reaches max_frames? Hi, Jakub. This EQ/CQ is related to the GL(gap limiting, interrupt coalesce based on the gap time). More exactly, the coalesced timer will restart when the first new completion occurs. Will fix the commit log. I think that if you need such a configuration knob we should add this as an option to the official ethtool -c/-C interface, now that we have the ability to extend the netlink API. It seems need more jobs to be did later. Thanks. .
Re: [PATCH V2 net-next 05/11] net: hns3: add support for dynamic interrupt moderation
On 2020/11/11 9:20, Jakub Kicinski wrote: On Mon, 9 Nov 2020 11:22:33 +0800 Huazhong Tan wrote: Add dynamic interrupt moderation support for the HNS3 driver. Signed-off-by: Huazhong Tan I'm slightly confused here. What does the adaptive moderation do in your driver/device if you still need DIM on top of it? The driver's adaptive moderation and DIM are mutually-exclusive options, and DIM is used by default. Since it is hard to verify all cases that DIM can get a better result, so keep the adaptive moderation as an option now, which can be swicthed by the ethtool priv-flag. If DIM is ok for a long time, then we will send a separate patch to remove the adaptive moderation from the driver. Thanks.
Re: [PATCH V2 net-next 01/11] net: hns3: add support for configuring interrupt quantity limiting
On 2020/11/11 9:13, Jakub Kicinski wrote: On Mon, 9 Nov 2020 11:22:29 +0800 Huazhong Tan wrote: + if (rx_vector->tx_group.coal.ql_enable) Is this supposed to be rx_group, not tx? yes, will fix it. thanks. + hns3_set_vector_coalesce_rx_ql(rx_vector, + rx_vector->rx_group.coal.int_ql); .
Re: [PATCH net-next 10/11] net: hns3: add ethtool priv-flag for EQ/CQ
On 2020/11/8 1:47, Jakub Kicinski wrote: On Sat, 7 Nov 2020 14:31:20 +0800 Huazhong Tan wrote: +static void hns3_update_cqe_mode(struct net_device *netdev, bool enable, bool is_tx) Wrap this to 80 characters, please. Will fix it. Thanks .
Re: [PATCH net-next 02/11] net: hns3: add support for 1us unit GL configuration
On 2020/11/8 1:46, Jakub Kicinski wrote: On Sat, 7 Nov 2020 14:31:12 +0800 Huazhong Tan wrote: For device whose version is above V3(include V3), the GL configuration can set as 1us unit, so adds support for configuring this field. Signed-off-by: Huazhong Tan Doesn't build. drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c: In function ‘hns3_check_gl_coalesce_para’: drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c:1152:6: error: ‘ae_dev’ undeclared (first use in this function); did you mean ‘netdev’? 1152 | if (ae_dev->dev_version >= HNAE3_DEVICE_VERSION_V3) | ^~ | netdev drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c:1152:6: note: each undeclared identifier is reported only once for each function it appears in make[6]: *** [drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.o] Error 1 make[5]: *** [drivers/net/ethernet/hisilicon/hns3] Error 2 make[4]: *** [drivers/net/ethernet/hisilicon] Error 2 make[4]: *** Waiting for unfinished jobs make[3]: *** [drivers/net/ethernet] Error 2 make[2]: *** [drivers/net] Error 2 make[2]: *** Waiting for unfinished jobs make[1]: *** [drivers] Error 2 make: *** [__sub-make] Error 2 Will fix it. Thanks. .
Re: [PATCH net-next 0/2] net: two updates related to UDP GSO
On 2020/9/7 23:35, Willem de Bruijn wrote:
On Mon, Sep 7, 2020 at 3:38 PM tanhuazhong wrote:
On 2020/9/7 17:22, Willem de Bruijn wrote:
On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski wrote:
On Sat, 5 Sep 2020 14:11:11 +0800 Huazhong Tan wrote:
There are two updates relates to UDP GSO.
#1 adds a new GSO type for UDPv6
#2 adds check for UDP GSO when csum is disable in netdev_fix_features().
Changes since RFC V2:
- modifies the timing of setting UDP GSO type when doing UDP GRO in #1.
Changes since RFC V1:
- updates NETIF_F_GSO_LAST suggested by Willem de Bruijn.
and add NETIF_F_GSO_UDPV6_L4 feature for each driver who support UDP GSO in
#1.
- add #2 who needs #1.
Please CC people who gave you feedback (Willem).
I don't feel good about this series. IPv6 is not optional any more.
AFAIU you have some issues with csum support in your device? Can you
use .ndo_features_check() to handle this?
The change in semantics of NETIF_F_GSO_UDP_L4 from "v4 and v6" to
"just v4" can trip people over; this is not a new feature people
may be depending on the current semantics.
Willem, what are your thoughts on this?
If that is the only reason, +1 on fixing it up in the driver's
ndo_features_check.
Hi, Willem & Jakub.
This series mainly fixes the feature dependency between hardware
checksum and UDP GSO.
When turn off hardware checksum offload, run 'ethtool -k [devname]'
we can see TSO is off as well, but udp gso still is on.
I see. That does not entirely require separate IPv4 and IPv6 flags. It
can be disabled if either checksum offload is disabled. I'm not aware
of any hardware that only supports checksum offload for one of the two
network protocols.
below patch is acceptable? i have sent this patch before
(https://patchwork.ozlabs.org/project/netdev/patch/1594180136-15912-3-git-send-email-tanhuazh...@huawei.com/)
diff --git a/net/core/dev.c b/net/core/dev.c
index c02bae9..dcb6b35 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9095,6 +9095,12 @@ static netdev_features_t
netdev_fix_features(struct net_device *dev,
features &= ~NETIF_F_TSO6;
}
+ if ((features & NETIF_F_GSO_UDP_L4) && !(features & NETIF_F_HW_CSUM) &&
+ (!(features & NETIF_F_IP_CSUM) || !(features & NETIF_F_IPV6_CSUM)))
{
+ netdev_dbg(dev, "Dropping UDP GSO features since no CSUM
feature.\n");
+ features &= ~NETIF_F_GSO_UDP_L4;
+ }
+
/* TSO with IPv4 ID mangling requires IPv4 TSO be enabled */
if ((features & NETIF_F_TSO_MANGLEID) && !(features & NETIF_F_TSO))
features &= ~NETIF_F_TSO_MANGLEID;
As Eric Dumazet commented "This would prevent a device providing IPv4
checksum only (no IPv6 csum support) from sending IPv4 UDP GSO packets ?",
so i send this series to decouple them. Is there any good ways to
shuttle this issue? Or as you said there is not device only support
checksum offload for one of the two network protocols.
Alternatively, the real value of splitting the type is in advertising
the features separately through ethtool. That requires additional
changes.
.
Re: [PATCH net-next 0/2] net: two updates related to UDP GSO
On 2020/9/7 17:22, Willem de Bruijn wrote:
On Sun, Sep 6, 2020 at 8:42 PM Jakub Kicinski wrote:
On Sat, 5 Sep 2020 14:11:11 +0800 Huazhong Tan wrote:
There are two updates relates to UDP GSO.
#1 adds a new GSO type for UDPv6
#2 adds check for UDP GSO when csum is disable in netdev_fix_features().
Changes since RFC V2:
- modifies the timing of setting UDP GSO type when doing UDP GRO in #1.
Changes since RFC V1:
- updates NETIF_F_GSO_LAST suggested by Willem de Bruijn.
and add NETIF_F_GSO_UDPV6_L4 feature for each driver who support UDP GSO in
#1.
- add #2 who needs #1.
Please CC people who gave you feedback (Willem).
I don't feel good about this series. IPv6 is not optional any more.
AFAIU you have some issues with csum support in your device? Can you
use .ndo_features_check() to handle this?
The change in semantics of NETIF_F_GSO_UDP_L4 from "v4 and v6" to
"just v4" can trip people over; this is not a new feature people
may be depending on the current semantics.
Willem, what are your thoughts on this?
If that is the only reason, +1 on fixing it up in the driver's
ndo_features_check.
Hi, Willem & Jakub.
This series mainly fixes the feature dependency between hardware
checksum and UDP GSO.
When turn off hardware checksum offload, run 'ethtool -k [devname]'
we can see TSO is off as well, but udp gso still is on.
[root@localhost ~]# ethtool -K eth0 tx off
Actual changes:
tx-checksumming: off
tx-checksum-ipv4: off
tx-checksum-ipv6: off
tx-checksum-sctp: off
tcp-segmentation-offload: off
tx-tcp-segmentation: off [requested on]
tx-tcp-ecn-segmentation: off [requested on]
tx-tcp6-segmentation: off [requested on]
[root@localhost ~]# ethtool -k eth0
Features for eth0:
rx-checksumming: on
tx-checksumming: off
tx-checksum-ipv4: off
tx-checksum-ip-generic: off [fixed]
tx-checksum-ipv6: off
tx-checksum-fcoe-crc: off [fixed]
tx-checksum-sctp: off
...
tcp-segmentation-offload: off
tx-tcp-segmentation: off [requested on]
tx-tcp-ecn-segmentation: off [requested on]
tx-tcp-mangleid-segmentation: off
tx-tcp6-segmentation: off [requested on]
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
...
tx-udp-segmentation: on
...
.ndo_feature_check seems unnecessary.
Because the stack has already do this check.
in __ip_append_data() below if branch will not run if hardware checksum
offload is off.
if (transhdrlen &&
length + fragheaderlen <= mtu &&
rt->dst.dev->features & (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM) &&
(!(flags & MSG_MORE) || cork->gso_size) &&
(!exthdrlen || (rt->dst.dev->features &
NETIF_F_HW_ESP_TX_CSUM)))
csummode = CHECKSUM_PARTIAL;
...
so skb->ip_summed set as CHECKSUM_NONE,
then in udp_send_skb()
if (cork->gso_size) {
...
if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite ||
dst_xfrm(skb_dst(skb))) {
kfree_skb(skb);
return -EIO;
}
the packet who needs udp gso will return ERROR.
For this kind of problem how could we fix it?
Thanks.
Huazhong.
.
Re: [RFC net-next] udp: add a GSO type for UDPv6
On 2020/9/3 6:43, David Miller wrote: From: Huazhong Tan Date: Wed, 2 Sep 2020 20:15:11 +0800 In some cases, for UDP GSO, UDPv4 and UDPv6 need to be handled separately, for example, checksum offload, so add new GSO type SKB_GSO_UDPV6_L4 for UDPv6, and the old SKB_GSO_UDP_L4 stands for UDPv4. Signed-off-by: Huazhong Tan Please submit this alongside something that needs to use it. will add in V2, thanks. Thank you. .
Re: [RFC net-next] udp: add a GSO type for UDPv6
On 2020/9/2 22:33, Willem de Bruijn wrote:
On Wed, Sep 2, 2020 at 2:18 PM Huazhong Tan wrote:
In some cases, for UDP GSO, UDPv4 and UDPv6 need to be handled
separately, for example, checksum offload, so add new GSO type
SKB_GSO_UDPV6_L4 for UDPv6, and the old SKB_GSO_UDP_L4 stands
for UDPv4.
This is in preparation for hardware you have that actually cares about
this distinction, I guess?
it is mainly for separating checksum offload of IPv4 and IPv6 right now.
with this patch, the user can switch checksum offload of IPv4 and not
affect IPv6's, vice versa.
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 2cc3cf8..b7c1a76 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -54,6 +54,7 @@ enum {
NETIF_F_GSO_UDP_BIT,/* ... UFO, deprecated except tuntap */
NETIF_F_GSO_UDP_L4_BIT, /* ... UDP payload GSO (not UFO) */
NETIF_F_GSO_FRAGLIST_BIT, /* ... Fraglist GSO */
+ NETIF_F_GSO_UDPV6_L4_BIT, /* ... UDPv6 payload GSO (not UFO) */
/**/NETIF_F_GSO_LAST = /* last bit, see GSO_MASK */
NETIF_F_GSO_FRAGLIST_BIT,
Need to update NETIF_F_GSO_LAST then, too.
ok, thanks.
Re: [PATCH 02/12] net: hns3: Destroy a mutex after initialisation failure in hclge_init_ad_dev()
On 2020/5/29 2:42, Markus Elfring wrote: Add a mutex destroy call in hclge_init_ae_dev() when fails. How do you think about a wording variant like the following? Change description: The function “mutex_init” was called before a call of the function “hclge_pci_init”. But the function “mutex_destroy” was not called after initialisation steps failed. Thus add the missed function call for the completion of the exception handling. It looks better. I will try to improve the skill of patch description and make as many as people can understand the patch. Thanks for help. Would you like to add the tag “Fixes” to the commit message? Since it seems not a very urgent issue, so i send it to the -next and make it as a code optimization. Thanks:) … +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c @@ -10108,6 +10108,7 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev) pci_release_regions(pdev); pci_disable_device(pdev); out: + mutex_destroy(>vport_lock); return ret; } How do you think about to use the label “destroy_mutex” instead? Will use label 'destroy_mutex‘ instead if there is another patch need to modify this code, which is more readable. Thanks for your comments. Regards, Markus
Re: [PATCH net-next 00/11] net: hns3: misc updates for -next
Sorry, please ignore this patchset, will resend it later. On 2020/5/28 20:45, Huazhong Tan wrote: This patchset includes some updates for the HNS3 ethernet driver. #1 adds a missing mutex destroy. #2&3 refactor two function, make them more readable and maintainable. #4&5 fix unsuitable type of gro enable field both for PF & VF. #6-#10 removes some unused fields, macro and redundant definitions. #11 adds more debug info for parsing speed fails. Huazhong Tan (11): net: hns3: add a missing mutex destroy in hclge_init_ad_dev() net: hns3: refactor hclge_config_tso() net: hns3: refactor hclge_query_bd_num_cmd_send() net: hns3: modify an incorrect type in struct hclge_cfg_gro_status_cmd net: hns3: modify an incorrect type in struct hclgevf_cfg_gro_status_cmd net: hns3: remove some unused fields in struct hns3_nic_priv net: hns3; remove unused HNAE3_RESTORE_CLIENT in enum hnae3_reset_notify_type net: hns3: remove unused struct hnae3_unic_private_info net: hns3: remove two duplicated register macros in hclgevf_main.h net: hns3: remove some unused fields in struct hclge_dev net: hns3: print out speed info when parsing speed fails drivers/net/ethernet/hisilicon/hns3/hnae3.h| 12 -- drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 22 --- .../net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.h | 4 +- .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c| 44 ++ .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h| 6 --- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.h | 4 +- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 8 ++-- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h | 4 +- 8 files changed, 29 insertions(+), 75 deletions(-)
Re: [PATCH V2 net-next 0/2] net: hns3: adds two VLAN feature
On 2020/5/22 17:35, tanhuazhong wrote: On 2020/5/22 5:37, David Miller wrote: From: Jakub Kicinski Date: Thu, 21 May 2020 12:17:07 -0700 On Thu, 21 May 2020 19:38:23 +0800 Huazhong Tan wrote: This patchset adds two new VLAN feature. [patch 1] adds a new dynamic VLAN mode. [patch 2] adds support for 'QoS' field to PVID. Change log: V1->V2: modifies [patch 1]'s commit log, suggested by Jakub Kicinski. I don't like the idea that FW is choosing the driver behavior in a way that's not observable via standard Linux APIs. This is the second time a feature like that posted for a driver this week, and we should discourage it. Agreed, this is an unacceptable approach to driver features. Hi, Jakub & David. As decribed in patch #1, there is a scenario which needs the dynamic mode(port VLAN filter is always disabled, andVF VLAN filter is keep disable until a non-zero VLAN ID being used for the function). Is this mode selection provided through "ethtool --set-priv-flags" more acceptable? Or is there any other better suggestion for this? Thanks. Hi, Jakub & David. For patch#1, is it acceptable adding "ethtool --get-priv-flags" to query the VLAN. If yes, I will send a RFC for it. Best Regards. Thanks. .
Re: [PATCH net-next 1/5] net: hns3: add support for VF to query ring and vector mapping
On 2020/5/23 1:39, Jakub Kicinski wrote: On Fri, 22 May 2020 10:49:42 +0800 Huazhong Tan wrote: From: Guangbin Huang This patch adds support for VF to query the mapping of ring and vector. Signed-off-by: Guangbin Huang Signed-off-by: Huazhong Tan Hi, Jakub. Could you explain a little more what this is doing? This patch just adds a new type of mailbox for VF to the mapping of ring and vector through PF. not a complicated feature;). Also what's using this? In the series nothing is making this request. As mentioned in the cover, "this is needed by the hns3 DPDK VF PMD driver", current the VF driver of linux kernel does need this info. Should this also mention in this commit log? Thanks. .
Re: [PATCH V2 net-next 0/2] net: hns3: adds two VLAN feature
On 2020/5/22 5:37, David Miller wrote: From: Jakub Kicinski Date: Thu, 21 May 2020 12:17:07 -0700 On Thu, 21 May 2020 19:38:23 +0800 Huazhong Tan wrote: This patchset adds two new VLAN feature. [patch 1] adds a new dynamic VLAN mode. [patch 2] adds support for 'QoS' field to PVID. Change log: V1->V2: modifies [patch 1]'s commit log, suggested by Jakub Kicinski. I don't like the idea that FW is choosing the driver behavior in a way that's not observable via standard Linux APIs. This is the second time a feature like that posted for a driver this week, and we should discourage it. Agreed, this is an unacceptable approach to driver features. Hi, Jakub & David. As decribed in patch #1, there is a scenario which needs the dynamic mode(port VLAN filter is always disabled, andVF VLAN filter is keep disable until a non-zero VLAN ID being used for the function). Is this mode selection provided through "ethtool --set-priv-flags" more acceptable? Or is there any other better suggestion for this? Thanks. .
Re: [PATCH net-next 1/2] net: hns3: adds support for dynamic VLAN mode
On 2020/5/21 9:36, Jakub Kicinski wrote: On Thu, 21 May 2020 09:33:14 +0800 tanhuazhong wrote: On 2020/5/21 5:06, Jakub Kicinski wrote: On Wed, 20 May 2020 09:20:12 +0800 Huazhong Tan wrote: From: GuoJia Liao There is a scenario which needs vNICs enable the VLAN filter in access port, while disable the VLAN filter in trunk port. Access port and trunk port can switch according to the user's configuration. This patch adds support for the dynamic VLAN mode. then the HNS3 driver can support two VLAN modes: default VLAN mode and dynamic VLAN mode. User can switch the mode through the configuration file. What configuration file? Sounds like you're reimplementing trusted VFs (ndo_set_vf_trust). Hi, Jakub. Maybe this configuration file here is a little misleading, this VLAN mode is decided by the firmware, the driver will query the VLAN mode from firmware during intializing. And the FW got that configuration from? It depends on the user's demand, the user can choose the firmware which supports the default VLAN mode or the dynamic VLAN mode. I will modified this description in V2. BTW, is there any other suggestion about this patch? The other suggestion was to trusted vf. What's the difference between trusted VF and "dynamic VLAN mode"? Trust VF is not related to dynamic VLAN mode. So far it's only be used for privilege checking for the VF promisc. And dynamic VLAN mode is designed to adapt specified scenario which want enable/disable VLAN filter base on VLAN used. Thanks. In default VLAN mode, port based VLAN filter and VF VLAN filter should always be enabled. In dynamic VLAN mode, port based VLAN filter is disabled, and VF VLAN filter is disabled defaultly, and should be enabled when there is a non-zero VLAN ID. In addition, VF VLAN filter is enabled if PVID is enabled for vNIC. When enable promisc, VLAN filter should be disabled. When disable promisc, VLAN filter's status depends on the value of 'vport->vf_vlan_en', which is used to record the VF VLAN filter status. In default VLAN mode, 'vport->vf_vlan_en' always be 'true', so VF VLAN filter will set to be enabled after disabling promisc. In dynamic VLAN mode, 'vport->vf_vlan_en' lies on whether there is a non-zero VLAN ID. Signed-off-by: GuoJia Liao Signed-off-by: Huazhong Tan .
Re: [PATCH net-next 1/2] net: hns3: adds support for dynamic VLAN mode
On 2020/5/21 5:06, Jakub Kicinski wrote: On Wed, 20 May 2020 09:20:12 +0800 Huazhong Tan wrote: From: GuoJia Liao There is a scenario which needs vNICs enable the VLAN filter in access port, while disable the VLAN filter in trunk port. Access port and trunk port can switch according to the user's configuration. This patch adds support for the dynamic VLAN mode. then the HNS3 driver can support two VLAN modes: default VLAN mode and dynamic VLAN mode. User can switch the mode through the configuration file. What configuration file? Sounds like you're reimplementing trusted VFs (ndo_set_vf_trust). Hi, Jakub. Maybe this configuration file here is a little misleading, this VLAN mode is decided by the firmware, the driver will query the VLAN mode from firmware during intializing. I will modified this description in V2. BTW, is there any other suggestion about this patch? Thanks:) In default VLAN mode, port based VLAN filter and VF VLAN filter should always be enabled. In dynamic VLAN mode, port based VLAN filter is disabled, and VF VLAN filter is disabled defaultly, and should be enabled when there is a non-zero VLAN ID. In addition, VF VLAN filter is enabled if PVID is enabled for vNIC. When enable promisc, VLAN filter should be disabled. When disable promisc, VLAN filter's status depends on the value of 'vport->vf_vlan_en', which is used to record the VF VLAN filter status. In default VLAN mode, 'vport->vf_vlan_en' always be 'true', so VF VLAN filter will set to be enabled after disabling promisc. In dynamic VLAN mode, 'vport->vf_vlan_en' lies on whether there is a non-zero VLAN ID. Signed-off-by: GuoJia Liao Signed-off-by: Huazhong Tan .
Re: [PATCH net-next] net: phy: realtek: add loopback support for RTL8211F
On 2020/5/13 21:12, Andrew Lunn wrote:
On Wed, May 13, 2020 at 04:25:44PM +0800, Huazhong Tan wrote:
From: Yufeng Mo
PHY loopback is already supported by genphy driver. This patch
adds the set_loopback interface to RTL8211F PHY driver, so the PHY
selftest can run properly on it.
Signed-off-by: Yufeng Mo
Signed-off-by: Jian Shen
Signed-off-by: Huazhong Tan
It took three people to write a 1 line patch?
---
drivers/net/phy/realtek.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index c7229d0..6c5918c 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -615,6 +615,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = genphy_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .set_loopback = genphy_loopback,
}, {
.name = "Generic FE-GE Realtek PHY",
.match_phy_device = rtlgen_match_phy_device,
Do you have access to the data sheets? Can you check if the other PHYs
supported by this driver also support loopback in the standard way?
They probably do.
Andrew
Hi, Andrew.
There are two type of phys we are using, rtl8211f and "Marvell 88E1512".
"Marvell 88E1512" has already supported loopback
(f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework")).
So now we adds loopback support to the rtl8211f.
From the data sheet other phys should support this loopback as well, but
we have no way to verify it. What's your suggestion?
Thanks:)
.
Re: [PATCH net-next] net: phy: realtek: add loopback support for RTL8211F
On 2020/5/13 21:12, Andrew Lunn wrote:
On Wed, May 13, 2020 at 04:25:44PM +0800, Huazhong Tan wrote:
From: Yufeng Mo
PHY loopback is already supported by genphy driver. This patch
adds the set_loopback interface to RTL8211F PHY driver, so the PHY
selftest can run properly on it.
Signed-off-by: Yufeng Mo
Signed-off-by: Jian Shen
Signed-off-by: Huazhong Tan
It took three people to write a 1 line patch?
Yufeng Mo is the author of this patch, since he has not right to send mail
so I just help him to do than。 If necessary, Jian Shen could be deleted.
---
drivers/net/phy/realtek.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index c7229d0..6c5918c 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -615,6 +615,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = genphy_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .set_loopback = genphy_loopback,
}, {
.name = "Generic FE-GE Realtek PHY",
.match_phy_device = rtlgen_match_phy_device,
Do you have access to the data sheets? Can you check if the other PHYs
supported by this driver also support loopback in the standard way?
They probably do.
Andrew
We have checked the data sheet. Since we only have rtl8211f phy on our
hand, so only support loopback on this phy.
Thanks:)
.
Re: [PATCH net-next 3/5] net: hns3: provide .get_cmdq_stat interface for the client
On 2020/5/10 4:48, Jakub Kicinski wrote:
On Sat, 9 May 2020 17:27:39 +0800 Huazhong Tan wrote:
This patch provides a new interface for the client to query
whether CMDQ is ready to work.
Signed-off-by: Huazhong Tan
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index 5602bf2..7506cab 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -552,6 +552,7 @@ struct hnae3_ae_ops {
int (*set_vf_mac)(struct hnae3_handle *handle, int vf, u8 *p);
int (*get_module_eeprom)(struct hnae3_handle *handle, u32 offset,
u32 len, u8 *data);
+ bool (*get_cmdq_stat)(struct hnae3_handle *handle);
};
I don't see anything in this series using this new interface, why is it
added now?
Hi, Jakub.
This interface is needed by the roce client, whose patch will be
upstreamed to the rdma tree, it is other branch. So we provide this
interface previously, then the rdma guy will upstream their patch later,
maybe linux-5.8-rc*.
Thanks.
.
Re: [PATCH net-next] net: hns3: adds support for reading module eeprom info
On 2020/4/29 2:49, Jakub Kicinski wrote:
On Tue, 28 Apr 2020 19:58:25 +0800 Huazhong Tan wrote:
From: Yonglong Liu
This patch adds support for reading the optical module eeprom
info via "ethtool -m".
Signed-off-by: Yonglong Liu
Signed-off-by: Huazhong Tan
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 4d9c85f..8364e1b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -12,6 +12,16 @@ struct hns3_stats {
int stats_offset;
};
+#define HNS3_MODULE_TYPE_QSFP 0x0C
+#define HNS3_MODULE_TYPE_QSFP_P0x0D
+#define HNS3_MODULE_TYPE_QSFP_28 0x11
+#define HNS3_MODULE_TYPE_SFP 0x03
Could you use the SFF8024_ID_* defines from sfp.h here as well?
Yes, will send V2 to do that.
Otherwise looks good to me!
Thanks:)
.
Re: [PATCH net-next 00/12] net: hns3: add some bugfixes and optimizations
On 2019/10/17 23:47, Jakub Kicinski wrote: On Thu, 17 Oct 2019 11:27:09 +0800, tanhuazhong wrote: On 2019/10/17 1:50, David Miller wrote: From: Jakub Kicinski Date: Wed, 16 Oct 2019 10:19:43 -0700 On Wed, 16 Oct 2019 15:16:59 +0800, Huazhong Tan wrote: This patch-set includes some bugfixes and code optimizations for the HNS3 ethernet controller driver. The code LGTM, mostly, but it certainly seems like patches 2, 3 and 4 should be a separate series targeting the net tree :( Agreed, there are legitimate bug fixes. I have to say that I see this happening a lot, hns3 bug fixes targetting net-next in a larger series of cleanups and other kinds of changes. Please handle this delegation properly. Send bug fixes as a series targetting 'net', and send everything else targetting 'net-next'. Hi, David & Jakub. BTW, patch01 is a cleanup which is needed by patch02, if patch01 targetting 'net-next', patch02 targetting 'net', there will be a gap again. How should I deal with this case? You'll need to reorder the cleanup so that the fixes apply to the unmodified net tree. Then preferably wait for the net tree to be merged back to net-next before posting the cleanup that'd conflict. If the conflict is not too hard to resolve you can just post the net-next patches and give some instructions on how to resolve the merge conflict under the --- lines in the commit message. ok, thanks. .
Re: [PATCH net-next 00/12] net: hns3: add some bugfixes and optimizations
On 2019/10/17 1:50, David Miller wrote: From: Jakub Kicinski Date: Wed, 16 Oct 2019 10:19:43 -0700 On Wed, 16 Oct 2019 15:16:59 +0800, Huazhong Tan wrote: This patch-set includes some bugfixes and code optimizations for the HNS3 ethernet controller driver. The code LGTM, mostly, but it certainly seems like patches 2, 3 and 4 should be a separate series targeting the net tree :( Agreed, there are legitimate bug fixes. I have to say that I see this happening a lot, hns3 bug fixes targetting net-next in a larger series of cleanups and other kinds of changes. Please handle this delegation properly. Send bug fixes as a series targetting 'net', and send everything else targetting 'net-next'. Hi, David & Jakub. BTW, patch01 is a cleanup which is needed by patch02, if patch01 targetting 'net-next', patch02 targetting 'net', there will be a gap again. How should I deal with this case? MBR. Huazhong. .
Re: [PATCH net-next 00/12] net: hns3: add some bugfixes and optimizations
On 2019/10/17 1:50, David Miller wrote: From: Jakub Kicinski Date: Wed, 16 Oct 2019 10:19:43 -0700 On Wed, 16 Oct 2019 15:16:59 +0800, Huazhong Tan wrote: This patch-set includes some bugfixes and code optimizations for the HNS3 ethernet controller driver. The code LGTM, mostly, but it certainly seems like patches 2, 3 and 4 should be a separate series targeting the net tree :( Agreed, there are legitimate bug fixes. I have to say that I see this happening a lot, hns3 bug fixes targetting net-next in a larger series of cleanups and other kinds of changes. Please handle this delegation properly. Send bug fixes as a series targetting 'net', and send everything else targetting 'net-next'. ok, thanks. .
Re: [PATCH net-next 00/12] net: hns3: add some bugfixes and optimizations
On 2019/10/17 1:19, Jakub Kicinski wrote: On Wed, 16 Oct 2019 15:16:59 +0800, Huazhong Tan wrote: This patch-set includes some bugfixes and code optimizations for the HNS3 ethernet controller driver. The code LGTM, mostly, but it certainly seems like patches 2, 3 and 4 should be a separate series targeting the net tree :( ok, I will pick out the bugfix and upstream to net tree firstly. Thanks.
Re: [PATCH net-next 11/12] net: hns3: do not allocate linear data for fraglist skb
On 2019/10/17 1:18, Jakub Kicinski wrote:
On Wed, 16 Oct 2019 15:17:10 +0800, Huazhong Tan wrote:
From: Yunsheng Lin
Currently, napi_alloc_skb() is used to allocate skb for fraglist
when the head skb is not enough to hold the remaining data, and
the remaining data is added to the frags part of the fraglist skb,
leaving the linear part unused.
So this patch passes length of 0 to allocate fraglist skb with
zero size of linear data.
Fixes: 81ae0e0491f3 ("net: hns3: Add skb chain when num of RX buf exceeds
MAX_SKB_FRAGS")
Signed-off-by: Yunsheng Lin
Signed-off-by: Huazhong Tan
Is this really a fix? I just wastes memory, right?
It is a minor optimizations. This fix tag is a mistake.
Thanks.
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 6172eb2..14111af 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2866,8 +2866,7 @@ static int hns3_add_frag(struct hns3_enet_ring *ring,
struct hns3_desc *desc,
return -ENXIO;
if (unlikely(ring->frag_num >= MAX_SKB_FRAGS)) {
- new_skb = napi_alloc_skb(>tqp_vector->napi,
-HNS3_RX_HEAD_SIZE);
+ new_skb = napi_alloc_skb(>tqp_vector->napi, 0);
if (unlikely(!new_skb)) {
hns3_rl_err(ring_to_netdev(ring),
"alloc rx fraglist skb fail\n");
.
Re: [PATCH net-next 08/12] net: hns3: introduce ring_to_netdev() in enet module
On 2019/10/17 1:10, Jakub Kicinski wrote: On Wed, 16 Oct 2019 15:17:07 +0800, Huazhong Tan wrote: From: Yunsheng Lin There are a few places that need to access the netdev of a ring through ring->tqp->handle->kinfo.netdev, and ring->tqp is a struct which both in enet and hclge modules, it is better to use the struct that is only used in enet module. This patch adds the ring_to_netdev() to access the netdev of ring through ring->tqp_vector->napi.dev. Also, struct hns3_enet_ring is a frequently used in critical data path, so make it cacheline aligned as struct hns3_enet_tqp_vector. That part seems logically separate, should it be a separate patch? ok, thanks. Signed-off-by: Yunsheng Lin Signed-off-by: Huazhong Tan
Re: [PATCH V2 net-next 1/7] net: hns3: add ethtool_ops.set_channels support for HNS3 VF driver
Hi, Michal
On 2019/9/12 14:23, Michal Kubecek wrote:
On Wed, Sep 11, 2019 at 10:40:33AM +0800, Huazhong Tan wrote:
From: Guangbin Huang
This patch adds ethtool_ops.set_channels support for HNS3 VF driver,
and updates related TQP information and RSS information, to support
modification of VF TQP number, and uses current rss_size instead of
max_rss_size to initialize RSS.
Also, fixes a format error in hclgevf_get_rss().
Signed-off-by: Guangbin Huang
Signed-off-by: Huazhong Tan
---
drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 1 +
.../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 83 --
2 files changed, 79 insertions(+), 5 deletions(-)
...
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 594cae8..e3090b3 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
...
+static void hclgevf_update_rss_size(struct hnae3_handle *handle,
+ u32 new_tqps_num)
+{
+ struct hnae3_knic_private_info *kinfo = >kinfo;
+ struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
+ u16 max_rss_size;
+
+ kinfo->req_rss_size = new_tqps_num;
+
+ max_rss_size = min_t(u16, hdev->rss_size_max,
+hdev->num_tqps / kinfo->num_tc);
+
+ /* Use the user's configuration when it is not larger than
+* max_rss_size, otherwise, use the maximum specification value.
+*/
+ if (kinfo->req_rss_size != kinfo->rss_size && kinfo->req_rss_size &&
+ kinfo->req_rss_size <= max_rss_size)
+ kinfo->rss_size = kinfo->req_rss_size;
+ else if (kinfo->rss_size > max_rss_size ||
+(!kinfo->req_rss_size && kinfo->rss_size < max_rss_size))
+ kinfo->rss_size = max_rss_size;
I don't think requested channel count can be larger than max_rss_size
here. In ethtool_set_channels(), we check that requested channel counts
do not exceed maximum channel counts as reported by ->get_channels().
And hclgevf_get_max_channels() cannot return more than max_rss_size.
When we can modify the TC number (which PF has already supported, VF may
implement in the future) using lldptool or tc cmd,
hclgevf_update_rss_size will be called to update the rss information,
which may also change max_rss_size, so we will use max_rss_size instead
if the kinfo->rss_size configured using ethtool is bigger than max_rss_size.
+
+ kinfo->num_tqps = kinfo->num_tc * kinfo->rss_size;
+}
+
+static int hclgevf_set_channels(struct hnae3_handle *handle, u32 new_tqps_num,
+ bool rxfh_configured)
+{
+ struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
+ struct hnae3_knic_private_info *kinfo = >kinfo;
+ u16 cur_rss_size = kinfo->rss_size;
+ u16 cur_tqps = kinfo->num_tqps;
+ u32 *rss_indir;
+ unsigned int i;
+ int ret;
+
+ hclgevf_update_rss_size(handle, new_tqps_num);
+
+ ret = hclgevf_set_rss_tc_mode(hdev, kinfo->rss_size);
+ if (ret)
+ return ret;
+
+ /* RSS indirection table has been configuared by user */
+ if (rxfh_configured)
+ goto out;
+
+ /* Reinitializes the rss indirect table according to the new RSS size */
+ rss_indir = kcalloc(HCLGEVF_RSS_IND_TBL_SIZE, sizeof(u32), GFP_KERNEL);
+ if (!rss_indir)
+ return -ENOMEM;
+
+ for (i = 0; i < HCLGEVF_RSS_IND_TBL_SIZE; i++)
+ rss_indir[i] = i % kinfo->rss_size;
+
+ ret = hclgevf_set_rss(handle, rss_indir, NULL, 0);
+ if (ret)
+ dev_err(>pdev->dev, "set rss indir table fail, ret=%d\n",
+ ret);
+
+ kfree(rss_indir);
+
+out:
+ if (!ret)
+ dev_info(>pdev->dev,
+"Channels changed, rss_size from %u to %u, tqps from %u to
%u",
+cur_rss_size, kinfo->rss_size,
+cur_tqps, kinfo->rss_size * kinfo->num_tc);
+
+ return ret;
+}
IIRC David asked you not to issue this log message in v1 review.
Michal Kubecek
Sorry for missing this log.
Thanks.
.
Re: [PATCH V2 net-next 4/7] net: hns3: fix port setting handle for fibre port
On 2019/9/11 18:16, Sergei Shtylyov wrote:
Hello!
On 11.09.2019 5:40, Huazhong Tan wrote:
From: Guangbin Huang
For hardware doesn't support use specified speed and duplex
Can't pasre that. "For hardware that does not support using", perhaps?
Yes, thanks. Will check the grammar more carefully next time.
to negotiate, it's unnecessary to check and modify the port
speed and duplex for fibre port when autoneg is on.
Fixes: 22f48e24a23d ("net: hns3: add autoneg and change speed support
for fibre port")
Signed-off-by: Guangbin Huang
Signed-off-by: Huazhong Tan
---
drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index f5a681d..680c350 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -726,6 +726,12 @@ static int hns3_check_ksettings_param(const
struct net_device *netdev,
u8 duplex;
int ret;
+ /* hw doesn't support use specified speed and duplex to negotiate,
I can't parse that, did you mean "using"?
yes, thanks.
+ * unnecessary to check them when autoneg on.
+ */
+ if (cmd->base.autoneg)
+ return 0;
+
if (ops->get_ksettings_an_result) {
ops->get_ksettings_an_result(handle, , ,
);
if (cmd->base.autoneg == autoneg && cmd->base.speed == speed &&
@@ -787,6 +793,15 @@ static int hns3_set_link_ksettings(struct
net_device *netdev,
return ret;
}
+ /* hw doesn't support use specified speed and duplex to negotiate,
Here too...
yes, thanks.
+ * ignore them when autoneg on.
+ */
+ if (cmd->base.autoneg) {
+ netdev_info(netdev,
+ "autoneg is on, ignore the speed and duplex\n");
+ return 0;
+ }
+
if (ops->cfg_mac_speed_dup_h)
ret = ops->cfg_mac_speed_dup_h(handle, cmd->base.speed,
cmd->base.duplex);
MBR, Sergei
.
Re: [PATCH net-next 1/7] net: hns3: add ethtool_ops.set_channels support for HNS3 VF driver
On 2019/9/11 1:25, David Miller wrote:
From: Huazhong Tan
Date: Tue, 10 Sep 2019 16:58:22 +0800
+ /* Set to user value, no larger than max_rss_size. */
+ if (kinfo->req_rss_size != kinfo->rss_size && kinfo->req_rss_size &&
+ kinfo->req_rss_size <= max_rss_size) {
+ dev_info(>pdev->dev, "rss changes from %u to %u\n",
+kinfo->rss_size, kinfo->req_rss_size);
+ kinfo->rss_size = kinfo->req_rss_size;
Please do not emit kernel log messages for normal operations.
Will remove this log in V2.
Thanks.
.
Re: [PATCH net-next 4/7] net: hns3: add client node validity judgment
On 2019/9/5 18:12, Sergei Shtylyov wrote:
On 04.09.2019 17:06, Huazhong Tan wrote:
From: Peng Li
HNS3 driver can only unregister client which included in
hnae3_client_list.
This patch adds the client node validity judgment.
Signed-off-by: Peng Li
Signed-off-by: Huazhong Tan
---
drivers/net/ethernet/hisilicon/hns3/hnae3.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.c
b/drivers/net/ethernet/hisilicon/hns3/hnae3.c
index 528f624..6aa5257 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.c
@@ -138,12 +138,28 @@ EXPORT_SYMBOL(hnae3_register_client);
void hnae3_unregister_client(struct hnae3_client *client)
{
+ struct hnae3_client *client_tmp;
struct hnae3_ae_dev *ae_dev;
+ bool existed = false;
if (!client)
return;
mutex_lock(_common_lock);
+
+ list_for_each_entry(client_tmp, _client_list, node) {
+ if (client_tmp->type == client->type) {
+ existed = true;
+ break;
+ }
+ }
+
+ if (!existed) {
+ mutex_unlock(_common_lock);
+ pr_err("client %s not existed!\n", client->name);
Did not exist, you mean?
yes
[...]
MBR, Sergei
.
Re: [PATCH V3 net-next 06/10] net: hns3: add debug messages to identify eth down cause
On 2019/7/28 10:03, David Miller wrote: From: Huazhong Tan Date: Sat, 27 Jul 2019 13:46:08 +0800 From: Yonglong Liu Some times just see the eth interface have been down/up via dmesg, but can not know why the eth down. So adds some debug messages to identify the cause for this. Signed-off-by: Yonglong Liu Signed-off-by: Peng Li Signed-off-by: Huazhong Tan --- drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 18 ++ drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c| 19 +++ .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c| 11 +++ 3 files changed, 48 insertions(+) diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c index 4d58c53..973c57b 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c @@ -459,6 +459,9 @@ static int hns3_nic_net_open(struct net_device *netdev) h->ae_algo->ops->set_timer_task(priv->ae_handle, true); hns3_config_xps(priv); + + netif_info(h, drv, netdev, "net open\n"); These will pollute everyone's kernel logs for normal operations. This is not reasonable at all, sorry. Furthermore, even if it was appropriate, "netif_info()" is not "debug". Will replace it with netif_dbg. thanks. .
Re: [PATCH V2 net-next 00/11] net: hns3: some code optimizations & bugfixes & features
Sorry, please ignore this patchset. Will resend it later:) On 2019/7/26 11:24, Huazhong Tan wrote: This patch-set includes code optimizations, bugfixes and features for the HNS3 ethernet controller driver. [patch 1/11] checks reset status before setting channel. [patch 2/11] adds a NULL pointer checking. [patch 3/11] removes reset level upgrading when current reset fails. [patch 4/11] fixes a bug related to IRQ vector number initialization. [patch 5/11] fixes a GFP flags errors when holding spin_lock. [patch 6/11] modifies firmware version format. [patch 7/11] adds some print information which is off by default. [patch 8/11 - 9/11] adds two code optimizations about interrupt handler and work task. [patch 10/11] adds support for using order 1 pages with a 4K buffer. [patch 11/11] modifies messages prints with dev_info() instead of pr_info(). Change log: V1->V2: fixes comments from Saeed Mahameed and removes previous [patch 11/11] which needs further discussion, adds a new patch [11/11] suggested by Saeed Mahameed. Guangbin Huang (1): net: hns3: add a check for get_reset_level Huazhong Tan (2): net: hns3: remove upgrade reset level when reset fail net: hns3: use dev_info() instead of pr_info() Jian Shen (1): net: hns3: add reset checking before set channels Yonglong Liu (2): net: hns3: fix mis-counting IRQ vector numbers issue net: hns3: adds debug messages to identify eth down cause Yufeng Mo (2): net: hns3: change GFP flag during lock period net: hns3: modify firmware version display format Yunsheng Lin (3): net: hns3: make hclge_service use delayed workqueue net: hns3: add interrupt affinity support for misc interrupt net: hns3: Add support for using order 1 pages with a 4K buffer drivers/net/ethernet/hisilicon/hns3/hnae3.h| 9 ++ drivers/net/ethernet/hisilicon/hns3/hns3_enet.c| 39 +- drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 15 ++- drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 41 +- .../net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c | 10 +- .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 14 +++ .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c| 137 - .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h| 7 +- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 10 +- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 15 ++- 10 files changed, 223 insertions(+), 74 deletions(-)
Re: [PATCH net-next 06/11] net: hns3: modify firmware version display format
On 2019/7/26 5:32, Saeed Mahameed wrote:
On Thu, 2019-07-25 at 10:34 +0800, tanhuazhong wrote:
On 2019/7/25 2:34, Saeed Mahameed wrote:
On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
From: Yufeng Mo
This patch modifies firmware version display format in
hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
some optimizations for firmware version display format.
Signed-off-by: Yufeng Mo
Signed-off-by: Peng Li
Signed-off-by: Huazhong Tan
---
drivers/net/ethernet/hisilicon/hns3/hnae3.h | 9
+
drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 15
+--
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c | 10
+-
drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11
+--
4 files changed, 40 insertions(+), 5 deletions(-)
[...]
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
@@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
}
hdev->fw_version = version;
- dev_info(>pdev->dev, "The firmware version is %08x\n",
version);
+ pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE3_MASK,
+HNAE3_FW_VERSION_BYTE3_SHIFT),
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE2_MASK,
+HNAE3_FW_VERSION_BYTE2_SHIFT),
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE1_MASK,
+HNAE3_FW_VERSION_BYTE1_SHIFT),
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE0_MASK,
+HNAE3_FW_VERSION_BYTE0_SHIFT));
Device name/string will not be printed now, what happens if i have
multiple devices ? at least print the device name as it was before
Since on each board we only have one firmware, the firmware
version is same per device, and will not change when running.
So pr_info_once() looks good for this case.
boards change too often to have such static assumption.
Ok, I will use dev_info instead of pr_info here.
BTW, maybe we should change below print in the end of
hclge_init_ae_dev(), use dev_info() instead of pr_info(),
then we can know that which device has already initialized.
I will send other patch to do that, is it acceptable for you?
"pr_info("%s driver initialization finished.\n", HCLGE_DRIVER_NAME);"
I would avoid using pr_info when i can ! if you have the option to
print with dev information as it was before that is preferable.
Thanks,
Saeed.
Thanks,
Huazhong.
Re: [PATCH net-next 06/11] net: hns3: modify firmware version display format
On 2019/7/25 2:34, Saeed Mahameed wrote:
On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
From: Yufeng Mo
This patch modifies firmware version display format in
hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
some optimizations for firmware version display format.
Signed-off-by: Yufeng Mo
Signed-off-by: Peng Li
Signed-off-by: Huazhong Tan
---
drivers/net/ethernet/hisilicon/hns3/hnae3.h | 9
+
drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 15
+--
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c | 10
+-
drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11
+--
4 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index 48c7b70..a4624db 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -179,6 +179,15 @@ struct hnae3_vector_info {
#define HNAE3_RING_GL_RX 0
#define HNAE3_RING_GL_TX 1
+#define HNAE3_FW_VERSION_BYTE3_SHIFT 24
+#define HNAE3_FW_VERSION_BYTE3_MASKGENMASK(31, 24)
+#define HNAE3_FW_VERSION_BYTE2_SHIFT 16
+#define HNAE3_FW_VERSION_BYTE2_MASKGENMASK(23, 16)
+#define HNAE3_FW_VERSION_BYTE1_SHIFT 8
+#define HNAE3_FW_VERSION_BYTE1_MASKGENMASK(15, 8)
+#define HNAE3_FW_VERSION_BYTE0_SHIFT 0
+#define HNAE3_FW_VERSION_BYTE0_MASKGENMASK(7, 0)
+
struct hnae3_ring_chain_node {
struct hnae3_ring_chain_node *next;
u32 tqp_index;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 5bff98a..e71c92b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -527,6 +527,7 @@ static void hns3_get_drvinfo(struct net_device
*netdev,
{
struct hns3_nic_priv *priv = netdev_priv(netdev);
struct hnae3_handle *h = priv->ae_handle;
+ u32 fw_version;
if (!h->ae_algo->ops->get_fw_version) {
netdev_err(netdev, "could not get fw version!\n");
@@ -545,8 +546,18 @@ static void hns3_get_drvinfo(struct net_device
*netdev,
sizeof(drvinfo->bus_info));
drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
- snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
"0x%08x",
-priv->ae_handle->ae_algo->ops->get_fw_version(h));
+ fw_version = priv->ae_handle->ae_algo->ops->get_fw_version(h);
+
+ snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
+"%lu.%lu.%lu.%lu",
+hnae3_get_field(fw_version,
HNAE3_FW_VERSION_BYTE3_MASK,
+HNAE3_FW_VERSION_BYTE3_SHIFT),
+hnae3_get_field(fw_version,
HNAE3_FW_VERSION_BYTE2_MASK,
+HNAE3_FW_VERSION_BYTE2_SHIFT),
+hnae3_get_field(fw_version,
HNAE3_FW_VERSION_BYTE1_MASK,
+HNAE3_FW_VERSION_BYTE1_SHIFT),
+hnae3_get_field(fw_version,
HNAE3_FW_VERSION_BYTE0_MASK,
+HNAE3_FW_VERSION_BYTE0_SHIFT));
}
static u32 hns3_get_link(struct net_device *netdev)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
index 22f6acd..c2320bf 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
@@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
}
hdev->fw_version = version;
- dev_info(>pdev->dev, "The firmware version is %08x\n",
version);
+ pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE3_MASK,
+HNAE3_FW_VERSION_BYTE3_SHIFT),
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE2_MASK,
+HNAE3_FW_VERSION_BYTE2_SHIFT),
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE1_MASK,
+HNAE3_FW_VERSION_BYTE1_SHIFT),
+hnae3_get_field(version,
HNAE3_FW_VERSION_BYTE0_MASK,
+HNAE3_FW_VERSION_BYTE0_SHIFT));
Device name/string will not be printed now, what happens if i have
multiple devices ? at least print the device name as it was before
Since on each board we only have one firmware, the firmware
version is same per device, and will not change when running.
So pr_info_once() looks good for this case.
BTW, maybe we should change below print in the end of
hclge_init_ae_dev(), use dev_info() instead of pr_info(),
then we can know that which device has already initialized.
I will send other patch to do that, is it acceptable for you?
"pr_info("%s driver initialization finished.\n", HCLGE_DRIVER_NAME);"
Thanks.
Re: [PATCH net-next 04/11] net: hns3: fix mis-counting IRQ vector numbers issue
On 2019/7/25 2:28, Saeed Mahameed wrote:
On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
From: Yonglong Liu
The num_msi_left means the vector numbers of NIC, but if the
PF supported RoCE, it contains the vector numbers of NIC and
RoCE(Not expected).
This may cause interrupts lost in some case, because of the
NIC module used the vector resources which belongs to RoCE.
This patch corrects the value of num_msi_left to be equals to
the vector numbers of NIC, and adjust the default tqp numbers
according to the value of num_msi_left.
Fixes: 46a3df9f9718 ("net: hns3: Add HNS3 Acceleration Engine &
Compatibility Layer Support")
Signed-off-by: Yonglong Liu
Signed-off-by: Peng Li
Signed-off-by: Huazhong Tan
---
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 5 -
drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 12
++--
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 3c64d70..a59d13f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1470,13 +1470,16 @@ static int hclge_vport_setup(struct
hclge_vport *vport, u16 num_tqps)
{
struct hnae3_handle *nic = >nic;
struct hclge_dev *hdev = vport->back;
+ u16 alloc_tqps;
int ret;
nic->pdev = hdev->pdev;
nic->ae_algo = _algo;
nic->numa_node_mask = hdev->numa_node_mask;
- ret = hclge_knic_setup(vport, num_tqps,
+ alloc_tqps = min_t(u16, hdev->roce_base_msix_offset - 1,
Why do you need the extra alloc_tqps ? just overwrite num_tqps, the
original value is not needed afterwards.
Yes, using num_tqps is better.
I will remove the extra alloc_tqps in V2.
Thanks.
num_tqps);
+
+ ret = hclge_knic_setup(vport, alloc_tqps,
hdev->num_tx_desc, hdev->num_rx_desc);
if (ret)
dev_err(>pdev->dev, "knic setup failed %d\n",
ret);
Re: [PATCH net-next 00/11] net: hns3: some code optimizations & bugfixes
On 2019/6/26 23:59, David Miller wrote: From: tanhuazhong Date: Wed, 26 Jun 2019 15:44:05 +0800 Hi, david, has this patchset merged into net-next, why I cannot see it after pulling net-next? Or is there some problem about this patchset I have missed? Sorry, I forgot to push it out from my laptop while traveling. It should be there now. Thanks:) .
Re: [PATCH net-next 00/11] net: hns3: some code optimizations & bugfixes
On 2019/6/22 21:53, David Miller wrote: From: Huazhong Tan Date: Thu, 20 Jun 2019 16:52:34 +0800 This patch-set includes code optimizations and bugfixes for the HNS3 ethernet controller driver. [patch 1/11] fixes a selftest issue when doing autoneg. [patch 2/11 - 3-11] adds two code optimizations about VLAN issue. [patch 4/11] restores the MAC autoneg state after reset. [patch 5/11 - 8/11] adds some code optimizations and bugfixes about HW errors handling. [patch 9/11 - 11/11] fixes some issues related to driver loading and unloading. Series applied, thanks. Hi, david, has this patchset merged into net-next, why I cannot see it after pulling net-next? Or is there some problem about this patchset I have missed? .
Re: [PATCH net-next 01/12] net: hns3: log detail error info of ROCEE ECC and AXI errors
On 2019/6/7 1:36, David Miller wrote:
From: Huazhong Tan
Date: Thu, 6 Jun 2019 16:20:56 +0800
+static int hclge_log_rocee_axi_error(struct hclge_dev *hdev)
+{
...
+ ret = hclge_cmd_send(>hw, [0], 3);
+ if (ret) {
+ dev_err(dev, "failed(%d) to query ROCEE AXI error sts\n", ret);
+ return ret;
+ }
...
+ ret = hclge_log_rocee_axi_error(hdev);
+ if (ret) {
+ dev_err(dev, "failed(%d) to process axi error\n", ret);
You log the error twice which is unnecessary.
ok, thanks.
.
Re: [PATCH net-next 00/12] code optimizations & bugfixes for HNS3 driver
On 2019/6/1 8:18, David Miller wrote: From: David Miller Date: Fri, 31 May 2019 17:15:29 -0700 (PDT) From: Huazhong Tan Date: Fri, 31 May 2019 16:54:46 +0800 This patch-set includes code optimizations and bugfixes for the HNS3 ethernet controller driver. [patch 1/12] removes the redundant core reset type [patch 2/12 - 3/12] fixes two VLAN related issues [patch 4/12] fixes a TM issue [patch 5/12 - 12/12] includes some patches related to RAS & MSI-X error Series applied. I reverted, you need to actually build test the infiniband side of your driver. drivers/infiniband/hw/hns/hns_roce_hw_v2.c: In function ‘hns_roce_v2_msix_interrupt_abn’: drivers/infiniband/hw/hns/hns_roce_hw_v2.c:5032:14: warning: passing argument 2 of ‘ops->set_default_reset_request’ makes pointer from integer without a cast [-Wint-conversion] HNAE3_FUNC_RESET); ^~~~ drivers/infiniband/hw/hns/hns_roce_hw_v2.c:5032:14: note: expected ‘long unsigned int *’ but argument is of type ‘int’ C-c C-cmake[5]: *** Deleting file 'drivers/net/wireless/ath/carl9170/cmd.o' Sorry, I will remove [10/12 - 11/12] for V2, these two patches needs to modify HNS's infiniband driver at the same time, so they will be upstreamed later with the infiniband's one.
Re: [PATCH net-next 00/12] code optimizations & bugfixes for HNS3 driver
Sorry, please ignore this patchset. I will send V2 to fix something else. On 2019/4/24 11:21, Huazhong Tan wrote: This patch-set includes code optimizations and bugfixes for the HNS3 ethernet controller driver. [patch 1/12 - 3/12] fixes some bugs about the IO path [patch 4/12 - 6/12] includes some optimization and bugfixes about mailbox message handling [patch 7/12 - 12/12] adds misc code optimizations and bugfixes. Huazhong Tan (7): net: hns3: stop sending keep alive msg when VF command queue needs reinit net: hns3: use atomic_t replace u32 for arq's count net: hns3: use a reserved byte to identify need_resp flag net: hns3: not reset TQP in the DOWN while VF resetting net: hns3: stop schedule reset service while unloading driver net: hns3: fix pause configure fail problem net: hns3: prevent double free in hns3_put_ring_config() Weihang Li (1): net: hns3: remove reset after command send failed Yunsheng Lin (3): net: hns3: fix data race between ring->next_to_clean net: hns3: fix for TX clean num when cleaning TX BD net: hns3: handle the BD info on the last BD of the packet liuzhongzhu (1): net: hns3: extend the loopback state acquisition time drivers/net/ethernet/hisilicon/hns3/hclge_mbx.h| 7 ++- drivers/net/ethernet/hisilicon/hns3/hns3_enet.c| 65 +- drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 7 ++- .../net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c | 10 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c| 8 ++- .../net/ethernet/hisilicon/hns3/hns3pf/hclge_mbx.c | 7 +-- .../net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c | 5 +- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 2 +- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 12 ++-- .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h | 1 + .../ethernet/hisilicon/hns3/hns3vf/hclgevf_mbx.c | 9 ++- 11 files changed, 76 insertions(+), 57 deletions(-)
Re: [PATCH net] net: hns3: add rmb() for rx description
On 2019/3/4 13:15, David Miller wrote:
From: Huazhong Tan
Date: Sat, 2 Mar 2019 16:49:30 +0800
From: Jian Shen
HW can not guarantee complete write desc->rx.size, even though
HNS3_RXD_VLD_B has been set. Driver needs to add rmb() instruction
to make sure desc->rx.size is always valid.
Fixes: e55970950556 ("net: hns3: Add handling of GRO Pkts not fully RX'ed in NAPI
poll")
Signed-off-by: Jian Shen
Signed-off-by: Huazhong Tan
dma_rmb() is more appropriate here and more efficient.
Thanks, will send V2 to fix it.
.
Re: linux-next: Fixes tag needs some work in the net-next tree
On 2019/2/27 10:23, Stephen Rothwell wrote: Hi, On Wed, 27 Feb 2019 09:12:57 +0800 tanhuazhong wrote: It is my mistake, so sorry about this. There is a redundant "net: hns3: " in the fixes tag. How could I fix it? Since Dave doesn't rebase his tree, there is no way to fix it (and it is not that important any in this case), just use this as a learning experience for next time :-) Thanks :)
Re: linux-next: Fixes tag needs some work in the net-next tree
Hi Stephen & David,
It is my mistake, so sorry about this. There is a redundant "net: hns3:
" in the fixes tag.
How could I fix it?
Thanks.
On 2019/2/25 16:00, Stephen Rothwell wrote:
Hi all,
In commit
a638b1d8cc87 ("net: hns3: fix get VF RSS issue")
Fixes tag
Fixes: 374ad291762a ("net: hns3: net: hns3: Add RSS general configuration support
for VF")
has these problem(s):
- Subject does not match target commit subject
Just use
git log -1 --format='Fixes: %h (%s)'
RE: [PATCH net-next 02/12] net: hns3: add rx multicast packets statistic
-Original Message-
From: Eric Dumazet [mailto:eric.duma...@gmail.com]
Sent: Wednesday, January 23, 2019 2:39 AM
To: tanhuazhong ; da...@davemloft.net
Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; huangdaode
; Zhuangyuzeng (Yisen) ;
Salil Mehta ; Linuxarm ; shenjian
(K) ; lipeng (Y)
Subject: Re: [PATCH net-next 02/12] net: hns3: add rx multicast packets
statistic
On 01/22/2019 08:39 AM, Huazhong Tan wrote:
> From: Jian Shen
>
> This patch adds rx multicast packets statistic for each ring.
>
> Signed-off-by: Jian Shen
> Signed-off-by: Peng Li
> Signed-off-by: Huazhong Tan
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c| 9 +
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h| 8
> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 1 +
> 3 files changed, 18 insertions(+)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 9dd8949381bc..f1ab2e4ca49e 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -2572,6 +2572,7 @@ static int hns3_handle_rx_bd(struct hns3_enet_ring
> *ring,
>struct sk_buff **out_skb)
> {
> struct net_device *netdev = ring->tqp->handle->kinfo.netdev;
> + enum hns3_pkt_l2t_type l2_frame_type;
> struct sk_buff *skb = ring->skb;
> struct hns3_desc_cb *desc_cb;
> struct hns3_desc *desc;
> @@ -2680,6 +2681,14 @@ static int hns3_handle_rx_bd(struct hns3_enet_ring
> *ring,
> return -EFAULT;
> }
>
> + l2_frame_type = hnae3_get_field(l234info, HNS3_RXD_DMAC_M,
> + HNS3_RXD_DMAC_S);
> + if (l2_frame_type == HNS3_L2_TYPE_MULTICAST) {
> + u64_stats_update_begin(>syncp);
> + ring->stats.rx_multicast++;
> + u64_stats_update_end(>syncp);
> + }
> +
> u64_stats_update_begin(>syncp);
This is a bit suboptimal to call u64_stats_update_begin() twice.
You could rewrite the thing to be :
u64_stats_update_begin(>syncp);
if (l2_frame_type == HNS3_L2_TYPE_MULTICAST)
ring->stats.rx_multicast++;
ring->stats.rx_pkts++;
...
Ok, thanks.
> ring->stats.rx_pkts++;
> ring->stats.rx_bytes += skb->len;
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> index f59ab7387b1f..f3d248626ab3 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> @@ -202,6 +202,13 @@ enum hns3_nic_state {
>
> #define HNS3_RING_EN_B 0
>
> +enum hns3_pkt_l2t_type {
> + HNS3_L2_TYPE_UNICAST,
> + HNS3_L2_TYPE_MULTICAST,
> + HNS3_L2_TYPE_BROADCAST,
> + HNS3_L2_TYPE_INVALID,
> +};
> +
> enum hns3_pkt_l3t_type {
> HNS3_L3T_NONE,
> HNS3_L3T_IPV6,
> @@ -376,6 +383,7 @@ struct ring_stats {
> u64 err_bd_num;
> u64 l2_err;
> u64 l3l4_csum_err;
> + u64 rx_multicast;
> };
> };
> };
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index e678b6939da3..abb78696d7ce 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -47,6 +47,7 @@ static const struct hns3_stats hns3_rxq_stats[] = {
> HNS3_TQP_STAT("err_bd_num", err_bd_num),
> HNS3_TQP_STAT("l2_err", l2_err),
> HNS3_TQP_STAT("l3l4_csum_err", l3l4_csum_err),
> + HNS3_TQP_STAT("multicast", rx_multicast),
> };
>
> #define HNS3_RXQ_STATS_COUNT ARRAY_SIZE(hns3_rxq_stats)
>
