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]

Date:   Sat, 27 Aug 2022 15:48:34 +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? 
>

I could be confused but I think the value was wrong from the start. (the
driver is a bit old and the regmap config struct was very wrong at
times)
This should declare how wide is each address right?

If this is the case then at times who declared the regmap config was
confused by the fact that the mdio is limited to 16 bits and require
special handling.

This is not problematic for the bits ops but is problematic for the bulk
ops as they base the calculation on these values.

Or I could be totally wrong... Anyway without this change the wrong
address is passed to the bulk ops so it's necessary (and for the said
reason, the value was wrong from the start)

> >  	.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.
> 

Yes you are right. Any suggestion on how to improve? 

> >  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.
> 

Will split in v3 hoping I get some feedback from Mark and you.

>     Andrew

-- 
	Ansuel