Re: [PATCH net-next v4 2/3] net: ti: icssm-prueth: Add priority based RX IRQ handlers

[Parvathi Pudi]

Hi,

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at:
> https://sashiko.dev/#/patchset/20260921134343.200426-4-parvathi@xxxxxxxxxxx?part=2
> ---
> Subject: net: ti: icssm-prueth: Add priority based RX IRQ handlers
> 
> This commit adds support for priority-based interrupt handling for the STP/RSTP
> Switch, HSR, and PRP protocols to address port dominance. It records an IEP
> hardware timestamp into the receive buffer to guarantee frames are delivered in
> wire-arrival order. The patch also converts statistics counters to atomic64_t
> and adds a firmware revision field to support different timestamp wraparound
> behaviors.
> 
>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>> b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>> index 6c8888d9d66a6..4a37935b8d352 100644
>> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> [ ... ]
>> @@ -1071,11 +1083,25 @@ static int icssm_emac_ndo_open(struct net_device *ndev)
>>  			goto iep_exit;
>>  	}
>>  
>> -	ret = icssm_emac_request_irqs(emac);
>> -	if (ret)
>> -		goto rproc_shutdown;
>> +	if (PRUETH_IS_EMAC(prueth)) {
>> +		napi_enable(&emac->napi);
>> +	} else {
>> +		if (!prueth->emac_configured &&
>> +		    (PRUETH_IS_SWITCH(prueth) || prueth_is_lre(prueth))) {
>> +			napi_enable(&prueth->napi_hpq);
>> +			napi_enable(&prueth->napi_lpq);
> 
> [Severity: High]
> Does this code risk a NULL pointer dereference?
> 
> If MII0 is missing (e.g., absent from the device tree), the shared NAPI
> structures napi_hpq and napi_lpq are not initialized in
> icssm_prueth_netdev_init(). If the MII1 interface is later placed into SWITCH
> mode and brought up, it appears icssm_emac_ndo_open() will unconditionally call
> napi_enable() on these zero-initialized structures, causing a NULL pointer
> dereference on n->dev.
> 

No, this condition will never occur. The shared NAPIs are used only in SWITCH/HSR/PRP
modes, and the driver switches to those modes only when both MII0 and MII1 are members
of the same bridge/HSR device. If MII0 is absent from the DT, its netdev is never
created, so it can never join a bridge/HSR device and the both-ports condition is never
met. The device therefore always stays in EMAC mode, and icssm_emac_ndo_open() enables
only the per-port emac->napi, which is always initialized.

>> +		}
>> +	}
> [ ... ]
>> @@ -1780,9 +1820,25 @@ static int icssm_prueth_netdev_init(struct prueth
>> *prueth,
>>  
>>  	netif_napi_add(ndev, &emac->napi, icssm_emac_napi_poll);
>>  
>> +	if ((prueth->support_lre || fw_data->support_switch) &&
>> +	    emac->port_id == PRUETH_PORT_MII0) {
>> +		netif_napi_add(ndev, &prueth->napi_hpq,
>> +			       icssm_prueth_common_napi_poll_hpq);
>> +		netif_napi_add(ndev, &prueth->napi_lpq,
>> +			       icssm_prueth_common_napi_poll_lpq);
>> +	}
>> +
>>  	hrtimer_setup(&emac->tx_hrtimer, &icssm_emac_tx_timer_callback,
>>  		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
>>  
>> +	if ((prueth->support_lre || fw_data->support_switch) &&
>> +	    emac->port_id == PRUETH_PORT_MII0) {
>> +		prueth->hp->ndev = ndev;
>> +		prueth->hp->priority = 0;
>> +		prueth->lp->ndev = ndev;
>> +		prueth->lp->priority = 1;
>> +	}
>> +
>>  	return 0;
> [ ... ]
>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
>> b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
>> new file mode 100644
>> index 0000000000000..9b7b876240b5f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> [ ... ]
>> +static irqreturn_t icssm_prueth_common_emac_rx_hardirq(int irq, void *dev_id)
>> +{
>> +	struct prueth_ndev_priority *ndev_prio;
>> +	struct prueth_emac *emac;
>> +	struct net_device *ndev;
>> +	struct prueth *prueth;
>> +
>> +	ndev_prio = (struct prueth_ndev_priority *)dev_id;
>> +	ndev = ndev_prio->ndev;
> 
> [Severity: High]
> Similarly, can this IRQ handler dereference an uninitialized pointer?
> 
> If MII0 is missing and these priority structures were skipped during
> initialization, it looks like the registered IRQ handler will dereference the
> zero-initialized ndev_prio->ndev here when an interrupt occurs.
> 

Same as above. This handler is registered only by icssm_prueth_common_request_irqs(),
which is called only in SWITCH/HSR/PRP modes. In EMAC mode, each port uses its own
RX IRQ and handler through icssm_emac_request_irqs(). Since the device always
stays in EMAC mode when MII0 is absent, this handler is never registered and never
runs with a zeroed ndev_prio→ndev.

>> +	emac = netdev_priv(ndev);
>> +	prueth = emac->prueth;
> 
> [Severity: Medium]
> This is a pre-existing issue, but can the transmission path read out of bounds?
> 
> In drivers/net/ethernet/ti/icssm/icssm_prueth.c:icssm_prueth_get_tx_queue_id():
> 
>    err = vlan_get_tag(skb, &vlan_tci);
> 
> icssm_emac_ndo_start_xmit() calls icssm_prueth_get_tx_queue_id() before
> validating the length of the transmitted skb or padding it. Because the device
> lacks NETIF_F_HW_VLAN_CTAG_TX, vlan_get_tag() falls back to __vlan_get_tag(),
> which reads the VLAN TCI at offset 14. If a raw socket transmits a packet
> smaller than 18 bytes (e.g., exactly 14 bytes) with h_vlan_proto ==
> ETH_P_8021Q, this reads unallocated memory past the buffer.


As this is a pre-existing issue related to the TX path VLAN tag check in
icssm_prueth_get_tx_queue_id(), we will address it separately in a different series.

Thanks and Regards,
Parvathi.