Re: [PATCH net] octeontx2-pf: Fix ethtool timestamping capabilities for SDP representors

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-pf: Fix ethtool timestamping capabilities for SDP representors

The patch makes otx2_get_ts_info() fall back to
ethtool_op_get_ts_info() for SDP representors, so ethtool -T reports
software timestamping only and phc_index = -1 for those interfaces.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> index a0340f3422bf9..f7a2202c4a3e0 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> @@ -978,7 +978,7 @@ static int otx2_get_ts_info(struct net_device *netdev,
>  {
>  	struct otx2_nic *pfvf = netdev_priv(netdev);
>  
> -	if (!pfvf->ptp)
> +	if (!pfvf->ptp || is_otx2_sdp_rep(pfvf->pdev))
>  		return ethtool_op_get_ts_info(netdev, info);

[Severity: Medium]
Accepting hardware timestamping on SDP representors isn't a bug
introduced by this patch, but should the configuration path be closed
too, so the advertised capabilities and the set path agree?

otx2_config_hwtstamp_set() in otx2_pf.c is registered as
.ndo_hwtstamp_set in otx2vf_netdev_ops, which SDP representors use
(PCI_DEVID_OCTEONTX2_SDP_REP is in otx2_vf_id_table), and its only
guard is the pointer check:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c:otx2_config_hwtstamp_set() {
	struct otx2_nic *pfvf = netdev_priv(netdev);

	if (!pfvf->ptp)
		return -ENODEV;

	switch (config->tx_type) {
	...
}

pfvf->ptp is non-NULL for SDP representors, since otx2vf_probe() calls
otx2_ptp_init(vf) unconditionally and otx2_ptp_init() only clears the
pointer for LBK VFs:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c:otx2_ptp_init() {
	if (is_otx2_lbkvf(pfvf->pdev)) {
		pfvf->ptp = NULL;
		return 0;
	}
	...
}

The SIOCSHWTSTAMP path only range-checks the request in
net_hwtstamp_validate() and never consults get_ts_info, so
HWTSTAMP_TX_ON / HWTSTAMP_FILTER_ALL is still accepted on an interface
that ethtool -T now reports as software-only, and
otx2_config_hwtstamp_get() reports that hardware mode back from
pfvf->tstamp.

On the TX side, does anything actually get programmed? The AF handler
rvu_mbox_handler_nix_lf_ptp_tx_enable() -> rvu_nix_lf_ptp_tx_cfg()
returns success without writing NIX_AF_LFX_TX_CFG_PTP_EN for a
non-CGX-mapped (SDP) PF:

drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:rvu_nix_lf_ptp_tx_cfg() {
	if (!is_mac_feature_supported(rvu, pf, RVU_LMAC_FEAT_PTP))
		return 0;
	...
}

So OTX2_FLAG_TX_TSTAMP_ENABLED gets set and otx2_set_txtstamp() starts
marking skbs SKBTX_IN_PROGRESS and appending the
NIX_SENDMEMALG_E_SETTSTMP MEM subdesc while the LF was never enabled
for PTP.

The RX half does not appear to be affected: rvu_mbox_handler_cgx_ptp_rx_enable()
returns -EPERM for a non-CGX-mapped PF, so OTX2_FLAG_RX_TSTAMP_ENABLED
stays clear.

Would it be simpler to mirror the existing LBK handling in
otx2_ptp_init() and also set pfvf->ptp = NULL for is_otx2_sdp_rep()?
That covers otx2_get_ts_info(), otx2_config_hwtstamp_set() and avoids
registering a PHC that the patch now hides with phc_index = -1 while it
remains reachable as /dev/ptpN.

Cross-instance finding from sashiko-gemini (5b35a8e8292d6d592200e0103dea956967dd21185968d0440290af6d0ae99293):
[Severity: Medium]
Incomplete fix for SDP representor hardware timestamping allows ioctl configuration and useless PTP clock registration.
-- 
pw-bot: cr