| Message ID | 20250414-jk-supported-perout-flags-v2-0-f6b17d15475c@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: ptp: driver opt-in for supported PTP ioctl flags | expand |
On Mon, 14 Apr 2025 14:26:29 -0700 Jacob Keller <jacob.e.keller@intel.com> wrote: > Both the PTP_EXTTS_REQUEST(2) and PTP_PEROUT_REQUEST(2) ioctls take flags > from userspace to modify their behavior. Drivers are supposed to check > these flags, rejecting requests for flags they do not support. > > Many drivers today do not check these flags, despite many attempts to > squash individual drivers as these mistakes are discovered. Additionally, > any new flags added can require updating every driver if their validation > checks are poorly implemented. > > It is clear that driver authors will not reliably check for unsupported > flags. The root of the issue is that drivers must essentially opt out of > every flag, rather than opt in to the ones they support. > > Instead, lets introduce .supported_perout_flags and .supported_extts_flags > to the ptp_clock_info structure. This is a pattern taken from several > ethtool ioctls which enabled validation to move out of the drivers and into > the shared ioctl handlers. This pattern has worked quite well and makes it > much more difficult for drivers to accidentally accept flags they do not > support. > > With this approach, drivers which do not set the supported fields will have > the core automatically reject any request which has flags. Drivers must opt > in to each flag they support by adding it to the list, with the sole > exception being the PTP_ENABLE_FEATURE flag of the PTP_EXTTS_REQUEST ioctl > since it is entirely handled by the ptp_chardev.c file. > > This change will ensure that all current and future drivers are safe for > extension when we need to extend these ioctls. > > I opted to keep all the driver changes into one patch per ioctl type. The > changes are relatively small and straight forward. Splitting it per-driver > would make the series large, and also break flags between the introduction > of the supported field and setting it in each driver. > > The non-Intel drivers are compile-tested only, and I would appreciate > confirmation and testing from their respective maintainers. (It is also > likely that I missed some of the driver authors especially for drivers > which didn't make any checks at all and do not set either of the supported > flags yet) > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com> Thank you!
On Mon, 14 Apr 2025 14:26:29 -0700 Jacob Keller wrote: > Both the PTP_EXTTS_REQUEST(2) and PTP_PEROUT_REQUEST(2) ioctls take flags > from userspace to modify their behavior. Drivers are supposed to check > these flags, rejecting requests for flags they do not support. Applied, thanks!
Both the PTP_EXTTS_REQUEST(2) and PTP_PEROUT_REQUEST(2) ioctls take flags from userspace to modify their behavior. Drivers are supposed to check these flags, rejecting requests for flags they do not support. Many drivers today do not check these flags, despite many attempts to squash individual drivers as these mistakes are discovered. Additionally, any new flags added can require updating every driver if their validation checks are poorly implemented. It is clear that driver authors will not reliably check for unsupported flags. The root of the issue is that drivers must essentially opt out of every flag, rather than opt in to the ones they support. Instead, lets introduce .supported_perout_flags and .supported_extts_flags to the ptp_clock_info structure. This is a pattern taken from several ethtool ioctls which enabled validation to move out of the drivers and into the shared ioctl handlers. This pattern has worked quite well and makes it much more difficult for drivers to accidentally accept flags they do not support. With this approach, drivers which do not set the supported fields will have the core automatically reject any request which has flags. Drivers must opt in to each flag they support by adding it to the list, with the sole exception being the PTP_ENABLE_FEATURE flag of the PTP_EXTTS_REQUEST ioctl since it is entirely handled by the ptp_chardev.c file. This change will ensure that all current and future drivers are safe for extension when we need to extend these ioctls. I opted to keep all the driver changes into one patch per ioctl type. The changes are relatively small and straight forward. Splitting it per-driver would make the series large, and also break flags between the introduction of the supported field and setting it in each driver. The non-Intel drivers are compile-tested only, and I would appreciate confirmation and testing from their respective maintainers. (It is also likely that I missed some of the driver authors especially for drivers which didn't make any checks at all and do not set either of the supported flags yet) Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- Changes in v2: - Expand PTP_EXTTS_EDGES in all .supported_extts_flags assignment - Remove PTP_ENABLE_FEATURE from all .supported_extts_flags assignment - Add a .supported_extts_flags assignment to ravb driver, even tho it doesn't currently support PTP_STRICT_FLAGS. The driver did previously check these, so I think its better to add them even if its equivalent. - Added Vadim's Reviewed-by to patch 2/2 - Link to v1: https://lore.kernel.org/r/20250408-jk-supported-perout-flags-v1-0-d2f8e3df64f3@intel.com --- Jacob Keller (2): net: ptp: introduce .supported_extts_flags to ptp_clock_info net: ptp: introduce .supported_perout_flags to ptp_clock_info include/linux/ptp_clock_kernel.h | 18 +++++++++++++++ drivers/net/dsa/mv88e6xxx/ptp.c | 11 ++++----- drivers/net/dsa/sja1105/sja1105_ptp.c | 14 +++--------- drivers/net/ethernet/intel/ice/ice_ptp.c | 16 +++++-------- drivers/net/ethernet/intel/igb/igb_ptp.c | 20 +++++------------ drivers/net/ethernet/intel/igc/igc_ptp.c | 14 +++--------- .../net/ethernet/mellanox/mlx5/core/lib/clock.c | 26 ++++++---------------- drivers/net/ethernet/microchip/lan743x_ptp.c | 14 ++++-------- .../net/ethernet/microchip/lan966x/lan966x_ptp.c | 14 ++++-------- drivers/net/ethernet/mscc/ocelot_ptp.c | 5 ----- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 2 ++ drivers/net/ethernet/renesas/ravb_ptp.c | 11 +-------- drivers/net/phy/dp83640.c | 13 +++-------- drivers/net/phy/micrel.c | 17 +++++--------- drivers/net/phy/microchip_rds_ptp.c | 5 +---- drivers/net/phy/nxp-c45-tja11xx.c | 13 ++++------- drivers/ptp/ptp_chardev.c | 16 ++++++++++++- drivers/ptp/ptp_clockmatrix.c | 14 ++---------- drivers/ptp/ptp_fc3.c | 1 + drivers/ptp/ptp_idt82p33.c | 15 +++---------- 20 files changed, 91 insertions(+), 168 deletions(-) --- base-commit: b65999e7238e6f2a48dc77c8c2109c48318ff41b change-id: 20250408-jk-supported-perout-flags-43219dcee093 Best regards,