Re: PATCH [1/2] gamecube/wii: graphic quantization registers driver (GQR)

[Al Viro <viro@zeniv.linux.org.uk>]

On Sun, Dec 04, 2022 at 02:06:06PM +1100, Zopolis0 wrote:

> +static u32 gqr_values[8];
> +static struct ctl_table_header *gqr_table_header;
> +
> +#define SPR_GQR0 912
> +#define SPR_GQR1 913
> +#define SPR_GQR2 914
> +#define SPR_GQR3 915
> +#define SPR_GQR4 916
> +#define SPR_GQR5 917
> +#define SPR_GQR6 918
> +#define SPR_GQR7 919
> +
> +#define MFSPR_CASE(i) case (i): (*((u32 *)table->data) = mfspr(SPR_GQR##i))
> +#define MTSPR_CASE(i) case (i): mtspr(SPR_GQR##i, *((u32 *)table->data))
> +
> +static int proc_dogqr(ctl_table *table, int write, struct file *file,
> +       void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + int r;
> +
> + if (!write) { /* if they are reading, update the variable */
> + switch (table->data - (void *)gqr_values) {

That looks very fishy.  First of all, you clearly have table->data set
(by DECLARE_GQR below) to gqr_values + <interger from 0 to 7>.  Trivial
C quiz: what would the value of
	(void *)(gqr_values + 7) - (void *)gqr_values
be, if gqr_values is declared as an array of unsigned int?

	IOW, the values in that switch are wrong.  If anything,
you want (u32 *)table->data - gqr_values.  What's more, SPR_GQR<n>
is simply 912 + n, innit?  So all the crap with switches and ##
is pointless - that thing should simply be

	unsigned reg = (u32 *)table->data - gqr_values;
	
	if (WARN_ON_ONCE(reg > 7))
		return -EINVAL;

	if (!write)
		gqr_values[reg] = mfspr(SPR_GQR0 + reg);

	r = proc_dointvec(table, write, buffer, lenp, ppos);

	if (!r && write) /* if they are writing, update the reg */
		mtspr(SPR_GQR0 + reg, gqr_values[reg]);

	return r;