| Message ID | 20210116211916.8329-1-code@simerda.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: mdio: access c22 registers via debugfs | expand |
On Sat, Jan 16, 2021 at 22:19, Pavel Šimerda <code@simerda.eu> 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 <code@simerda.eu> 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.
On 1/19/21 12:27 AM, Tobias Waldekranz wrote: > On Sat, Jan 16, 2021 at 22:19, Pavel Šimerda <code@simerda.eu> 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 <code@simerda.eu> > > 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
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
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)
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 <code@simerda.eu> --- 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