[PATCH 0/4] kfifo: small cleanup + linear and skip helpers

[PATCH 0/4] kfifo: small cleanup + linear and skip helpers

[Jiri Slaby (SUSE)]

Hi,

this is a small cleanup of kfifo.

More importantly, it introduces three helpers: kfifo_skip_count(),
kfifo_out_linear() and kfifo_out_linear_ptr(). They allow using kfifo in
the serial layer.

They are needed as up to now, there is no way to "peek" the data without
actually copying to a temporary buffer. kfifo_out_linear*() allow that
and kfifo_skip_count() then allows for tail moving after the copy is
done. Note the copy is usually performed directly to hardware e.g. via
outb(), hence the need for peek without copying anywhere.

Jiri Slaby (SUSE) (4):
  kfifo: drop __kfifo_dma_out_finish_r()
  kfifo: introduce and use kfifo_skip_count()
  kfifo: add kfifo_out_linear{,_ptr}()
  kfifo: fix typos in kernel-doc

 include/linux/kfifo.h | 105 +++++++++++++++++++++++++++++++++---------
 lib/kfifo.c           |  36 +++++++++++----
 2 files changed, 111 insertions(+), 30 deletions(-)

-- 
2.43.2

[PATCH 1/4] kfifo: drop __kfifo_dma_out_finish_r()

[Jiri Slaby (SUSE)]

It is the same as __kfifo_skip_r(), so:
* drop __kfifo_dma_out_finish_r() completely, and
* replace its (only) use by __kfifo_skip_r().

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Stefani Seibold <stefani@seibold.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/kfifo.h | 4 +---
 lib/kfifo.c           | 8 --------
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 0b35a41440ff..bc7a1f5bb0ce 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -797,7 +797,7 @@ __kfifo_int_must_check_helper( \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
 	if (__recsize) \
-		__kfifo_dma_out_finish_r(__kfifo, __recsize); \
+		__kfifo_skip_r(__kfifo, __recsize); \
 	else \
 		__kfifo->out += __len / sizeof(*__tmp->type); \
 })
@@ -879,8 +879,6 @@ extern void __kfifo_dma_in_finish_r(struct __kfifo *fifo,
 extern unsigned int __kfifo_dma_out_prepare_r(struct __kfifo *fifo,
 	struct scatterlist *sgl, int nents, unsigned int len, size_t recsize);
 
-extern void __kfifo_dma_out_finish_r(struct __kfifo *fifo, size_t recsize);
-
 extern unsigned int __kfifo_len_r(struct __kfifo *fifo, size_t recsize);
 
 extern void __kfifo_skip_r(struct __kfifo *fifo, size_t recsize);
diff --git a/lib/kfifo.c b/lib/kfifo.c
index 12f5a347aa13..958099cc4914 100644
--- a/lib/kfifo.c
+++ b/lib/kfifo.c
@@ -582,11 +582,3 @@ unsigned int __kfifo_dma_out_prepare_r(struct __kfifo *fifo,
 }
 EXPORT_SYMBOL(__kfifo_dma_out_prepare_r);
 
-void __kfifo_dma_out_finish_r(struct __kfifo *fifo, size_t recsize)
-{
-	unsigned int len;
-
-	len = __kfifo_peek_n(fifo, recsize);
-	fifo->out += len + recsize;
-}
-EXPORT_SYMBOL(__kfifo_dma_out_finish_r);
-- 
2.43.2

Re: [PATCH 1/4] kfifo: drop __kfifo_dma_out_finish_r()

[Andrew Morton]

These four look OK to me.  Would it make sense to upstream them
via the serial tree?

[PATCH 2/4] kfifo: introduce and use kfifo_skip_count()

[Jiri Slaby (SUSE)]

kfifo_skip_count() is an extended version of kfifo_skip(), accepting
also count. This will be useful in the serial code later.

Now, it can be used to implement both kfifo_skip() and
kfifo_dma_out_finish(). In the latter, 'len' only needs to be divided by
'type' size (as it was until now).

And stop using statement expressions when the return value is cast to
'void'. Use classic 'do {} while (0)' instead.

Note: perhaps we should skip 'count' records for the 'recsize' case, but
the original (kfifo_dma_out_finish()) used to skip only one record. So
this is kept unchanged and 'count' is still ignored in the recsize case.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Stefani Seibold <stefani@seibold.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 include/linux/kfifo.h | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index bc7a1f5bb0ce..9e64eb03c535 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -304,19 +304,25 @@ __kfifo_uint_must_check_helper( \
 )
 
 /**
- * kfifo_skip - skip output data
+ * kfifo_skip_count - skip output data
  * @fifo: address of the fifo to be used
+ * @count: count of data to skip
  */
-#define	kfifo_skip(fifo) \
-(void)({ \
+#define	kfifo_skip_count(fifo, count) do { \
 	typeof((fifo) + 1) __tmp = (fifo); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
 	if (__recsize) \
 		__kfifo_skip_r(__kfifo, __recsize); \
 	else \
-		__kfifo->out++; \
-})
+		__kfifo->out += (count); \
+} while(0)
+
+/**
+ * kfifo_skip - skip output data
+ * @fifo: address of the fifo to be used
+ */
+#define	kfifo_skip(fifo)	kfifo_skip_count(fifo, 1)
 
 /**
  * kfifo_peek_len - gets the size of the next fifo record
@@ -790,17 +796,10 @@ __kfifo_int_must_check_helper( \
  * Note that with only one concurrent reader and one concurrent
  * writer, you don't need extra locking to use these macros.
  */
-#define kfifo_dma_out_finish(fifo, len) \
-(void)({ \
+#define kfifo_dma_out_finish(fifo, len) do { \
 	typeof((fifo) + 1) __tmp = (fifo); \
-	unsigned int __len = (len); \
-	const size_t __recsize = sizeof(*__tmp->rectype); \
-	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (__recsize) \
-		__kfifo_skip_r(__kfifo, __recsize); \
-	else \
-		__kfifo->out += __len / sizeof(*__tmp->type); \
-})
+	kfifo_skip_count(__tmp, (len) / sizeof(*__tmp->type)); \
+} while (0)
 
 /**
  * kfifo_out_peek - gets some data from the fifo
-- 
2.43.2

[PATCH 3/4] kfifo: add kfifo_out_linear{,_ptr}()

[Jiri Slaby (SUSE)]

These are helpers which are going to be used in the serial layer. We
need a wrapper around kfifo which provides us with a tail (sometimes
"tail" offset, sometimes a pointer) to the kfifo data. And which returns
count of available data -- but not larger than to the end of the buffer
(hence _linear in the names). I.e. something like CIRC_CNT_TO_END() in
the legacy circ_buf.

This patch adds such two helpers.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Stefani Seibold <stefani@seibold.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/kfifo.h | 64 +++++++++++++++++++++++++++++++++++++++++++
 lib/kfifo.c           | 28 +++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 9e64eb03c535..0491bc526240 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -827,6 +827,64 @@ __kfifo_uint_must_check_helper( \
 }) \
 )
 
+/**
+ * kfifo_out_linear - gets a tail of/offset to available data
+ * @fifo: address of the fifo to be used
+ * @tail: pointer to an unsigned int to store the value of tail
+ * @n: max. number of elements to point at
+ *
+ * This macro obtains the offset (tail) to the available data in the fifo
+ * buffer and returns the
+ * numbers of elements available. It returns the available count till the end
+ * of data or till the end of the buffer. So that it can be used for linear
+ * data processing (like memcpy() of (@fifo->data + @tail) with count
+ * returned).
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these macro.
+ */
+#define kfifo_out_linear(fifo, tail, n) \
+__kfifo_uint_must_check_helper( \
+({ \
+	typeof((fifo) + 1) __tmp = (fifo); \
+	unsigned int *__tail = (tail); \
+	unsigned long __n = (n); \
+	const size_t __recsize = sizeof(*__tmp->rectype); \
+	struct __kfifo *__kfifo = &__tmp->kfifo; \
+	(__recsize) ? \
+	__kfifo_out_linear_r(__kfifo, __tail, __n, __recsize) : \
+	__kfifo_out_linear(__kfifo, __tail, __n); \
+}) \
+)
+
+/**
+ * kfifo_out_linear_ptr - gets a pointer to the available data
+ * @fifo: address of the fifo to be used
+ * @ptr: pointer to data to store the pointer to tail
+ * @n: max. number of elements to point at
+ *
+ * Similarly to kfifo_out_linear(), this macro obtains the pointer to the
+ * available data in the fifo buffer and returns the numbers of elements
+ * available. It returns the available count till the end of available data or
+ * till the end of the buffer. So that it can be used for linear data
+ * processing (like memcpy() of @ptr with count returned).
+ *
+ * Note that with only one concurrent reader and one concurrent
+ * writer, you don't need extra locking to use these macro.
+ */
+#define kfifo_out_linear_ptr(fifo, ptr, n) \
+__kfifo_uint_must_check_helper( \
+({ \
+	typeof((fifo) + 1) ___tmp = (fifo); \
+	unsigned int ___tail; \
+	unsigned int ___n = kfifo_out_linear(___tmp, &___tail, (n)); \
+	struct __kfifo *___kfifo = &___tmp->kfifo; \
+	*(ptr) = ___kfifo->data + ___tail * kfifo_esize(___tmp); \
+	___n; \
+}) \
+)
+
+
 extern int __kfifo_alloc(struct __kfifo *fifo, unsigned int size,
 	size_t esize, gfp_t gfp_mask);
 
@@ -856,6 +914,9 @@ extern unsigned int __kfifo_dma_out_prepare(struct __kfifo *fifo,
 extern unsigned int __kfifo_out_peek(struct __kfifo *fifo,
 	void *buf, unsigned int len);
 
+extern unsigned int __kfifo_out_linear(struct __kfifo *fifo,
+	unsigned int *tail, unsigned int n);
+
 extern unsigned int __kfifo_in_r(struct __kfifo *fifo,
 	const void *buf, unsigned int len, size_t recsize);
 
@@ -885,6 +946,9 @@ extern void __kfifo_skip_r(struct __kfifo *fifo, size_t recsize);
 extern unsigned int __kfifo_out_peek_r(struct __kfifo *fifo,
 	void *buf, unsigned int len, size_t recsize);
 
+extern unsigned int __kfifo_out_linear_r(struct __kfifo *fifo,
+	unsigned int *tail, unsigned int n, size_t recsize);
+
 extern unsigned int __kfifo_max_r(unsigned int len, size_t recsize);
 
 #endif
diff --git a/lib/kfifo.c b/lib/kfifo.c
index 958099cc4914..9f1d8f68deb8 100644
--- a/lib/kfifo.c
+++ b/lib/kfifo.c
@@ -163,6 +163,21 @@ unsigned int __kfifo_out_peek(struct __kfifo *fifo,
 }
 EXPORT_SYMBOL(__kfifo_out_peek);
 
+unsigned int __kfifo_out_linear(struct __kfifo *fifo,
+		unsigned int *tail, unsigned int n)
+{
+	unsigned int size = fifo->mask + 1;
+	unsigned int off = fifo->out & fifo->mask;
+
+	n = min3(n, fifo->in - fifo->out, size - off);
+
+	if (tail)
+		*tail = off;
+
+	return n;
+}
+EXPORT_SYMBOL(__kfifo_out_linear);
+
 unsigned int __kfifo_out(struct __kfifo *fifo,
 		void *buf, unsigned int len)
 {
@@ -473,6 +488,19 @@ unsigned int __kfifo_out_peek_r(struct __kfifo *fifo, void *buf,
 }
 EXPORT_SYMBOL(__kfifo_out_peek_r);
 
+unsigned int __kfifo_out_linear_r(struct __kfifo *fifo,
+		unsigned int *tail, unsigned int n, size_t recsize)
+{
+	if (fifo->in == fifo->out)
+		return 0;
+
+	if (tail)
+		*tail = fifo->out + recsize;
+
+	return min(n, __kfifo_peek_n(fifo, recsize);
+}
+EXPORT_SYMBOL(__kfifo_out_linear_r);
+
 unsigned int __kfifo_out_r(struct __kfifo *fifo, void *buf,
 		unsigned int len, size_t recsize)
 {
-- 
2.43.2

Re: [PATCH 3/4] kfifo: add kfifo_out_linear{,_ptr}()

[Andrew Morton]

On Fri, 23 Feb 2024 12:51:54 +0100 "Jiri Slaby (SUSE)" <jirislaby@kernel.org> wrote:

> +	return min(n, __kfifo_peek_n(fifo, recsize);

When things like this happen I wonder "was I sent the correct version"
and "was this tested" and "wtf".

Please confirm that with this patch:

--- a/lib/kfifo.c~a
+++ a/lib/kfifo.c
@@ -497,7 +497,7 @@ unsigned int __kfifo_out_linear_r(struct
 	if (tail)
 		*tail = fifo->out + recsize;
 
-	return min(n, __kfifo_peek_n(fifo, recsize);
+	return min(n, __kfifo_peek_n(fifo, recsize));
 }
 EXPORT_SYMBOL(__kfifo_out_linear_r);
 
@@ -609,4 +609,3 @@ unsigned int __kfifo_dma_out_prepare_r(s
 	return setup_sgl(fifo, sgl, nents, len, fifo->out + recsize);
 }
 EXPORT_SYMBOL(__kfifo_dma_out_prepare_r);

we have a fully tested, production quality kernel change?

Re: [PATCH 3/4] kfifo: add kfifo_out_linear{,_ptr}()

[Jiri Slaby]

On 24. 02. 24, 3:31, Andrew Morton wrote:
> On Fri, 23 Feb 2024 12:51:54 +0100 "Jiri Slaby (SUSE)" <jirislaby@kernel.org> wrote:
> 
>> +	return min(n, __kfifo_peek_n(fifo, recsize);
> 
> When things like this happen I wonder "was I sent the correct version"
> and "was this tested" and "wtf".
> 
> Please confirm that with this patch:
> 
> --- a/lib/kfifo.c~a
> +++ a/lib/kfifo.c
> @@ -497,7 +497,7 @@ unsigned int __kfifo_out_linear_r(struct
>   	if (tail)
>   		*tail = fifo->out + recsize;
>   
> -	return min(n, __kfifo_peek_n(fifo, recsize);
> +	return min(n, __kfifo_peek_n(fifo, recsize));
>   }
>   EXPORT_SYMBOL(__kfifo_out_linear_r);
>   
> @@ -609,4 +609,3 @@ unsigned int __kfifo_dma_out_prepare_r(s
>   	return setup_sgl(fifo, sgl, nents, len, fifo->out + recsize);
>   }
>   EXPORT_SYMBOL(__kfifo_dma_out_prepare_r);
> 
> we have a fully tested, production quality kernel change?

After Friday's last minute changes, there are apparently more issues (as 
reported by the test robot). Please drop this series, I will send a 
better one.

And I understand that you are OK for this to go through serial, so I 
will post with the serial "users" too. Of course with you in Cc, so that 
you can (N)Ack.

thanks and sorry for the bogus submission,
-- 
js
suse labs

[PATCH 4/4] kfifo: fix typos in kernel-doc

[Jiri Slaby (SUSE)]

Obviously:
"This macro finish" -> "This macro finishes"
and similar.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Stefani Seibold <stefani@seibold.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/kfifo.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 0491bc526240..d8533e700bed 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -584,7 +584,7 @@ __kfifo_uint_must_check_helper( \
  * @buf: pointer to the storage buffer
  * @n: max. number of elements to get
  *
- * This macro get some data from the fifo and return the numbers of elements
+ * This macro gets some data from the fifo and returns the numbers of elements
  * copied.
  *
  * Note that with only one concurrent reader and one concurrent
@@ -611,7 +611,7 @@ __kfifo_uint_must_check_helper( \
  * @n: max. number of elements to get
  * @lock: pointer to the spinlock to use for locking
  *
- * This macro get the data from the fifo and return the numbers of elements
+ * This macro gets the data from the fifo and returns the numbers of elements
  * copied.
  */
 #define	kfifo_out_spinlocked(fifo, buf, n, lock) \
@@ -739,7 +739,7 @@ __kfifo_int_must_check_helper( \
  * @fifo: address of the fifo to be used
  * @len: number of bytes to received
  *
- * This macro finish a DMA IN operation. The in counter will be updated by
+ * This macro finishes a DMA IN operation. The in counter will be updated by
  * the len parameter. No error checking will be done.
  *
  * Note that with only one concurrent reader and one concurrent
@@ -790,7 +790,7 @@ __kfifo_int_must_check_helper( \
  * @fifo: address of the fifo to be used
  * @len: number of bytes transferred
  *
- * This macro finish a DMA OUT operation. The out counter will be updated by
+ * This macro finishes a DMA OUT operation. The out counter will be updated by
  * the len parameter. No error checking will be done.
  *
  * Note that with only one concurrent reader and one concurrent
@@ -807,7 +807,7 @@ __kfifo_int_must_check_helper( \
  * @buf: pointer to the storage buffer
  * @n: max. number of elements to get
  *
- * This macro get the data from the fifo and return the numbers of elements
+ * This macro gets the data from the fifo and returns the numbers of elements
  * copied. The data is not removed from the fifo.
  *
  * Note that with only one concurrent reader and one concurrent
-- 
2.43.2

Re: [PATCH 0/4] kfifo: small cleanup + linear and skip helpers

[Jiri Slaby]

On 23. 02. 24, 12:51, Jiri Slaby (SUSE) wrote:
> Hi,
> 
> this is a small cleanup of kfifo.
> 
> More importantly, it introduces three helpers: kfifo_skip_count(),
> kfifo_out_linear() and kfifo_out_linear_ptr(). They allow using kfifo in
> the serial layer.
> 
> They are needed as up to now, there is no way to "peek" the data without
> actually copying to a temporary buffer. kfifo_out_linear*() allow that
> and kfifo_skip_count() then allows for tail moving after the copy is
> done. Note the copy is usually performed directly to hardware e.g. via
> outb(), hence the need for peek without copying anywhere.

Sorry, this reasoning is bogus. outb() would be using kfifo_out(), of 
course.

The requirement comes from (for example):
* memcpy_toio() in neo_copy_data_from_queue_to_uart(),
* regmap_noinc_write() in max310x_handle_tx()),
* and similar.

The kfifo data are there directly copied to HW in a batch, without the 
need of temp buffers.

thanks,
-- 
js
suse labs