netdev - Re: [net-next PATCH v3 01/14] net: dsa: qca8k: cache match data to speed up access

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:   Mon, 25 Jul 2022 02:06:26 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [net-next PATCH v3 01/14] net: dsa: qca8k: cache match data to
 speed up access

On Sun, Jul 24, 2022 at 10:27:13PM +0200, Christian Marangi wrote:
> > > diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
> > > index 1cbb05b0323f..212b284f9f73 100644
> > > --- a/drivers/net/dsa/qca/qca8k.c
> > > +++ b/drivers/net/dsa/qca/qca8k.c
> > > @@ -3168,6 +3155,11 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/* Cache match data in priv struct.
> > > +	 * Match data is already checked in read_switch_id.
> > > +	 */
> > > +	priv->info = of_device_get_match_data(priv->dev);
> > > +
> > 
> > So why don't you set priv->info right before calling qca8k_read_switch_id(),
> > then?
> > 
> 
> The idea was to make the read_switch_id a function to check if the
> switch is compatible... But yhea now that i think about it doesn't
> really make sense.

I am not saying qca8k_read_switch_id() should do anything more than
reading the switch id. I am saying why can't qca8k_read_switch_id()
already find priv->info be pre-populated, just like any other function.
Why don't you set priv->info a lot earlier, see below.

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index fa91517e930b..590ff810c95e 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1892,6 +1892,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	priv->bus = mdiodev->bus;
 	priv->dev = &mdiodev->dev;
+	priv->info = of_device_get_match_data(priv->dev);
 
 	priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
 						   GPIOD_ASIS);
@@ -1924,11 +1925,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	if (ret)
 		return ret;
 
-	/* Cache match data in priv struct.
-	 * Match data is already checked in read_switch_id.
-	 */
-	priv->info = of_device_get_match_data(priv->dev);
-
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
 		return -ENOMEM;
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index e6294d6a7b8f..8f634edc52c2 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -1211,23 +1211,19 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
 
 int qca8k_read_switch_id(struct qca8k_priv *priv)
 {
-	const struct qca8k_match_data *data;
 	u32 val;
 	u8 id;
 	int ret;
 
-	/* get the switches ID from the compatible */
-	data = of_device_get_match_data(priv->dev);
-	if (!data)
-		return -ENODEV;
-
 	ret = qca8k_read(priv, QCA8K_REG_MASK_CTRL, &val);
 	if (ret < 0)
 		return -ENODEV;
 
 	id = QCA8K_MASK_CTRL_DEVICE_ID(val);
-	if (id != data->id) {
-		dev_err(priv->dev, "Switch id detected %x but expected %x", id, data->id);
+	if (id != priv->info->id) {
+		dev_err(priv->dev,
+			"Switch id detected %x but expected %x",
+			id, priv->info->id);
 		return -ENODEV;
 	}
 

Also note how the "Switch id detected ... but expected ..." message
lacks a trailing \n.

> (Just for reference I just sent v4 as I got a report from kernel test
> bot... it's really just this series with a change in 0002 patch that set
> the struct for ops as a pointer... didn't encounter this with gcc but it
> seems kernel test bot use some special config...)

Yea, I was still kinda reviewing v3... Anyway, now you'll have to wait
for me to finish my v3 feedback on the v4, and then send the v5 after at
least 24 more hours.