Using of_device_get_match_data is expensive. Cache match data to speed
up access and rework user of match data to use the new cached value.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/dsa/qca/qca8k.c | 28 ++++++++++------------------
drivers/net/dsa/qca/qca8k.h | 1 +
2 files changed, 11 insertions(+), 18 deletions(-)
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
@@ -1462,8 +1462,8 @@ static int qca8k_find_cpu_port(struct dsa_switch *ds)
static int
qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
{
+ const struct qca8k_match_data *data = priv->info;
struct device_node *node = priv->dev->of_node;
- const struct qca8k_match_data *data;
u32 val = 0;
int ret;
@@ -1472,8 +1472,6 @@ qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
* Should be applied by default but we set this just to make sure.
*/
if (priv->switch_id == QCA8K_ID_QCA8327) {
- data = of_device_get_match_data(priv->dev);
-
/* Set the correct package of 148 pin for QCA8327 */
if (data->reduced_package)
val |= QCA8327_PWS_PACKAGE148_EN;
@@ -1996,23 +1994,19 @@ static void qca8k_setup_pcs(struct qca8k_priv *priv, struct qca8k_pcs *qpcs,
static void
qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
{
- const struct qca8k_match_data *match_data;
struct qca8k_priv *priv = ds->priv;
int i;
if (stringset != ETH_SS_STATS)
return;
- match_data = of_device_get_match_data(priv->dev);
-
- for (i = 0; i < match_data->mib_count; i++)
+ for (i = 0; i < priv->info->mib_count; i++)
strncpy(data + i * ETH_GSTRING_LEN, ar8327_mib[i].name,
ETH_GSTRING_LEN);
}
static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *skb)
{
- const struct qca8k_match_data *match_data;
struct qca8k_mib_eth_data *mib_eth_data;
struct qca8k_priv *priv = ds->priv;
const struct qca8k_mib_desc *mib;
@@ -2031,10 +2025,9 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
if (port != mib_eth_data->req_port)
goto exit;
- match_data = device_get_match_data(priv->dev);
data = mib_eth_data->data;
- for (i = 0; i < match_data->mib_count; i++) {
+ for (i = 0; i < priv->info->mib_count; i++) {
mib = &ar8327_mib[i];
/* First 3 mib are present in the skb head */
@@ -2106,7 +2099,6 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
uint64_t *data)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
- const struct qca8k_match_data *match_data;
const struct qca8k_mib_desc *mib;
u32 reg, i, val;
u32 hi = 0;
@@ -2116,9 +2108,7 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
qca8k_get_ethtool_stats_eth(ds, port, data) > 0)
return;
- match_data = of_device_get_match_data(priv->dev);
-
- for (i = 0; i < match_data->mib_count; i++) {
+ for (i = 0; i < priv->info->mib_count; i++) {
mib = &ar8327_mib[i];
reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
@@ -2141,15 +2131,12 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
static int
qca8k_get_sset_count(struct dsa_switch *ds, int port, int sset)
{
- const struct qca8k_match_data *match_data;
struct qca8k_priv *priv = ds->priv;
if (sset != ETH_SS_STATS)
return 0;
- match_data = of_device_get_match_data(priv->dev);
-
- return match_data->mib_count;
+ return priv->info->mib_count;
}
static int
@@ -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);
+
priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
if (!priv->ds)
return -ENOMEM;
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index ec58d0e80a70..0b990b46890a 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -401,6 +401,7 @@ struct qca8k_priv {
struct qca8k_mdio_cache mdio_cache;
struct qca8k_pcs pcs_port_0;
struct qca8k_pcs pcs_port_6;
+ const struct qca8k_match_data *info;
};
struct qca8k_mib_desc {
--
2.36.1
On Sat, Jul 23, 2022 at 04:18:32PM +0200, Christian Marangi wrote:
> Using of_device_get_match_data is expensive. Cache match data to speed
'is expensive' sounds like it's terribly expensive. It's just more than
it needs to be.
> up access and rework user of match data to use the new cached value.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> drivers/net/dsa/qca/qca8k.c | 28 ++++++++++------------------
> drivers/net/dsa/qca/qca8k.h | 1 +
> 2 files changed, 11 insertions(+), 18 deletions(-)
>
> 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?
> priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
> if (!priv->ds)
> return -ENOMEM;
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index ec58d0e80a70..0b990b46890a 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -401,6 +401,7 @@ struct qca8k_priv {
> struct qca8k_mdio_cache mdio_cache;
> struct qca8k_pcs pcs_port_0;
> struct qca8k_pcs pcs_port_6;
> + const struct qca8k_match_data *info;
> };
>
> struct qca8k_mib_desc {
> --
> 2.36.1
>
On Mon, Jul 25, 2022 at 02:06:26AM +0300, Vladimir Oltean wrote:
> 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.
>
Sure, it was just a stupid idea to set everything not strictly neeeded
only after verifying that we have a correct switch... But it doesn't
make sense as qca8k_priv is freed anyway if that's the case.
Will do the change in v5.
> 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.
>
Will fix this when the read_switch_id function is moved to common file.
> > (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.
Sure and sorry for the mess.
--
Ansuel
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.
On Mon, Jul 25, 2022 at 02:18:43AM +0300, Vladimir Oltean wrote:
> On Sun, Jul 24, 2022 at 10:42:20PM +0200, Christian Marangi wrote:
> > Sure, it was just a stupid idea to set everything not strictly neeeded
> > only after verifying that we have a correct switch... But it doesn't
> > make sense as qca8k_priv is freed anyway if that's the case.
>
> I don't understand what you're saying. With your proposed logic,
> of_device_get_match_data() will be called anyway in qca8k_read_switch_id(),
> and if the switch id is valid, it will be called once more in qca8k_sw_probe().
> With my proposed logic, of_device_get_match_data() will be called exactly
> once, to populate priv->info *before* the first instance of when it's
> going to be needed.
Just ignore... it's me trying to give a reason for my broken logic.
--
Ansuel
On Sun, Jul 24, 2022 at 10:42:20PM +0200, Christian Marangi wrote:
> Sure, it was just a stupid idea to set everything not strictly neeeded
> only after verifying that we have a correct switch... But it doesn't
> make sense as qca8k_priv is freed anyway if that's the case.
I don't understand what you're saying. With your proposed logic,
of_device_get_match_data() will be called anyway in qca8k_read_switch_id(),
and if the switch id is valid, it will be called once more in qca8k_sw_probe().
With my proposed logic, of_device_get_match_data() will be called exactly
once, to populate priv->info *before* the first instance of when it's
going to be needed.
On Mon, Jul 25, 2022 at 01:30:31AM +0300, Vladimir Oltean wrote:
> On Sat, Jul 23, 2022 at 04:18:32PM +0200, Christian Marangi wrote:
> > Using of_device_get_match_data is expensive. Cache match data to speed
>
> 'is expensive' sounds like it's terribly expensive. It's just more than
> it needs to be.
>
Ok will reword.
> > up access and rework user of match data to use the new cached value.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > drivers/net/dsa/qca/qca8k.c | 28 ++++++++++------------------
> > drivers/net/dsa/qca/qca8k.h | 1 +
> > 2 files changed, 11 insertions(+), 18 deletions(-)
> >
> > 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.
(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...)
> > priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
> > if (!priv->ds)
> > return -ENOMEM;
> > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > index ec58d0e80a70..0b990b46890a 100644
> > --- a/drivers/net/dsa/qca/qca8k.h
> > +++ b/drivers/net/dsa/qca/qca8k.h
> > @@ -401,6 +401,7 @@ struct qca8k_priv {
> > struct qca8k_mdio_cache mdio_cache;
> > struct qca8k_pcs pcs_port_0;
> > struct qca8k_pcs pcs_port_6;
> > + const struct qca8k_match_data *info;
> > };
> >
> > struct qca8k_mib_desc {
> > --
> > 2.36.1
> >
>
--
Ansuel