1. Patchew
  2. linux
  3. [patch 00/13] ptp: Belated spring cleaning of the chardev driver
  4. View patch

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

Thomas Gleixner posted 13 patches 3 months, 2 weeks ago
There is a newer version of this series
[patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code
Posted by Thomas Gleixner 3 months, 2 weeks ago 
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
Posted by Paolo Abeni 3 months, 2 weeks ago 
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
Posted by Thomas Gleixner 3 months, 2 weeks ago 
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
Posted by Thomas Gleixner 3 months, 2 weeks ago 
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
Re: [patch 08/13] ptp: Split out PTP_PIN_GETFUNC ioctl code
Posted by Vadim Fedorenko 3 months, 2 weeks ago 
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>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.