Re: [PATCH 1/2] ramoops: use pstore interface

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 16 November 2011, Kees Cook wrote:
> Instead of using /dev/mem directly, use the common pstore infrastructure
> to handle Oops gathering and extraction.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Sounds like a very good plan to me. It probably makes sense to move the
entire driver into fs/pstore after this. Otherwise, I have only trivial
style comments:

> +static int ramoops_pstore_open(struct pstore_info *psi);
> +static int ramoops_pstore_close(struct pstore_info *psi);
> +static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
> +				   struct timespec *time,
> +				   char **buf,
> +				   struct pstore_info *psi);
> +static int ramoops_pstore_write(enum pstore_type_id type,
> +				enum kmsg_dump_reason reason, u64 *id,
> +				unsigned int part,
> +				size_t size, struct pstore_info *psi);
> +static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
> +				struct pstore_info *psi);

Can you do it without forward declarations? Many people find code
more readable if it is structure in the natural order that avoids
these.

> +static int ramoops_pstore_close(struct pstore_info *psi)
> +{
> +	return 0;
> +}

Do you actually have to provide this if it's empty?

If yes, it might make sense to change the pstore code so that
it works without a close function.

	Arnd