Re: [PATCH net-next v3 1/6] net: xilinx: axienet: Introduce axienet_config for MAC-specific ops

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 9/1/2026 8:39 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Incomplete abstraction for TX checksum offload leads to broken checksums if misconfigured.
> 
> Pre-existing issues:
> - [Critical] TX ring desynchronization in axienet_start_xmit_dmaengine() error paths leads to use-after-free and double-free in DMA callbacks.
> --
> 
> commit 166384442bde8d7169bb6c9fd071e535fa3f9c73
> Author: Suraj Gupta <suraj.gupta2@amd.com>
> 
> net: xilinx: axienet: Introduce axienet_config for MAC-specific ops
> 
> Introduce an axienet_config structure to abstract MAC-type-specific
> operations, preparing the driver for supporting additional MAC types
> beyond the existing 1G Ethernet.
> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index 1722b7038f340..a4963da53b306 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> [ ... ]
>> @@ -939,19 +976,21 @@ axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device *ndev)
>>   		goto xmit_error_drop_skb;
>>   
>>   	/* Fill up app fields for checksum */
>> -	if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> -		if (lp->features & XAE_FEATURE_FULL_TX_CSUM) {
>> -			/* Tx Full Checksum Offload Enabled */
>> -			app_metadata[0] |= 2;
>> -		} else if (lp->features & XAE_FEATURE_PARTIAL_TX_CSUM) {
>> -			csum_start_off = skb_transport_offset(skb);
>> -			csum_index_off = csum_start_off + skb->csum_offset;
>> -			/* Tx Partial Checksum Offload Enabled */
>> -			app_metadata[0] |= 1;
>> -			app_metadata[1] = (csum_start_off << 16) | csum_index_off;
>> +	if (lp->axienet_config->dma_tx_csum) {
>> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this error path permanently
> desynchronize the software ring from the hardware ring?
> 
> Looking at axienet_start_xmit_dmaengine(), if a failure occurs during
> mapping or preparation, the code jumps to xmit_error_drop_skb. However,
> lp->tx_ring_head was already incremented earlier and is not rolled back.
> This leaves the abandoned ring slot containing the skb pointer and sg_len
> from the previous time the slot was used.
> 
> When the next successful packet finishes transmission, axienet_dma_tx_cb()
> will pop the oldest entry using lp->tx_ring_tail++ and retrieve this
> abandoned slot. Can this lead to unmapping the wrong scatterlist and
> double-freeing the old skb?
> 



This is a pre-existing issue. I'll address it separately.

> [ ... ]
>> @@ -2861,9 +2993,6 @@ static int axienet_probe(struct platform_device *pdev)
>>   	/* Setup checksum offload, but default to off if not specified */
>>   	lp->features = 0;
>>   
>> -	if (axienet_ior(lp, XAE_ABILITY_OFFSET) & XAE_ABILITY_STATS)
>> -		lp->features |= XAE_FEATURE_STATS;
>> -
>>   	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,txcsum", &value);
>>   	if (!ret) {
>>   		switch (value) {
>>   		case 1:
>>   			lp->features |= XAE_FEATURE_PARTIAL_TX_CSUM;
>>   			/* Can checksum any contiguous range */
>>   			ndev->features |= NETIF_F_HW_CSUM;
> 
> [Severity: High]
> Does axienet_probe() advertise hardware checksum capabilities without
> verifying the new MAC capability flag?
> 
> While axienet_start_xmit_dmaengine() now correctly gates the DMA metadata
> setup behind lp->axienet_config->dma_tx_csum, axienet_probe() still parses
> the xlnx,txcsum device tree property and unconditionally sets
> NETIF_F_HW_CSUM.
> 
> If the device tree specifies this property for a MAC that does not support
> TX checksums, will the network stack pass partial checksums that the
> hardware never completes?
> 

I agree, axienet_probe() should verify MAC capability flag before 
checking for checksum capabilities, otherwise it might be problematic if 
device tree wrongly specifies this property for unsupported MAC. I'll 
address this in next version.

Thanks,
Suraj