Re: [PATCH V2 net-next 0/6] net: hns3: enhance tc flow offload support

[Jijie Shao <shaojijie@huawei.com>]

on 2026/5/28 17:06, Paolo Abeni wrote:
> On 5/27/26 3:12 PM, Jijie Shao wrote:
>> on 2026/5/27 17:59, Simon Horman wrote:
>>> On Sat, May 23, 2026 at 06:54:43PM +0800, Jijie Shao wrote:
>>>> This patchset enhances the tc flow offload support for hns3 driver:
>>>>
>>>> - Patch 1: Refactor hclge_add_cls_flower() to support more actions
>>>> - Patch 2: Improve unused_tuple parameter setting for separate src/dst configuration
>>>> - Patch 3: Add support for HCLGE_FD_ACTION_SELECT_QUEUE and HCLGE_FD_ACTION_DROP_PACKET actions
>>>> - Patch 4: Add support for FLOW_DISSECTOR_KEY_IP and FLOW_DISSECTOR_KEY_ENC_KEYID dissectors
>>>> - Patch 5: Add debugfs support for dumping FD rules
>>>> - Patch 6: Move FD code to a separate file (hclge_fd.c) for better code organization
>>>>
>>>> ---
>>>> ChangeLog:
>>>> v1 -> v2:
>>>>     - Fixed warnings similar to "warning: restricted __le32 degrades to integer",
>>>>       pointed out by Jakub
>>>>     - Fixed warnings related to
>>>>       "warning: diagnostic behavior may be improved by adding the 'format(printf, 5, 7)'",
>>>>       pointed out by Jakub
>>>>     v1: https://lore.kernel.org/all/20260518093526.1109595-1-shaojijie@huawei.com/
>>> Hi Juijie,
>>>
>>> There are AI-generated reviews of this patch-set available at
>>> https://netdev-ai.bots.linux.dev/ and https://sashiko.dev/
>> Hi,
>>
>> I'm sorry, I don't understand how to use https://netdev-ai.bots.linux.dev/, it seems to require an API Token?
>> I find it difficult to locate my own patch-set in Recent Reviews.
> The nipa sashiko instance has the same web UI as sashiko.dev. The full
> URL for the former is:
>
> https://netdev-ai.bots.linux.dev/sashiko/
>
> You can lookup your series by the cover letter title or you can use the
> direct link including the msg id for the cover:
>
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260523105449.661848-1-shaojijie%40huawei.com

Thanks,

It appears that the two URLs mentioned by Simon Horman are the results of the one AI-generated reviews.


>
>> Regarding https://sashiko.dev/, I checked and some of the issues are pre-existing issues,
>> not caused by the new features added in this patch-set.
>>
>> Some others are related to the solution, which is the same as the ethtool -U  already supported by the driver.
>> I suggest keeping it consistent with ethtool -U for now.
> I'm not sure which comment do you refer above.
>
> This one:
>
> ---
>> +		if (is_zero_ether_addr(match.key->dst))
>> +			rule->unused_tuple |= BIT(INNER_DST_MAC);
>> +		if (is_zero_ether_addr(match.key->src))
>> +			rule->unused_tuple |= BIT(INNER_SRC_MAC);
> Should the "is this tuple unused" decision be driven by the mask
> rather than the key?
> A tc-flower rule that explicitly matches an all-zero address, for
> example:
>      tc filter add ... flower dst_mac 00:00:00:00:00:00 ...
>      tc filter add ... flower src_ip 0.0.0.0 ...
>      tc filter add ... flower src_ip :: ...
> produces match.key->{src,dst} == 0 with match.mask->{src,dst} set to
> all-ones. With the new checks above, those rules now have
> INNER_{SRC,DST}_MAC / INNER_{SRC,DST}_IP added to rule->unused_tuple
> even though the user asked to match exactly that address.
> Downstream, hclge_fd_convert_tuple() short-circuits on unused_tuple:
>      if (rule->unused_tuple & BIT(tuple_bit))
>          return true;
> so the tuple is not programmed into the TCAM key and the field is
> effectively wildcarded in hardware, with no -EINVAL or extack returned
> to the user.
> Real-world patterns that rely on all-zero addresses include DHCPv4
> DISCOVER (src 0.0.0.0), IPv6 DAD/NUD probes and DHCPv6 (src ::), and
> deliberate matches on the all-zero MAC.
> ---
>
> and similar later ones look real/relevant to me, and IMHO should be
> addressed.

Yes, I saw this issue. There is similar logic in the existing function hclge_fd_check_ether_tuple():

static int hclge_fd_check_ether_tuple(struct ethhdr *spec, u32 *unused_tuple)
{
	if (!spec || !unused_tuple)
		return -EINVAL;

	*unused_tuple |= BIT(INNER_SRC_IP) | BIT(INNER_DST_IP) |
		BIT(INNER_SRC_PORT) | BIT(INNER_DST_PORT) |
		BIT(INNER_IP_TOS) | BIT(INNER_IP_PROTO);

	if (is_zero_ether_addr(spec->h_source))
		*unused_tuple |= BIT(INNER_SRC_MAC);

	if (is_zero_ether_addr(spec->h_dest))
		*unused_tuple |= BIT(INNER_DST_MAC);

	if (!spec->h_proto)
		*unused_tuple |= BIT(INNER_ETH_TYPE);

	return 0;
}

This looks like a common problem for hns3. My original idea was to send a separate bugfix later to fix all the all-zero MAC or IP addresses.
The current tc flow implementation logic is consistent with ethtool -U.

Alternatively, I can first fix the tc flow issue in v4, and then submit a bugfix later to fix the potential issues in ethtool -U.

Thanks,
Jijie Shao.