[net-next] net: mdio: access c22 registers via debugfs

Commit Message

Pavel Šimerda Jan. 16, 2021, 9:19 p.m. UTC

Provide a debugging interface to read and write MDIO registers directly
without the need for a device driver.

This is extremely useful when debugging switch hardware and phy hardware
issues. The interface provides proper locking for communication that
consists of a sequence of MDIO read/write commands.

The interface binds directly to the MDIO bus abstraction in order to
provide support for all devices whether there's a hardware driver for
them or not. Registers are written by writing address, offset, and
value in hex, separated by colon. Registeres are read by writing only
address and offset, then reading the value.

It can be easily tested using `socat`:

    # socat - /sys/kernel/debug/mdio/f802c000.ethernet-ffffffff/control

Example: Reading address 0x00 offset 0x00, value is 0x3000

    Input: 00:00
    Output: 3000

Example: Writing address 0x00 offset 0x00, value 0x2100

    Input: 00:00:2100

Signed-off-by: Pavel Šimerda <[e-mail]>
---
 drivers/net/phy/Makefile       |   1 +
 drivers/net/phy/mdio-debugfs.c | 196 +++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio-debugfs.h |   2 +
 drivers/net/phy/mdio_bus.c     |   5 +
 include/linux/phy.h            |   3 +
 5 files changed, 207 insertions(+)
 create mode 100644 drivers/net/phy/mdio-debugfs.c
 create mode 100644 drivers/net/phy/mdio-debugfs.h

Comments

kernel test robot Jan. 16, 2021, 10:41 p.m. UTC | #1

Hi "Pavel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Pavel-imerda/net-mdio-access-c22-registers-via-debugfs/20210117-053409
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9ab7e76aefc97a9aa664accb59d6e8dc5e52514a
config: powerpc-adder875_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/22d74f4896850840331d36d6867f7bc5ce728bbd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-imerda/net-mdio-access-c22-registers-via-debugfs/20210117-053409
        git checkout 22d74f4896850840331d36d6867f7bc5ce728bbd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/net/phy/mdio-debugfs.c:170:6: warning: no previous prototype for 'mdio_debugfs_add' [-Wmissing-prototypes]
     170 | void mdio_debugfs_add(struct mii_bus *bus)
         |      ^~~~~~~~~~~~~~~~
>> drivers/net/phy/mdio-debugfs.c:177:6: warning: no previous prototype for 'mdio_debugfs_remove' [-Wmissing-prototypes]
     177 | void mdio_debugfs_remove(struct mii_bus *bus)
         |      ^~~~~~~~~~~~~~~~~~~
>> drivers/net/phy/mdio-debugfs.c:184:12: warning: no previous prototype for 'mdio_debugfs_init' [-Wmissing-prototypes]
     184 | int __init mdio_debugfs_init(void)
         |            ^~~~~~~~~~~~~~~~~
>> drivers/net/phy/mdio-debugfs.c:192:13: warning: no previous prototype for 'mdio_debugfs_exit' [-Wmissing-prototypes]
     192 | void __exit mdio_debugfs_exit(void)
         |             ^~~~~~~~~~~~~~~~~


vim +/mdio_debugfs_add +170 drivers/net/phy/mdio-debugfs.c

   169	
 > 170	void mdio_debugfs_add(struct mii_bus *bus)
   171	{
   172		bus->debugfs_dentry = debugfs_create_dir(dev_name(&bus->dev), mdio_debugfs_dentry);
   173		debugfs_create_file("control", 0600, bus->debugfs_dentry, bus, &mdio_debug_fops);
   174	}
   175	EXPORT_SYMBOL_GPL(mdio_debugfs_add);
   176	
 > 177	void mdio_debugfs_remove(struct mii_bus *bus)
   178	{
   179		debugfs_remove(bus->debugfs_dentry);
   180		bus->debugfs_dentry = NULL;
   181	}
   182	EXPORT_SYMBOL_GPL(mdio_debugfs_remove);
   183	
 > 184	int __init mdio_debugfs_init(void)
   185	{
   186		mdio_debugfs_dentry = debugfs_create_dir("mdio", NULL);
   187	
   188		return PTR_ERR_OR_ZERO(mdio_debugfs_dentry);
   189	}
   190	module_init(mdio_debugfs_init);
   191	
 > 192	void __exit mdio_debugfs_exit(void)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

kernel test robot Jan. 16, 2021, 11:20 p.m. UTC | #2

Hi "Pavel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Pavel-imerda/net-mdio-access-c22-registers-via-debugfs/20210117-053409
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9ab7e76aefc97a9aa664accb59d6e8dc5e52514a
config: i386-randconfig-s002-20210117 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-208-g46a52ca4-dirty
        # https://github.com/0day-ci/linux/commit/22d74f4896850840331d36d6867f7bc5ce728bbd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-imerda/net-mdio-access-c22-registers-via-debugfs/20210117-053409
        git checkout 22d74f4896850840331d36d6867f7bc5ce728bbd
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/net/phy/mdio-debugfs.c:160:17: sparse: sparse: incorrect type in initializer (different base types) @@     expected restricted __poll_t ( *poll )( ... ) @@     got unsigned int ( * )( ... ) @@
   drivers/net/phy/mdio-debugfs.c:160:17: sparse:     expected restricted __poll_t ( *poll )( ... )
   drivers/net/phy/mdio-debugfs.c:160:17: sparse:     got unsigned int ( * )( ... )
>> drivers/net/phy/mdio-debugfs.c:154:24: sparse: sparse: symbol 'mdio_debug_fops' was not declared. Should it be static?
>> drivers/net/phy/mdio-debugfs.c:184:12: sparse: sparse: symbol 'mdio_debugfs_init' was not declared. Should it be static?
>> drivers/net/phy/mdio-debugfs.c:192:13: sparse: sparse: symbol 'mdio_debugfs_exit' was not declared. Should it be static?

Please review and possibly fold the followup patch.

vim +160 drivers/net/phy/mdio-debugfs.c

   153	
 > 154	struct file_operations mdio_debug_fops = {
   155		.owner = THIS_MODULE,
   156		.open = mdio_debug_open,
   157		.release = mdio_debug_release,
   158		.write = mdio_debug_write,
   159		.read = mdio_debug_read,
 > 160		.poll = mdio_debug_poll,
   161	};
   162	
   163	/*
   164	 * TODO: This implementation doesn't support module load/unload and has no
   165	 * error checking.
   166	 */
   167	
   168	static struct dentry *mdio_debugfs_dentry;
   169	
   170	void mdio_debugfs_add(struct mii_bus *bus)
   171	{
   172		bus->debugfs_dentry = debugfs_create_dir(dev_name(&bus->dev), mdio_debugfs_dentry);
   173		debugfs_create_file("control", 0600, bus->debugfs_dentry, bus, &mdio_debug_fops);
   174	}
   175	EXPORT_SYMBOL_GPL(mdio_debugfs_add);
   176	
   177	void mdio_debugfs_remove(struct mii_bus *bus)
   178	{
   179		debugfs_remove(bus->debugfs_dentry);
   180		bus->debugfs_dentry = NULL;
   181	}
   182	EXPORT_SYMBOL_GPL(mdio_debugfs_remove);
   183	
 > 184	int __init mdio_debugfs_init(void)
   185	{
   186		mdio_debugfs_dentry = debugfs_create_dir("mdio", NULL);
   187	
   188		return PTR_ERR_OR_ZERO(mdio_debugfs_dentry);
   189	}
   190	module_init(mdio_debugfs_init);
   191	
 > 192	void __exit mdio_debugfs_exit(void)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

kernel test robot Jan. 16, 2021, 11:23 p.m. UTC | #3

Hi "Pavel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Pavel-imerda/net-mdio-access-c22-registers-via-debugfs/20210117-053409
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9ab7e76aefc97a9aa664accb59d6e8dc5e52514a
config: s390-randconfig-r015-20210117 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2082b10d100e8dbaffc2ba8f497db5d2ab61beb2)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/22d74f4896850840331d36d6867f7bc5ce728bbd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pavel-imerda/net-mdio-access-c22-registers-via-debugfs/20210117-053409
        git checkout 22d74f4896850840331d36d6867f7bc5ce728bbd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from drivers/net/phy/mdio-debugfs.c:3:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:18:
   In file included from include/uapi/linux/ethtool.h:19:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from drivers/net/phy/mdio-debugfs.c:3:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:18:
   In file included from include/uapi/linux/ethtool.h:19:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from drivers/net/phy/mdio-debugfs.c:3:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:18:
   In file included from include/uapi/linux/ethtool.h:19:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from drivers/net/phy/mdio-debugfs.c:3:
   In file included from include/linux/phy.h:16:
   In file included from include/linux/ethtool.h:18:
   In file included from include/uapi/linux/ethtool.h:19:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/net/phy/mdio-debugfs.c:170:6: warning: no previous prototype for function 'mdio_debugfs_add' [-Wmissing-prototypes]
   void mdio_debugfs_add(struct mii_bus *bus)
        ^
   drivers/net/phy/mdio-debugfs.c:170:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mdio_debugfs_add(struct mii_bus *bus)
   ^
   static 
>> drivers/net/phy/mdio-debugfs.c:177:6: warning: no previous prototype for function 'mdio_debugfs_remove' [-Wmissing-prototypes]
   void mdio_debugfs_remove(struct mii_bus *bus)
        ^
   drivers/net/phy/mdio-debugfs.c:177:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mdio_debugfs_remove(struct mii_bus *bus)
   ^
   static 
>> drivers/net/phy/mdio-debugfs.c:184:12: warning: no previous prototype for function 'mdio_debugfs_init' [-Wmissing-prototypes]
   int __init mdio_debugfs_init(void)
              ^
   drivers/net/phy/mdio-debugfs.c:184:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __init mdio_debugfs_init(void)
   ^
   static 
>> drivers/net/phy/mdio-debugfs.c:192:13: warning: no previous prototype for function 'mdio_debugfs_exit' [-Wmissing-prototypes]
   void __exit mdio_debugfs_exit(void)
               ^
   drivers/net/phy/mdio-debugfs.c:192:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void __exit mdio_debugfs_exit(void)
   ^
   static 
   24 warnings generated.


vim +/mdio_debugfs_add +170 drivers/net/phy/mdio-debugfs.c

   169	
 > 170	void mdio_debugfs_add(struct mii_bus *bus)
   171	{
   172		bus->debugfs_dentry = debugfs_create_dir(dev_name(&bus->dev), mdio_debugfs_dentry);
   173		debugfs_create_file("control", 0600, bus->debugfs_dentry, bus, &mdio_debug_fops);
   174	}
   175	EXPORT_SYMBOL_GPL(mdio_debugfs_add);
   176	
 > 177	void mdio_debugfs_remove(struct mii_bus *bus)
   178	{
   179		debugfs_remove(bus->debugfs_dentry);
   180		bus->debugfs_dentry = NULL;
   181	}
   182	EXPORT_SYMBOL_GPL(mdio_debugfs_remove);
   183	
 > 184	int __init mdio_debugfs_init(void)
   185	{
   186		mdio_debugfs_dentry = debugfs_create_dir("mdio", NULL);
   187	
   188		return PTR_ERR_OR_ZERO(mdio_debugfs_dentry);
   189	}
   190	module_init(mdio_debugfs_init);
   191	
 > 192	void __exit mdio_debugfs_exit(void)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Tobias Waldekranz Jan. 18, 2021, 11:27 p.m. UTC | #4

On Sat, Jan 16, 2021 at 22:19, Pavel Šimerda <[e-mail]> wrote:
> Provide a debugging interface to read and write MDIO registers directly
> without the need for a device driver.
>
> This is extremely useful when debugging switch hardware and phy hardware
> issues. The interface provides proper locking for communication that
> consists of a sequence of MDIO read/write commands.
>
> The interface binds directly to the MDIO bus abstraction in order to
> provide support for all devices whether there's a hardware driver for
> them or not. Registers are written by writing address, offset, and
> value in hex, separated by colon. Registeres are read by writing only
> address and offset, then reading the value.
>
> It can be easily tested using `socat`:
>
>     # socat - /sys/kernel/debug/mdio/f802c000.ethernet-ffffffff/control
>
> Example: Reading address 0x00 offset 0x00, value is 0x3000
>
>     Input: 00:00
>     Output: 3000
>
> Example: Writing address 0x00 offset 0x00, value 0x2100
>
>     Input: 00:00:2100
>
> Signed-off-by: Pavel Šimerda <[e-mail]>

Hi Pavel,

I also tried my luck at adding an MDIO debug interface to the kernel a
while back:

https://lore.kernel.org/netdev/C42DZQLTPHM5.2THDSRK84BI3T@wkz-x280

The conclusion was that, while nice to have, it makes it too easy for
shady vendors to write out-of-tree drivers.

You might want to have a look at https://github.com/wkz/mdio-tools. It
solves the same issue that your debugfs interface does, and also some
other nice things like clause 45 addressing and atomic read/mask/write
operations.

Pavel Šimerda Jan. 19, 2021, 12:55 a.m. UTC | #5

On 1/19/21 12:27 AM, Tobias Waldekranz wrote:
> On Sat, Jan 16, 2021 at 22:19, Pavel Šimerda <[e-mail]> wrote:
>> Provide a debugging interface to read and write MDIO registers directly
>> without the need for a device driver.
>>
>> This is extremely useful when debugging switch hardware and phy hardware
>> issues. The interface provides proper locking for communication that
>> consists of a sequence of MDIO read/write commands.
>>
>> The interface binds directly to the MDIO bus abstraction in order to
>> provide support for all devices whether there's a hardware driver for
>> them or not. Registers are written by writing address, offset, and
>> value in hex, separated by colon. Registeres are read by writing only
>> address and offset, then reading the value.
>>
>> It can be easily tested using `socat`:
>>
>>      # socat - /sys/kernel/debug/mdio/f802c000.ethernet-ffffffff/control
>>
>> Example: Reading address 0x00 offset 0x00, value is 0x3000
>>
>>      Input: 00:00
>>      Output: 3000
>>
>> Example: Writing address 0x00 offset 0x00, value 0x2100
>>
>>      Input: 00:00:2100
>>
>> Signed-off-by: Pavel Šimerda <[e-mail]>
> 
> Hi Pavel,
> 
> I also tried my luck at adding an MDIO debug interface to the kernel a
> while back:
> 
> https://lore.kernel.org/netdev/C42DZQLTPHM5.2THDSRK84BI3T@wkz-x280

Hey Tobias,

nice to meet you!

> The conclusion was that, while nice to have, it makes it too easy for
> shady vendors to write out-of-tree drivers.

That was exactly what I was afraid of.

> You might want to have a look at https://github.com/wkz/mdio-tools. It
> solves the same issue that your debugfs interface does, and also some
> other nice things like clause 45 addressing and atomic read/mask/write
> operations.

Thank you very much!

Cheers,

Pavel

Pavel Šimerda Jan. 19, 2021, 1:08 a.m. UTC | #6

Hi Tobias,

given the reasons stated in the mailing list, I'd like to discuss the situation off-list. I would be more than happy to join your effort and provide an OpenWRT package. I understand the motivation to reject that, and I do use it partially also for the “bad purpose” and therefore I'd like to solve it as consistently as possible.

I'm working with mv88e6xxx where c45 can be coded in c22 anyway, so I didn't care to implement it in the MDIO driver. I'd like to share with you the user space script I'm using to access both mv88e6xxx and direct PHY registers.

I see you're working with mv88e6xxx as well. Can you access all of the inderect registers up to multichip+indirect+paged/c45 registers even without the mv88e6xxx driver loaded, or not? I'm using this feature to bootstrap the switch and get it onto the network when the driver doesn't work yet.

I've seen a few new patches submitted to next-next regarding mv88e6393 and the lag support. I'm also going to explore MSTP and more. I published some of my changes that might not be accepted upstream any soon, or like this one, at all:

https://github.com/switchwrt

Regards,

Pavel

Pavel Šimerda Jan. 19, 2021, 1:11 a.m. UTC | #7

My mdio acces script attached.

Regards,

Pavel


On 1/19/21 2:08 AM, Pavel Šimerda wrote:
> Hi Tobias,
> 
> given the reasons stated in the mailing list, I'd like to discuss the situation off-list. I would be more than happy to join your effort and provide an OpenWRT package. I understand the motivation to reject that, and I do use it partially also for the “bad purpose” and therefore I'd like to solve it as consistently as possible.
> 
> I'm working with mv88e6xxx where c45 can be coded in c22 anyway, so I didn't care to implement it in the MDIO driver. I'd like to share with you the user space script I'm using to access both mv88e6xxx and direct PHY registers.
> 
> I see you're working with mv88e6xxx as well. Can you access all of the inderect registers up to multichip+indirect+paged/c45 registers even without the mv88e6xxx driver loaded, or not? I'm using this feature to bootstrap the switch and get it onto the network when the driver doesn't work yet.
> 
> I've seen a few new patches submitted to next-next regarding mv88e6393 and the lag support. I'm also going to explore MSTP and more. I published some of my changes that might not be accepted upstream any soon, or like this one, at all:
> 
> https://github.com/switchwrt
> 
> Regards,
> 
> Pavel
#!/bin/sh

IFS=,

if [ -z "$MDIO_CONTROL" ]; then
	MDIO_CONTROL=/sys/kernel/debug/mdio/f0028000.ethernet/control
fi

verbose=
phy_chipaddr=
phy_page=
phy_address=
phy_device=
phy_offset=
phy_bits=
phy_value=

while [ $# != 0 ]; do
	case $1 in
	-v|--verbose)
		verbose=1
		;;
	-n|--no-write)
		no_write=1
		;;
	-L|--link)
		MDIO_CONTROL=/sys/kernel/debug/mdio/f802c000.ethernet/control
		;;
	-S|--switch)
		MDIO_CONTROL=/sys/kernel/debug/mdio/f0028000.ethernet/control
		;;
	-P|--phy)
		MDIO_CONTROL=$(echo /sys/kernel/debug/mdio/!ahb!apb!ethernet@f0028000!switch0@*!mdio/control)
		;;
	-m|--multichip)
		shift
		phy_chipaddr=$1
		;;
	-p|--page)
		shift
		phy_page=$1
		;;
	-a|--address)
		shift
		phy_addresses=$1
		;;
	-d|--device)
		shift
		phy_device=$1
		;;
	-o|--offset)
		shift
		phy_offset=$1
		;;
	-b|--bits)
		shift
		phy_bits=$1
		;;
	-w|--write)
		shift
		phy_value=$1
		;;
	esac
	shift
done

parse_bits() {
	bit_stop=${1%:*}
	bit_start=${1#*:}
	bit_stop=$((bit_stop + 1))
}

_get_bits() {
	local bit_start bit_stop
	parse_bits "$1"
	value="$2"

	printf "0x%.04x\n" "$(( (value & ((1<<bit_stop) - 1)) >> bit_start ))"
}

_set_bits() {
	local original="$1"
	local bit_start bit_stop
	parse_bits "$2"
	local value="$3"

	local mask="$(( (1<<bit_stop) - (1<<bit_start) ))"
	printf "0x%.04x\n" "$(( (original & ~mask) | ((value << bit_start) & mask) ))"
}

direct_cmd() {
	echo -n "$1" >&3
}

direct_read() {
	direct_cmd $(printf "%.2x:%.2x\n" "$1" "$2")
	echo "0x$(head -n 1 <&3)"
}

direct_write() {
	direct_cmd $(printf "%.2x:%.2x:%.4x\n" "$1" "$2" "$3")
}

indirect_wait() {
	ret=$(direct_read "$phy_chipaddr" 0x00)
	while [ $(( ret & 0x8000 )) != 0 ]; do
		sleep 1
		ret=$(direct_read "$phy_chipaddr" 0x00)
	done
}

indirect_read() {
	address="$1"
	offset="$2"
	cmd=$(printf "0x%.4x" "$(( 0x9800 | ((address & 0x1f) << 5) | (offset & 0x1f) ))")
	indirect_wait
	direct_write "$phy_chipaddr" 0x00 "$cmd"
	indirect_wait
	direct_read "$phy_chipaddr" 0x01
}

indirect_write() {
	address="$1"
	offset="$2"
	data="$3"
	cmd=$(printf "0x%.4x" "$(( 0x9400 | ((address & 0x1f) << 5) | (offset & 0x1f) ))")
	indirect_wait
	direct_write "$phy_chipaddr" 0x01 "$data"
	direct_write "$phy_chipaddr" 0x00 "$cmd"
	indirect_wait
}

mdio_read() {
	local address="$1"
	local offset="$2"
	local bits="$3"
	[ "$verbose" = 1 ] && printf "read: 0x%.2x:0x%.2x\n" "$address" "$offset" >&2
	if [ -n "$phy_chipaddr" ]; then
		value=$(indirect_read "$address" "$offset")
	else
		value=$(direct_read "$address" "$offset")
	fi
	[ $? = 0 ] || echo "Read error: address=$1 offset=$2" >&2

	if [ -z "$bits" ]; then
		echo $value
	else
		_get_bits "$bits" "$value"
	fi
}

mdio_write() {
	local address="$1"
	local offset="$2"
	local bits="$3"
	local value="$4"

	if [ -n "$bits" ]; then
		original="$(mdio_read "$address" "$offset")"
		value="$(_set_bits "$original" "$bits" "$value")"
	fi

	[ -n "$verbose" -a -n "$no_write" ] && printf "no "
	[ -n "$verbose" ] && printf "write: 0x%.2x:0x%.2x = %.4x\n" "$address" "$offset" "$value" >&2
	if [ -z "$no_write" ]; then
		if [ -n "$phy_chipaddr" ]; then
			indirect_write "$address" "$offset" "$value"
		else
			direct_write "$address" "$offset" "$value"
		fi
		[ $? = 0 ] || echo "Write error: address=$1 offset=$2 value=$3" >&2
	fi
}

# TODO: Remember and reset original page rather than 0x0001.
mdio_page_set() {
	if [ -n "$phy_page" ]; then
		phy_orig_page="$(mdio_read "$1" 0x16)"
		mdio_write "$1" 0x16 "$phy_page"
	fi
}

mdio_page_reset() {
	[ -n "$phy_page" ] && mdio_write "$1" 0x16 "$(phy_orig_page)"
}

usage() {
	echo "Get c22 register:"
	echo "  mdio --phy --address 0 --offset 0x00"
	echo "  mdio -P -a 0 -o 0x00"
	echo "Set c22 register:"
	echo "  mdio --phy --address 0 --offset 0x00 --write 0x8000"
	echo "  mdio -P -a 0 -o 0x00 -w 0x8000"
	echo "Note: Read the source."
	exit 1
}

exec 3<>"$MDIO_CONTROL"
[ -n "$phy_addresses" -a -n "$phy_offset" ] || usage

for phy_address in $phy_addresses; do
	[ -n "$phy_device" ] && phy_offset=$(( phy_offset | phy_device << 16 | 0x40000000 ))

	mdio_page_set "$phy_address"
	if [ -n "$phy_value" ]; then
		mdio_write "$phy_address" "$phy_offset" "$phy_bits" "$phy_value"
	else
		mdio_read "$phy_address" "$phy_offset" "$phy_bits"
	fi
	mdio_page_reset "$phy_address"
done

Patch

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf..4999cb97844b 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -17,6 +17,7 @@  libphy-y			+= $(mdio-bus-y)
 else
 obj-$(CONFIG_MDIO_DEVICE)	+= mdio-bus.o
 endif
+obj-$(CONFIG_MDIO_DEVICE)	+= mdio-debugfs.o
 obj-$(CONFIG_MDIO_DEVRES)	+= mdio_devres.o
 libphy-$(CONFIG_SWPHY)		+= swphy.o
 libphy-$(CONFIG_LED_TRIGGER_PHY)	+= phy_led_triggers.o
diff --git a/drivers/net/phy/mdio-debugfs.c b/drivers/net/phy/mdio-debugfs.c
new file mode 100644
index 000000000000..abed40052c20
--- /dev/null
+++ b/drivers/net/phy/mdio-debugfs.c
@@ -0,0 +1,196 @@ 
+#include <linux/module.h>
+#include <linux/debugfs.h>
+#include <linux/phy.h>
+#include <linux/poll.h>
+
+/*
+ * TODO: May need locking implementation to avoid being susceptible to file
+ * descriptor sharing concurrency issues
+ */
+struct mdio_debug {
+	wait_queue_head_t queue;
+	int value;
+};
+
+static int mdio_debug_open(struct inode *inode, struct file *file)
+{
+	struct mii_bus *bus = file->f_inode->i_private;
+	struct mdio_debug *data;
+	int err;
+
+	data = kzalloc(sizeof *data, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	err = mutex_lock_interruptible(&bus->mdio_lock);
+	if (err) {
+		kfree(data);
+		return err;
+	}
+	dev_dbg(&bus->dev, "MDIO locked for user space program.\n");
+
+	init_waitqueue_head(&data->queue);
+	data->value = -1;
+	file->private_data = data;
+
+	return 0;
+}
+
+static int mdio_debug_release(struct inode *inode, struct file *file)
+{
+	struct mii_bus *bus = file->f_inode->i_private;
+	struct mdio_debug *data = file->private_data;
+
+	file->private_data = NULL;
+
+	mutex_unlock(&bus->mdio_lock);
+	dev_dbg(&bus->dev, "MDIO unlocked.\n");
+
+	kfree(data);
+
+	return 0;
+}
+
+static ssize_t mdio_debug_write(struct file *file, const char __user *buffer, size_t size, loff_t *off)
+{
+	struct mii_bus *bus = file->f_inode->i_private;
+	struct mdio_debug *data = file->private_data;
+	char str[64] = {};
+	char *s = str;
+	char *token;
+	int addr;
+	int offset;
+	int value;
+	int ret;
+
+	if (data->value != -1)
+		return -EWOULDBLOCK;
+
+	if (size > sizeof str - 1)
+		return -EINVAL;
+
+	ret = copy_from_user(str, buffer, size);
+	if (ret)
+		return -EFAULT;
+
+	if (str[size-1] == '\n')
+		str[size-1] = '\0';
+
+	token = strsep(&s, ":");
+	if (!token)
+		return -EINVAL;
+	ret = kstrtoint(token, 16, &addr);
+	if (ret)
+		return ret;
+
+	token = strsep(&s, ":");
+	if (!token)
+		return -EINVAL;
+	ret = kstrtoint(token, 16, &offset);
+	if (ret)
+		return ret;
+
+	token = strsep(&s, ":");
+
+	if (token) {
+		ret = kstrtoint(token, 16, &value);
+		if (ret)
+			return ret;
+
+		ret = __mdiobus_write(bus, addr, offset, value);
+		if (ret)
+			return ret;
+
+		dev_dbg(&bus->dev, "write: addr=0x%.2x offset=0x%.2x value=%.4x\n",
+			addr, offset, value);
+	} else {
+		value = __mdiobus_read(bus, addr, offset);
+		if (value < 0)
+			return value;
+
+		dev_dbg(&bus->dev, "read: addr=0x%.2x offset=0x%.2x value=%.4x\n",
+			addr, offset, value);
+
+		data->value = value;
+		wake_up_all(&data->queue);
+	}
+
+	return size;
+}
+
+static ssize_t mdio_debug_read(struct file *file, char __user *buffer, size_t size, loff_t *off)
+{
+	struct mdio_debug *data = file->private_data;
+	char str[6];
+	int ret;
+	ssize_t rsize;
+
+	if (data->value == -1)
+		return -EWOULDBLOCK;
+
+	rsize = snprintf(str, sizeof str, "%04x\n", data->value);
+	if (rsize > size)
+		return -EINVAL;
+
+	ret = copy_to_user(buffer, str, rsize);
+	if (ret)
+		return -EFAULT;
+
+	data->value = -1;
+	wake_up_all(&data->queue);
+
+	return rsize;
+}
+
+static unsigned int mdio_debug_poll(struct file *file, poll_table *wait)
+{
+	struct mdio_debug *data = file->private_data;
+
+	poll_wait(file, &data->queue, wait);
+
+	return data->value == -1 ? POLLOUT : POLLIN;
+}
+
+struct file_operations mdio_debug_fops = {
+	.owner = THIS_MODULE,
+	.open = mdio_debug_open,
+	.release = mdio_debug_release,
+	.write = mdio_debug_write,
+	.read = mdio_debug_read,
+	.poll = mdio_debug_poll,
+};
+
+/*
+ * TODO: This implementation doesn't support module load/unload and has no
+ * error checking.
+ */
+
+static struct dentry *mdio_debugfs_dentry;
+
+void mdio_debugfs_add(struct mii_bus *bus)
+{
+	bus->debugfs_dentry = debugfs_create_dir(dev_name(&bus->dev), mdio_debugfs_dentry);
+	debugfs_create_file("control", 0600, bus->debugfs_dentry, bus, &mdio_debug_fops);
+}
+EXPORT_SYMBOL_GPL(mdio_debugfs_add);
+
+void mdio_debugfs_remove(struct mii_bus *bus)
+{
+	debugfs_remove(bus->debugfs_dentry);
+	bus->debugfs_dentry = NULL;
+}
+EXPORT_SYMBOL_GPL(mdio_debugfs_remove);
+
+int __init mdio_debugfs_init(void)
+{
+	mdio_debugfs_dentry = debugfs_create_dir("mdio", NULL);
+
+	return PTR_ERR_OR_ZERO(mdio_debugfs_dentry);
+}
+module_init(mdio_debugfs_init);
+
+void __exit mdio_debugfs_exit(void)
+{
+	debugfs_remove(mdio_debugfs_dentry);
+}
+module_exit(mdio_debugfs_exit);
diff --git a/drivers/net/phy/mdio-debugfs.h b/drivers/net/phy/mdio-debugfs.h
new file mode 100644
index 000000000000..fe98dcdfcacf
--- /dev/null
+++ b/drivers/net/phy/mdio-debugfs.h
@@ -0,0 +1,2 @@ 
+void mdio_debugfs_add(struct mii_bus *bus);
+void mdio_debugfs_remove(struct mii_bus *bus);
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 040509b81f02..969cb8e1ebee 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -39,6 +39,7 @@ 
 #include <trace/events/mdio.h>
 
 #include "mdio-boardinfo.h"
+#include "mdio-debugfs.h"
 
 static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
 {
@@ -581,6 +582,8 @@  int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 
 	mdiobus_setup_mdiodev_from_board_info(bus, mdiobus_create_device);
 
+	mdio_debugfs_add(bus);
+
 	bus->state = MDIOBUS_REGISTERED;
 	pr_info("%s: probed\n", bus->name);
 	return 0;
@@ -612,6 +615,8 @@  void mdiobus_unregister(struct mii_bus *bus)
 	BUG_ON(bus->state != MDIOBUS_REGISTERED);
 	bus->state = MDIOBUS_UNREGISTERED;
 
+	mdio_debugfs_remove(bus);
+
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		mdiodev = bus->mdio_map[i];
 		if (!mdiodev)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 24fcc6456a9e..f4cc93c11006 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -371,6 +371,9 @@  struct mii_bus {
 
 	/** @shared: shared state across different PHYs */
 	struct phy_package_shared *shared[PHY_MAX_ADDR];
+
+	/* debugfs file for MDIO access */
+	struct dentry *debugfs_dentry;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)