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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1609081437070.5647@nanos>
Date:   Thu, 8 Sep 2016 14:44:12 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tejun Heo <tj@...nel.org>, Borislav Petkov <bp@...e.de>,
        Stephane Eranian <eranian@...gle.com>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        Shaohua Li <shli@...com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v2 21/33] x86/intel_rdt_rdtgroup.h: Header for user
 interface

On Thu, 8 Sep 2016, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@...el.com>
> 
> 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