[PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[Vivien Didelot]

Allow write access to the regs file in the debugfs interface, with the
following parameters:

    echo <name> <reg> <value> > regs

Where "name" is the register name (as shown in the header row), "reg" is
the register address (as shown in the first column) and "value" is the
16-bit value. e.g.:

    echo GLOBAL 1a 5550 > regs

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 8c130c0..9d14b1a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1648,6 +1648,38 @@ static int mv88e6xxx_regs_show(struct seq_file *s, void *p)
 	return 0;
 }
 
+static ssize_t mv88e6xxx_regs_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	struct seq_file *s = file->private_data;
+	struct dsa_switch *ds = s->private;
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	char name[32] = { 0 };
+	unsigned int port, reg, val;
+	int ret;
+
+	if (count > sizeof(name) - 1)
+		return -EINVAL;
+
+	ret = sscanf(buf, "%s %x %x", name, &reg, &val);
+	if (ret != 3)
+		return -EINVAL;
+
+	if (reg > 0x1f || val > 0xffff)
+		return -ERANGE;
+
+	if (strcasecmp(name, "GLOBAL") == 0)
+		ret = mv88e6xxx_reg_write(ds, REG_GLOBAL, reg, val);
+	else if (strcasecmp(name, "GLOBAL2") == 0)
+		ret = mv88e6xxx_reg_write(ds, REG_GLOBAL2, reg, val);
+	else if (kstrtouint(name, 10, &port) == 0 && port < ps->num_ports)
+		ret = mv88e6xxx_reg_write(ds, REG_PORT(port), reg, val);
+	else
+		return -EINVAL;
+
+	return ret < 0 ? ret : count;
+}
+
 static int mv88e6xxx_regs_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, mv88e6xxx_regs_show, inode->i_private);
@@ -1656,6 +1688,7 @@ static int mv88e6xxx_regs_open(struct inode *inode, struct file *file)
 static const struct file_operations mv88e6xxx_regs_fops = {
 	.open   = mv88e6xxx_regs_open,
 	.read   = seq_read,
+	.write	= mv88e6xxx_regs_write,
 	.llseek = no_llseek,
 	.release = single_release,
 	.owner  = THIS_MODULE,
@@ -1895,7 +1928,7 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds)
 	ps->dbgfs = debugfs_create_dir(name, NULL);
 	kfree(name);
 
-	debugfs_create_file("regs", S_IRUGO, ps->dbgfs, ds,
+	debugfs_create_file("regs", S_IRUGO | S_IWUSR, ps->dbgfs, ds,
 			    &mv88e6xxx_regs_fops);
 
 	debugfs_create_file("atu", S_IRUGO, ps->dbgfs, ds,
-- 
2.4.5

Re: [PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu,  9 Jul 2015 17:13:29 -0400

> Allow write access to the regs file in the debugfs interface, with the
> following parameters:
> 
>     echo <name> <reg> <value> > regs
> 
> Where "name" is the register name (as shown in the header row), "reg" is
> the register address (as shown in the first column) and "value" is the
> 16-bit value. e.g.:
> 
>     echo GLOBAL 1a 5550 > regs
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

I don't know about this.

This starts to smell like a back door for proprietary userspace SDKs to
program the switch hardware.

Yes, they can do it via other mechanisms, but we don't have to make it
any eaiser for them either.

If you want to poke registers, hack the module just like any other
person with appropriate privileges can do.

Frankly, all of this debugfs crap in the DSA drivers smells like poo.
I don't like it _AT_ _ALL_, and I shouldn't have allowed any of it
into the tree in the first place.

I might just remove it all myself, it bothers me so much.

Fetching information should be done by well typed, generic, interfaces
that apply to any similar device or object.  All of this debugfs stuff
smells of hacks and special case crap that's only usable for one
device type and that makes it the single most terrible interface to
give to users.

Thanks.

Re: [PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[Vivien Didelot]

Hi David,

On Jul 11, 2015, at 2:01 AM, David davem@davemloft.net wrote:

> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Thu,  9 Jul 2015 17:13:29 -0400
> 
>> Allow write access to the regs file in the debugfs interface, with the
>> following parameters:
>> 
>>     echo <name> <reg> <value> > regs
>> 
>> Where "name" is the register name (as shown in the header row), "reg" is
>> the register address (as shown in the first column) and "value" is the
>> 16-bit value. e.g.:
>> 
>>     echo GLOBAL 1a 5550 > regs
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
> I don't know about this.
> 
> This starts to smell like a back door for proprietary userspace SDKs to
> program the switch hardware.
> 
> Yes, they can do it via other mechanisms, but we don't have to make it
> any eaiser for them either.

I agree with you and I wouldn't want that neither.

> If you want to poke registers, hack the module just like any other
> person with appropriate privileges can do.

I'm not sure what you mean. Keeping some custom patches in our local tree?

> Frankly, all of this debugfs crap in the DSA drivers smells like poo.
> I don't like it _AT_ _ALL_, and I shouldn't have allowed any of it
> into the tree in the first place.
> 
> I might just remove it all myself, it bothers me so much.
> 
> Fetching information should be done by well typed, generic, interfaces
> that apply to any similar device or object.  All of this debugfs stuff
> smells of hacks and special case crap that's only usable for one
> device type and that makes it the single most terrible interface to
> give to users.

In the meantime, this is really useful for development. i.e. ensuring a good
switchdev/DSA interaction without being able to read and write directly the
hardware VLAN table, is a bit a PITA. A dynamic debugfs looked appropriate.

On the other hand, the mv88e6xxx driver gets cluttered with all this code. I'd
gladly move all this code in a mv88e6xxx-debugfs.c file, and conditionally
compile it with:

    mv88e6xxx_drv-$(CONFIG_DEBUG_FS) += mv88e6xxx-debugfs.o

similar to what the i2400m driver does.

Would that be appreciated?

Thanks,
-v

Re: [PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Sat, 11 Jul 2015 14:36:12 -0400 (EDT)

> In the meantime, this is really useful for development. i.e. ensuring a good
> switchdev/DSA interaction without being able to read and write directly the
> hardware VLAN table, is a bit a PITA. A dynamic debugfs looked appropriate.

For "development" you can hack the driver, add tracepoints, or use
another mechanism anyone hacking the kernel (which by definition
someone doing "development" is doing) can do.

I do not buy any of your arguments, and you really miss the grand
opportunity to export the knobs and values in a way which are going
to:

1) Be useful to users

2) Be usable by any similar DSA driver, not just _yours_

So please stop this myopic narrow thinking when you add facilities for
development or export values.  Think of the big picture and long term,
not just your personal perceived immediate needs of today.

Thanks.

Re: [PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[Vivien Didelot]

Hi David,

On Jul 11, 2015, at 10:08 PM, David davem@davemloft.net wrote:

> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Sat, 11 Jul 2015 14:36:12 -0400 (EDT)
> 
>> In the meantime, this is really useful for development. i.e. ensuring a good
>> switchdev/DSA interaction without being able to read and write directly the
>> hardware VLAN table, is a bit a PITA. A dynamic debugfs looked appropriate.
> 
> For "development" you can hack the driver, add tracepoints, or use
> another mechanism anyone hacking the kernel (which by definition
> someone doing "development" is doing) can do.
> 
> I do not buy any of your arguments, and you really miss the grand
> opportunity to export the knobs and values in a way which are going
> to:
> 
> 1) Be useful to users
> 
> 2) Be usable by any similar DSA driver, not just _yours_

I hardly see how this debug interface can be made generic to other DSA drivers,
since the format of hardware tables or some registers seem very specific to the
switch chip.

> So please stop this myopic narrow thinking when you add facilities for
> development or export values.  Think of the big picture and long term,
> not just your personal perceived immediate needs of today.

I understand. So it looks like the only reasonable solution here is to revert
this support for the debugfs interface.

Thanks,
-v

Re: [PATCH v2] net: dsa: mv88e6xxx: add write access to debugfs regs file

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Sun, 12 Jul 2015 21:39:30 -0400 (EDT)

> I hardly see how this debug interface can be made generic to other
> DSA drivers, since the format of hardware tables or some registers
> seem very specific to the switch chip.

You feel this way because you are focusing on register values and not what
those values represent.

Ie. could you export the values in those registers in a generic format
that other devices could convert their register values to as well?

Stop focusing so tightly on the exact thing you've implemented and
consider things on a much higher level.