RE: [PATCH net-next v10 05/11] net: ethernet: oa_tc6: Support for hardware timestamp

[Selvamani Rajagopal]

> -----Original Message-----
> From: netdev-bot+sashiko@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx>
> Sent: Saturday, November 7, 2026 4:16 PM
> To: Selvamani Rajagopal <Selvamani.Rajagopal@xxxxxxxxxx>
> Subject: Re: [PATCH net-next v10 05/11] net: ethernet: oa_tc6: Support for hardware
> timestamp
> 
> 
> 
> [Severity: Medium]
> What releases a slot if its capture event never arrives?
> 


Many transactions depend on prompt delivery of interrupts. As long as
there is no bug that caused the missed interrupt, we should be ok. 
I don't want to go for timeout without evidence of any issue.

> 
> [ ... ]
> 
> 
> [Severity: Medium]
> Can an old capture be delivered to the wrong skb after this?
> 
> Take a TX_ON to TX_OFF to TX_ON toggle. oa_tc6_set_hwtstamp_settings()

This is technically possible. I consider this as extreme corner case. 
If application is running, when timestamp support is flipped,  application may 
get wrong data. I don't consider this as worth fixing it.


> > + ret = oa_tc6_update_standard_capability(tc6);
> > + if (ret) {
> > + dev_err(&tc6->spi->dev, "Failed to read capability\n");
> > + goto phy_exit;
> > + }
> > +
> 
> [Severity: High]
> Can this goto dereference a NULL phydev? 

Yes. It is an issue. Will fix it in the next submission.


> > diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
> b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
> > new file mode 100644
> > index 0000000000000..7d21c0608ee80
> > --- /dev/null
> > +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
> 


> [Severity: Medium]
> What happens to INT_MASK0 if this CONFIG0 write fails? INT_MASK0 has
> already been written and nothing restores it. tc6->ts_config and
> hw_tstamp_enabled are also left as they were.


While question is legitimate, if read or write operation fails, it only means SPI
transaction fails, which is more serious issue than just having a misconfigured
hardware. So, not attempting to fix this.

> 
> [ ... ]
> 
> > +void oa_tc6_get_ts_stats(struct oa_tc6 *tc6,
> > + struct ethtool_ts_stats *stats)
> > +{
> > + stats->pkts = tc6->tx_hwtstamp_pkts;
> > + stats->err = tc6->tx_hwtstamp_err;
> > + stats->lost = tc6->tx_hwtstamp_lost;
> > +}
> 


> > +int oa_tc6_get_ts_info(struct oa_tc6 *tc6,
> > + struct kernel_ethtool_ts_info *info)
> > +{
> > + if (!tc6->ptp_clock)
> > + return ethtool_op_get_ts_info(tc6->netdev, info);
> 
> [Severity: Medium]
> Does this fallback advertise software TX timestamps that the driver never
> produces? 


Yes. It does. Fixing it.


> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-
> mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com
> <https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com
> netdev-ai.bots.linux.dev>