>-----Original Message-----
>From: Jakub Kicinski <k...@kernel.org>
>Sent: Tuesday, November 17, 2020 2:08 AM
>To: Nguyen, Anthony L <anthony.l.ngu...@intel.com>
>Cc: da...@davemloft.net; Kwapulinski, Piotr <piotr.kwapulin...@intel.com>;
>netdev@vger.kernel.org; sassm...@redhat.com; Loktionov, Aleksandr
><aleksandr.loktio...@intel.com>; Kubalewski, Arkadiusz
><arkadiusz.kubalew...@intel.com>; Andrew Bowers <andrewx.bow...@intel.com>;
>Richard Cochran <richardcoch...@gmail.com>; Vladimir Oltean <olte...@gmail.com>
>Subject: Re: [net-next 1/4] i40e: add support for PTP external synchronization
>clock
>
>On Fri, 13 Nov 2020 16:10:54 -0800 Tony Nguyen wrote:
>> From: Piotr Kwapulinski <piotr.kwapulin...@intel.com>
>>
>> Add support for external synchronization clock via GPIOs.
>> 1PPS signals are handled via the dedicated 3 GPIOs: SDP3_2,
>> SDP3_3 and GPIO_4.
>> Previously it was not possible to use the external PTP synchronization
>> clock.
>
>Please _always_ CC Richard on PTP changes.
Sure
>
>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
>> b/drivers/net/ethernet/intel/i40e/i40e.h
>> index 537300e762f0..8f5eecbff3d6 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
>> @@ -196,6 +196,11 @@ enum i40e_fd_stat_idx { #define
>> I40E_FD_ATR_TUNNEL_STAT_IDX(pf_id) \
>> (I40E_FD_STAT_PF_IDX(pf_id) + I40E_FD_STAT_ATR_TUNNEL)
>>
>> +/* get PTP pins for ioctl */
>> +#define SIOCGPINS (SIOCDEVPRIVATE + 0)
>> +/* set PTP pins for ioctl */
>> +#define SIOCSPINS (SIOCDEVPRIVATE + 1)
>
>This is unexpected.. is it really normal to declare private device IOCTLs to
>configure PPS pins? Or are you just exposing this so you're able to play with
>GPIOs from user space?
Right, this should not go upstream.
>
>> /* The following structure contains the data parsed from the user-defined
>> * field of the ethtool_rx_flow_spec structure.
>> */
>> @@ -344,7 +349,6 @@ struct i40e_ddp_old_profile_list {
>> I40E_FLEX_SET_FSIZE(fsize) | \
>> I40E_FLEX_SET_SRC_WORD(src))
>>
>> -
>
>Please move all the empty line removal to a separate patch.
>
>> #define I40E_MAX_FLEX_SRC_OFFSET 0x1F
>>
>> /* macros related to GLQF_ORT */
>
>> @@ -2692,7 +2692,15 @@ int i40e_ioctl(struct net_device *netdev, struct
>> ifreq *ifr, int cmd)
>> case SIOCGHWTSTAMP:
>> return i40e_ptp_get_ts_config(pf, ifr);
>> case SIOCSHWTSTAMP:
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EACCES;
>
>If you needed this, this should be a fix for net. But you don't, core checks
>it.
>
>> return i40e_ptp_set_ts_config(pf, ifr);
>> + case SIOCSPINS:
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EACCES;
>> + return i40e_ptp_set_pins_ioctl(pf, ifr);
>> + case SIOCGPINS:
>> + return i40e_ptp_get_pins(pf, ifr);
>> default:
>> return -EOPNOTSUPP;
>> }
>
I'll provide and updated patch.
Thank you for review.
