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
