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 16:52:37 +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

On Sat, Aug 27, 2022 at 03:48:34PM +0200, Christian Marangi wrote:
> 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)

So please figure it out, maybe use git blame to see who added it, and
ask etc. And then submit a patch which changes just this, including
and explanation why it should be changed.
 
> > >  	.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?

s/bytes/words/

	Andrew