Re: [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support

["Karumanchi, Vineeth" <vineeth@amd.com>]

Hi Simon,

On 7/24/2025 12:35 AM, Simon Horman wrote:

<...>

>> @@ -5345,7 +5349,7 @@ static const struct macb_config sama7g5_emac_config = {
>>   static const struct macb_config versal_config = {
>>   	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>>   		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK |
>> -		MACB_CAPS_QUEUE_DISABLE,
>> +		MACB_CAPS_QUEUE_DISABLE, MACB_CAPS_QBV,
> Hi Vineeth,
> 
> TL;DR: I think you mean
> 
> 		MACB_CAPS_QUEUE_DISABLE | MACB_CAPS_QBV,
> 		                       ^^^
> 

Yes, since there's no strict validation for the presence of 
NETIF_F_HW_TC, the tc add/replace command succeeded. I've submitted an 
RFC patch to enforce stricter checks for NETIF_F_HW_TC within 
dev->hw_features

> I assume that the intention here is to set the MACB_CAPS_QBV bit of .caps.
> However, because there is a comma rather than a pipe between
> it and MACB_CAPS_QUEUE_DISABLE the effect is to leave .caps as
> it was before, and set .dma_burst_length to MACB_CAPS_QBV.
> .dma_burst_length is then overwritten on the following line.
> 
> Flagged by W=1 builds with Clang 20.1.8 and 15.1.0.
> 
> Please build your patches with W=1 and try to avoid adding warnings
> it flags.
> 
> Also, while we are here, it would be nice to fix up the line wrapping so
> the adjacent code is 80 columns wide or less, as is still preferred in
> Networking code.
> 
> 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> 		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> 		MACB_CAPS_NEED_TSUCLK | MACB_CAPS_QUEUE_DISABLE |
> 		MACB_CAPS_QBV,
> 

OK.

Thanks,
-- 
🙏 vineeth