Re: [PATCH 0/7] new kfifo API

[Arnd Bergmann]

On Wednesday 12 August 2009, Stefani Seibold wrote:
> This is a proposal of a new generic kernel FIFO implementation.

Hi Stefani,

The patches look mostly good content-wise, but you don't have
separate changelog entries. The text of your introductory
mail will be lost in git history when the patches get
applied, so all the reaoning why a patch makes sense needs
to get into the patch description.

Similarly, the patch shortlog, i.e. the subject lines of
your mails, should be the lines you mention below.

> The patch is splitted into 7 parts:
>  Part 1: Code reorganization, no functional change

This one is not 'no functional change', because it
actually changes the interface, so the description needs
to be adapted.

Making the fifo itself a member of the data structure is
a very good idea, which should be what the changeset
comment describes (why that is done, not how of course).

The other change in here is that you add a 'spinlock_t *'
argument to kfifo_alloc and kfifo_init. AFAICT this is
unrelated, and it gets reverted by the next patch, so it
would be more straightforward to leave that change out.

>  Part 2: Move out spinlock

I don't see this one as worthwhile, but I don't mind
either. The contents look correct.

>  Part 3: Cleanup namespace

Looks good, it's the obvious consequence of the patch before.

>  Part 4: rename kfifo_put... -> kfifo_in... and kfifo_get... -> kfifo_out...

That one is new, right?

It's probably better to have this, just to make sure everyone
understands that the API is different now and no (out of tree)
drivers or patches accidentally break.

It only changes code that you already changed in patches 2 and 3
though, so I'd recommend folding the respective changes into
those patches.

>  Part 5: add DEFINE_KFIFO and friends, add very tiny functions
>  Part 6: add kfifo_from_user and kfifo_to_user

These look good. The macros in patch 5 could use some kerneldoc
comments so they show up in Documentation/Docbook/kernel-api.*.

>  Part 7: add record handling functions (does some reorg)

This one adds a lot of extra complexity in unused code.
I'll add my Acked-by to the first six patches if you do the
minor modifications I mention above, but for the last one,
I suggest you either add an actual user of that code, or find
someone else to advocate its inclusion.

	Arnd <><
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/