Re: [net-next v2 2/2] net: reject ptp requests with unsupported flags

[Richard Cochran <richardcochran@gmail.com>]

On Wed, Sep 25, 2019 at 07:28:20PM -0700, Jacob Keller wrote:
> This patch may not be correct for individual drivers, especially
> regarding the rising vs falling edge flags. I interpreted the default
> behavior to be to timestamp the rising edge of a pin transition.

So I think this patch goes too far.  It breaks the implied ABI.
 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index fd3071f55bd3..2867a2581a36 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -521,6 +521,10 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
>  
>  	switch (rq->type) {
>  	case PTP_CLK_REQ_EXTTS:
> +		/* Reject requests with unsupported flags */
> +		if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE))
> +			return -EOPNOTSUPP;

This HW always time stamps both edges, and that is not configurable.
Here you reject PTP_FALLING_EDGE, and that is clearly wrong.  If the
driver had been really picky (my fault I guess), it should have always
insisted on (PTP_RISING_EDGE | PTP_FALLING_EDGE) being set together.
But it is too late to enforce that now, because it could break user
space programs.

I do agree with the sentiment of checking the flags at the driver
level, but this needs to be done case by case, with the drivers'
author's input.

(The req.perout.flags can be done unconditionally in all drivers,
since there were never any valid flags, but req.extts.flags needs
careful attention.)

Thanks,
Richard