Re: [PATCH net-next 05/12] ionic: add hw timestamp support files

[Shannon Nelson <snelson@pensando.io>]

On 4/4/21 4:05 PM, Richard Cochran wrote:
> On Thu, Apr 01, 2021 at 10:56:03AM -0700, Shannon Nelson wrote:
>> @@ -0,0 +1,589 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2017 - 2021 Pensando Systems, Inc */
>> +
>> +#include <linux/netdevice.h>
>> +#include <linux/etherdevice.h>
>> +
>> +#include "ionic.h"
>> +#include "ionic_bus.h"
>> +#include "ionic_lif.h"
>> +#include "ionic_ethtool.h"
>> +
>> +static int ionic_hwstamp_tx_mode(int config_tx_type)
>> +{
>> +	switch (config_tx_type) {
>> +	case HWTSTAMP_TX_OFF:
>> +		return IONIC_TXSTAMP_OFF;
>> +	case HWTSTAMP_TX_ON:
>> +		return IONIC_TXSTAMP_ON;
>> +	case HWTSTAMP_TX_ONESTEP_SYNC:
>> +		return IONIC_TXSTAMP_ONESTEP_SYNC;
>> +#ifdef HAVE_HWSTAMP_TX_ONESTEP_P2P
>> +	case HWTSTAMP_TX_ONESTEP_P2P:
>> +		return IONIC_TXSTAMP_ONESTEP_P2P;
>> +#endif
> This ifdef is not needed.  (I guess you have to support older kernel
> versions, but my understanding of the policy is that new code
> shouldn't carry such stuff).

Yep, good catch - that's a carry over from our out-of-tree driver. I'll 
follow up with a patch to remove that bit of cruft.

>
>> +	default:
>> +		return -ERANGE;
>> +	}
>> +}
>
>> +int ionic_lif_hwstamp_set(struct ionic_lif *lif, struct ifreq *ifr)
>> +{
>> +	struct ionic *ionic = lif->ionic;
>> +	struct hwtstamp_config config;
>> +	int tx_mode = 0;
>> +	u64 rx_filt = 0;
>> +	int err, err2;
>> +	bool rx_all;
>> +	__le64 mask;
>> +
>> +	if (!lif->phc || !lif->phc->ptp)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (ifr) {
>> +		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
>> +			return -EFAULT;
>> +	} else {
>> +		/* if called with ifr == NULL, behave as if called with the
>> +		 * current ts_config from the initial cleared state.
>> +		 */
> This check is unneeded, because the ioctl layer never passes NULL here.

Yes, the ioctl layer never calls this with NULL, but we call it from 
within the driver when we spin operations back up after a FW reset.

Thanks,
sln

>
>> +		memcpy(&config, &lif->phc->ts_config, sizeof(config));
>> +		memset(&lif->phc->ts_config, 0, sizeof(config));
>> +	}
> Thanks,
> Richard