[PATCH] net: add regs attribute to phy device for user diagnose

[PATCH] net: add regs attribute to phy device for user diagnose

[yuan linyu]

From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

if phy device have register(s) configuration problem,
user can use this attribute to diagnose.
this feature need phy driver maintainer implement.

Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
 drivers/net/phy/phy_device.c | 26 ++++++++++++++++++++++++++
 include/linux/phy.h          |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 92b0838..a400748 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -617,10 +617,36 @@ phy_has_fixups_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_has_fixups);
 
+static ssize_t
+phy_regs_show(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	if (!phydev->drv || !phydev->drv->read_regs)
+		return 0;
+
+	return phydev->drv->read_regs(phydev, buf, PAGE_SIZE);
+}
+
+static ssize_t
+phy_regs_store(struct device *dev, struct device_attribute *attr,
+		    const char *buf, size_t count)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	if (!phydev->drv || !phydev->drv->write_regs)
+		return 0;
+
+	return phydev->drv->write_regs(phydev, buf, count);
+}
+static DEVICE_ATTR_RW(phy_regs);
+
 static struct attribute *phy_dev_attrs[] = {
 	&dev_attr_phy_id.attr,
 	&dev_attr_phy_interface.attr,
 	&dev_attr_phy_has_fixups.attr,
+	&dev_attr_phy_regs.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(phy_dev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f7d95f6..c9c4ab3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -622,6 +622,10 @@ struct phy_driver {
 	int (*set_tunable)(struct phy_device *dev,
 			    struct ethtool_tunable *tuna,
 			    const void *data);
+
+	/* Diagnose PHY register configuration issue from user space */
+	ssize_t (*read_regs)(struct phy_device *dev, char *buf, size_t size);
+	int (*write_regs)(struct phy_device *dev, const char *buf, size_t size);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.7.4

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Andrew Lunn]

On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> if phy device have register(s) configuration problem,
> user can use this attribute to diagnose.
> this feature need phy driver maintainer implement.

what is wrong with mii-tool -vv ?

     Andrew

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Florian Fainelli]

On 01/14/2017 08:24 AM, Andrew Lunn wrote:
> On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
>> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>>
>> if phy device have register(s) configuration problem,
>> user can use this attribute to diagnose.
>> this feature need phy driver maintainer implement.
> 
> what is wrong with mii-tool -vv ?

Agreed, and without an actual user of this API (ethtool?), nor a PHY
driver implementing it, we cannot quite see how you want to make use of
this.

Thank you
-- 
Florian

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[yuan linyu]

On 六, 2017-01-14 at 10:35 -0800, Florian Fainelli wrote:
> On 01/14/2017 08:24 AM, Andrew Lunn wrote:
> > 
> > On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
> > > 
> > > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > > 
> > > if phy device have register(s) configuration problem,
> > > user can use this attribute to diagnose.
> > > this feature need phy driver maintainer implement.
> > what is wrong with mii-tool -vv ?
> Agreed, and without an actual user of this API (ethtool?), nor a PHY
> driver implementing it, we cannot quite see how you want to make use of
> this.
I hope user/developer can read this attribute file "regs" to do
a full check of all registers value, and they can write any register
inside PHY through this file.

I think mii-tool or ethtool can't do it currently.
> 
> Thank you

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Florian Fainelli]

Le 01/14/17 à 17:51, yuan linyu a écrit :
> On 六, 2017-01-14 at 10:35 -0800, Florian Fainelli wrote:
>> On 01/14/2017 08:24 AM, Andrew Lunn wrote:
>>>
>>> On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
>>>>
>>>> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>>>>
>>>> if phy device have register(s) configuration problem,
>>>> user can use this attribute to diagnose.
>>>> this feature need phy driver maintainer implement.
>>> what is wrong with mii-tool -vv ?
>> Agreed, and without an actual user of this API (ethtool?), nor a PHY
>> driver implementing it, we cannot quite see how you want to make use of
>> this.
> I hope user/developer can read this attribute file "regs" to do
> a full check of all registers value, and they can write any register
> inside PHY through this file.

You need to submit a PHY driver that implements the API you are
proposing here. Right now no PHY driver implements these {set,get}_regs
function pointers, so this is essentially dead code.

> 
> I think mii-tool or ethtool can't do it currently.

Maybe they cannot right now but they can certainly be patched to support
that. sysfs is not an appropriate interface for what you are proposing
here. We already have a set/get register via ethtool (-d/-D) it would
seem natural to use this.

Besides that, are not the current ioctl() good enough for that?
-- 
Florian

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[yuan linyu]

On 六, 2017-01-14 at 17:57 -0800, Florian Fainelli wrote:
> Le 01/14/17 à 17:51, yuan linyu a écrit :
> > 
> > I think mii-tool or ethtool can't do it currently.
> Maybe they cannot right now but they can certainly be patched to support
> that. sysfs is not an appropriate interface for what you are proposing
> here. We already have a set/get register via ethtool (-d/-D) it would
> seem natural to use this.
I think most ethernet driver implement ethtool -d to dump mac register.

> 
> Besides that, are not the current ioctl() good enough for that?
I think user/developer diagnose through simple attribute file will be easy.

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Andrew Lunn]

On Sun, Jan 15, 2017 at 09:51:03AM +0800, yuan linyu wrote:
> On ???, 2017-01-14 at 10:35 -0800, Florian Fainelli wrote:
> > On 01/14/2017 08:24 AM, Andrew Lunn wrote:
> > > 
> > > On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
> > > > 
> > > > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > > > 
> > > > if phy device have register(s) configuration problem,
> > > > user can use this attribute to diagnose.
> > > > this feature need phy driver maintainer implement.
> > > what is wrong with mii-tool -vv ?
> > Agreed, and without an actual user of this API (ethtool?), nor a PHY
> > driver implementing it, we cannot quite see how you want to make use of
> > this.
> I hope user/developer can read this attribute file "regs" to do
> a full check of all registers value, and they can write any register
> inside PHY through this file.

Since this is intended for debug, it should not be sysfs, but debugfs.

However, in general, Linux does not allow user space to peek and poke
device registers. Can you point me at examples where i can do the same
to my GPU? SATA controller? Ethernet controller, I2C temperature
sensor? Any device?

   Andrew

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[yuan linyu]

On 日, 2017-01-15 at 18:21 +0100, Andrew Lunn wrote:
> On Sun, Jan 15, 2017 at 09:51:03AM +0800, yuan linyu wrote:
> > 
> > I hope user/developer can read this attribute file "regs" to do
> > a full check of all registers value, and they can write any register
> > inside PHY through this file.
> Since this is intended for debug, it should not be sysfs, but debugfs.
agree,
> However, in general, Linux does not allow user space to peek and poke
> device registers. Can you point me at examples where i can do the same
> to my GPU? SATA controller? Ethernet controller, I2C temperature
> sensor? Any device?
we can read registers of ethernet controller(memory register accessed) through devmem or ethtool
> 
>    Andrew
> 

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Florian Fainelli]

On 01/16/2017 04:59 AM, yuan linyu wrote:
> On 日, 2017-01-15 at 18:21 +0100, Andrew Lunn wrote:
>> On Sun, Jan 15, 2017 at 09:51:03AM +0800, yuan linyu wrote:
>>>  
>>> I hope user/developer can read this attribute file "regs" to do
>>> a full check of all registers value, and they can write any register
>>> inside PHY through this file.
>> Since this is intended for debug, it should not be sysfs, but debugfs.
> agree,
>> However, in general, Linux does not allow user space to peek and poke
>> device registers. Can you point me at examples where i can do the same
>> to my GPU? SATA controller? Ethernet controller, I2C temperature
>> sensor? Any device?
> we can read registers of ethernet controller(memory register accessed) through devmem or ethtool

So why not add support in ethtool for reading PHY registers if you need
it? There are handful of PHY "things" in ethtool, such as reading
counters, configuring downshift etc., adding support for dumping
registers does not sound out of space.
-- 
Florian

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 16 Jan 2017 12:22:16 -0800

> On 01/16/2017 04:59 AM, yuan linyu wrote:
>> On 日, 2017-01-15 at 18:21 +0100, Andrew Lunn wrote:
>>> On Sun, Jan 15, 2017 at 09:51:03AM +0800, yuan linyu wrote:
>>>>  
>>>> I hope user/developer can read this attribute file "regs" to do
>>>> a full check of all registers value, and they can write any register
>>>> inside PHY through this file.
>>> Since this is intended for debug, it should not be sysfs, but debugfs.
>> agree,
>>> However, in general, Linux does not allow user space to peek and poke
>>> device registers. Can you point me at examples where i can do the same
>>> to my GPU? SATA controller? Ethernet controller, I2C temperature
>>> sensor? Any device?
>> we can read registers of ethernet controller(memory register accessed) through devmem or ethtool
> 
> So why not add support in ethtool for reading PHY registers if you need
> it? There are handful of PHY "things" in ethtool, such as reading
> counters, configuring downshift etc., adding support for dumping
> registers does not sound out of space.

Agreed.

RE: [PATCH] net: add regs attribute to phy device for user diagnose

[YUAN Linyu]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Tuesday, January 17, 2017 5:54 AM
> To: f.fainelli@gmail.com
> Cc: cugyly@163.com; andrew@lunn.ch; netdev@vger.kernel.org; YUAN Linyu
> Subject: Re: [PATCH] net: add regs attribute to phy device for user diagnose
> 
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Mon, 16 Jan 2017 12:22:16 -0800
> 
> >
> > So why not add support in ethtool for reading PHY registers if you need
> > it? There are handful of PHY "things" in ethtool, such as reading
> > counters, configuring downshift etc., adding support for dumping
> > registers does not sound out of space.
Thanks, I will try this direction.
> 
> Agreed.

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Zefir Kurtisi]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On 01/17/2017 01:11 AM, YUAN Linyu wrote:
> 
> 
>> -----Original Message-----
>> From: David Miller [mailto:davem@davemloft.net]
>> Sent: Tuesday, January 17, 2017 5:54 AM
>> To: f.fainelli@gmail.com
>> Cc: cugyly@163.com; andrew@lunn.ch; netdev@vger.kernel.org; YUAN Linyu
>> Subject: Re: [PATCH] net: add regs attribute to phy device for user diagnose
>>
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Mon, 16 Jan 2017 12:22:16 -0800
>>
>>>
>>> So why not add support in ethtool for reading PHY registers if you need
>>> it? There are handful of PHY "things" in ethtool, such as reading
>>> counters, configuring downshift etc., adding support for dumping
>>> registers does not sound out of space.
> Thanks, I will try this direction.
>>
>> Agreed.

... on the other hand, having direct RW access to MDIO regs can ease your life
greatly during bring-up / debugging of PHYs.

Attached is a patch we are using to track down PHY problems at register level, not
for integrating into the kernel but as a handy tool for developers.


Cheers,
Zefir



[-- Attachment #2: 0001-phy_device-add-sysfs-access-to-mdio-registers.patch --]
[-- Type: text/x-patch, Size: 3606 bytes --]

>From f90fced08ca6094919483b0d9a4bde50053bbbd7 Mon Sep 17 00:00:00 2001
From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Date: Wed, 18 Jan 2017 13:17:42 +0100
Subject: [PATCH] phy_device: add sysfs access to mdio registers

This commit adds a direct RW access to MDIO registers
over sysfs.

It is meant only for debugging and testing of PHYs
at register level.

WARNING: Writing to registers directly in most cases
will interfere with the phylib and/or upper layer
components or even crash your system.

Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
 drivers/net/phy/phy_device.c | 111 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 92b0838..16646763 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -617,10 +617,121 @@ phy_has_fixups_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_has_fixups);
 
+static ssize_t
+mdio_reg_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	struct mii_bus *bus = to_mii_bus(dev);
+	int regnum;
+	int val;
+
+	if (sscanf(attr->attr.name, "%d", &regnum) != 1)
+		return -EINVAL;
+
+	val = mdiobus_read(bus, phydev->mdio.addr, regnum);
+	if (val < 0)
+		return -EIO;
+
+	return sprintf(buf, "0x%.4x\n", val);
+}
+
+static ssize_t mdio_reg_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	struct mii_bus *bus = to_mii_bus(dev);
+	int regnum;
+	int val;
+	int err;
+
+	if (sscanf(attr->attr.name, "%d", &regnum) != 1)
+		return -EINVAL;
+
+	if (sscanf(buf, "%x", &val) != 1)
+		return -EINVAL;
+
+	if (val < 0 || val > 0xffff)
+		return -EINVAL;
+
+	err = mdiobus_write(bus, phydev->mdio.addr, regnum, val);
+	if (err < 0)
+		return -EIO;
+
+	return size;
+}
+
+#define MDIO_REG(_name) \
+	DEVICE_ATTR(_name, (S_IWUSR | S_IRUGO), mdio_reg_show, mdio_reg_store)
+
+static MDIO_REG(0);
+static MDIO_REG(1);
+static MDIO_REG(2);
+static MDIO_REG(3);
+static MDIO_REG(4);
+static MDIO_REG(5);
+static MDIO_REG(6);
+static MDIO_REG(7);
+static MDIO_REG(8);
+static MDIO_REG(9);
+static MDIO_REG(10);
+static MDIO_REG(11);
+static MDIO_REG(12);
+static MDIO_REG(13);
+static MDIO_REG(14);
+static MDIO_REG(15);
+static MDIO_REG(16);
+static MDIO_REG(17);
+static MDIO_REG(18);
+static MDIO_REG(19);
+static MDIO_REG(20);
+static MDIO_REG(21);
+static MDIO_REG(22);
+static MDIO_REG(23);
+static MDIO_REG(24);
+static MDIO_REG(25);
+static MDIO_REG(26);
+static MDIO_REG(27);
+static MDIO_REG(28);
+static MDIO_REG(29);
+static MDIO_REG(30);
+static MDIO_REG(31);
+
 static struct attribute *phy_dev_attrs[] = {
 	&dev_attr_phy_id.attr,
 	&dev_attr_phy_interface.attr,
 	&dev_attr_phy_has_fixups.attr,
+	&dev_attr_0.attr,
+	&dev_attr_1.attr,
+	&dev_attr_2.attr,
+	&dev_attr_3.attr,
+	&dev_attr_4.attr,
+	&dev_attr_5.attr,
+	&dev_attr_6.attr,
+	&dev_attr_7.attr,
+	&dev_attr_8.attr,
+	&dev_attr_9.attr,
+	&dev_attr_10.attr,
+	&dev_attr_11.attr,
+	&dev_attr_12.attr,
+	&dev_attr_13.attr,
+	&dev_attr_14.attr,
+	&dev_attr_15.attr,
+	&dev_attr_16.attr,
+	&dev_attr_17.attr,
+	&dev_attr_18.attr,
+	&dev_attr_19.attr,
+	&dev_attr_20.attr,
+	&dev_attr_21.attr,
+	&dev_attr_22.attr,
+	&dev_attr_23.attr,
+	&dev_attr_24.attr,
+	&dev_attr_25.attr,
+	&dev_attr_26.attr,
+	&dev_attr_27.attr,
+	&dev_attr_28.attr,
+	&dev_attr_29.attr,
+	&dev_attr_30.attr,
+	&dev_attr_31.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(phy_dev);
-- 
2.7.4

RE: [PATCH] net: add regs attribute to phy device for user diagnose

[YUAN Linyu]

> -----Original Message-----
> From: Zefir Kurtisi [mailto:zefir.kurtisi@neratec.com]
> 
> ... on the other hand, having direct RW access to MDIO regs can ease your life
> greatly during bring-up / debugging of PHYs.
> 
> Attached is a patch we are using to track down PHY problems at register level,
> not
> for integrating into the kernel but as a handy tool for developers.
> 

We should follow previous discussion and move this debug feature to ethtool.
Sysfs is not suggested by experts. I think it's reasonable and it's netdev development way.

One thing that your patch only care a few registers.
It's better to find a way to check all register of PHY.

I will add two ethtool command in kernel to read and write register in PHY. 
ethtool can use these command to dump what user want, there is no more work to PHY driver.


(
additional, mii-tool have two ioctl command[SIOCSMIIREG, SIOCGMIIREG], 
but is not export to use in mii-tool.
It's better to add command to eth-tool which maintained closely with kernel.
)

> 
> Cheers,
> Zefir
> 

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[Andrew Lunn]

> I will add two ethtool command in kernel to read and write register in PHY. 

Write access will get NACKed by me. Read only please.

> ethtool can use these command to dump what user want, there is no
> more work to PHY driver.

Please think about how you handle PHYs with pages. This needs to be
part of the API.

     Andrew

Re: [PATCH] net: add regs attribute to phy device for user diagnose

[yuan linyu]

On 四, 2017-01-19 at 02:01 +0100, Andrew Lunn wrote:
> > 
> > I will add two ethtool command in kernel to read and write register in PHY. 
> Write access will get NACKed by me. Read only please.
some register need to write some value first then read.
if read only, it will not achieve the goal.
> 
> > 
> > ethtool can use these command to dump what user want, there is no
> > more work to PHY driver.
> Please think about how you handle PHYs with pages. This needs to be
> part of the API.
thank, I will.
> 
>      Andrew