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  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]
Date:   Mon, 3 Aug 2020 11:21:02 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Michael Grzeschik <m.grzeschik@...gutronix.de>, andrew@...n.ch
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kernel@...gutronix.de
Subject: Re: [PATCH v4 07/11] net: dsa: microchip: ksz8795: move register
 offsets and shifts to separate struct



On 8/2/2020 10:44 PM, Michael Grzeschik wrote:
> In order to get this driver used with other switches the functions need
> to use different offsets and register shifts. This patch changes the
> direct use of the register defines to register description structures,
> which can be set depending on the chips register layout.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
> 
> ---

[snip]

> +
> +struct ksz_regs {
> +	int ind_ctrl_0;
> +	int ind_data_8;
> +	int ind_data_check;
> +	int ind_data_hi;
> +	int ind_data_lo;
> +	int ind_mib_check;
> +	int p_force_ctrl;
> +	int p_link_status;
> +	int p_local_ctrl;
> +	int p_neg_restart_ctrl;
> +	int p_remote_status;
> +	int p_speed_status;
> +	int s_tail_tag_ctrl;
> +};

It might make more sense to define an array of registers that is indexed
by an enum token, and the array type be say, u16, such that the compiler
can tell you if you exceed what the type can represent. Then all of your
conversion looks a little more trivial to review and look like that:

-	ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
+	ksz_write16(dev, dev->regs[REG_IND_CTRL_0], ctrl_addr);

is more readable and easier to review IMHO.

All of your masks and shifts should also use unsigned types since they
are unsigned by definition.
-- 
Florian

Powered by blists - more mailing lists