[patch 00/13] ptp: Belated spring cleaning of the chardev driver

[patch 00/13] ptp: Belated spring cleaning of the chardev driver

[Thomas Gleixner]

When looking into supporting auxiliary clocks in the PTP ioctl, the
inpenetrable ptp_ioctl() letter soup bothered me enough to clean it up.

The code (~400 lines!) is really hard to follow due to a gazillion of
local variables, which are only used in certain case scopes, and a
mixture of gotos, breaks and direct error return paths.

Clean it up by splitting out the IOCTL functionality into seperate
functions, which contain only the required local variables and are trivial
to follow. Complete the cleanup by converting the code to lock guards and
_free(), which gets rid of all gotos.

That reduces the code size by 58 lines and also the binary text size is
90 bytes smaller than the current maze.

The series is split up into one patch per IOCTL command group for easy
review.

Applies against v6.16-rc1 and also cleanly against next. It's also
available from git:

     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git timers/ptp/driver

Thanks,

	tglx
---
 ptp_chardev.c |  754 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 348 insertions(+), 406 deletions(-)

[patch 01/13] ptp: Split out PTP_CLOCK_GETCAPS ioctl code

[Thomas Gleixner]

ptp_ioctl() is an inpenetrable letter soup with a gazillion of case (scope)
specific variables defined at the top of the function and pointless breaks
and gotos.

Start cleaning it up by splitting out the PTP_CLOCK_GETCAPS ioctl code into
a helper function. Use a argument pointer with a single sparse compliant
type cast instead of proliferating the type cast all over the place.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -157,6 +157,26 @@ int ptp_release(struct posix_clock_conte
 	return 0;
 }
 
+static long ptp_clock_getcaps(struct ptp_clock *ptp, void __user *arg)
+{
+	struct ptp_clock_caps caps = {
+		.max_adj		= ptp->info->max_adj,
+		.n_alarm		= ptp->info->n_alarm,
+		.n_ext_ts		= ptp->info->n_ext_ts,
+		.n_per_out		= ptp->info->n_per_out,
+		.pps			= ptp->info->pps,
+		.n_pins			= ptp->info->n_pins,
+		.cross_timestamping	= ptp->info->getcrosststamp != NULL,
+		.adjust_phase		= ptp->info->adjphase != NULL &&
+					  ptp->info->getmaxphase != NULL,
+	};
+
+	if (caps.adjust_phase)
+		caps.max_phase_adj = ptp->info->getmaxphase(ptp->info);
+
+	return copy_to_user(arg, &caps, sizeof(caps)) ? -EFAULT : 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
@@ -171,37 +191,22 @@ long ptp_ioctl(struct posix_clock_contex
 	struct timestamp_event_queue *tsevq;
 	struct ptp_system_timestamp sts;
 	struct ptp_clock_request req;
-	struct ptp_clock_caps caps;
 	struct ptp_clock_time *pct;
 	struct ptp_pin_desc pd;
 	struct timespec64 ts;
 	int enable, err = 0;
+	void __user *argptr;
 
 	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
 		arg = (unsigned long)compat_ptr(arg);
+	argptr = (void __force __user *)arg;
 
 	tsevq = pccontext->private_clkdata;
 
 	switch (cmd) {
-
 	case PTP_CLOCK_GETCAPS:
 	case PTP_CLOCK_GETCAPS2:
-		memset(&caps, 0, sizeof(caps));
-
-		caps.max_adj = ptp->info->max_adj;
-		caps.n_alarm = ptp->info->n_alarm;
-		caps.n_ext_ts = ptp->info->n_ext_ts;
-		caps.n_per_out = ptp->info->n_per_out;
-		caps.pps = ptp->info->pps;
-		caps.n_pins = ptp->info->n_pins;
-		caps.cross_timestamping = ptp->info->getcrosststamp != NULL;
-		caps.adjust_phase = ptp->info->adjphase != NULL &&
-				    ptp->info->getmaxphase != NULL;
-		if (caps.adjust_phase)
-			caps.max_phase_adj = ptp->info->getmaxphase(ptp->info);
-		if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
-			err = -EFAULT;
-		break;
+		return ptp_clock_getcaps(ptp, argptr);
 
 	case PTP_EXTTS_REQUEST:
 	case PTP_EXTTS_REQUEST2:

Re: [patch 01/13] ptp: Split out PTP_CLOCK_GETCAPS ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> ptp_ioctl() is an inpenetrable letter soup with a gazillion of case (scope)
> specific variables defined at the top of the function and pointless breaks
> and gotos.
> 
> Start cleaning it up by splitting out the PTP_CLOCK_GETCAPS ioctl code into
> a helper function. Use a argument pointer with a single sparse compliant
> type cast instead of proliferating the type cast all over the place.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   41 +++++++++++++++++++++++------------------
>   1 file changed, 23 insertions(+), 18 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -157,6 +157,26 @@ int ptp_release(struct posix_clock_conte
>   	return 0;
>   }
>   
> +static long ptp_clock_getcaps(struct ptp_clock *ptp, void __user *arg)
> +{
> +	struct ptp_clock_caps caps = {
> +		.max_adj		= ptp->info->max_adj,
> +		.n_alarm		= ptp->info->n_alarm,
> +		.n_ext_ts		= ptp->info->n_ext_ts,
> +		.n_per_out		= ptp->info->n_per_out,
> +		.pps			= ptp->info->pps,
> +		.n_pins			= ptp->info->n_pins,
> +		.cross_timestamping	= ptp->info->getcrosststamp != NULL,
> +		.adjust_phase		= ptp->info->adjphase != NULL &&
> +					  ptp->info->getmaxphase != NULL,
> +	};
> +
> +	if (caps.adjust_phase)
> +		caps.max_phase_adj = ptp->info->getmaxphase(ptp->info);
> +
> +	return copy_to_user(arg, &caps, sizeof(caps)) ? -EFAULT : 0;
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
> @@ -171,37 +191,22 @@ long ptp_ioctl(struct posix_clock_contex
>   	struct timestamp_event_queue *tsevq;
>   	struct ptp_system_timestamp sts;
>   	struct ptp_clock_request req;
> -	struct ptp_clock_caps caps;
>   	struct ptp_clock_time *pct;
>   	struct ptp_pin_desc pd;
>   	struct timespec64 ts;
>   	int enable, err = 0;
> +	void __user *argptr;
>   
>   	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
>   		arg = (unsigned long)compat_ptr(arg);
> +	argptr = (void __force __user *)arg;
>   
>   	tsevq = pccontext->private_clkdata;
>   
>   	switch (cmd) {
> -
>   	case PTP_CLOCK_GETCAPS:
>   	case PTP_CLOCK_GETCAPS2:
> -		memset(&caps, 0, sizeof(caps));
> -
> -		caps.max_adj = ptp->info->max_adj;
> -		caps.n_alarm = ptp->info->n_alarm;
> -		caps.n_ext_ts = ptp->info->n_ext_ts;
> -		caps.n_per_out = ptp->info->n_per_out;
> -		caps.pps = ptp->info->pps;
> -		caps.n_pins = ptp->info->n_pins;
> -		caps.cross_timestamping = ptp->info->getcrosststamp != NULL;
> -		caps.adjust_phase = ptp->info->adjphase != NULL &&
> -				    ptp->info->getmaxphase != NULL;
> -		if (caps.adjust_phase)
> -			caps.max_phase_adj = ptp->info->getmaxphase(ptp->info);
> -		if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
> -			err = -EFAULT;
> -		break;
> +		return ptp_clock_getcaps(ptp, argptr);
>   
>   	case PTP_EXTTS_REQUEST:
>   	case PTP_EXTTS_REQUEST2:
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 02/13] ptp: Split out PTP_EXTTS_REQUEST ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_EXTTS_REQUEST
ioctl code into a helper function. Convert to a lock guard while at it.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |  105 +++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 55 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -177,12 +177,57 @@ static long ptp_clock_getcaps(struct ptp
 	return copy_to_user(arg, &caps, sizeof(caps)) ? -EFAULT : 0;
 }
 
+static long ptp_extts_request(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
+{
+	struct ptp_clock_request req = { .type = PTP_CLK_REQ_EXTTS };
+	struct ptp_clock_info *ops = ptp->info;
+	unsigned int supported_extts_flags;
+
+	if (copy_from_user(&req.extts, arg, sizeof(req.extts)))
+		return -EFAULT;
+
+	if (cmd == PTP_EXTTS_REQUEST2) {
+		/* Tell the drivers to check the flags carefully. */
+		req.extts.flags |= PTP_STRICT_FLAGS;
+		/* Make sure no reserved bit is set. */
+		if ((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
+		    req.extts.rsv[0] || req.extts.rsv[1])
+			return -EINVAL;
+
+		/* Ensure one of the rising/falling edge bits is set. */
+		if ((req.extts.flags & PTP_ENABLE_FEATURE) &&
+		    (req.extts.flags & PTP_EXTTS_EDGES) == 0)
+			return -EINVAL;
+	} else {
+		req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
+		memset(req.extts.rsv, 0, sizeof(req.extts.rsv));
+	}
+
+	if (req.extts.index >= ops->n_ext_ts)
+		return -EINVAL;
+
+	supported_extts_flags = ptp->info->supported_extts_flags;
+	/* The PTP_ENABLE_FEATURE flag is always supported. */
+	supported_extts_flags |= PTP_ENABLE_FEATURE;
+	/* If the driver does not support strictly checking flags, the
+	 * PTP_RISING_EDGE and PTP_FALLING_EDGE flags are merely hints
+	 * which are not enforced.
+	 */
+	if (!(supported_extts_flags & PTP_STRICT_FLAGS))
+		supported_extts_flags |= PTP_EXTTS_EDGES;
+	/* Reject unsupported flags */
+	if (req.extts.flags & ~supported_extts_flags)
+		return -EOPNOTSUPP;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
+		return ops->enable(ops, &req, req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0);
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
-	unsigned int i, pin_index, supported_extts_flags;
 	struct ptp_sys_offset_extended *extoff = NULL;
 	struct ptp_sys_offset_precise precise_offset;
 	struct system_device_crosststamp xtstamp;
@@ -192,6 +237,7 @@ long ptp_ioctl(struct posix_clock_contex
 	struct ptp_system_timestamp sts;
 	struct ptp_clock_request req;
 	struct ptp_clock_time *pct;
+	unsigned int i, pin_index;
 	struct ptp_pin_desc pd;
 	struct timespec64 ts;
 	int enable, err = 0;
@@ -210,60 +256,9 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_EXTTS_REQUEST:
 	case PTP_EXTTS_REQUEST2:
-		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
-			err = -EACCES;
-			break;
-		}
-		memset(&req, 0, sizeof(req));
-
-		if (copy_from_user(&req.extts, (void __user *)arg,
-				   sizeof(req.extts))) {
-			err = -EFAULT;
-			break;
-		}
-		if (cmd == PTP_EXTTS_REQUEST2) {
-			/* Tell the drivers to check the flags carefully. */
-			req.extts.flags |= PTP_STRICT_FLAGS;
-			/* Make sure no reserved bit is set. */
-			if ((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
-			    req.extts.rsv[0] || req.extts.rsv[1]) {
-				err = -EINVAL;
-				break;
-			}
-			/* Ensure one of the rising/falling edge bits is set. */
-			if ((req.extts.flags & PTP_ENABLE_FEATURE) &&
-			    (req.extts.flags & PTP_EXTTS_EDGES) == 0) {
-				err = -EINVAL;
-				break;
-			}
-		} else if (cmd == PTP_EXTTS_REQUEST) {
-			req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
-			req.extts.rsv[0] = 0;
-			req.extts.rsv[1] = 0;
-		}
-		if (req.extts.index >= ops->n_ext_ts) {
-			err = -EINVAL;
-			break;
-		}
-		supported_extts_flags = ptp->info->supported_extts_flags;
-		/* The PTP_ENABLE_FEATURE flag is always supported. */
-		supported_extts_flags |= PTP_ENABLE_FEATURE;
-		/* If the driver does not support strictly checking flags, the
-		 * PTP_RISING_EDGE and PTP_FALLING_EDGE flags are merely
-		 * hints which are not enforced.
-		 */
-		if (!(supported_extts_flags & PTP_STRICT_FLAGS))
-			supported_extts_flags |= PTP_EXTTS_EDGES;
-		/* Reject unsupported flags */
-		if (req.extts.flags & ~supported_extts_flags)
-			return -EOPNOTSUPP;
-		req.type = PTP_CLK_REQ_EXTTS;
-		enable = req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0;
-		if (mutex_lock_interruptible(&ptp->pincfg_mux))
-			return -ERESTARTSYS;
-		err = ops->enable(ops, &req, enable);
-		mutex_unlock(&ptp->pincfg_mux);
-		break;
+		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
+			return -EACCES;
+		return ptp_extts_request(ptp, cmd, argptr);
 
 	case PTP_PEROUT_REQUEST:
 	case PTP_PEROUT_REQUEST2:

Re: [patch 02/13] ptp: Split out PTP_EXTTS_REQUEST ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_EXTTS_REQUEST
> ioctl code into a helper function. Convert to a lock guard while at it.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |  105 +++++++++++++++++++++-------------------------
>   1 file changed, 50 insertions(+), 55 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -177,12 +177,57 @@ static long ptp_clock_getcaps(struct ptp
>   	return copy_to_user(arg, &caps, sizeof(caps)) ? -EFAULT : 0;
>   }
>   
> +static long ptp_extts_request(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
> +{
> +	struct ptp_clock_request req = { .type = PTP_CLK_REQ_EXTTS };
> +	struct ptp_clock_info *ops = ptp->info;
> +	unsigned int supported_extts_flags;
> +
> +	if (copy_from_user(&req.extts, arg, sizeof(req.extts)))
> +		return -EFAULT;
> +
> +	if (cmd == PTP_EXTTS_REQUEST2) {
> +		/* Tell the drivers to check the flags carefully. */
> +		req.extts.flags |= PTP_STRICT_FLAGS;
> +		/* Make sure no reserved bit is set. */
> +		if ((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
> +		    req.extts.rsv[0] || req.extts.rsv[1])
> +			return -EINVAL;
> +
> +		/* Ensure one of the rising/falling edge bits is set. */
> +		if ((req.extts.flags & PTP_ENABLE_FEATURE) &&
> +		    (req.extts.flags & PTP_EXTTS_EDGES) == 0)
> +			return -EINVAL;
> +	} else {
> +		req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
> +		memset(req.extts.rsv, 0, sizeof(req.extts.rsv));
> +	}
> +
> +	if (req.extts.index >= ops->n_ext_ts)
> +		return -EINVAL;
> +
> +	supported_extts_flags = ptp->info->supported_extts_flags;
> +	/* The PTP_ENABLE_FEATURE flag is always supported. */
> +	supported_extts_flags |= PTP_ENABLE_FEATURE;
> +	/* If the driver does not support strictly checking flags, the
> +	 * PTP_RISING_EDGE and PTP_FALLING_EDGE flags are merely hints
> +	 * which are not enforced.
> +	 */
> +	if (!(supported_extts_flags & PTP_STRICT_FLAGS))
> +		supported_extts_flags |= PTP_EXTTS_EDGES;
> +	/* Reject unsupported flags */
> +	if (req.extts.flags & ~supported_extts_flags)
> +		return -EOPNOTSUPP;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
> +		return ops->enable(ops, &req, req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0);
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
>   	struct ptp_clock *ptp =
>   		container_of(pccontext->clk, struct ptp_clock, clock);
> -	unsigned int i, pin_index, supported_extts_flags;
>   	struct ptp_sys_offset_extended *extoff = NULL;
>   	struct ptp_sys_offset_precise precise_offset;
>   	struct system_device_crosststamp xtstamp;
> @@ -192,6 +237,7 @@ long ptp_ioctl(struct posix_clock_contex
>   	struct ptp_system_timestamp sts;
>   	struct ptp_clock_request req;
>   	struct ptp_clock_time *pct;
> +	unsigned int i, pin_index;
>   	struct ptp_pin_desc pd;
>   	struct timespec64 ts;
>   	int enable, err = 0;
> @@ -210,60 +256,9 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_EXTTS_REQUEST:
>   	case PTP_EXTTS_REQUEST2:
> -		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
> -			err = -EACCES;
> -			break;
> -		}
> -		memset(&req, 0, sizeof(req));
> -
> -		if (copy_from_user(&req.extts, (void __user *)arg,
> -				   sizeof(req.extts))) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		if (cmd == PTP_EXTTS_REQUEST2) {
> -			/* Tell the drivers to check the flags carefully. */
> -			req.extts.flags |= PTP_STRICT_FLAGS;
> -			/* Make sure no reserved bit is set. */
> -			if ((req.extts.flags & ~PTP_EXTTS_VALID_FLAGS) ||
> -			    req.extts.rsv[0] || req.extts.rsv[1]) {
> -				err = -EINVAL;
> -				break;
> -			}
> -			/* Ensure one of the rising/falling edge bits is set. */
> -			if ((req.extts.flags & PTP_ENABLE_FEATURE) &&
> -			    (req.extts.flags & PTP_EXTTS_EDGES) == 0) {
> -				err = -EINVAL;
> -				break;
> -			}
> -		} else if (cmd == PTP_EXTTS_REQUEST) {
> -			req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
> -			req.extts.rsv[0] = 0;
> -			req.extts.rsv[1] = 0;
> -		}
> -		if (req.extts.index >= ops->n_ext_ts) {
> -			err = -EINVAL;
> -			break;
> -		}
> -		supported_extts_flags = ptp->info->supported_extts_flags;
> -		/* The PTP_ENABLE_FEATURE flag is always supported. */
> -		supported_extts_flags |= PTP_ENABLE_FEATURE;
> -		/* If the driver does not support strictly checking flags, the
> -		 * PTP_RISING_EDGE and PTP_FALLING_EDGE flags are merely
> -		 * hints which are not enforced.
> -		 */
> -		if (!(supported_extts_flags & PTP_STRICT_FLAGS))
> -			supported_extts_flags |= PTP_EXTTS_EDGES;
> -		/* Reject unsupported flags */
> -		if (req.extts.flags & ~supported_extts_flags)
> -			return -EOPNOTSUPP;
> -		req.type = PTP_CLK_REQ_EXTTS;
> -		enable = req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0;
> -		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> -			return -ERESTARTSYS;
> -		err = ops->enable(ops, &req, enable);
> -		mutex_unlock(&ptp->pincfg_mux);
> -		break;
> +		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
> +			return -EACCES;
> +		return ptp_extts_request(ptp, cmd, argptr);
>   
>   	case PTP_PEROUT_REQUEST:
>   	case PTP_PEROUT_REQUEST2:
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 03/13] ptp: Split out PTP_PEROUT_REQUEST ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_PEROUT_REQUEST
ioctl code into a helper function. Convert to a lock guard while at it.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |  129 ++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 71 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -223,6 +223,61 @@ static long ptp_extts_request(struct ptp
 		return ops->enable(ops, &req, req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0);
 }
 
+static long ptp_perout_request(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
+{
+	struct ptp_clock_request req = { .type = PTP_CLK_REQ_PEROUT };
+	struct ptp_perout_request *perout = &req.perout;
+	struct ptp_clock_info *ops = ptp->info;
+
+	if (copy_from_user(perout, arg, sizeof(*perout)))
+		return -EFAULT;
+
+	if (cmd == PTP_PEROUT_REQUEST2) {
+		if (perout->flags & ~PTP_PEROUT_VALID_FLAGS)
+			return -EINVAL;
+
+		/*
+		 * The "on" field has undefined meaning if
+		 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat it
+		 * as reserved, which must be set to zero.
+		 */
+		if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
+		    !mem_is_zero(perout->rsv, sizeof(perout->rsv)))
+			return -EINVAL;
+
+		if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
+			/* The duty cycle must be subunitary. */
+			if (perout->on.sec > perout->period.sec ||
+			    (perout->on.sec == perout->period.sec &&
+			     perout->on.nsec > perout->period.nsec))
+				return -ERANGE;
+		}
+
+		if (perout->flags & PTP_PEROUT_PHASE) {
+			/*
+			 * The phase should be specified modulo the period,
+			 * therefore anything equal or larger than 1 period
+			 * is invalid.
+			 */
+			if (perout->phase.sec > perout->period.sec ||
+			    (perout->phase.sec == perout->period.sec &&
+			     perout->phase.nsec >= perout->period.nsec))
+				return -ERANGE;
+		}
+	} else {
+		perout->flags &= PTP_PEROUT_V1_VALID_FLAGS;
+		memset(perout->rsv, 0, sizeof(perout->rsv));
+	}
+
+	if (perout->index >= ops->n_per_out)
+		return -EINVAL;
+	if (perout->flags & ~ops->supported_perout_flags)
+		return -EOPNOTSUPP;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
+		return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
@@ -262,77 +317,9 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_PEROUT_REQUEST:
 	case PTP_PEROUT_REQUEST2:
-		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
-			err = -EACCES;
-			break;
-		}
-		memset(&req, 0, sizeof(req));
-
-		if (copy_from_user(&req.perout, (void __user *)arg,
-				   sizeof(req.perout))) {
-			err = -EFAULT;
-			break;
-		}
-		if (cmd == PTP_PEROUT_REQUEST2) {
-			struct ptp_perout_request *perout = &req.perout;
-
-			if (perout->flags & ~PTP_PEROUT_VALID_FLAGS) {
-				err = -EINVAL;
-				break;
-			}
-			/*
-			 * The "on" field has undefined meaning if
-			 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat
-			 * it as reserved, which must be set to zero.
-			 */
-			if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
-			    (perout->rsv[0] || perout->rsv[1] ||
-			     perout->rsv[2] || perout->rsv[3])) {
-				err = -EINVAL;
-				break;
-			}
-			if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
-				/* The duty cycle must be subunitary. */
-				if (perout->on.sec > perout->period.sec ||
-				    (perout->on.sec == perout->period.sec &&
-				     perout->on.nsec > perout->period.nsec)) {
-					err = -ERANGE;
-					break;
-				}
-			}
-			if (perout->flags & PTP_PEROUT_PHASE) {
-				/*
-				 * The phase should be specified modulo the
-				 * period, therefore anything equal or larger
-				 * than 1 period is invalid.
-				 */
-				if (perout->phase.sec > perout->period.sec ||
-				    (perout->phase.sec == perout->period.sec &&
-				     perout->phase.nsec >= perout->period.nsec)) {
-					err = -ERANGE;
-					break;
-				}
-			}
-		} else if (cmd == PTP_PEROUT_REQUEST) {
-			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
-			req.perout.rsv[0] = 0;
-			req.perout.rsv[1] = 0;
-			req.perout.rsv[2] = 0;
-			req.perout.rsv[3] = 0;
-		}
-		if (req.perout.index >= ops->n_per_out) {
-			err = -EINVAL;
-			break;
-		}
-		if (req.perout.flags & ~ptp->info->supported_perout_flags)
-			return -EOPNOTSUPP;
-		req.type = PTP_CLK_REQ_PEROUT;
-		enable = req.perout.period.sec || req.perout.period.nsec;
-		if (mutex_lock_interruptible(&ptp->pincfg_mux))
-			return -ERESTARTSYS;
-		err = ops->enable(ops, &req, enable);
-		mutex_unlock(&ptp->pincfg_mux);
-		break;
+		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
+			return -EACCES;
+		return ptp_perout_request(ptp, cmd, argptr);
 
 	case PTP_ENABLE_PPS:
 	case PTP_ENABLE_PPS2:

Re: [patch 03/13] ptp: Split out PTP_PEROUT_REQUEST ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_PEROUT_REQUEST
> ioctl code into a helper function. Convert to a lock guard while at it.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |  129 ++++++++++++++++++++--------------------------
>   1 file changed, 58 insertions(+), 71 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -223,6 +223,61 @@ static long ptp_extts_request(struct ptp
>   		return ops->enable(ops, &req, req.extts.flags & PTP_ENABLE_FEATURE ? 1 : 0);
>   }
>   
> +static long ptp_perout_request(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
> +{
> +	struct ptp_clock_request req = { .type = PTP_CLK_REQ_PEROUT };
> +	struct ptp_perout_request *perout = &req.perout;
> +	struct ptp_clock_info *ops = ptp->info;
> +
> +	if (copy_from_user(perout, arg, sizeof(*perout)))
> +		return -EFAULT;
> +
> +	if (cmd == PTP_PEROUT_REQUEST2) {
> +		if (perout->flags & ~PTP_PEROUT_VALID_FLAGS)
> +			return -EINVAL;
> +
> +		/*
> +		 * The "on" field has undefined meaning if
> +		 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat it
> +		 * as reserved, which must be set to zero.
> +		 */
> +		if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
> +		    !mem_is_zero(perout->rsv, sizeof(perout->rsv)))
> +			return -EINVAL;
> +
> +		if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
> +			/* The duty cycle must be subunitary. */
> +			if (perout->on.sec > perout->period.sec ||
> +			    (perout->on.sec == perout->period.sec &&
> +			     perout->on.nsec > perout->period.nsec))
> +				return -ERANGE;
> +		}
> +
> +		if (perout->flags & PTP_PEROUT_PHASE) {
> +			/*
> +			 * The phase should be specified modulo the period,
> +			 * therefore anything equal or larger than 1 period
> +			 * is invalid.
> +			 */
> +			if (perout->phase.sec > perout->period.sec ||
> +			    (perout->phase.sec == perout->period.sec &&
> +			     perout->phase.nsec >= perout->period.nsec))
> +				return -ERANGE;
> +		}
> +	} else {
> +		perout->flags &= PTP_PEROUT_V1_VALID_FLAGS;
> +		memset(perout->rsv, 0, sizeof(perout->rsv));
> +	}
> +
> +	if (perout->index >= ops->n_per_out)
> +		return -EINVAL;
> +	if (perout->flags & ~ops->supported_perout_flags)
> +		return -EOPNOTSUPP;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
> +		return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
> @@ -262,77 +317,9 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_PEROUT_REQUEST:
>   	case PTP_PEROUT_REQUEST2:
> -		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
> -			err = -EACCES;
> -			break;
> -		}
> -		memset(&req, 0, sizeof(req));
> -
> -		if (copy_from_user(&req.perout, (void __user *)arg,
> -				   sizeof(req.perout))) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		if (cmd == PTP_PEROUT_REQUEST2) {
> -			struct ptp_perout_request *perout = &req.perout;
> -
> -			if (perout->flags & ~PTP_PEROUT_VALID_FLAGS) {
> -				err = -EINVAL;
> -				break;
> -			}
> -			/*
> -			 * The "on" field has undefined meaning if
> -			 * PTP_PEROUT_DUTY_CYCLE isn't set, we must still treat
> -			 * it as reserved, which must be set to zero.
> -			 */
> -			if (!(perout->flags & PTP_PEROUT_DUTY_CYCLE) &&
> -			    (perout->rsv[0] || perout->rsv[1] ||
> -			     perout->rsv[2] || perout->rsv[3])) {
> -				err = -EINVAL;
> -				break;
> -			}
> -			if (perout->flags & PTP_PEROUT_DUTY_CYCLE) {
> -				/* The duty cycle must be subunitary. */
> -				if (perout->on.sec > perout->period.sec ||
> -				    (perout->on.sec == perout->period.sec &&
> -				     perout->on.nsec > perout->period.nsec)) {
> -					err = -ERANGE;
> -					break;
> -				}
> -			}
> -			if (perout->flags & PTP_PEROUT_PHASE) {
> -				/*
> -				 * The phase should be specified modulo the
> -				 * period, therefore anything equal or larger
> -				 * than 1 period is invalid.
> -				 */
> -				if (perout->phase.sec > perout->period.sec ||
> -				    (perout->phase.sec == perout->period.sec &&
> -				     perout->phase.nsec >= perout->period.nsec)) {
> -					err = -ERANGE;
> -					break;
> -				}
> -			}
> -		} else if (cmd == PTP_PEROUT_REQUEST) {
> -			req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
> -			req.perout.rsv[0] = 0;
> -			req.perout.rsv[1] = 0;
> -			req.perout.rsv[2] = 0;
> -			req.perout.rsv[3] = 0;
> -		}
> -		if (req.perout.index >= ops->n_per_out) {
> -			err = -EINVAL;
> -			break;
> -		}
> -		if (req.perout.flags & ~ptp->info->supported_perout_flags)
> -			return -EOPNOTSUPP;
> -		req.type = PTP_CLK_REQ_PEROUT;
> -		enable = req.perout.period.sec || req.perout.period.nsec;
> -		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> -			return -ERESTARTSYS;
> -		err = ops->enable(ops, &req, enable);
> -		mutex_unlock(&ptp->pincfg_mux);
> -		break;
> +		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
> +			return -EACCES;
> +		return ptp_perout_request(ptp, cmd, argptr);
>   
>   	case PTP_ENABLE_PPS:
>   	case PTP_ENABLE_PPS2:
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 04/13] ptp: Split out PTP_ENABLE_PPS ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_ENABLE_PPS
ioctl code into a helper function. Convert to a lock guard while at it.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -278,6 +278,18 @@ static long ptp_perout_request(struct pt
 		return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
 }
 
+static long ptp_enable_pps(struct ptp_clock *ptp, bool enable)
+{
+	struct ptp_clock_request req = { .type = PTP_CLK_REQ_PPS };
+	struct ptp_clock_info *ops = ptp->info;
+
+	if (!capable(CAP_SYS_TIME))
+		return -EPERM;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
+		return ops->enable(ops, &req, enable);
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
@@ -290,13 +302,12 @@ long ptp_ioctl(struct posix_clock_contex
 	struct ptp_sys_offset *sysoff = NULL;
 	struct timestamp_event_queue *tsevq;
 	struct ptp_system_timestamp sts;
-	struct ptp_clock_request req;
 	struct ptp_clock_time *pct;
 	unsigned int i, pin_index;
 	struct ptp_pin_desc pd;
 	struct timespec64 ts;
-	int enable, err = 0;
 	void __user *argptr;
+	int err = 0;
 
 	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
 		arg = (unsigned long)compat_ptr(arg);
@@ -323,21 +334,9 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_ENABLE_PPS:
 	case PTP_ENABLE_PPS2:
-		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
-			err = -EACCES;
-			break;
-		}
-		memset(&req, 0, sizeof(req));
-
-		if (!capable(CAP_SYS_TIME))
-			return -EPERM;
-		req.type = PTP_CLK_REQ_PPS;
-		enable = arg ? 1 : 0;
-		if (mutex_lock_interruptible(&ptp->pincfg_mux))
-			return -ERESTARTSYS;
-		err = ops->enable(ops, &req, enable);
-		mutex_unlock(&ptp->pincfg_mux);
-		break;
+		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
+			return -EACCES;
+		return ptp_enable_pps(ptp, !!arg);
 
 	case PTP_SYS_OFFSET_PRECISE:
 	case PTP_SYS_OFFSET_PRECISE2:

Re: [patch 04/13] ptp: Split out PTP_ENABLE_PPS ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_ENABLE_PPS
> ioctl code into a helper function. Convert to a lock guard while at it.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   33 ++++++++++++++++-----------------
>   1 file changed, 16 insertions(+), 17 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -278,6 +278,18 @@ static long ptp_perout_request(struct pt
>   		return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
>   }
>   
> +static long ptp_enable_pps(struct ptp_clock *ptp, bool enable)
> +{
> +	struct ptp_clock_request req = { .type = PTP_CLK_REQ_PPS };
> +	struct ptp_clock_info *ops = ptp->info;
> +
> +	if (!capable(CAP_SYS_TIME))
> +		return -EPERM;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
> +		return ops->enable(ops, &req, enable);
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
> @@ -290,13 +302,12 @@ long ptp_ioctl(struct posix_clock_contex
>   	struct ptp_sys_offset *sysoff = NULL;
>   	struct timestamp_event_queue *tsevq;
>   	struct ptp_system_timestamp sts;
> -	struct ptp_clock_request req;
>   	struct ptp_clock_time *pct;
>   	unsigned int i, pin_index;
>   	struct ptp_pin_desc pd;
>   	struct timespec64 ts;
> -	int enable, err = 0;
>   	void __user *argptr;
> +	int err = 0;
>   
>   	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
>   		arg = (unsigned long)compat_ptr(arg);
> @@ -323,21 +334,9 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_ENABLE_PPS:
>   	case PTP_ENABLE_PPS2:
> -		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
> -			err = -EACCES;
> -			break;
> -		}
> -		memset(&req, 0, sizeof(req));
> -
> -		if (!capable(CAP_SYS_TIME))
> -			return -EPERM;
> -		req.type = PTP_CLK_REQ_PPS;
> -		enable = arg ? 1 : 0;
> -		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> -			return -ERESTARTSYS;
> -		err = ops->enable(ops, &req, enable);
> -		mutex_unlock(&ptp->pincfg_mux);
> -		break;
> +		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
> +			return -EACCES;
> +		return ptp_enable_pps(ptp, !!arg);
>   
>   	case PTP_SYS_OFFSET_PRECISE:
>   	case PTP_SYS_OFFSET_PRECISE2:
> 

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 05/13] ptp: Split out PTP_SYS_OFFSET_PRECISE ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_SYS_OFFSET_PRECISE
ioctl code into a helper function.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   53 +++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -291,14 +291,40 @@ static long ptp_enable_pps(struct ptp_cl
 		return ops->enable(ops, &req, enable);
 }
 
+static long ptp_sys_offset_precise(struct ptp_clock *ptp, void __user *arg)
+{
+	struct ptp_sys_offset_precise precise_offset;
+	struct system_device_crosststamp xtstamp;
+	struct timespec64 ts;
+	int err;
+
+	if (!ptp->info->getcrosststamp)
+		return -EOPNOTSUPP;
+
+	err = ptp->info->getcrosststamp(ptp->info, &xtstamp);
+	if (err)
+		return err;
+
+	memset(&precise_offset, 0, sizeof(precise_offset));
+	ts = ktime_to_timespec64(xtstamp.device);
+	precise_offset.device.sec = ts.tv_sec;
+	precise_offset.device.nsec = ts.tv_nsec;
+	ts = ktime_to_timespec64(xtstamp.sys_realtime);
+	precise_offset.sys_realtime.sec = ts.tv_sec;
+	precise_offset.sys_realtime.nsec = ts.tv_nsec;
+	ts = ktime_to_timespec64(xtstamp.sys_monoraw);
+	precise_offset.sys_monoraw.sec = ts.tv_sec;
+	precise_offset.sys_monoraw.nsec = ts.tv_nsec;
+
+	return copy_to_user(arg, &precise_offset, sizeof(precise_offset)) ? -EFAULT : 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
 	struct ptp_sys_offset_extended *extoff = NULL;
-	struct ptp_sys_offset_precise precise_offset;
-	struct system_device_crosststamp xtstamp;
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_sys_offset *sysoff = NULL;
 	struct timestamp_event_queue *tsevq;
@@ -341,28 +367,7 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_SYS_OFFSET_PRECISE:
 	case PTP_SYS_OFFSET_PRECISE2:
-		if (!ptp->info->getcrosststamp) {
-			err = -EOPNOTSUPP;
-			break;
-		}
-		err = ptp->info->getcrosststamp(ptp->info, &xtstamp);
-		if (err)
-			break;
-
-		memset(&precise_offset, 0, sizeof(precise_offset));
-		ts = ktime_to_timespec64(xtstamp.device);
-		precise_offset.device.sec = ts.tv_sec;
-		precise_offset.device.nsec = ts.tv_nsec;
-		ts = ktime_to_timespec64(xtstamp.sys_realtime);
-		precise_offset.sys_realtime.sec = ts.tv_sec;
-		precise_offset.sys_realtime.nsec = ts.tv_nsec;
-		ts = ktime_to_timespec64(xtstamp.sys_monoraw);
-		precise_offset.sys_monoraw.sec = ts.tv_sec;
-		precise_offset.sys_monoraw.nsec = ts.tv_nsec;
-		if (copy_to_user((void __user *)arg, &precise_offset,
-				 sizeof(precise_offset)))
-			err = -EFAULT;
-		break;
+		return ptp_sys_offset_precise(ptp, argptr);
 
 	case PTP_SYS_OFFSET_EXTENDED:
 	case PTP_SYS_OFFSET_EXTENDED2:

Re: [patch 05/13] ptp: Split out PTP_SYS_OFFSET_PRECISE ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_SYS_OFFSET_PRECISE
> ioctl code into a helper function.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   53 +++++++++++++++++++++++++---------------------
>   1 file changed, 29 insertions(+), 24 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -291,14 +291,40 @@ static long ptp_enable_pps(struct ptp_cl
>   		return ops->enable(ops, &req, enable);
>   }
>   
> +static long ptp_sys_offset_precise(struct ptp_clock *ptp, void __user *arg)
> +{
> +	struct ptp_sys_offset_precise precise_offset;
> +	struct system_device_crosststamp xtstamp;
> +	struct timespec64 ts;
> +	int err;
> +
> +	if (!ptp->info->getcrosststamp)
> +		return -EOPNOTSUPP;
> +
> +	err = ptp->info->getcrosststamp(ptp->info, &xtstamp);
> +	if (err)
> +		return err;
> +
> +	memset(&precise_offset, 0, sizeof(precise_offset));
> +	ts = ktime_to_timespec64(xtstamp.device);
> +	precise_offset.device.sec = ts.tv_sec;
> +	precise_offset.device.nsec = ts.tv_nsec;
> +	ts = ktime_to_timespec64(xtstamp.sys_realtime);
> +	precise_offset.sys_realtime.sec = ts.tv_sec;
> +	precise_offset.sys_realtime.nsec = ts.tv_nsec;
> +	ts = ktime_to_timespec64(xtstamp.sys_monoraw);
> +	precise_offset.sys_monoraw.sec = ts.tv_sec;
> +	precise_offset.sys_monoraw.nsec = ts.tv_nsec;
> +
> +	return copy_to_user(arg, &precise_offset, sizeof(precise_offset)) ? -EFAULT : 0;
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
>   	struct ptp_clock *ptp =
>   		container_of(pccontext->clk, struct ptp_clock, clock);
>   	struct ptp_sys_offset_extended *extoff = NULL;
> -	struct ptp_sys_offset_precise precise_offset;
> -	struct system_device_crosststamp xtstamp;
>   	struct ptp_clock_info *ops = ptp->info;
>   	struct ptp_sys_offset *sysoff = NULL;
>   	struct timestamp_event_queue *tsevq;
> @@ -341,28 +367,7 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_SYS_OFFSET_PRECISE:
>   	case PTP_SYS_OFFSET_PRECISE2:
> -		if (!ptp->info->getcrosststamp) {
> -			err = -EOPNOTSUPP;
> -			break;
> -		}
> -		err = ptp->info->getcrosststamp(ptp->info, &xtstamp);
> -		if (err)
> -			break;
> -
> -		memset(&precise_offset, 0, sizeof(precise_offset));
> -		ts = ktime_to_timespec64(xtstamp.device);
> -		precise_offset.device.sec = ts.tv_sec;
> -		precise_offset.device.nsec = ts.tv_nsec;
> -		ts = ktime_to_timespec64(xtstamp.sys_realtime);
> -		precise_offset.sys_realtime.sec = ts.tv_sec;
> -		precise_offset.sys_realtime.nsec = ts.tv_nsec;
> -		ts = ktime_to_timespec64(xtstamp.sys_monoraw);
> -		precise_offset.sys_monoraw.sec = ts.tv_sec;
> -		precise_offset.sys_monoraw.nsec = ts.tv_nsec;
> -		if (copy_to_user((void __user *)arg, &precise_offset,
> -				 sizeof(precise_offset)))
> -			err = -EFAULT;
> -		break;
> +		return ptp_sys_offset_precise(ptp, argptr);
>   
>   	case PTP_SYS_OFFSET_EXTENDED:
>   	case PTP_SYS_OFFSET_EXTENDED2:
> 

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 06/13] ptp: Split out PTP_SYS_OFFSET_EXTENDED ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the
PTP_SYS_OFFSET_EXTENDED ioctl code into a helper function.

Convert it to __free() to avoid gotos.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   75 +++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 36 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -319,16 +319,52 @@ static long ptp_sys_offset_precise(struc
 	return copy_to_user(arg, &precise_offset, sizeof(precise_offset)) ? -EFAULT : 0;
 }
 
+static long ptp_sys_offset_extended(struct ptp_clock *ptp, void __user *arg)
+{
+	struct ptp_sys_offset_extended *extoff __free(kfree) = NULL;
+	struct ptp_system_timestamp sts;
+
+	if (!ptp->info->gettimex64)
+		return -EOPNOTSUPP;
+
+	extoff = memdup_user(arg, sizeof(*extoff));
+	if (IS_ERR(extoff))
+		return PTR_ERR(extoff);
+
+	if (extoff->n_samples > PTP_MAX_SAMPLES ||
+	    extoff->rsv[0] || extoff->rsv[1] ||
+	    (extoff->clockid != CLOCK_REALTIME &&
+	     extoff->clockid != CLOCK_MONOTONIC &&
+	     extoff->clockid != CLOCK_MONOTONIC_RAW))
+		return -EINVAL;
+
+	sts.clockid = extoff->clockid;
+	for (unsigned int i = 0; i < extoff->n_samples; i++) {
+		struct timespec64 ts;
+		int err;
+
+		err = ptp->info->gettimex64(ptp->info, &ts, &sts);
+		if (err)
+			return err;
+		extoff->ts[i][0].sec = sts.pre_ts.tv_sec;
+		extoff->ts[i][0].nsec = sts.pre_ts.tv_nsec;
+		extoff->ts[i][1].sec = ts.tv_sec;
+		extoff->ts[i][1].nsec = ts.tv_nsec;
+		extoff->ts[i][2].sec = sts.post_ts.tv_sec;
+		extoff->ts[i][2].nsec = sts.post_ts.tv_nsec;
+	}
+
+	return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
-	struct ptp_sys_offset_extended *extoff = NULL;
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_sys_offset *sysoff = NULL;
 	struct timestamp_event_queue *tsevq;
-	struct ptp_system_timestamp sts;
 	struct ptp_clock_time *pct;
 	unsigned int i, pin_index;
 	struct ptp_pin_desc pd;
@@ -371,39 +407,7 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_SYS_OFFSET_EXTENDED:
 	case PTP_SYS_OFFSET_EXTENDED2:
-		if (!ptp->info->gettimex64) {
-			err = -EOPNOTSUPP;
-			break;
-		}
-		extoff = memdup_user((void __user *)arg, sizeof(*extoff));
-		if (IS_ERR(extoff)) {
-			err = PTR_ERR(extoff);
-			extoff = NULL;
-			break;
-		}
-		if (extoff->n_samples > PTP_MAX_SAMPLES ||
-		    extoff->rsv[0] || extoff->rsv[1] ||
-		    (extoff->clockid != CLOCK_REALTIME &&
-		     extoff->clockid != CLOCK_MONOTONIC &&
-		     extoff->clockid != CLOCK_MONOTONIC_RAW)) {
-			err = -EINVAL;
-			break;
-		}
-		sts.clockid = extoff->clockid;
-		for (i = 0; i < extoff->n_samples; i++) {
-			err = ptp->info->gettimex64(ptp->info, &ts, &sts);
-			if (err)
-				goto out;
-			extoff->ts[i][0].sec = sts.pre_ts.tv_sec;
-			extoff->ts[i][0].nsec = sts.pre_ts.tv_nsec;
-			extoff->ts[i][1].sec = ts.tv_sec;
-			extoff->ts[i][1].nsec = ts.tv_nsec;
-			extoff->ts[i][2].sec = sts.post_ts.tv_sec;
-			extoff->ts[i][2].nsec = sts.post_ts.tv_nsec;
-		}
-		if (copy_to_user((void __user *)arg, extoff, sizeof(*extoff)))
-			err = -EFAULT;
-		break;
+		return ptp_sys_offset_extended(ptp, argptr);
 
 	case PTP_SYS_OFFSET:
 	case PTP_SYS_OFFSET2:
@@ -528,7 +532,6 @@ long ptp_ioctl(struct posix_clock_contex
 	}
 
 out:
-	kfree(extoff);
 	kfree(sysoff);
 	return err;
 }

Re: [patch 06/13] ptp: Split out PTP_SYS_OFFSET_EXTENDED ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the
> PTP_SYS_OFFSET_EXTENDED ioctl code into a helper function.
> 
> Convert it to __free() to avoid gotos.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   75 +++++++++++++++++++++++-----------------------
>   1 file changed, 39 insertions(+), 36 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -319,16 +319,52 @@ static long ptp_sys_offset_precise(struc
>   	return copy_to_user(arg, &precise_offset, sizeof(precise_offset)) ? -EFAULT : 0;
>   }
>   
> +static long ptp_sys_offset_extended(struct ptp_clock *ptp, void __user *arg)
> +{
> +	struct ptp_sys_offset_extended *extoff __free(kfree) = NULL;
> +	struct ptp_system_timestamp sts;
> +
> +	if (!ptp->info->gettimex64)
> +		return -EOPNOTSUPP;
> +
> +	extoff = memdup_user(arg, sizeof(*extoff));
> +	if (IS_ERR(extoff))
> +		return PTR_ERR(extoff);
> +
> +	if (extoff->n_samples > PTP_MAX_SAMPLES ||
> +	    extoff->rsv[0] || extoff->rsv[1] ||
> +	    (extoff->clockid != CLOCK_REALTIME &&
> +	     extoff->clockid != CLOCK_MONOTONIC &&
> +	     extoff->clockid != CLOCK_MONOTONIC_RAW))
> +		return -EINVAL;
> +
> +	sts.clockid = extoff->clockid;
> +	for (unsigned int i = 0; i < extoff->n_samples; i++) {
> +		struct timespec64 ts;
> +		int err;
> +
> +		err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> +		if (err)
> +			return err;
> +		extoff->ts[i][0].sec = sts.pre_ts.tv_sec;
> +		extoff->ts[i][0].nsec = sts.pre_ts.tv_nsec;
> +		extoff->ts[i][1].sec = ts.tv_sec;
> +		extoff->ts[i][1].nsec = ts.tv_nsec;
> +		extoff->ts[i][2].sec = sts.post_ts.tv_sec;
> +		extoff->ts[i][2].nsec = sts.post_ts.tv_nsec;
> +	}
> +
> +	return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0;
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
>   	struct ptp_clock *ptp =
>   		container_of(pccontext->clk, struct ptp_clock, clock);
> -	struct ptp_sys_offset_extended *extoff = NULL;
>   	struct ptp_clock_info *ops = ptp->info;
>   	struct ptp_sys_offset *sysoff = NULL;
>   	struct timestamp_event_queue *tsevq;
> -	struct ptp_system_timestamp sts;
>   	struct ptp_clock_time *pct;
>   	unsigned int i, pin_index;
>   	struct ptp_pin_desc pd;
> @@ -371,39 +407,7 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_SYS_OFFSET_EXTENDED:
>   	case PTP_SYS_OFFSET_EXTENDED2:
> -		if (!ptp->info->gettimex64) {
> -			err = -EOPNOTSUPP;
> -			break;
> -		}
> -		extoff = memdup_user((void __user *)arg, sizeof(*extoff));
> -		if (IS_ERR(extoff)) {
> -			err = PTR_ERR(extoff);
> -			extoff = NULL;
> -			break;
> -		}
> -		if (extoff->n_samples > PTP_MAX_SAMPLES ||
> -		    extoff->rsv[0] || extoff->rsv[1] ||
> -		    (extoff->clockid != CLOCK_REALTIME &&
> -		     extoff->clockid != CLOCK_MONOTONIC &&
> -		     extoff->clockid != CLOCK_MONOTONIC_RAW)) {
> -			err = -EINVAL;
> -			break;
> -		}
> -		sts.clockid = extoff->clockid;
> -		for (i = 0; i < extoff->n_samples; i++) {
> -			err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> -			if (err)
> -				goto out;
> -			extoff->ts[i][0].sec = sts.pre_ts.tv_sec;
> -			extoff->ts[i][0].nsec = sts.pre_ts.tv_nsec;
> -			extoff->ts[i][1].sec = ts.tv_sec;
> -			extoff->ts[i][1].nsec = ts.tv_nsec;
> -			extoff->ts[i][2].sec = sts.post_ts.tv_sec;
> -			extoff->ts[i][2].nsec = sts.post_ts.tv_nsec;
> -		}
> -		if (copy_to_user((void __user *)arg, extoff, sizeof(*extoff)))
> -			err = -EFAULT;
> -		break;
> +		return ptp_sys_offset_extended(ptp, argptr);
>   
>   	case PTP_SYS_OFFSET:
>   	case PTP_SYS_OFFSET2:
> @@ -528,7 +532,6 @@ long ptp_ioctl(struct posix_clock_contex
>   	}
>   
>   out:
> -	kfree(extoff);
>   	kfree(sysoff);
>   	return err;
>   }
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 07/13] ptp: Split out PTP_SYS_OFFSET ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_SYS_OFFSET ioctl
code into a helper function.

Convert it to __free() to avoid gotos.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   78 +++++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 38 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -357,18 +357,54 @@ static long ptp_sys_offset_extended(stru
 	return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0;
 }
 
+static long ptp_sys_offset(struct ptp_clock *ptp, void __user *arg)
+{
+	struct ptp_sys_offset *sysoff __free(kfree) = NULL;
+	struct ptp_clock_time *pct;
+	struct timespec64 ts;
+
+	sysoff = memdup_user(arg, sizeof(*sysoff));
+	if (IS_ERR(sysoff))
+		return PTR_ERR(sysoff);
+
+	if (sysoff->n_samples > PTP_MAX_SAMPLES)
+		return -EINVAL;
+
+	pct = &sysoff->ts[0];
+	for (unsigned int i = 0; i < sysoff->n_samples; i++) {
+		struct ptp_clock_info *ops = ptp->info;
+		int err;
+
+		ktime_get_real_ts64(&ts);
+		pct->sec = ts.tv_sec;
+		pct->nsec = ts.tv_nsec;
+		pct++;
+		if (ops->gettimex64)
+			err = ops->gettimex64(ops, &ts, NULL);
+		else
+			err = ops->gettime64(ops, &ts);
+		if (err)
+			return err;
+		pct->sec = ts.tv_sec;
+		pct->nsec = ts.tv_nsec;
+		pct++;
+	}
+	ktime_get_real_ts64(&ts);
+	pct->sec = ts.tv_sec;
+	pct->nsec = ts.tv_nsec;
+
+	return copy_to_user(arg, sysoff, sizeof(*sysoff)) ? -EFAULT : 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
 	struct ptp_clock_info *ops = ptp->info;
-	struct ptp_sys_offset *sysoff = NULL;
 	struct timestamp_event_queue *tsevq;
-	struct ptp_clock_time *pct;
 	unsigned int i, pin_index;
 	struct ptp_pin_desc pd;
-	struct timespec64 ts;
 	void __user *argptr;
 	int err = 0;
 
@@ -411,38 +447,7 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_SYS_OFFSET:
 	case PTP_SYS_OFFSET2:
-		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
-		if (IS_ERR(sysoff)) {
-			err = PTR_ERR(sysoff);
-			sysoff = NULL;
-			break;
-		}
-		if (sysoff->n_samples > PTP_MAX_SAMPLES) {
-			err = -EINVAL;
-			break;
-		}
-		pct = &sysoff->ts[0];
-		for (i = 0; i < sysoff->n_samples; i++) {
-			ktime_get_real_ts64(&ts);
-			pct->sec = ts.tv_sec;
-			pct->nsec = ts.tv_nsec;
-			pct++;
-			if (ops->gettimex64)
-				err = ops->gettimex64(ops, &ts, NULL);
-			else
-				err = ops->gettime64(ops, &ts);
-			if (err)
-				goto out;
-			pct->sec = ts.tv_sec;
-			pct->nsec = ts.tv_nsec;
-			pct++;
-		}
-		ktime_get_real_ts64(&ts);
-		pct->sec = ts.tv_sec;
-		pct->nsec = ts.tv_nsec;
-		if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
-			err = -EFAULT;
-		break;
+		return ptp_sys_offset(ptp, argptr);
 
 	case PTP_PIN_GETFUNC:
 	case PTP_PIN_GETFUNC2:
@@ -530,9 +535,6 @@ long ptp_ioctl(struct posix_clock_contex
 		err = -ENOTTY;
 		break;
 	}
-
-out:
-	kfree(sysoff);
 	return err;
 }
 

Re: [patch 07/13] ptp: Split out PTP_SYS_OFFSET ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_SYS_OFFSET ioctl
> code into a helper function.
> 
> Convert it to __free() to avoid gotos.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   78 +++++++++++++++++++++++-----------------------
>   1 file changed, 40 insertions(+), 38 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -357,18 +357,54 @@ static long ptp_sys_offset_extended(stru
>   	return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0;
>   }
>   
> +static long ptp_sys_offset(struct ptp_clock *ptp, void __user *arg)
> +{
> +	struct ptp_sys_offset *sysoff __free(kfree) = NULL;
> +	struct ptp_clock_time *pct;
> +	struct timespec64 ts;
> +
> +	sysoff = memdup_user(arg, sizeof(*sysoff));
> +	if (IS_ERR(sysoff))
> +		return PTR_ERR(sysoff);
> +
> +	if (sysoff->n_samples > PTP_MAX_SAMPLES)
> +		return -EINVAL;
> +
> +	pct = &sysoff->ts[0];
> +	for (unsigned int i = 0; i < sysoff->n_samples; i++) {
> +		struct ptp_clock_info *ops = ptp->info;

Looks like *ops initialization can be moved outside of the loop.

> +		int err;
> +
> +		ktime_get_real_ts64(&ts);
> +		pct->sec = ts.tv_sec;
> +		pct->nsec = ts.tv_nsec;
> +		pct++;
> +		if (ops->gettimex64)
> +			err = ops->gettimex64(ops, &ts, NULL);
> +		else
> +			err = ops->gettime64(ops, &ts);
> +		if (err)
> +			return err;
> +		pct->sec = ts.tv_sec;
> +		pct->nsec = ts.tv_nsec;
> +		pct++;
> +	}
> +	ktime_get_real_ts64(&ts);
> +	pct->sec = ts.tv_sec;
> +	pct->nsec = ts.tv_nsec;
> +
> +	return copy_to_user(arg, sysoff, sizeof(*sysoff)) ? -EFAULT : 0;
> +}
> +

[...]

Re: [patch 07/13] ptp: Split out PTP_SYS_OFFSET ioctl code

[Thomas Gleixner]

On Sat, Jun 21 2025 at 21:14, Vadim Fedorenko wrote:
> On 20/06/2025 14:24, Thomas Gleixner wrote:
>> +	pct = &sysoff->ts[0];
>> +	for (unsigned int i = 0; i < sysoff->n_samples; i++) {
>> +		struct ptp_clock_info *ops = ptp->info;
>
> Looks like *ops initialization can be moved outside of the loop.

Well it can, but does it matter? No, because this is only a coding
artifact. The compiler can evaluate ptp->info inside of the loop at his
own peril even on both usage sites.

Though what's more important is that from a context point of view, ops
belongs into the loop, because that's where it is used and not outside,
no?

Thanks,

        tglx

Re: [patch 07/13] ptp: Split out PTP_SYS_OFFSET ioctl code

[Vadim Fedorenko]

On 21/06/2025 21:42, Thomas Gleixner wrote:
> On Sat, Jun 21 2025 at 21:14, Vadim Fedorenko wrote:
>> On 20/06/2025 14:24, Thomas Gleixner wrote:
>>> +	pct = &sysoff->ts[0];
>>> +	for (unsigned int i = 0; i < sysoff->n_samples; i++) {
>>> +		struct ptp_clock_info *ops = ptp->info;
>>
>> Looks like *ops initialization can be moved outside of the loop.
> 
> Well it can, but does it matter? No, because this is only a coding
> artifact. The compiler can evaluate ptp->info inside of the loop at his
> own peril even on both usage sites.
> 
> Though what's more important is that from a context point of view, ops
> belongs into the loop, because that's where it is used and not outside,
> no?

Well, in the original code it was always outside of any loops. And the
scope a bit bigger, all these functions work with ptp_clock object, so
the context is really subjective. It just looks a bit weird to de-
reference something, which is known not to change, every iteration. Of
course the compiler is smart enough to optimize it, but still..


> Thanks,
> 
>          tglx

[patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_PIN_GETFUNC ioctl
code into a helper function. Convert to lock guard while at it.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   52 ++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -396,6 +396,28 @@ static long ptp_sys_offset(struct ptp_cl
 	return copy_to_user(arg, sysoff, sizeof(*sysoff)) ? -EFAULT : 0;
 }
 
+static long ptp_pin_getfunc(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
+{
+	struct ptp_clock_info *ops = ptp->info;
+	struct ptp_pin_desc pd;
+
+	if (copy_from_user(&pd, arg, sizeof(pd)))
+		return -EFAULT;
+
+	if (cmd == PTP_PIN_GETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
+		return -EINVAL;
+	else
+		memset(pd.rsv, 0, sizeof(pd.rsv));
+
+	if (pd.index >= ops->n_pins)
+		return -EINVAL;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
+		pd = ops->pin_config[array_index_nospec(pd.index, ops->n_pins)];
+
+	return copy_to_user(arg, &pd, sizeof(pd)) ? -EFAULT : 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
@@ -451,35 +473,7 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_PIN_GETFUNC:
 	case PTP_PIN_GETFUNC2:
-		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
-			err = -EFAULT;
-			break;
-		}
-		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
-				|| pd.rsv[3] || pd.rsv[4])
-			&& cmd == PTP_PIN_GETFUNC2) {
-			err = -EINVAL;
-			break;
-		} else if (cmd == PTP_PIN_GETFUNC) {
-			pd.rsv[0] = 0;
-			pd.rsv[1] = 0;
-			pd.rsv[2] = 0;
-			pd.rsv[3] = 0;
-			pd.rsv[4] = 0;
-		}
-		pin_index = pd.index;
-		if (pin_index >= ops->n_pins) {
-			err = -EINVAL;
-			break;
-		}
-		pin_index = array_index_nospec(pin_index, ops->n_pins);
-		if (mutex_lock_interruptible(&ptp->pincfg_mux))
-			return -ERESTARTSYS;
-		pd = ops->pin_config[pin_index];
-		mutex_unlock(&ptp->pincfg_mux);
-		if (!err && copy_to_user((void __user *)arg, &pd, sizeof(pd)))
-			err = -EFAULT;
-		break;
+		return ptp_pin_getfunc(ptp, cmd, argptr);
 
 	case PTP_PIN_SETFUNC:
 	case PTP_PIN_SETFUNC2:

Re: [patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_PIN_GETFUNC ioctl
> code into a helper function. Convert to lock guard while at it.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   52 ++++++++++++++++++++--------------------------
>   1 file changed, 23 insertions(+), 29 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -396,6 +396,28 @@ static long ptp_sys_offset(struct ptp_cl
>   	return copy_to_user(arg, sysoff, sizeof(*sysoff)) ? -EFAULT : 0;
>   }
>   
> +static long ptp_pin_getfunc(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
> +{
> +	struct ptp_clock_info *ops = ptp->info;
> +	struct ptp_pin_desc pd;
> +
> +	if (copy_from_user(&pd, arg, sizeof(pd)))
> +		return -EFAULT;
> +
> +	if (cmd == PTP_PIN_GETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
> +		return -EINVAL;
> +	else
> +		memset(pd.rsv, 0, sizeof(pd.rsv));
> +
> +	if (pd.index >= ops->n_pins)
> +		return -EINVAL;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
> +		pd = ops->pin_config[array_index_nospec(pd.index, ops->n_pins)];
> +
> +	return copy_to_user(arg, &pd, sizeof(pd)) ? -EFAULT : 0;
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
> @@ -451,35 +473,7 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_PIN_GETFUNC:
>   	case PTP_PIN_GETFUNC2:
> -		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> -				|| pd.rsv[3] || pd.rsv[4])
> -			&& cmd == PTP_PIN_GETFUNC2) {
> -			err = -EINVAL;
> -			break;
> -		} else if (cmd == PTP_PIN_GETFUNC) {
> -			pd.rsv[0] = 0;
> -			pd.rsv[1] = 0;
> -			pd.rsv[2] = 0;
> -			pd.rsv[3] = 0;
> -			pd.rsv[4] = 0;
> -		}
> -		pin_index = pd.index;
> -		if (pin_index >= ops->n_pins) {
> -			err = -EINVAL;
> -			break;
> -		}
> -		pin_index = array_index_nospec(pin_index, ops->n_pins);
> -		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> -			return -ERESTARTSYS;
> -		pd = ops->pin_config[pin_index];
> -		mutex_unlock(&ptp->pincfg_mux);
> -		if (!err && copy_to_user((void __user *)arg, &pd, sizeof(pd)))
> -			err = -EFAULT;
> -		break;
> +		return ptp_pin_getfunc(ptp, cmd, argptr);
>   
>   	case PTP_PIN_SETFUNC:
>   	case PTP_PIN_SETFUNC2:
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code

[Paolo Abeni]

On 6/20/25 3:24 PM, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_PIN_GETFUNC ioctl
> code into a helper function. Convert to lock guard while at it.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/ptp/ptp_chardev.c |   52 ++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 29 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -396,6 +396,28 @@ static long ptp_sys_offset(struct ptp_cl
>  	return copy_to_user(arg, sysoff, sizeof(*sysoff)) ? -EFAULT : 0;
>  }
>  
> +static long ptp_pin_getfunc(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
> +{
> +	struct ptp_clock_info *ops = ptp->info;
> +	struct ptp_pin_desc pd;
> +
> +	if (copy_from_user(&pd, arg, sizeof(pd)))
> +		return -EFAULT;
> +
> +	if (cmd == PTP_PIN_GETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
> +		return -EINVAL;
> +	else
> +		memset(pd.rsv, 0, sizeof(pd.rsv));

Minor nit: I personally find the 'else' statement after return
counter-intuitive and dropping it would save an additional LoC.

Thanks,

Paolo

Re: [patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code

[Thomas Gleixner]

On Tue, Jun 24 2025 at 11:22, Paolo Abeni wrote:
> On 6/20/25 3:24 PM, Thomas Gleixner wrote:
>> Continue the ptp_ioctl() cleanup by splitting out the PTP_PIN_GETFUNC ioctl
>> code into a helper function. Convert to lock guard while at it.
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  drivers/ptp/ptp_chardev.c |   52 ++++++++++++++++++++--------------------------
>>  1 file changed, 23 insertions(+), 29 deletions(-)
>> 
>> --- a/drivers/ptp/ptp_chardev.c
>> +++ b/drivers/ptp/ptp_chardev.c
>> @@ -396,6 +396,28 @@ static long ptp_sys_offset(struct ptp_cl
>>  	return copy_to_user(arg, sysoff, sizeof(*sysoff)) ? -EFAULT : 0;
>>  }
>>  
>> +static long ptp_pin_getfunc(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
>> +{
>> +	struct ptp_clock_info *ops = ptp->info;
>> +	struct ptp_pin_desc pd;
>> +
>> +	if (copy_from_user(&pd, arg, sizeof(pd)))
>> +		return -EFAULT;
>> +
>> +	if (cmd == PTP_PIN_GETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
>> +		return -EINVAL;
>> +	else
>> +		memset(pd.rsv, 0, sizeof(pd.rsv));
>
> Minor nit: I personally find the 'else' statement after return
> counter-intuitive and dropping it would save an additional LoC.

Of course ...

Re: [patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code

[Thomas Gleixner]

On Tue, Jun 24 2025 at 15:39, Thomas Gleixner wrote:
> On Tue, Jun 24 2025 at 11:22, Paolo Abeni wrote:
>>> +	if (cmd == PTP_PIN_GETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
>>> +		return -EINVAL;
>>> +	else
>>> +		memset(pd.rsv, 0, sizeof(pd.rsv));
>>
>> Minor nit: I personally find the 'else' statement after return
>> counter-intuitive and dropping it would save an additional LoC.
>
> Of course ...

But second thoughts. The actual logic here is:

	if (cmd == PTP_PIN_GETFUNC2) {
		if (!mem_is_zero(pd.rsv, sizeof(pd.rsv)))
			return -EINVAL;
	} else {
		memset(pd.rsv, 0, sizeof(pd.rsv));
	}

because PTP_PIN_GETFUNC did not mandate the reserved fields to be zero,
which means the reserved fields can never be used with that opcode.

But as it stands today, pd.rsv is not used at all in that function and
pd is fully overwritten via pd = pd->ops_config[] later. So the memset
is completely useless right now and can go away completely.

Thanks,

        tglx

[patch 09/13] ptp: Split out PTP_PIN_SETFUNC ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_PIN_SETFUNC ioctl
code into a helper function. Convert to lock guard while at it.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   60 +++++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -420,16 +420,36 @@ static long ptp_pin_getfunc(struct ptp_c
 	return copy_to_user(arg, &pd, sizeof(pd)) ? -EFAULT : 0;
 }
 
+static long ptp_pin_setfunc(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
+{
+	struct ptp_clock_info *ops = ptp->info;
+	struct ptp_pin_desc pd;
+	unsigned int pin_index;
+
+	if (copy_from_user(&pd, arg, sizeof(pd)))
+		return -EFAULT;
+
+	if (cmd == PTP_PIN_SETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
+		return -EINVAL;
+	else
+		memset(pd.rsv, 0, sizeof(pd.rsv));
+
+	if (pd.index >= ops->n_pins)
+		return -EINVAL;
+
+	pin_index = array_index_nospec(pd.index, ops->n_pins);
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
+		return ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
-	struct ptp_clock_info *ops = ptp->info;
 	struct timestamp_event_queue *tsevq;
-	unsigned int i, pin_index;
-	struct ptp_pin_desc pd;
 	void __user *argptr;
+	unsigned int i;
 	int err = 0;
 
 	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
@@ -479,37 +499,9 @@ long ptp_ioctl(struct posix_clock_contex
 
 	case PTP_PIN_SETFUNC:
 	case PTP_PIN_SETFUNC2:
-		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
-			err = -EACCES;
-			break;
-		}
-		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
-			err = -EFAULT;
-			break;
-		}
-		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
-				|| pd.rsv[3] || pd.rsv[4])
-			&& cmd == PTP_PIN_SETFUNC2) {
-			err = -EINVAL;
-			break;
-		} else if (cmd == PTP_PIN_SETFUNC) {
-			pd.rsv[0] = 0;
-			pd.rsv[1] = 0;
-			pd.rsv[2] = 0;
-			pd.rsv[3] = 0;
-			pd.rsv[4] = 0;
-		}
-		pin_index = pd.index;
-		if (pin_index >= ops->n_pins) {
-			err = -EINVAL;
-			break;
-		}
-		pin_index = array_index_nospec(pin_index, ops->n_pins);
-		if (mutex_lock_interruptible(&ptp->pincfg_mux))
-			return -ERESTARTSYS;
-		err = ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
-		mutex_unlock(&ptp->pincfg_mux);
-		break;
+		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
+			return -EACCES;
+		return ptp_pin_setfunc(ptp, cmd, argptr);
 
 	case PTP_MASK_CLEAR_ALL:
 		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);

Re: [patch 09/13] ptp: Split out PTP_PIN_SETFUNC ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_PIN_SETFUNC ioctl
> code into a helper function. Convert to lock guard while at it.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   60 +++++++++++++++++++---------------------------
>   1 file changed, 26 insertions(+), 34 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -420,16 +420,36 @@ static long ptp_pin_getfunc(struct ptp_c
>   	return copy_to_user(arg, &pd, sizeof(pd)) ? -EFAULT : 0;
>   }
>   
> +static long ptp_pin_setfunc(struct ptp_clock *ptp, unsigned int cmd, void __user *arg)
> +{
> +	struct ptp_clock_info *ops = ptp->info;
> +	struct ptp_pin_desc pd;
> +	unsigned int pin_index;
> +
> +	if (copy_from_user(&pd, arg, sizeof(pd)))
> +		return -EFAULT;
> +
> +	if (cmd == PTP_PIN_SETFUNC2 && !mem_is_zero(pd.rsv, sizeof(pd.rsv)))
> +		return -EINVAL;
> +	else
> +		memset(pd.rsv, 0, sizeof(pd.rsv));
> +
> +	if (pd.index >= ops->n_pins)
> +		return -EINVAL;
> +
> +	pin_index = array_index_nospec(pd.index, ops->n_pins);
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
> +		return ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
>   	struct ptp_clock *ptp =
>   		container_of(pccontext->clk, struct ptp_clock, clock);
> -	struct ptp_clock_info *ops = ptp->info;
>   	struct timestamp_event_queue *tsevq;
> -	unsigned int i, pin_index;
> -	struct ptp_pin_desc pd;
>   	void __user *argptr;
> +	unsigned int i;
>   	int err = 0;
>   
>   	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
> @@ -479,37 +499,9 @@ long ptp_ioctl(struct posix_clock_contex
>   
>   	case PTP_PIN_SETFUNC:
>   	case PTP_PIN_SETFUNC2:
> -		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) {
> -			err = -EACCES;
> -			break;
> -		}
> -		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		if ((pd.rsv[0] || pd.rsv[1] || pd.rsv[2]
> -				|| pd.rsv[3] || pd.rsv[4])
> -			&& cmd == PTP_PIN_SETFUNC2) {
> -			err = -EINVAL;
> -			break;
> -		} else if (cmd == PTP_PIN_SETFUNC) {
> -			pd.rsv[0] = 0;
> -			pd.rsv[1] = 0;
> -			pd.rsv[2] = 0;
> -			pd.rsv[3] = 0;
> -			pd.rsv[4] = 0;
> -		}
> -		pin_index = pd.index;
> -		if (pin_index >= ops->n_pins) {
> -			err = -EINVAL;
> -			break;
> -		}
> -		pin_index = array_index_nospec(pin_index, ops->n_pins);
> -		if (mutex_lock_interruptible(&ptp->pincfg_mux))
> -			return -ERESTARTSYS;
> -		err = ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
> -		mutex_unlock(&ptp->pincfg_mux);
> -		break;
> +		if ((pccontext->fp->f_mode & FMODE_WRITE) == 0)
> +			return -EACCES;
> +		return ptp_pin_setfunc(ptp, cmd, argptr);
>   
>   	case PTP_MASK_CLEAR_ALL:
>   		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 10/13] ptp: Split out PTP_MASK_CLEAR_ALL ioctl code

[Thomas Gleixner]

Continue the ptp_ioctl() cleanup by splitting out the PTP_MASK_CLEAR_ALL ioctl
code into a helper function.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -442,6 +442,12 @@ static long ptp_pin_setfunc(struct ptp_c
 		return ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
 }
 
+static long ptp_mask_clear_all(struct timestamp_event_queue *tsevq)
+{
+	bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
+	return 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
@@ -504,8 +510,7 @@ long ptp_ioctl(struct posix_clock_contex
 		return ptp_pin_setfunc(ptp, cmd, argptr);
 
 	case PTP_MASK_CLEAR_ALL:
-		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
-		break;
+		return ptp_mask_clear_all(pccontext->private_clkdata);
 
 	case PTP_MASK_EN_SINGLE:
 		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {

Re: [patch 10/13] ptp: Split out PTP_MASK_CLEAR_ALL ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Continue the ptp_ioctl() cleanup by splitting out the PTP_MASK_CLEAR_ALL ioctl
> code into a helper function.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |    9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -442,6 +442,12 @@ static long ptp_pin_setfunc(struct ptp_c
>   		return ptp_set_pinfunc(ptp, pin_index, pd.func, pd.chan);
>   }
>   
> +static long ptp_mask_clear_all(struct timestamp_event_queue *tsevq)
> +{
> +	bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
> +	return 0;
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
> @@ -504,8 +510,7 @@ long ptp_ioctl(struct posix_clock_contex
>   		return ptp_pin_setfunc(ptp, cmd, argptr);
>   
>   	case PTP_MASK_CLEAR_ALL:
> -		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
> -		break;
> +		return ptp_mask_clear_all(pccontext->private_clkdata);
>   
>   	case PTP_MASK_EN_SINGLE:
>   		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
> 

Not quite sure there is a benefit of having a function for this type,
apart from having one style. But it adds some LoC...

Re: [patch 10/13] ptp: Split out PTP_MASK_CLEAR_ALL ioctl code

[Thomas Gleixner]

On Sat, Jun 21 2025 at 21:36, Vadim Fedorenko wrote:
> On 20/06/2025 14:24, Thomas Gleixner wrote:
>> Continue the ptp_ioctl() cleanup by splitting out the PTP_MASK_CLEAR_ALL ioctl
>> code into a helper function.
>>   	case PTP_MASK_CLEAR_ALL:
>> -		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
>> -		break;
>> +		return ptp_mask_clear_all(pccontext->private_clkdata);
>>   
>>   	case PTP_MASK_EN_SINGLE:
>>   		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
>> 
>
> Not quite sure there is a benefit of having a function for this type,
> apart from having one style. But it adds some LoC...

Sure it's debatable benefit, but it makes the code more consistent and
does not introduce this oddball in the middle of the other function
calls.

Thanks,

        tglx

Re: [patch 10/13] ptp: Split out PTP_MASK_CLEAR_ALL ioctl code

[Vadim Fedorenko]

On 21/06/2025 21:44, Thomas Gleixner wrote:
> On Sat, Jun 21 2025 at 21:36, Vadim Fedorenko wrote:
>> On 20/06/2025 14:24, Thomas Gleixner wrote:
>>> Continue the ptp_ioctl() cleanup by splitting out the PTP_MASK_CLEAR_ALL ioctl
>>> code into a helper function.
>>>    	case PTP_MASK_CLEAR_ALL:
>>> -		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
>>> -		break;
>>> +		return ptp_mask_clear_all(pccontext->private_clkdata);
>>>    
>>>    	case PTP_MASK_EN_SINGLE:
>>>    		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
>>>
>>
>> Not quite sure there is a benefit of having a function for this type,
>> apart from having one style. But it adds some LoC...
> 
> Sure it's debatable benefit, but it makes the code more consistent and
> does not introduce this oddball in the middle of the other function
> calls.

Fair.

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 11/13] ptp: Split out PTP_MASK_EN_SINGLE ioctl code

[Thomas Gleixner]

Finish the ptp_ioctl() cleanup by splitting out the PTP_MASK_EN_SINGLE
ioctl code and removing the remaining local variables and return
statements.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -448,22 +448,28 @@ static long ptp_mask_clear_all(struct ti
 	return 0;
 }
 
+static long ptp_mask_en_single(struct timestamp_event_queue *tsevq, void __user *arg)
+{
+	unsigned int channel;
+
+	if (copy_from_user(&channel, arg, sizeof(channel)))
+		return -EFAULT;
+	if (channel >= PTP_MAX_CHANNELS)
+		return -EFAULT;
+	set_bit(channel, tsevq->mask);
+	return 0;
+}
+
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg)
 {
-	struct ptp_clock *ptp =
-		container_of(pccontext->clk, struct ptp_clock, clock);
-	struct timestamp_event_queue *tsevq;
+	struct ptp_clock *ptp = container_of(pccontext->clk, struct ptp_clock, clock);
 	void __user *argptr;
-	unsigned int i;
-	int err = 0;
 
 	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
 		arg = (unsigned long)compat_ptr(arg);
 	argptr = (void __force __user *)arg;
 
-	tsevq = pccontext->private_clkdata;
-
 	switch (cmd) {
 	case PTP_CLOCK_GETCAPS:
 	case PTP_CLOCK_GETCAPS2:
@@ -513,22 +519,11 @@ long ptp_ioctl(struct posix_clock_contex
 		return ptp_mask_clear_all(pccontext->private_clkdata);
 
 	case PTP_MASK_EN_SINGLE:
-		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
-			err = -EFAULT;
-			break;
-		}
-		if (i >= PTP_MAX_CHANNELS) {
-			err = -EFAULT;
-			break;
-		}
-		set_bit(i, tsevq->mask);
-		break;
+		return ptp_mask_en_single(pccontext->private_clkdata, argptr);
 
 	default:
-		err = -ENOTTY;
-		break;
+		return -ENOTTY;
 	}
-	return err;
 }
 
 __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,

Re: [patch 11/13] ptp: Split out PTP_MASK_EN_SINGLE ioctl code

[Vadim Fedorenko]

On 20/06/2025 14:24, Thomas Gleixner wrote:
> Finish the ptp_ioctl() cleanup by splitting out the PTP_MASK_EN_SINGLE
> ioctl code and removing the remaining local variables and return
> statements.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/ptp/ptp_chardev.c |   35 +++++++++++++++--------------------
>   1 file changed, 15 insertions(+), 20 deletions(-)
> 
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -448,22 +448,28 @@ static long ptp_mask_clear_all(struct ti
>   	return 0;
>   }
>   
> +static long ptp_mask_en_single(struct timestamp_event_queue *tsevq, void __user *arg)
> +{
> +	unsigned int channel;
> +
> +	if (copy_from_user(&channel, arg, sizeof(channel)))
> +		return -EFAULT;
> +	if (channel >= PTP_MAX_CHANNELS)
> +		return -EFAULT;
> +	set_bit(channel, tsevq->mask);
> +	return 0;
> +}
> +
>   long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   	       unsigned long arg)
>   {
> -	struct ptp_clock *ptp =
> -		container_of(pccontext->clk, struct ptp_clock, clock);
> -	struct timestamp_event_queue *tsevq;
> +	struct ptp_clock *ptp = container_of(pccontext->clk, struct ptp_clock, clock);
>   	void __user *argptr;
> -	unsigned int i;
> -	int err = 0;
>   
>   	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
>   		arg = (unsigned long)compat_ptr(arg);
>   	argptr = (void __force __user *)arg;
>   
> -	tsevq = pccontext->private_clkdata;
> -
>   	switch (cmd) {
>   	case PTP_CLOCK_GETCAPS:
>   	case PTP_CLOCK_GETCAPS2:
> @@ -513,22 +519,11 @@ long ptp_ioctl(struct posix_clock_contex
>   		return ptp_mask_clear_all(pccontext->private_clkdata);
>   
>   	case PTP_MASK_EN_SINGLE:
> -		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		if (i >= PTP_MAX_CHANNELS) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		set_bit(i, tsevq->mask);
> -		break;
> +		return ptp_mask_en_single(pccontext->private_clkdata, argptr);
>   
>   	default:
> -		err = -ENOTTY;
> -		break;
> +		return -ENOTTY;
>   	}
> -	return err;
>   }
>   
>   __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,
> 
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[patch 12/13] ptp: Convert chardev code to lock guards

[Thomas Gleixner]

Convert the various spin_lock_irqsave() protected critical regions to
scoped guards. Use spinlock_irq instead of spinlock_irqsave as all the
functions are invoked in thread context with interrupts enabled.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -110,7 +110,6 @@ int ptp_open(struct posix_clock_context
 		container_of(pccontext->clk, struct ptp_clock, clock);
 	struct timestamp_event_queue *queue;
 	char debugfsname[32];
-	unsigned long flags;
 
 	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
 	if (!queue)
@@ -122,9 +121,8 @@ int ptp_open(struct posix_clock_context
 	}
 	bitmap_set(queue->mask, 0, PTP_MAX_CHANNELS);
 	spin_lock_init(&queue->lock);
-	spin_lock_irqsave(&ptp->tsevqs_lock, flags);
-	list_add_tail(&queue->qlist, &ptp->tsevqs);
-	spin_unlock_irqrestore(&ptp->tsevqs_lock, flags);
+	scoped_guard(spinlock_irq, &ptp->tsevqs_lock)
+		list_add_tail(&queue->qlist, &ptp->tsevqs);
 	pccontext->private_clkdata = queue;
 
 	/* Debugfs contents */
@@ -143,15 +141,13 @@ int ptp_open(struct posix_clock_context
 int ptp_release(struct posix_clock_context *pccontext)
 {
 	struct timestamp_event_queue *queue = pccontext->private_clkdata;
-	unsigned long flags;
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
 
 	debugfs_remove(queue->debugfs_instance);
 	pccontext->private_clkdata = NULL;
-	spin_lock_irqsave(&ptp->tsevqs_lock, flags);
-	list_del(&queue->qlist);
-	spin_unlock_irqrestore(&ptp->tsevqs_lock, flags);
+	scoped_guard(spinlock_irq, &ptp->tsevqs_lock)
+		list_del(&queue->qlist);
 	bitmap_free(queue->mask);
 	kfree(queue);
 	return 0;
@@ -548,8 +544,6 @@ ssize_t ptp_read(struct posix_clock_cont
 		container_of(pccontext->clk, struct ptp_clock, clock);
 	struct timestamp_event_queue *queue;
 	struct ptp_extts_event *event;
-	unsigned long flags;
-	size_t qcnt, i;
 	int result;
 
 	queue = pccontext->private_clkdata;
@@ -584,21 +578,19 @@ ssize_t ptp_read(struct posix_clock_cont
 		goto exit;
 	}
 
-	spin_lock_irqsave(&queue->lock, flags);
+	scoped_guard(spinlock_irq, &queue->lock) {
+		size_t qcnt = queue_cnt(queue);
 
-	qcnt = queue_cnt(queue);
+		if (cnt > qcnt)
+			cnt = qcnt;
 
-	if (cnt > qcnt)
-		cnt = qcnt;
-
-	for (i = 0; i < cnt; i++) {
-		event[i] = queue->buf[queue->head];
-		/* Paired with READ_ONCE() in queue_cnt() */
-		WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
+		for (size_t i = 0; i < cnt; i++) {
+			event[i] = queue->buf[queue->head];
+			/* Paired with READ_ONCE() in queue_cnt() */
+			WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
+		}
 	}
 
-	spin_unlock_irqrestore(&queue->lock, flags);
-
 	cnt = cnt * sizeof(struct ptp_extts_event);
 
 	result = cnt;

[patch 13/13] ptp: Convert ptp_open/read() to __free()

[Thomas Gleixner]

Get rid of the kfree() and goto maze and just return error codes directly.

No functional change intended.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/ptp_chardev.c |   70 ++++++++++++----------------------------------
 1 file changed, 19 insertions(+), 51 deletions(-)

--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -106,19 +106,17 @@ int ptp_set_pinfunc(struct ptp_clock *pt
 
 int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
 {
-	struct ptp_clock *ptp =
-		container_of(pccontext->clk, struct ptp_clock, clock);
-	struct timestamp_event_queue *queue;
+	struct ptp_clock *ptp =	container_of(pccontext->clk, struct ptp_clock, clock);
+	struct timestamp_event_queue *queue __free(kfree) = NULL;
 	char debugfsname[32];
 
 	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
 	if (!queue)
 		return -EINVAL;
 	queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
-	if (!queue->mask) {
-		kfree(queue);
+	if (!queue->mask)
 		return -EINVAL;
-	}
+
 	bitmap_set(queue->mask, 0, PTP_MAX_CHANNELS);
 	spin_lock_init(&queue->lock);
 	scoped_guard(spinlock_irq, &ptp->tsevqs_lock)
@@ -134,7 +132,6 @@ int ptp_open(struct posix_clock_context
 		DIV_ROUND_UP(PTP_MAX_CHANNELS, BITS_PER_BYTE * sizeof(u32));
 	debugfs_create_u32_array("mask", 0444, queue->debugfs_instance,
 				 &queue->dfs_bitmap);
-
 	return 0;
 }
 
@@ -540,67 +537,38 @@ long ptp_ioctl(struct posix_clock_contex
 ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags,
 		 char __user *buf, size_t cnt)
 {
-	struct ptp_clock *ptp =
-		container_of(pccontext->clk, struct ptp_clock, clock);
-	struct timestamp_event_queue *queue;
-	struct ptp_extts_event *event;
-	int result;
+	struct ptp_clock *ptp =	container_of(pccontext->clk, struct ptp_clock, clock);
+	struct timestamp_event_queue *queue = pccontext->private_clkdata;
+	struct ptp_extts_event *event __free(kfree) = NULL;
 
-	queue = pccontext->private_clkdata;
-	if (!queue) {
-		result = -EINVAL;
-		goto exit;
-	}
+	if (!queue)
+		return -EINVAL;
 
-	if (cnt % sizeof(struct ptp_extts_event) != 0) {
-		result = -EINVAL;
-		goto exit;
-	}
+	if (cnt % sizeof(struct ptp_extts_event) != 0)
+		return -EINVAL;
 
 	if (cnt > EXTTS_BUFSIZE)
 		cnt = EXTTS_BUFSIZE;
 
-	cnt = cnt / sizeof(struct ptp_extts_event);
-
-	if (wait_event_interruptible(ptp->tsev_wq,
-				     ptp->defunct || queue_cnt(queue))) {
+	if (wait_event_interruptible(ptp->tsev_wq, ptp->defunct || queue_cnt(queue)))
 		return -ERESTARTSYS;
-	}
 
-	if (ptp->defunct) {
-		result = -ENODEV;
-		goto exit;
-	}
+	if (ptp->defunct)
+		return -ENODEV;
 
 	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
-	if (!event) {
-		result = -ENOMEM;
-		goto exit;
-	}
+	if (!event)
+		return -ENOMEM;
 
 	scoped_guard(spinlock_irq, &queue->lock) {
-		size_t qcnt = queue_cnt(queue);
-
-		if (cnt > qcnt)
-			cnt = qcnt;
+		size_t qcnt = min((size_t)queue_cnt(queue), cnt / sizeof(*event));
 
-		for (size_t i = 0; i < cnt; i++) {
+		for (size_t i = 0; i < qcnt; i++) {
 			event[i] = queue->buf[queue->head];
 			/* Paired with READ_ONCE() in queue_cnt() */
 			WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
 		}
 	}
 
-	cnt = cnt * sizeof(struct ptp_extts_event);
-
-	result = cnt;
-	if (copy_to_user(buf, event, cnt)) {
-		result = -EFAULT;
-		goto free_event;
-	}
-
-free_event:
-	kfree(event);
-exit:
-	return result;
+	return copy_to_user(buf, event, cnt) ? -EFAULT : cnt;
 }

Re: [patch 13/13] ptp: Convert ptp_open/read() to __free()

[Paolo Abeni]

On 6/20/25 3:24 PM, Thomas Gleixner wrote:
>  	scoped_guard(spinlock_irq, &queue->lock) {
> -		size_t qcnt = queue_cnt(queue);
> -
> -		if (cnt > qcnt)
> -			cnt = qcnt;
> +		size_t qcnt = min((size_t)queue_cnt(queue), cnt / sizeof(*event));
>  
> -		for (size_t i = 0; i < cnt; i++) {
> +		for (size_t i = 0; i < qcnt; i++) {
>  			event[i] = queue->buf[queue->head];
>  			/* Paired with READ_ONCE() in queue_cnt() */
>  			WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
>  		}
>  	}
>  
> -	cnt = cnt * sizeof(struct ptp_extts_event);
> -
> -	result = cnt;
> -	if (copy_to_user(buf, event, cnt)) {
> -		result = -EFAULT;
> -		goto free_event;
> -	}
> -
> -free_event:
> -	kfree(event);
> -exit:
> -	return result;
> +	return copy_to_user(buf, event, cnt) ? -EFAULT : cnt;
>  }

I'm likely low on coffee, but it looks like the above code is now
returning the original amount of provided events, while the existing
code returns the value bounded to the number of queue events.

Cheers,

Paolo

	

Re: [patch 13/13] ptp: Convert ptp_open/read() to __free()

[Thomas Gleixner]

On Tue, Jun 24 2025 at 11:48, Paolo Abeni wrote:
> On 6/20/25 3:24 PM, Thomas Gleixner wrote:
>>  	scoped_guard(spinlock_irq, &queue->lock) {
>> -		size_t qcnt = queue_cnt(queue);
>> -
>> -		if (cnt > qcnt)
>> -			cnt = qcnt;
>> +		size_t qcnt = min((size_t)queue_cnt(queue), cnt / sizeof(*event));
>>  
>> -		for (size_t i = 0; i < cnt; i++) {
>> +		for (size_t i = 0; i < qcnt; i++) {
>>  			event[i] = queue->buf[queue->head];
>>  			/* Paired with READ_ONCE() in queue_cnt() */
>>  			WRITE_ONCE(queue->head, (queue->head + 1) % PTP_MAX_TIMESTAMPS);
>>  		}
>>  	}
>>  
>> -	cnt = cnt * sizeof(struct ptp_extts_event);
>> -
>> -	result = cnt;
>> -	if (copy_to_user(buf, event, cnt)) {
>> -		result = -EFAULT;
>> -		goto free_event;
>> -	}
>> -
>> -free_event:
>> -	kfree(event);
>> -exit:
>> -	return result;
>> +	return copy_to_user(buf, event, cnt) ? -EFAULT : cnt;
>>  }
>
> I'm likely low on coffee, but it looks like the above code is now
> returning the original amount of provided events, while the existing
> code returns the value bounded to the number of queue events.

Duh. Indeed. Well spotted.

Thanks,

        tglx

Re: [patch 13/13] ptp: Convert ptp_open/read() to __free()

[Jakub Kicinski]

On Fri, 20 Jun 2025 15:24:50 +0200 (CEST) Thomas Gleixner wrote:
> Get rid of the kfree() and goto maze and just return error codes directly.

Maybe just skip this patch?  FWIW we prefer not to use __free()
within networking code.  But this is as much time as networking
so up to you.

  Using device-managed and cleanup.h constructs
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   [...]

  Low level cleanup constructs (such as ``__free()``) can be used when building
  APIs and helpers, especially scoped iterators. However, direct use of
  ``__free()`` within networking core and drivers is discouraged.
  Similar guidance applies to declaring variables mid-function.
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

Re: [patch 13/13] ptp: Convert ptp_open/read() to __free()

[Thomas Gleixner]

On Tue, Jun 24 2025 at 09:36, Jakub Kicinski wrote:
> On Fri, 20 Jun 2025 15:24:50 +0200 (CEST) Thomas Gleixner wrote:
>> Get rid of the kfree() and goto maze and just return error codes directly.
>
> Maybe just skip this patch?  FWIW we prefer not to use __free()
> within networking code.  But this is as much time as networking
> so up to you.
>
>   Using device-managed and cleanup.h constructs
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>    [...]
>
>   Low level cleanup constructs (such as ``__free()``) can be used when building
>   APIs and helpers, especially scoped iterators. However, direct use of
>   ``__free()`` within networking core and drivers is discouraged.
>   Similar guidance applies to declaring variables mid-function.

Interesting decision, unfortunately it lacks a rationale in that
documentation.

I reworked the patch without the __free(), which still cleans up the mix
of goto exit and return ERRCODE inconsistencies.

Let me send out V3 with that and network/ptp people can still decided to
ignore it :)

Thanks,

        tglx