On 2020-02-21, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>> new file mode 100644
>> index 000000000000..796257f226ee
>> --- /dev/null
>> +++ b/kernel/printk/printk_ringbuffer.c
>> +static struct prb_data_block *to_block(struct prb_data_ring *data_ring,
>> + unsigned long begin_lpos)
>> +{
>> + char *data = &data_ring->data[DATA_INDEX(data_ring, begin_lpos)];
>> +
>> + return (struct prb_data_block *)data;
>
> Nit: Please, use "blk" instead of "data". I was slightly confused
> because "data" is also one member of struct prb_data_block.
OK.
>> +/* The possible responses of a descriptor state-query. */
>> +enum desc_state {
>> + desc_miss, /* ID mismatch */
>> + desc_reserved, /* reserved, but still in use by writer */
>> + desc_committed, /* committed, writer is done */
>> + desc_reusable, /* free, not used by any writer */
>
> s/not used/not yet used/
OK.
>> +EXPORT_SYMBOL(prb_reserve);
>
> Please, do not export symbols if there are no plans to actually
> use them from modules. It will be easier to rework the code
> in the future. Nobody would need to worry about external
> users.
>
> Please, do so everywhere in the patchset.
You are correct.
The reason I exported them is that I could run my test module. But since
the test module will not be part of the kernel source, I'll just hack
the exports in when doing my testing.
>> +static char *get_data(struct prb_data_ring *data_ring,
>> + struct prb_data_blk_lpos *blk_lpos,
>> + unsigned long *data_size)
>> +{
>> + struct prb_data_block *db;
>> +
>> + /* Data-less data block description. */
>> + if (blk_lpos->begin == INVALID_LPOS &&
>> + blk_lpos->next == INVALID_LPOS) {
>> + return NULL;
>
> Nit: There is no need for "else" after return. checkpatch.pl usually
> complains about it ;-)
OK.
>> +/*
>> + * Read the record @id and verify that it is committed and has the sequence
>> + * number @seq. On success, 0 is returned.
>> + *
>> + * Error return values:
>> + * -EINVAL: A committed record @seq does not exist.
>> + * -ENOENT: The record @seq exists, but its data is not available. This is a
>> + * valid record, so readers should continue with the next seq.
>> + */
>> +static int desc_read_committed(struct prb_desc_ring *desc_ring,
>> + unsigned long id, u64 seq,
>> + struct prb_desc *desc)
>> +{
>
> I was few times confused whether this function reads the descriptor
> a safe way or not.
>
> Please, rename it to make it clear that does only a check.
> For example, check_state_commited().
This function _does_ read. It is a helper function of prb_read() to
_read_ the descriptor. It is an extended version of desc_read() that
also performs various checks that the descriptor is committed.
I will update the function description to be more similar to desc_read()
so that it is obvious that it is "getting a copy of a specified
descriptor".
John Ogness
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
