Re: [PATCH v2 21/33] x86/intel_rdt_rdtgroup.h: Header for user interface

[Thomas Gleixner]

On Thu, 8 Sep 2016, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> 
> This is header file for user interface file intel_rdt_rdtgroup.c.

Really useful information complementary to $subject - NOT!

And again you introduce stuff without an implementation. How the hell is
that useful? It's just annoying having to lookup this patch when reviewing
the one which adds the actual code ....
 
> +#define MAX_RDTGROUP_TYPE_NAMELEN	32
> +#define MAX_RDTGROUP_ROOT_NAMELEN	64
> +#define MAX_RFTYPE_NAME			64
> +
> +#include <linux/kernfs.h>
> +#include <asm/intel_rdt.h>
> +
> +extern void rdtgroup_exit(struct task_struct *tsk);
> +
> +/* cftype->flags */
> +enum {
> +	RFTYPE_WORLD_WRITABLE = (1 << 4),/* (DON'T USE FOR NEW FILES) S_IWUGO */

Huch? What's the point of this?

> +
> +	/* internal flags, do not use outside rdtgroup core proper */
> +	__RFTYPE_ONLY_ON_DFL  = (1 << 16),/* only on default hierarchy */
> +	__RFTYPE_NOT_ON_DFL   = (1 << 17),/* not on default hierarchy */
> +};
> +
> +#define CACHE_LEVEL3		3
> +
> +struct cache_resource {
> +	u64 *cbm;
> +	u64 *cbm2;
> +	int *closid;
> +	int *refcnt;
> +};

Some more undocumented structs.

> +
> +struct rdt_resource {
> +	bool valid;
> +	int closid[MAX_CACHE_DOMAINS];
> +	/* Add more resources here. */
> +};
> +
> +struct rdtgroup {
> +	struct kernfs_node *kn;		/* rdtgroup kernfs entry */

I told you before not to use tail comments and of course you comment the
obvious and not anything else. We have kerneldoc for this.

> +struct rftype {
> +	/*
> +	 * By convention, the name should begin with the name of the
> +	 * subsystem, followed by a period.  Zero length string indicates
> +	 * end of cftype array.
> +	 */

See above.

> +	char name[MAX_CFTYPE_NAME];
> +	unsigned long private;

And please align the struct members proper. Reading this is a PITA.

Thanks,

	tglx