Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 17 Jun 2019 22:44:21 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Ferdinand Blomqvist <ferdinand.blomqvist@gmail.com>
cc:     linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/7] rslib: Add tests for the encoder and decoder
In-Reply-To: <20190330182947.8823-2-ferdinand.blomqvist@gmail.com>
Message-ID: <alpine.DEB.2.21.1906172229190.1963@nanos.tec.linutronix.de>
References: <20190330182947.8823-1-ferdinand.blomqvist@gmail.com> <20190330182947.8823-2-ferdinand.blomqvist@gmail.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Ferdinand,

On Sat, 30 Mar 2019, Ferdinand Blomqvist wrote:

Sorry for the horrible long delay. I'm just drowned in backlog.

....

> There are a couple of options for the tests:
> 
> - Verbosity.
> 
> - Whether to test for correct behaviour beyond capacity. Default is to
>   test beyond capacity.
> 
> - Whether to allow erasures without symbol corruption. Defaults to yes.
> 
> Note that the tests take a couple of minutes to complete.

Very well written changelog!


> +/* List of codes to test */
> +static struct etab Tab[] = {
> +	{2,	0x7,	1,	1,	1,	100000	},
> +	{3,	0xb,	1,	1,	2,	100000	},
> +	{3,	0xb,	1,	1,	3,	100000	},
> +	{3,	0xb,	2,	1,	4,	100000	},
> +	{4,	0x13,	1,	1,	4,	10000	},
> +	{5,	0x25,	1,	1,	6,	1000	},
> +	{6,	0x43,	3,	1,	8,	100	},
> +	{7,	0x89,	1,	1,	10,	100	},
> +	{8,	0x11d,	1,	1,	32,	100	},
> +	{8,	0x187,	112,	11,	32,	100	},
> +	/*
> +	 * {9,	0x211,	1,	1,	32,	100	},
> +	 * {10,	0x409,	1,	1,	32,	100	},
> +	 * {11,	0x805,	1,	1,	32,	100	},
> +	 * {12,	0x1053	1,	1,	32,	50	},
> +	 * {12,	0x1053	1,	1,	64,	50	},
> +	 * {13,	0x201b	1,	1,	32,	20	},
> +	 * {13,	0x201b	1,	1,	64,	20	},
> +	 * {14,	0x4443	1,	1,	32,	10	},
> +	 * {14,	0x4443	1,	1,	64,	10	},
> +	 * {15,	0x8003	1,	1,	32,	5	},
> +	 * {15,	0x8003	1,	1,	64,	5	},
> +	 * {16,	0x1100	1,	1,	32,	5	},
> +	 */

I assume these are enabled later. We don't do that commented out stuff in
general. If it's used later, then add it with a separate patch. If not just
leave it alone.

> +	{0, 0, 0, 0, 0, 0},
> +};
> +
> +
> +struct estat {
> +	int dwrong;
> +	int irv;
> +	int wepos;
> +	int nwords;
> +};
> +
> +struct bcstat {
> +	int rfail;
> +	int rsuccess;
> +	int noncw;
> +	int nwords;
> +};
> +
> +struct wspace {
> +	uint16_t *c;		/* sent codeword */
> +	uint16_t *r;		/* received word */
> +	uint16_t *s;		/* syndrome */
> +	uint16_t *corr;		/* correction buffer */
> +	int *errlocs;
> +	int *derrlocs;

Pet pieve comment. I generally prefer tabular layout of structs as it's
simpler to follow

struct wspace {
	uint16_t	*c;		/* sent codeword */
	uint16_t	*r;		/* received word */
	uint16_t	*s;		/* syndrome */
	uint16_t	*corr;		/* correction buffer */
	int		*errlocs;
	int		*derrlocs;

Hmm?

> +
> +static double Pad[] = {0, 0.25, 0.5, 0.75, 1.0};

This is kernel code. You cannot use the FPU without special care. But for
that use case doing so would be actually overkill.

> +	for (i = 0; i < ARRAY_SIZE(Pad); i++) {
> +		int pad = Pad[i] * max_pad;

That can be simply expressed:

struct pad {
	int	mult;
	int	shift;
};

static struct pad pad[] = {
	{ 0, 0 },
	{ 1, 2 },
	{ 1, 1 },
	{ 3, 2 },
	{ 1, 0 },
};

	for (i = 0; i < ARRAY_SIZE(pad); i++) {
		int pad = (pad[i].mult * max_pad) >> pad[i].shift;

Also note, that I got rid of the CamelCase name.

> +static struct wspace *alloc_ws(struct rs_codec *rs)
> +{
> +	int nn = rs->nn;
> +	int nroots = rs->nroots;
> +	struct wspace *ws;

Yet another pet pieve comment for readability. Order variables in reverse
fir tree order.

	int nroots = rs->nroots;
	struct wspace *ws;
	int nn = rs->nn;

> +	ws = kzalloc(sizeof(*ws), GFP_KERNEL);
> +	if (!ws)
> +		return NULL;
> +
> +	ws->c = kmalloc_array(2 * (nn + nroots),
> +				sizeof(uint16_t), GFP_KERNEL);
> +	if (!ws->c)
> +		goto err;
> +
> +	ws->r = ws->c + nn;
> +	ws->s = ws->r + nn;
> +	ws->corr = ws->s + nroots;
> +
> +	ws->errlocs = kmalloc_array(nn + nroots, sizeof(int), GFP_KERNEL);
> +	if (!ws->c)
> +		goto err;
> +
> +	ws->derrlocs = ws->errlocs + nn;
> +	return ws;
> +
> +err:
> +	kfree(ws->errlocs);
> +	kfree(ws->c);
> +	kfree(ws);

If you move free_ws() above this function you can replace this kfree()
sequence with

	free_ws();

> +	return NULL;

Just nitpicks, except for the FPU issue. Other than that this looks great.

Thanks,

	tglx