Re:Re: [PATCH net v2] net: macb: rate limit netdev error info print in the data path

[taozj888 <taozj888@163.com>]

Hello Theo, Nicolai:

Please see answer inline.

Thanks,

Zijin Tao




At 2026-10-02 20:08:03, "Théo Lebrun" <theo.lebrun@bootlin.com> wrote:
>Hello Nicolai,
>
>On Fri Oct 2, 2026 at 12:10 PM CEST, Nicolai Buchwitz wrote:
>> On 30.9.2026 13:35, taozj888@163.com wrote:
>>> From: taozijin <taozj888@163.com>
>>> 
>>> Now the MACB ethernet driver print the netdev error information
>>> directly by netdev_err(), which would lead to a large number of
>>> error information print if there was a significant number of
>>> error or just jumbo packets exceeding the MTU received when booting.
>>> For example, it would print a large number of:
>>> 
>>> macb PHYT0036:00 eth0: not whole frame pointed by descriptor
>>> macb PHYT0036:00 eth0: not whole frame pointed by descriptor
>>> ...
>>
>> Out of interest: Is this a Phytium vendor kernel?
>
>I looked in the past so I'll answer: yes!
>
>Internet search gives one worthy result: a 2025 series to DPDK adding
>MACB support. Some extracts about the exact string match and also how
>they support ACPI or OF and no other platform apparently.
>
>#define OF_PHYTIUM_GEM1P0_MAC	"cdns,phytium-gem-1.0"	/* Phytium 1.0 MAC */
>#define OF_PHYTIUM_GEM2P0_MAC	"cdns,phytium-gem-2.0"	/* Phytium 2.0 MAC */
>#define ACPI_PHYTIUM_GEM1P0_MAC	"PHYT0036"	/* Phytium 1.0  MAC */
>
>static int macb_get_dev_type(struct rte_eth_dev *dev)
>{
>	// ...
>
>	if (!strcmp(dev_type, OF_PHYTIUM_GEM1P0_MAC) ||
>	    !strcmp(dev_type, ACPI_PHYTIUM_GEM1P0_MAC)) {
>		priv->dev_type = DEV_TYPE_PHYTIUM_GEM1P0_MAC;
>	} else if (!strcmp(dev_type, OF_PHYTIUM_GEM2P0_MAC)) {
>		priv->dev_type = DEV_TYPE_PHYTIUM_GEM2P0_MAC;
>	} else {
>		MACB_LOG(ERR, "Unsupported device type: %s.", dev_type);
>		ret = -EINVAL;
>	}
>
>	// ...
>}
>
>@Zijin: do you have any other Linux MACB patches for it to work?

No, I have no other patches for Linux MACB except the current one.


>
>>> in gem_rx() by received a large number of packets without
>>> RX_EOF flag set, especially with unknown packet
>>> type that would penetrate the hardware offload for the IP packets.
>>> 
>>> The unlimited prints here would greatly bother and delay
>>> the system booting process unless the source stop sending
>>> packets since they occupy the console output bandwidth and
>>> other processes have to wait for the completion of printing those
>>> messages.
>>> 
>>> So rate limit the netdev error information print in the receive
>>> and transmit data path.
>>> 
>>> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
>>
>> IMHO the correct tag is 4df95131ea80 ("net/macb: change RX path for 
>> GEM")?
>> At least the gem_rx() messages were introduced here.
>
>The patch used to touch macb_start_xmit(), which explains why I told
>Zijin to target the initial commit on previous revision.
>
OK, I will still use the initial commit indicated by Theo.

>@Zijin: where is the V2 changelog? Each new revision must list (below
>the fold line for single patches) their exhaustive list of changes
>compared to the previous revision.

Sorry for missing the V2 changelog. I will add it in the V3 patch.

>
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#respond-to-review-comments
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
>
>>> Cc: stable@vger.kernel.org
>>> 
>>
>> Drop the blank line as otherwise tooling might get confused and doesn't
>> get all tags correctly.

OK, I will take care of it in the V3 patch.

>>
>>> Signed-off-by: Zijin Tao <taozj888@163.com>
>>> ---
>>>  drivers/net/ethernet/cadence/macb_main.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>>> b/drivers/net/ethernet/cadence/macb_main.c
>>> index 8e5c034dc3a4..ca60960bec36 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -1617,16 +1617,16 @@ static int gem_rx(struct macb_queue *queue, 
>>> struct napi_struct *napi,
>>>  		count++;
>>> 
>>>  		if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF))) {
>>> -			netdev_err(bp->netdev,
>>> -				   "not whole frame pointed by descriptor\n");
>>> +			if (net_ratelimit())
>>> +				netdev_err(bp->netdev, "not whole frame pointed by descriptor\n");
>>
>> This will just hide the error message, but the split/drop is still 
>> present.
>> How about limiting JML in macb_init_hw() properly?
>>
>>          if ((bp->caps & MACB_CAPS_JUMBO) && bp->jumbo_max_len) {
>>                  u32 jml = bp->rx_buffer_size - NET_IP_ALIGN + 
>> ETH_FCS_LEN;
>>                  gem_writel(bp, JML, min(jml, bp->jumbo_max_len));
>>          }
>>
>> The code above is untested, so probably needs further tweaking. An 
>> alternative could
>> be to handle the split frames in gem_rx() correctly.
>
>I agree with you but to clarify for Zijin: if you do this then it should
>be a separate patch as changes are pretty unrelated.

Yes, with your experience in this area, maybe a separate patch for it is preferred.
I think the error reported is not just related the Jumbo frame, but the jumbo frame
would trigger the error. So rate limit printing this kind of msg is needed but not
totally hide those msgs.  


>
>Thanks,
>
>--
>Théo Lebrun, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com