[patch net-next 0/3] mlxsw: small driver update

[patch net-next 0/3] mlxsw: small driver update

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Ido Schimmel (1):
  mlxsw: Remove duplicate included header

Jiri Pirko (2):
  mlxsw: expose EMAD transactions statistics via debugfs
  mlxsw: adjust log messages level in __mlxsw_emad_transmit

 drivers/net/ethernet/mellanox/mlxsw/core.c | 60 ++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 8 deletions(-)

-- 
1.9.3

[patch net-next 1/3] mlxsw: Remove duplicate included header

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Elad Raz <eladr@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 09325b7..0415ff6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -48,7 +48,6 @@
 #include <linux/skbuff.h>
 #include <linux/etherdevice.h>
 #include <linux/types.h>
-#include <linux/wait.h>
 #include <linux/string.h>
 #include <linux/gfp.h>
 #include <linux/random.h>
-- 
1.9.3

[patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Elad Raz <eladr@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 51 ++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 0415ff6..6ee3f45 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -98,6 +98,12 @@ struct mlxsw_core {
 		bool trans_active;
 		struct mutex lock; /* One EMAD transaction at a time. */
 		bool use_emad;
+		struct {
+			u64 trans;
+			u32 fails;
+			u32 retries;
+			u32 timeouts;
+		} stats;
 	} emad;
 	struct mlxsw_core_pcpu_stats __percpu *pcpu_stats;
 	struct dentry *dbg_dir;
@@ -390,6 +396,7 @@ static int __mlxsw_emad_transmit(struct mlxsw_core *mlxsw_core,
 		dev_warn(mlxsw_core->bus_info->dev, "EMAD timed-out (tid=%llx)\n",
 			 mlxsw_core->emad.tid);
 		mlxsw_core->emad.trans_active = false;
+		mlxsw_core->emad.stats.timeouts++;
 		return -EIO;
 	}
 
@@ -463,8 +470,10 @@ retry:
 		if (!err || err != -EAGAIN)
 			goto out;
 	}
-	if (n_retry++ < MLXSW_EMAD_MAX_RETRY)
+	if (n_retry++ < MLXSW_EMAD_MAX_RETRY) {
+		mlxsw_core->emad.stats.retries++;
 		goto retry;
+	}
 
 out:
 	dev_kfree_skb(skb);
@@ -671,6 +680,35 @@ static const struct file_operations mlxsw_core_rx_stats_dbg_ops = {
 	.llseek = seq_lseek
 };
 
+static int mlxsw_core_emad_stats_dbg_read(struct seq_file *file, void *data)
+{
+	struct mlxsw_core *mlxsw_core = file->private;
+
+	if (mutex_lock_interruptible(&mlxsw_core->emad.lock))
+		return -EINTR;
+	seq_printf(file, "transactions: %llu\n", mlxsw_core->emad.stats.trans);
+	seq_printf(file, "fails: %u\n", mlxsw_core->emad.stats.fails);
+	seq_printf(file, "retries: %u\n", mlxsw_core->emad.stats.retries);
+	seq_printf(file, "timeouts: %u\n", mlxsw_core->emad.stats.timeouts);
+	mutex_unlock(&mlxsw_core->emad.lock);
+	return 0;
+}
+
+static int mlxsw_core_emad_stats_dbg_open(struct inode *inode, struct file *f)
+{
+	struct mlxsw_core *mlxsw_core = inode->i_private;
+
+	return single_open(f, mlxsw_core_emad_stats_dbg_read, mlxsw_core);
+}
+
+static const struct file_operations mlxsw_core_emad_stats_dbg_ops = {
+	.owner = THIS_MODULE,
+	.open = mlxsw_core_emad_stats_dbg_open,
+	.release = single_release,
+	.read = seq_read,
+	.llseek = seq_lseek
+};
+
 static void mlxsw_core_buf_dump_dbg(struct mlxsw_core *mlxsw_core,
 				    const char *buf, size_t size)
 {
@@ -768,6 +806,8 @@ static int mlxsw_core_debugfs_init(struct mlxsw_core *mlxsw_core)
 	mlxsw_core->dbg.psid_blob.size = sizeof(bus_info->psid);
 	debugfs_create_blob("psid", S_IRUGO, mlxsw_core->dbg_dir,
 			    &mlxsw_core->dbg.psid_blob);
+	debugfs_create_file("emad_stats", S_IRUGO, mlxsw_core->dbg_dir,
+			    mlxsw_core, &mlxsw_core_emad_stats_dbg_ops);
 	return 0;
 }
 
@@ -1107,8 +1147,10 @@ retry:
 	err = mlxsw_cmd_access_reg(mlxsw_core, in_mbox, out_mbox);
 	if (!err) {
 		err = mlxsw_emad_process_status(mlxsw_core, out_mbox);
-		if (err == -EAGAIN && n_retry++ < MLXSW_EMAD_MAX_RETRY)
+		if (err == -EAGAIN && n_retry++ < MLXSW_EMAD_MAX_RETRY) {
+			mlxsw_core->emad.stats.retries++;
 			goto retry;
+		}
 	}
 
 	if (!err)
@@ -1137,6 +1179,7 @@ static int mlxsw_core_reg_access(struct mlxsw_core *mlxsw_core,
 		return -EINTR;
 	}
 
+	mlxsw_core->emad.stats.trans++;
 	cur_tid = mlxsw_core->emad.tid;
 	dev_dbg(mlxsw_core->bus_info->dev, "Reg access (tid=%llx,reg_id=%x(%s),type=%s)\n",
 		cur_tid, reg->id, mlxsw_reg_id_str(reg->id),
@@ -1153,10 +1196,12 @@ static int mlxsw_core_reg_access(struct mlxsw_core *mlxsw_core,
 		err = mlxsw_core_reg_access_emad(mlxsw_core, reg,
 						 payload, type);
 
-	if (err)
+	if (err) {
 		dev_err(mlxsw_core->bus_info->dev, "Reg access failed (tid=%llx,reg_id=%x(%s),type=%s)\n",
 			cur_tid, reg->id, mlxsw_reg_id_str(reg->id),
 			mlxsw_core_reg_access_type_str(type));
+		mlxsw_core->emad.stats.fails++;
+	}
 
 	mutex_unlock(&mlxsw_core->emad.lock);
 	return err;
-- 
1.9.3

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 24 Aug 2015 16:45:46 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Elad Raz <eladr@mellanox.com>

Enough with this debugfs madness.

Expose this stuff through standard interfaces.

They are simple statistics for crying out loud!

I'm not applying this, and I'm really getting irritated about how much
garbage people put into debugfs when it has _NO_ business being there.

Sorry.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Jiri Pirko]

Tue, Aug 25, 2015 at 11:25:21PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Mon, 24 Aug 2015 16:45:46 +0200
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Elad Raz <eladr@mellanox.com>
>
>Enough with this debugfs madness.
>
>Expose this stuff through standard interfaces.
>
>They are simple statistics for crying out loud!

They are simple statistics. But they does not fit into any existing
interface. This is about EMAD packets. They are not per-netdevice, but
per-pcidevice. So I cannot put them into ethtool.

I see no other iface to expose this other than debugfs. Please suggest
some other way, I don't see it :/

Thanks.


>
>I'm not applying this, and I'm really getting irritated about how much
>garbage people put into debugfs when it has _NO_ business being there.

I think that is the primary purpose of this iface, To put arbitrary
debugging garbage there. Am I missing something?


>
>Sorry.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 26 Aug 2015 07:52:15 +0200

> They are simple statistics. But they does not fit into any existing
> interface. This is about EMAD packets. They are not per-netdevice, but
> per-pcidevice. So I cannot put them into ethtool.
> 
> I see no other iface to expose this other than debugfs. Please suggest
> some other way, I don't see it :/

Then create one, instead of crapping up the driver with debugfs
craziness.

>>I'm not applying this, and I'm really getting irritated about how much
>>garbage people put into debugfs when it has _NO_ business being there.
> 
> I think that is the primary purpose of this iface, To put arbitrary
> debugging garbage there. Am I missing something?

It's not garbage if it's useful for someone.

If it's not useful, why even bother?

This is why I hate debugfs, it's a fundamentally flawed facility.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Jiri Pirko]

Wed, Aug 26, 2015 at 08:08:21AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Wed, 26 Aug 2015 07:52:15 +0200
>
>> They are simple statistics. But they does not fit into any existing
>> interface. This is about EMAD packets. They are not per-netdevice, but
>> per-pcidevice. So I cannot put them into ethtool.
>> 
>> I see no other iface to expose this other than debugfs. Please suggest
>> some other way, I don't see it :/
>
>Then create one, instead of crapping up the driver with debugfs
>craziness.

I'm not sure it is possible to come up with a generic interface for
arbitraty statistics for some generic PCI device.

I can imagine the pushback saying "hey, put that statistics into subtree
specific area, like netdev etc". And that is correct. In vast majority
of cases, that can be done.

In mlxsw case however, 36 netdevices are sharing 1 pci device. And the
stuff related to that pci device cannot be exposed via netdev.

I don't think that are much more cases like this. Therefore I think that
for this cases, debugfs might be a good way to expose debugging stats.


>
>>>I'm not applying this, and I'm really getting irritated about how much
>>>garbage people put into debugfs when it has _NO_ business being there.
>> 
>> I think that is the primary purpose of this iface, To put arbitrary
>> debugging garbage there. Am I missing something?
>
>It's not garbage if it's useful for someone.
>
>If it's not useful, why even bother?
>
>This is why I hate debugfs, it's a fundamentally flawed facility.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 26 Aug 2015 09:37:57 +0200

> I don't think that are much more cases like this. Therefore I think that
> for this cases, debugfs might be a good way to expose debugging stats.

Scott wanted to do similar things in rocker.  DSA guys too.

Every switch device is going to have some kind of hierarchy like
this, it's not a unique situation.

So I don't buy the "this is a special situation" thing at all.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Scott Feldman]

On Wed, Aug 26, 2015 at 10:49 AM, David Miller <davem@davemloft.net> wrote:
> From: Jiri Pirko <jiri@resnulli.us>
> Date: Wed, 26 Aug 2015 09:37:57 +0200
>
>> I don't think that are much more cases like this. Therefore I think that
>> for this cases, debugfs might be a good way to expose debugging stats.
>
> Scott wanted to do similar things in rocker.  DSA guys too.
>
> Every switch device is going to have some kind of hierarchy like
> this, it's not a unique situation.

We've been able to get buy so far without a user-visible device for
the switch.  The switch ports are represented by netdevs, so that's
easy.  How can we create an object for the switch itself, so we can
attach common interfaces for the user to dump switch-level stats or
tables?   Using another netdev doesn't seem right.  Do we need a new
device class for switches, and then create some common tool/interfaces
for switch device class?

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Florian Fainelli]

On 26/08/15 11:21, Scott Feldman wrote:
> On Wed, Aug 26, 2015 at 10:49 AM, David Miller <davem@davemloft.net> wrote:
>> From: Jiri Pirko <jiri@resnulli.us>
>> Date: Wed, 26 Aug 2015 09:37:57 +0200
>>
>>> I don't think that are much more cases like this. Therefore I think that
>>> for this cases, debugfs might be a good way to expose debugging stats.
>>
>> Scott wanted to do similar things in rocker.  DSA guys too.
>>
>> Every switch device is going to have some kind of hierarchy like
>> this, it's not a unique situation.
> 
> We've been able to get buy so far without a user-visible device for
> the switch.  The switch ports are represented by netdevs, so that's
> easy.  How can we create an object for the switch itself, so we can
> attach common interfaces for the user to dump switch-level stats or
> tables?   Using another netdev doesn't seem right.  Do we need a new
> device class for switches, and then create some common tool/interfaces
> for switch device class?

I agree this is something crucially missing. If we try to list what
could be missing currently, there is mostly:

* switch-wide statistics, tables, databases
* controlling a firmware agent running on the switch
* restarting/re-configuring the switch hardware

All of these already have proper ethtool control interfaces, so using
something that understands a specialized net_device might be the easiest
way to go, but we would need a way to put it in the non network device
name space so tools and users to do not get confused?

We could also have a specific SET_NETDEV_DEVTYPE() which helps make that
specific device be part of a "switch-mgmt" class for instance?
-- 
Florian

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[David Miller]

From: Scott Feldman <sfeldma@gmail.com>
Date: Wed, 26 Aug 2015 11:21:59 -0700

> Using another netdev doesn't seem right.  Do we need a new device
> class for switches, and then create some common tool/interfaces for
> switch device class?

This is probably what we will need to do.

There has also been a discussion lately about making light weight
netdev objects, of which these new switch things can be a super-class
of.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Jiri Pirko]

Wed, Aug 26, 2015 at 08:21:59PM CEST, sfeldma@gmail.com wrote:
>On Wed, Aug 26, 2015 at 10:49 AM, David Miller <davem@davemloft.net> wrote:
>> From: Jiri Pirko <jiri@resnulli.us>
>> Date: Wed, 26 Aug 2015 09:37:57 +0200
>>
>>> I don't think that are much more cases like this. Therefore I think that
>>> for this cases, debugfs might be a good way to expose debugging stats.
>>
>> Scott wanted to do similar things in rocker.  DSA guys too.
>>
>> Every switch device is going to have some kind of hierarchy like
>> this, it's not a unique situation.
>
>We've been able to get buy so far without a user-visible device for
>the switch.  The switch ports are represented by netdevs, so that's
>easy.  How can we create an object for the switch itself, so we can
>attach common interfaces for the user to dump switch-level stats or
>tables?   Using another netdev doesn't seem right.  Do we need a new
>device class for switches, and then create some common tool/interfaces
>for switch device class?

Switch object itselt would not help you to expose rocker internals. I
don't think that you can find generic way, same for all drivers, to
expose internal tables and stuff. That is hw specific.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 27 Aug 2015 07:40:04 +0200

> Switch object itselt would not help you to expose rocker internals. I
> don't think that you can find generic way, same for all drivers, to
> expose internal tables and stuff. That is hw specific.

Tables are datastructures with names and types.

Is it not possible to describe datastructures and their types with
user visible interfaces?

Anyone against what I am saying right now is simply lazy.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Jiri Pirko]

Thu, Aug 27, 2015 at 08:01:15AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 27 Aug 2015 07:40:04 +0200
>
>> Switch object itselt would not help you to expose rocker internals. I
>> don't think that you can find generic way, same for all drivers, to
>> expose internal tables and stuff. That is hw specific.
>
>Tables are datastructures with names and types.
>
>Is it not possible to describe datastructures and their types with
>user visible interfaces?

I'm not saying it is not possible, it certainly is. But I think that for
example rocker internals have no value to default user, he should not
care and he cannot find out what is going on there without knowledge or
rocker.c code. The question is, do we need some standard interface to
expose random debugging data? I don't think so, I think that debugfs is
exactly the tool to be used in that case.

>
>Anyone against what I am saying right now is simply lazy.

Not lazy, just thinking :)

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 27 Aug 2015 08:27:04 +0200

> I'm not saying it is not possible, it certainly is. But I think that
> for example rocker internals have no value to default user, he
> should not care and he cannot find out what is going on there
> without knowledge or rocker.c code. The question is, do we need some
> standard interface to expose random debugging data? I don't think
> so, I think that debugfs is exactly the tool to be used in that
> case.

If it is only interesting to rocker.c maintainer, he can keep a local
patch he applies when he needs such a facility.

This discussion is becomming circular.

If it's useful, it needs a well defined interface.

If it's not useful, it doesn't belong in the tree.

Therefore, debugfs is useless.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Jiri Pirko]

Thu, Aug 27, 2015 at 08:36:03AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 27 Aug 2015 08:27:04 +0200
>
>> I'm not saying it is not possible, it certainly is. But I think that
>> for example rocker internals have no value to default user, he
>> should not care and he cannot find out what is going on there
>> without knowledge or rocker.c code. The question is, do we need some
>> standard interface to expose random debugging data? I don't think
>> so, I think that debugfs is exactly the tool to be used in that
>> case.
>
>If it is only interesting to rocker.c maintainer, he can keep a local
>patch he applies when he needs such a facility.
>
>This discussion is becomming circular.
>
>If it's useful, it needs a well defined interface.
>
>If it's not useful, it doesn't belong in the tree.
>
>Therefore, debugfs is useless.

Fair enough.

Re: [patch net-next 2/3] mlxsw: expose EMAD transactions statistics via debugfs

[Marcelo Ricardo Leitner]

On Thu, Aug 27, 2015 at 08:40:29AM +0200, Jiri Pirko wrote:
> Thu, Aug 27, 2015 at 08:36:03AM CEST, davem@davemloft.net wrote:
> >From: Jiri Pirko <jiri@resnulli.us>
> >Date: Thu, 27 Aug 2015 08:27:04 +0200
> >
> >> I'm not saying it is not possible, it certainly is. But I think that
> >> for example rocker internals have no value to default user, he
> >> should not care and he cannot find out what is going on there
> >> without knowledge or rocker.c code. The question is, do we need some
> >> standard interface to expose random debugging data? I don't think
> >> so, I think that debugfs is exactly the tool to be used in that
> >> case.
> >
> >If it is only interesting to rocker.c maintainer, he can keep a local
> >patch he applies when he needs such a facility.
> >
> >This discussion is becomming circular.
> >
> >If it's useful, it needs a well defined interface.
> >
> >If it's not useful, it doesn't belong in the tree.
> >
> >Therefore, debugfs is useless.
> 
> Fair enough.

Late reply, sorry, but another idea is to leave the stats in place (as
they were going to be calculated even with debugfs unmounted) and (for
now at least) fetch them with systemtap, perf or something like that.
Then the stats are there for when you need them and with an interface as
flexible as it can get. Even if you happen to do a post-mortem analysis,
the info would at least be there.

  Marcelo

[patch net-next 3/3] mlxsw: adjust log messages level in __mlxsw_emad_transmit

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

When transmit fails, it is an error, not a warning.
Do not warn when timeout happens as that is handled by a counter.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Elad Raz <eladr@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 6ee3f45..dfafb83 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -382,8 +382,8 @@ static int __mlxsw_emad_transmit(struct mlxsw_core *mlxsw_core,
 
 	err = mlxsw_core_skb_transmit(mlxsw_core->driver_priv, skb, tx_info);
 	if (err) {
-		dev_warn(mlxsw_core->bus_info->dev, "Failed to transmit EMAD (tid=%llx)\n",
-			 mlxsw_core->emad.tid);
+		dev_err(mlxsw_core->bus_info->dev, "Failed to transmit EMAD (tid=%llx)\n",
+			mlxsw_core->emad.tid);
 		dev_kfree_skb(skb);
 		return err;
 	}
@@ -393,8 +393,8 @@ static int __mlxsw_emad_transmit(struct mlxsw_core *mlxsw_core,
 				 !(mlxsw_core->emad.trans_active),
 				 msecs_to_jiffies(MLXSW_EMAD_TIMEOUT_MS));
 	if (!ret) {
-		dev_warn(mlxsw_core->bus_info->dev, "EMAD timed-out (tid=%llx)\n",
-			 mlxsw_core->emad.tid);
+		dev_dbg(mlxsw_core->bus_info->dev, "EMAD timed-out (tid=%llx)\n",
+			mlxsw_core->emad.tid);
 		mlxsw_core->emad.trans_active = false;
 		mlxsw_core->emad.stats.timeouts++;
 		return -EIO;
-- 
1.9.3