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] [day] [month] [year] [list]

Date:   Sun, 4 Sep 2022 23:34:58 +0200
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
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

On Sat, Aug 27, 2022 at 04:00:40PM +0200, Andrew Lunn wrote:
> >  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

I'm working on v3.
I manage to make everything work with 16 bit. (It was set as the max reg
for this switch is 0x16ac)

Anyway on second look, this change is necessary and I think split this
in another patch would be redundant and problematic. (for backporting
purpose and to correctly write a sane commit description)

The regmap bulk implementation take as the 4th arg the count of reg to
read/write. Current implementation instead take the actual size of the
array. So I think this has to be changed in the bulk conversion patch.

I can split it but it will be something like

qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, 
		 QCA8K_ATU_TABLE_SIZE * sizeof(u32));

and on the bulk conversion commit I have to drop it again to 

regmap_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg,
                 QCA8K_ATU_TABLE_SIZE);

Seems a bad thing to make a change and than fix that change on the next
commit.

Will wait for a response about this and propose v3 with what is the
correct approach.

-- 
	Ansuel