netdev - Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

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: <YwojiJdIsz/qL1XC@lunn.ch>
Date:   Sat, 27 Aug 2022 16:00:40 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap
 read/write API

>  static struct regmap_config qca8k_regmap_config = {
> -	.reg_bits = 16,
> +	.reg_bits = 32,

Does this change really allow you to access more registers? 

>  	.val_bits = 32,
>  	.reg_stride = 4,
>  	.max_register = 0x16ac, /* end MIB - Port6 range */
> -	.reg_read = qca8k_regmap_read,
> -	.reg_write = qca8k_regmap_write,
> +	.read = qca8k_bulk_read,
> +	.write = qca8k_bulk_write,
>  	.reg_update_bits = qca8k_regmap_update_bits,
>  	.rd_table = &qca8k_readable_table,
>  	.disable_locking = true, /* Locking is handled by qca8k read/write */
>  	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
> +	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
> +	.max_raw_write = 16,

I think the word 'bytes' in the comment is wrong. I assume you can
access 4 registers, each register is one 32-bit work in size.

>  static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
>  {
> -	u32 reg[3];
> +	u32 reg[QCA8K_ATU_TABLE_SIZE];
>  	int ret;
>  
>  	/* load the ARL table into an array */
> -	ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
> +	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
> +			       QCA8K_ATU_TABLE_SIZE);
>  	if (ret)
>  		return ret;

Please split the 3 -> QCA8K_ATU_TABLE_SIZE change out into a patch of
its own.

Ideally you want lots of small, obviously correct patches. A change
which replaces 3 for QCA8K_ATU_TABLE_SIZE should be small and
obviously correct, meaning it is quick and easy to review, and makes
the more complex to review change smaller and also simpler to review.

    Andrew