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;
 }

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

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

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

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