netdev - Re: [PATCH v4 07/11] net: dsa: microchip: ksz8795: move register offsets and shifts to separate struct

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: <33431f48-d3f1-9821-3106-ec86fef76d5d@gmail.com>
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