printk: ringbuffer: fix error in comment

[PATCH] printk: ringbuffer: fix error in comment

Posted by Loïc Grégoire 5 days, 10 hours ago

The printk ringbuffer implementation is described in the comment as
using three ringbuffers, but the current implementation uses two (desc
and data). Update the comment so it matches the code.

Signed-off-by: Loïc Grégoire <loicgre@gmail.com>
---
 kernel/printk/printk_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 56c8e3d031f4..11dae8229103 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -14,7 +14,7 @@
  *
  * Data Structure
  * --------------
- * The printk_ringbuffer is made up of 3 internal ringbuffers:
+ * The printk_ringbuffer is made up of 2 internal ringbuffers:
  *
  *   desc_ring
  *     A ring of descriptors and their meta data (such as sequence number,
-- 
2.53.0

Re: [PATCH] printk: ringbuffer: fix error in comment

Posted by John Ogness 3 days, 4 hours ago

On 2026-03-27, Loïc Grégoire <loicgre@gmail.com> wrote:
> The printk ringbuffer implementation is described in the comment as
> using three ringbuffers, but the current implementation uses two (desc
> and data). Update the comment so it matches the code.
>
> Signed-off-by: Loïc Grégoire <loicgre@gmail.com>

Reviewed-by: John Ogness <john.ogness@linutronix.de>

However, this is only a single fixup when there are other documentation
errors that need fixing as well. Does it make sense to commit such tiny
fixups separately? For example, below are needed fixes that I am aware
of.

Perhaps we should fold these into a single doc-fixup patch?

John

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 56c8e3d031f49..3f019207d2688 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -224,7 +224,7 @@
  *
  *	prb_rec_init_rd(&r, &info, &text_buf[0], sizeof(text_buf));
  *
- *	prb_for_each_record(0, &test_rb, &seq, &r) {
+ *	prb_for_each_record(0, &test_rb, seq, &r) {
  *		if (info.seq != seq)
  *			pr_warn("lost %llu records\n", info.seq - seq);
  *
@@ -1365,7 +1365,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
 	 *
 	 * WMB from _prb_commit:A to _prb_commit:B
 	 *    matching
-	 * MB If desc_reopen_last:A to prb_reserve_in_last:A
+	 * MB from desc_reopen_last:A to prb_reserve_in_last:A
 	 */
 	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
 			DESC_SV(id, desc_reserved))) { /* LMM(desc_reopen_last:A) */
@@ -1770,9 +1770,9 @@ static void _prb_commit(struct prb_reserved_entry *e, unsigned long state_val)
 	 *
 	 *    Relies on:
 	 *
-	 *    MB _prb_commit:B to prb_commit:A
+	 *    MB from _prb_commit:B to prb_commit:A
 	 *       matching
-	 *    MB desc_reserve:D to desc_make_final:A
+	 *    MB from desc_reserve:D to desc_make_final:A
 	 */
 	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
 			DESC_SV(e->id, state_val))) { /* LMM(_prb_commit:B) */
@@ -2035,7 +2035,7 @@ u64 prb_first_seq(struct printk_ringbuffer *rb)
 		 *
 		 * MB from desc_push_tail:B to desc_reserve:F
 		 *    matching
-		 * RMB prb_first_seq:B to prb_first_seq:A
+		 * RMB from prb_first_seq:B to prb_first_seq:A
 		 */
 		smp_rmb(); /* LMM(prb_first_seq:C) */
 	}
diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
index 1651b53ece34f..fb7d2ee29af23 100644
--- a/kernel/printk/printk_ringbuffer.h
+++ b/kernel/printk/printk_ringbuffer.h
@@ -388,7 +388,7 @@ for ((s) = from; prb_read_valid(rb, s, r); (s) = (r)->info->seq + 1)
  *
  * This is a macro for conveniently iterating over a ringbuffer.
  * Note that @s may not be the sequence number of the record on each
- * iteration. For the sequence number, @r->info->seq should be checked.
+ * iteration. For the sequence number, @i->seq should be checked.
  *
  * Context: Any context.
  */

Re: [PATCH] printk: ringbuffer: fix error in comment

Posted by Petr Mladek 2 days, 23 hours ago

On Mon 2026-03-30 10:12:08, John Ogness wrote:
> On 2026-03-27, Loïc Grégoire <loicgre@gmail.com> wrote:
> > The printk ringbuffer implementation is described in the comment as
> > using three ringbuffers, but the current implementation uses two (desc
> > and data). Update the comment so it matches the code.
> >
> > Signed-off-by: Loïc Grégoire <loicgre@gmail.com>
> 
> Reviewed-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

> However, this is only a single fixup when there are other documentation
> errors that need fixing as well. Does it make sense to commit such tiny
> fixups separately? For example, below are needed fixes that I am aware
> of.
> 
> Perhaps we should fold these into a single doc-fixup patch?
>
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 56c8e3d031f49..3f019207d2688 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -224,7 +224,7 @@
>   *
>   *	prb_rec_init_rd(&r, &info, &text_buf[0], sizeof(text_buf));
>   *
> - *	prb_for_each_record(0, &test_rb, &seq, &r) {
> + *	prb_for_each_record(0, &test_rb, seq, &r) {
>   *		if (info.seq != seq)
>   *			pr_warn("lost %llu records\n", info.seq - seq);
>   *
> @@ -1365,7 +1365,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
>  	 *
>  	 * WMB from _prb_commit:A to _prb_commit:B
>  	 *    matching
> -	 * MB If desc_reopen_last:A to prb_reserve_in_last:A
> +	 * MB from desc_reopen_last:A to prb_reserve_in_last:A
>  	 */
>  	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
>  			DESC_SV(id, desc_reserved))) { /* LMM(desc_reopen_last:A) */
> @@ -1770,9 +1770,9 @@ static void _prb_commit(struct prb_reserved_entry *e, unsigned long state_val)
>  	 *
>  	 *    Relies on:
>  	 *
> -	 *    MB _prb_commit:B to prb_commit:A
> +	 *    MB from _prb_commit:B to prb_commit:A
>  	 *       matching
> -	 *    MB desc_reserve:D to desc_make_final:A
> +	 *    MB from desc_reserve:D to desc_make_final:A
>  	 */
>  	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
>  			DESC_SV(e->id, state_val))) { /* LMM(_prb_commit:B) */
> @@ -2035,7 +2035,7 @@ u64 prb_first_seq(struct printk_ringbuffer *rb)
>  		 *
>  		 * MB from desc_push_tail:B to desc_reserve:F
>  		 *    matching
> -		 * RMB prb_first_seq:B to prb_first_seq:A
> +		 * RMB from prb_first_seq:B to prb_first_seq:A
>  		 */
>  		smp_rmb(); /* LMM(prb_first_seq:C) */
>  	}
> diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
> index 1651b53ece34f..fb7d2ee29af23 100644
> --- a/kernel/printk/printk_ringbuffer.h
> +++ b/kernel/printk/printk_ringbuffer.h
> @@ -388,7 +388,7 @@ for ((s) = from; prb_read_valid(rb, s, r); (s) = (r)->info->seq + 1)
>   *
>   * This is a macro for conveniently iterating over a ringbuffer.
>   * Note that @s may not be the sequence number of the record on each
> - * iteration. For the sequence number, @r->info->seq should be checked.
> + * iteration. For the sequence number, @i->seq should be checked.
>   *
>   * Context: Any context.
>   */

All these changes look correct as well:

Reviewed-by: Petr Mladek <pmladek@suse.com>

I could squash these changes into the original patch when comitting
into printk/linux.git.

John, could I use your Signed-off-by to mark you as a co-author
and keep Loïc as the author of the patch?

Best Regards,
Petr

Re: [PATCH] printk: ringbuffer: fix error in comment

Posted by John Ogness 2 days, 22 hours ago

On 2026-03-30, Petr Mladek <pmladek@suse.com> wrote:
> On Mon 2026-03-30 10:12:08, John Ogness wrote:
>> On 2026-03-27, Loïc Grégoire <loicgre@gmail.com> wrote:
>> > The printk ringbuffer implementation is described in the comment as
>> > using three ringbuffers, but the current implementation uses two (desc
>> > and data). Update the comment so it matches the code.
>> >
>> > Signed-off-by: Loïc Grégoire <loicgre@gmail.com>
>> 
>> Reviewed-by: John Ogness <john.ogness@linutronix.de>
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
>> However, this is only a single fixup when there are other documentation
>> errors that need fixing as well. Does it make sense to commit such tiny
>> fixups separately? For example, below are needed fixes that I am aware
>> of.
>> 
>> Perhaps we should fold these into a single doc-fixup patch?
>>
>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>> index 56c8e3d031f49..3f019207d2688 100644
>> --- a/kernel/printk/printk_ringbuffer.c
>> +++ b/kernel/printk/printk_ringbuffer.c
>> @@ -224,7 +224,7 @@
>>   *
>>   *	prb_rec_init_rd(&r, &info, &text_buf[0], sizeof(text_buf));
>>   *
>> - *	prb_for_each_record(0, &test_rb, &seq, &r) {
>> + *	prb_for_each_record(0, &test_rb, seq, &r) {
>>   *		if (info.seq != seq)
>>   *			pr_warn("lost %llu records\n", info.seq - seq);
>>   *
>> @@ -1365,7 +1365,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
>>  	 *
>>  	 * WMB from _prb_commit:A to _prb_commit:B
>>  	 *    matching
>> -	 * MB If desc_reopen_last:A to prb_reserve_in_last:A
>> +	 * MB from desc_reopen_last:A to prb_reserve_in_last:A
>>  	 */
>>  	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
>>  			DESC_SV(id, desc_reserved))) { /* LMM(desc_reopen_last:A) */
>> @@ -1770,9 +1770,9 @@ static void _prb_commit(struct prb_reserved_entry *e, unsigned long state_val)
>>  	 *
>>  	 *    Relies on:
>>  	 *
>> -	 *    MB _prb_commit:B to prb_commit:A
>> +	 *    MB from _prb_commit:B to prb_commit:A
>>  	 *       matching
>> -	 *    MB desc_reserve:D to desc_make_final:A
>> +	 *    MB from desc_reserve:D to desc_make_final:A
>>  	 */
>>  	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
>>  			DESC_SV(e->id, state_val))) { /* LMM(_prb_commit:B) */
>> @@ -2035,7 +2035,7 @@ u64 prb_first_seq(struct printk_ringbuffer *rb)
>>  		 *
>>  		 * MB from desc_push_tail:B to desc_reserve:F
>>  		 *    matching
>> -		 * RMB prb_first_seq:B to prb_first_seq:A
>> +		 * RMB from prb_first_seq:B to prb_first_seq:A
>>  		 */
>>  		smp_rmb(); /* LMM(prb_first_seq:C) */
>>  	}
>> diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
>> index 1651b53ece34f..fb7d2ee29af23 100644
>> --- a/kernel/printk/printk_ringbuffer.h
>> +++ b/kernel/printk/printk_ringbuffer.h
>> @@ -388,7 +388,7 @@ for ((s) = from; prb_read_valid(rb, s, r); (s) = (r)->info->seq + 1)
>>   *
>>   * This is a macro for conveniently iterating over a ringbuffer.
>>   * Note that @s may not be the sequence number of the record on each
>> - * iteration. For the sequence number, @r->info->seq should be checked.
>> + * iteration. For the sequence number, @i->seq should be checked.
>>   *
>>   * Context: Any context.
>>   */
>
> All these changes look correct as well:
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
> I could squash these changes into the original patch when comitting
> into printk/linux.git.
>
> John, could I use your Signed-off-by to mark you as a co-author
> and keep Loïc as the author of the patch?

Sure.

Signed-off-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH] printk: ringbuffer: fix error in comment

Posted by Petr Mladek 1 day, 20 hours ago

On Mon 2026-03-30 16:20:39, John Ogness wrote:
> On 2026-03-30, Petr Mladek <pmladek@suse.com> wrote:
> > On Mon 2026-03-30 10:12:08, John Ogness wrote:
> >> On 2026-03-27, Loïc Grégoire <loicgre@gmail.com> wrote:
> >> > The printk ringbuffer implementation is described in the comment as
> >> > using three ringbuffers, but the current implementation uses two (desc
> >> > and data). Update the comment so it matches the code.
> >> >
> >> > Signed-off-by: Loïc Grégoire <loicgre@gmail.com>
> >> 
> >> Reviewed-by: John Ogness <john.ogness@linutronix.de>
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> >> However, this is only a single fixup when there are other documentation
> >> errors that need fixing as well. Does it make sense to commit such tiny
> >> fixups separately? For example, below are needed fixes that I am aware
> >> of.
> >> 
> >> Perhaps we should fold these into a single doc-fixup patch?
> >>
> >> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> >> index 56c8e3d031f49..3f019207d2688 100644
> >> --- a/kernel/printk/printk_ringbuffer.c
> >> +++ b/kernel/printk/printk_ringbuffer.c
> >> @@ -224,7 +224,7 @@
> >>   *
> >>   *	prb_rec_init_rd(&r, &info, &text_buf[0], sizeof(text_buf));
> >>   *
> >> - *	prb_for_each_record(0, &test_rb, &seq, &r) {
> >> + *	prb_for_each_record(0, &test_rb, seq, &r) {
> >>   *		if (info.seq != seq)
> >>   *			pr_warn("lost %llu records\n", info.seq - seq);
> >>   *
> >> @@ -1365,7 +1365,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
> >>  	 *
> >>  	 * WMB from _prb_commit:A to _prb_commit:B
> >>  	 *    matching
> >> -	 * MB If desc_reopen_last:A to prb_reserve_in_last:A
> >> +	 * MB from desc_reopen_last:A to prb_reserve_in_last:A
> >>  	 */
> >>  	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
> >>  			DESC_SV(id, desc_reserved))) { /* LMM(desc_reopen_last:A) */
> >> @@ -1770,9 +1770,9 @@ static void _prb_commit(struct prb_reserved_entry *e, unsigned long state_val)
> >>  	 *
> >>  	 *    Relies on:
> >>  	 *
> >> -	 *    MB _prb_commit:B to prb_commit:A
> >> +	 *    MB from _prb_commit:B to prb_commit:A
> >>  	 *       matching
> >> -	 *    MB desc_reserve:D to desc_make_final:A
> >> +	 *    MB from desc_reserve:D to desc_make_final:A
> >>  	 */
> >>  	if (!atomic_long_try_cmpxchg(&d->state_var, &prev_state_val,
> >>  			DESC_SV(e->id, state_val))) { /* LMM(_prb_commit:B) */
> >> @@ -2035,7 +2035,7 @@ u64 prb_first_seq(struct printk_ringbuffer *rb)
> >>  		 *
> >>  		 * MB from desc_push_tail:B to desc_reserve:F
> >>  		 *    matching
> >> -		 * RMB prb_first_seq:B to prb_first_seq:A
> >> +		 * RMB from prb_first_seq:B to prb_first_seq:A
> >>  		 */
> >>  		smp_rmb(); /* LMM(prb_first_seq:C) */
> >>  	}
> >> diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
> >> index 1651b53ece34f..fb7d2ee29af23 100644
> >> --- a/kernel/printk/printk_ringbuffer.h
> >> +++ b/kernel/printk/printk_ringbuffer.h
> >> @@ -388,7 +388,7 @@ for ((s) = from; prb_read_valid(rb, s, r); (s) = (r)->info->seq + 1)
> >>   *
> >>   * This is a macro for conveniently iterating over a ringbuffer.
> >>   * Note that @s may not be the sequence number of the record on each
> >> - * iteration. For the sequence number, @r->info->seq should be checked.
> >> + * iteration. For the sequence number, @i->seq should be checked.
> >>   *
> >>   * Context: Any context.
> >>   */
> >
> > All these changes look correct as well:
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> > I could squash these changes into the original patch when comitting
> > into printk/linux.git.
> >
> > John, could I use your Signed-off-by to mark you as a co-author
> > and keep Loïc as the author of the patch?
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

JFYI, the patch has been comitted into printk/linux.git,
branch rework/prb-fixes.

I have merged the extra fixes proposed by John, see
https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/commit/?h=rework/prb-fixes&id=bf56987c111372a54ae877934a42f7fb0953a6ca

Best Regards,
Petr