Re: [PATCH net-next v2 8/8] net: mscc: PTP Hardware Clock (PHC) support

[Antoine Tenart <antoine.tenart@bootlin.com>]

Hello Jakub,

On Fri, Jul 05, 2019 at 03:10:38PM -0700, Jakub Kicinski wrote:
> On Fri,  5 Jul 2019 21:52:13 +0200, Antoine Tenart wrote:
> > @@ -596,11 +606,50 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  	dev->stats.tx_packets++;
> >  	dev->stats.tx_bytes += skb->len;
> > -	dev_kfree_skb_any(skb);
> > +
> > +	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
> > +	    port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> > +		struct ocelot_skb *oskb =
> > +			kzalloc(sizeof(struct ocelot_skb), GFP_KERNEL);
> 
> I think this is the TX path, you can't use GFP_KERNEL here.

I'll fix it.

> > +static int ocelot_hwstamp_set(struct ocelot_port *port, struct ifreq *ifr)
> > +{
> > +	struct ocelot *ocelot = port->ocelot;
> > +	struct hwtstamp_config cfg;
> > +
> > +	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > +		return -EFAULT;
> > +
> > +	/* reserved for future extensions */
> > +	if (cfg.flags)
> > +		return -EINVAL;
> > +
> > +	/* Tx type sanity check */
> > +	switch (cfg.tx_type) {
> > +	case HWTSTAMP_TX_ON:
> > +		port->ptp_cmd = IFH_REW_OP_TWO_STEP_PTP;
> > +		break;
> > +	case HWTSTAMP_TX_ONESTEP_SYNC:
> > +		/* IFH_REW_OP_ONE_STEP_PTP updates the correctional field, we
> > +		 * need to update the origin time.
> > +		 */
> > +		port->ptp_cmd = IFH_REW_OP_ORIGIN_PTP;
> > +		break;
> > +	case HWTSTAMP_TX_OFF:
> > +		port->ptp_cmd = 0;
> > +		break;
> > +	default:
> > +		return -ERANGE;
> > +	}
> > +
> > +	mutex_lock(&ocelot->ptp_lock);
> > +
> > +	switch (cfg.rx_filter) {
> > +	case HWTSTAMP_FILTER_NONE:
> > +		break;
> > +	case HWTSTAMP_FILTER_ALL:
> > +	case HWTSTAMP_FILTER_SOME:
> > +	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > +	case HWTSTAMP_FILTER_NTP_ALL:
> > +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > +		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> > +		break;
> > +	default:
> > +		mutex_unlock(&ocelot->ptp_lock);
> > +		return -ERANGE;
> > +	}
> 
> No device reconfig, so the PTP RX stamping is always enabled?  Perhaps
> consider setting 
> 
> 	ocelot->hwtstamp_config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT
> 
> at probe?

That's right. Would set the ptp flag to 0 also be an option here (so
that we respect HWTSTAMP_FILTER_NONE at least in the driver)?

> > +	/* Commit back the result & save it */
> > +	memcpy(&ocelot->hwtstamp_config, &cfg, sizeof(cfg));
> > +	mutex_unlock(&ocelot->ptp_lock);
> > +
> > +	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> > +}
> >  
> > +static int ocelot_get_ts_info(struct net_device *dev,
> > +			      struct ethtool_ts_info *info)
> > +{
> > +	struct ocelot_port *ocelot_port = netdev_priv(dev);
> > +	struct ocelot *ocelot = ocelot_port->ocelot;
> > +	int ret;
> > +
> > +	if (!ocelot->ptp)
> > +		return -EOPNOTSUPP;
> 
> Hmm.. why does software timestamping depend on PTP?

Because it depends on the "PTP" register bank (and the "PTP" interrupt)
being described and available. This is why I named the flag 'ptp', but
it could be named 'timestamp' or 'ts' as well.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com