[PATCH 1/1] Add mkopci driver

[sergej . bauer]

mkopci (MB11.xx) device (RC Module project) provides data transference through 
a serial bus bar according to MIL-STD-1553.
the driver used for operating devices, reads PCI configuration space and pass 
interrupts to user-space applications.

Please consider adding this patch to the linux-next queue.

Signed-off-by: Sergej Bauer 
---
 Documentation/misc-devices/mkopci.txt |   51 ++
 drivers/misc/Kconfig  |9 +
 drivers/misc/Makefile |1 +
 drivers/misc/mkopci.c | 1272 +
 include/misc/mkopci.h |   81 +++
 5 files changed, 1414 insertions(+)
create mode 100644 Documentation/misc-devices/mkopci.txt
create mode 100644 drivers/misc/mkopci.c
create mode 100644 include/misc/mkopci.h
diff --git a/Documentation/misc-devices/mkopci.txt 
b/Documentation/misc-devices/mkopci.txt
new file mode 100644
index 000..0fcec6d
--- /dev/null
+++ b/Documentation/misc-devices/mkopci.txt
@@ -0,0 +1,51 @@
+PCI-based MKO bus driver.
+
+
+For dealing with driver without using of root's account it will be helpfull
+to add group `mkopci' with appropriate users and put file, say 60-mkopci.rules 
to
+/etc/udev/rules.d in your system.
+--- cut 60-mkopci.rules ---
+# MKO devices
+KERNEL=="mkopci*", SUBSYSTEM=="mkopci", ACTION=="add", DRIVERS=="?*", 
ATTRS{idVendor}=="0x6403"
+GROUP="mkopci"
+---
+
+
+Kernel module parameters
+
+Kernel module can take parameters 'v' and 'omited'
+- 'v' is 0 to 3, and affects the amount of information
+output to the system log.
+0 - (default) comletely silent
+1 - prints only detected BARs, reports loading / unloading
+2 - + prints IRQ and memory mapping events
+3 - + prints ioctl events
+
+- 'plx9050bug_quirk' controls workaround controller PLX9050. Can
+the following values:
+0 - workaround is disabled (default)
+1 - workaround is enabled
+2 - forced bug
+
+- 'omited' tells the driver which device should not be initialized
+at load time.
+The value of this parameter to the kernel module 2.4 is a physical address
+device on the bus, such as "0x10800" without the quotes.
+In kernel versions 2.6+ can exclude multiple devices, transferring them
+values separated by commas, but not more than 4.
+
+The parameter values can be specified as follows:
+$ insmod/modprobe mkopci.[ko/o] parameter1_name=value [parameter2_name=value]
+
+Record the physical address of the device in the file /proc/mkopci/core
+also controls "visibility" for user programs. Missed return device
+You can command 'echo ADDR > /proc/mkopci/core'. ADDR can be either a simple
+number of device, but always in hexadecimal, or (for Linux-2.6+)
+the number of devices in a standard format Linux kind NM:XY.z.
+
+
+PROC filesystem
+
+/proc/mkopci/devices - read only text device table
+/proc/mkopci/core- device table for user space applications
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 006242c..7db247e 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -124,6 +124,15 @@ config PHANTOM
  If you choose to build module, its name will be phantom. If unsure,
  say N here.
 
+config MKOPCI 
+   tristate "Module PCI bus driver"
+   depends on PCI && PROC_FS
+   help
+ Say Y here if you want to build a driver for Module(RC) MKOPCI 
devices.
+
+ If you choose to build module, its name will be mkopci. If unsure,
+ say N here.
+
 config INTEL_MID_PTI
tristate "Parallel Trace Interface for MIPI P1149.7 cJTAG standard"
depends on PCI && TTY && (X86_INTEL_MID || COMPILE_TEST)
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7d5c4cd..afb92b4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE)  += genwqe/
 obj-$(CONFIG_ECHO) += echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)  += vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE) += cxl/
+obj-$(CONFIG_MKOPCI)   += mkopci.o
diff --git a/drivers/misc/mkopci.c b/drivers/misc/mkopci.c
new file mode 100644
index 000..d223478
--- /dev/null
+++ b/drivers/misc/mkopci.c
@@ -0,0 +1,1272 @@
+/*
+ *  MKOPCI driver
+ *
+ *  Copyright (C) 2007-2015 Sergej Bauer 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License, as published by
+ *  the Free Software Foundation, version 2.
+*/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "misc/mkopci.h"
+
+#define MKO_VENDOR  0x6403
+#define MKO_DEVICE1 0x0430
+#define MKO_DEVICE2 0x0431
+#define MKO_DEVICE3 0x0434
+
+#define PLX9050BUG_BAR0 0x1
+#define PLX9050BUG_B

Re: [PATCH 1/1] Add mkopci driver

[Sergej Bauer]

 bar according to MIL-STD-1553.
+
+ Say Y here if you want to build a driver for Module(RC) MKOPCI 
devices.
+
+ If you choose to build module, its name will be mkopci. If unsure,
+ say N here.
+
 config INTEL_MID_PTI
tristate "Parallel Trace Interface for MIPI P1149.7 cJTAG standard"
depends on PCI && TTY && (X86_INTEL_MID || COMPILE_TEST)
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7d5c4cd..afb92b4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE)  += genwqe/
 obj-$(CONFIG_ECHO) += echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)  += vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE) += cxl/
+obj-$(CONFIG_MKOPCI)   += mkopci.o
diff --git a/drivers/misc/mkopci.c b/drivers/misc/mkopci.c
new file mode 100644
index 000..1e2b77a
--- /dev/null
+++ b/drivers/misc/mkopci.c
@@ -0,0 +1,1272 @@
+/*
+ *  MKOPCI driver
+ *
+ *  Copyright (C) 2007-2015 Sergej Bauer 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License, as published by
+ *  the Free Software Foundation, version 2.
+*/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "misc/mkopci.h"
+
+#define MKO_VENDOR  0x6403
+#define MKO_DEVICE1 0x0430
+#define MKO_DEVICE2 0x0431
+#define MKO_DEVICE3 0x0434
+
+#define PLX9050BUG_BAR0 0x1
+#define PLX9050BUG_BAR1 0x2
+#define PLX9050BUG_INJECT 0x4
+
+#if !defined(__user)
+#define __user
+#endif
+
+static struct pci_device_id ids[] = {
+   {MKO_VENDOR, MKO_DEVICE1, PCI_ANY_ID, PCI_ANY_ID,},
+   {MKO_VENDOR, MKO_DEVICE2, PCI_ANY_ID, PCI_ANY_ID,},
+   {MKO_VENDOR, MKO_DEVICE3, PCI_ANY_ID, PCI_ANY_ID,},
+   {0, 0,}
+};
+
+MODULE_DEVICE_TABLE(pci, ids);
+
+#ifdef __LP64__
+#define PFMT   "llx"
+#else
+#define PFMT   "x"
+#endif
+
+unsigned char drv_version = 0x11;
+#define mko_pci_addr(bus, device, func, regoffs) (\
+   ((bus  & 0xFF) << 16) | ((device & 0x1F) << 11) | \
+   ((func & 0x7)  <<  8) | (regoffs & 0xFC))
+
+static struct kmem_cache *mkopci_device_cache;
+static dev_t devp;
+static struct class *mkopci_class;
+static struct rw_semaphore devices_sem;
+static LIST_HEAD(devices);
+static atomic_t devices_nr = ATOMIC_INIT(0);
+
+/*** module parameters ***/
+static int plx9050bug_quirk = 1;
+/* verbosity level */
+static int v;
+module_param(plx9050bug_quirk, int, S_IRUGO);
+module_param(v, int, S_IRUGO);
+MODULE_PARM_DESC(plx9050bug_quirk, "PLX9050 bug quirk");
+MODULE_PARM_DESC(v, "verbosity level");
+
+/* only MAX_DEVICES_NR devices at once can be omited */
+static int omited[MAX_DEVICES_NR];
+static int omited_nr;
+module_param_array(omited, int, &omited_nr, S_IRUGO);
+MODULE_PARM_DESC(omited, "device(s) to omit");
+#ifndef VM_RESERVED
+#define VM_RESERVED (VM_DONTEXPAND | VM_DONTDUMP)
+#endif
+#define __unused (__attribute__ ((unused)))
+
+/* Interrupt handling 
*/
+static int mkopci_wait_irq(struct mkopci_device *dev)
+{
+   volatile unsigned long *LPCI_4C =
+   (unsigned long *)(dev->core.lin_base[0] + 0x4C);
+
+   *LPCI_4C |= 0x49;
+
+   if (wait_event_interruptible(dev->wq, *LPCI_4C & 0x24))
+   return -ERESTARTSYS;
+   if (v > 1)
+   pr_info("mkopci%d: irq received\n", dev->core.n_dev);
+   *LPCI_4C &= ~0x40;
+
+   return 0;
+}
+
+static irqreturn_t mkopci_int_handler(int __unused irq, void *dev)
+{
+   struct mkopci_device *mko_dev = (struct mkopci_device *)dev;
+   volatile unsigned long *LPCI_4C =
+   (unsigned long *)((mko_dev->core.lin_base[0] + 0x4C));
+
+   if (*LPCI_4C & 0x24) {
+   *LPCI_4C &= ~0x40;
+   wake_up_interruptible(&mko_dev->wq);
+   } else
+   return IRQ_NONE;
+
+   return IRQ_HANDLED;
+}
+
+static int mkopci_request_irq(struct mkopci_device *dev)
+{
+   int ret = 0;
+
+   if (dev->core.irq_requested) {
+   pr_err("mkopci%d: irq already requested\n", dev->core.n_dev);
+   return -EBUSY;
+   }
+   ret = request_irq(dev->core.irq, &mkopci_int_handler, IRQF_SHARED,
+   dev->core.name, dev);
+   if (ret) {
+   pr_err("mkopci%d: failed to request irq %d\n", dev->core.n_dev,
+   dev->core.irq);
+   return ret;
+   }
+
+   dev->core.irq_requested++;
+
+   pr_info("mkopci%d: irq %d requested (%s)\n", dev->core.n_dev,
+   dev->core.irq, current->comm);
+
+   return ret;
+}
+
+

Re: [PATCH 1/1] Add mkopci driver

[Sergej Bauer]

Ok, I realized uselessness of merging this driver...

And you brought me to a standstill:
> passthrough. In either case, both the ioctl interface and the procfs 
> interface have no future
But what will be after ioctl?


On Saturday 21 March 2015 18:41:42 Arnd Bergmann wrote:
> On Saturday 21 March 2015, Sergej Bauer wrote:
> > Richard, thanks for your review.
> > 
> > But I still have several notes about driver:
> > 
> > > - You add new proc files, which is not really welcomed. Please consider 
> > > sysfs.
> > That will break a bunch of userspace applications, which use proc-files for 
> > several years (as long as from 2006
> > year)
> > 
> 
> > > BTW: Forgot to mention that this sounds like a job for UIO or VFIO.
> > And again, you are right. But, again, there a number of applications wich 
> > use /proc/mkopci/core
> > 
> > But, of course, there may be decided that the kernel main line - this is 
> > not the place for such a driver. :)
> > If the driver is suitable anyway, patch is at the end of this message
> 
> I don't think we should merge the driver with the proposed user interface. 
> You can either
> create a high-level abstraction for MIL-STD-1553, or use UIO or VFIO to 
> provide a trivial
> passthrough. In either case, both the ioctl interface and the procfs 
> interface have no
> future, and existing user space programs need to adapt.
> 
> There is nothing wrong with adding a driver for this hardware, but I'd rather 
> see it done
> properly than having an ad-hoc user space interface that was never reviewed 
> publically
> before it got used by applications.
> 
>   Arnd
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] Add mkopci driver

[Sergej Bauer]

Thanks a lot :)

But the driver is really very insignificant.
I was wondering about sysfs before, but consumers will not be very happy to get 
another revision of userspace tools/library.
So this is their own problems :)

Thanks again and sorry for this uselessness dialogue :)

On Saturday 21 March 2015 20:32:16 Richard Weinberger wrote:
> Am 21.03.2015 um 18:24 schrieb Sergej Bauer:
> > Ok, I realized uselessness of merging this driver...
> 
> Please don't give up that fast!
> See: http://airlied.livejournal.com/#item80112
> 
> > And you brought me to a standstill:
> >> passthrough. In either case, both the ioctl interface and the procfs 
> >> interface have no future
> > But what will be after ioctl?
> 
> We will not drop the ioctl() system call. But having a character device with 
> random
> ioctl()s is not really a good driver design.
> Today we have much more elegant ways to interact between kernel and userspace 
> than ioctl().
> For example sysfs or netlink.
> 
> Thanks,
> //richard
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net-next v2 1/5] lan743x: boost performance on cpu archs w/o dma cache snooping

[Sergej Bauer]

On Thursday, February 11, 2021 7:18:26 PM MSK you wrote:
> From: Sven Van Asbroeck 
> 
> The buffers in the lan743x driver's receive ring are always 9K,
> even when the largest packet that can be received (the mtu) is
> much smaller. This performs particularly badly on cpu archs
> without dma cache snooping (such as ARM): each received packet
> results in a 9K dma_{map|unmap} operation, which is very expensive
> because cpu caches need to be invalidated.
> 
> Careful measurement of the driver rx path on armv7 reveals that
> the cpu spends the majority of its time waiting for cache
> invalidation.
> 
> Optimize by keeping the rx ring buffer size as close as possible
> to the mtu. This limits the amount of cache that requires
> invalidation.
> 
> This optimization would normally force us to re-allocate all
> ring buffers when the mtu is changed - a disruptive event,
> because it can only happen when the network interface is down.
> 
> Remove the need to re-allocate all ring buffers by adding support
> for multi-buffer frames. Now any combination of mtu and ring
> buffer size will work. When the mtu changes from mtu1 to mtu2,
> consumed buffers of size mtu1 are lazily replaced by newly
> allocated buffers of size mtu2.
> 
> These optimizations double the rx performance on armv7.
> Third parties report 3x rx speedup on armv8.
> 
> Tested with iperf3 on a freescale imx6qp + lan7430, both sides
> set to mtu 1500 bytes, measure rx performance:
> 
> Before:
> [ ID] Interval   Transfer Bandwidth   Retr
> [  4]   0.00-20.00  sec   550 MBytes   231 Mbits/sec0
> After:
> [ ID] Interval   Transfer Bandwidth   Retr
> [  4]   0.00-20.00  sec  1.33 GBytes   570 Mbits/sec0
> 
> Signed-off-by: Sven Van Asbroeck 
> ---

( for the reference to current speed, response to v1 of the patch can be found 
at
https://lkml.org/lkml/2021/2/5/472 )

Hi Sven
although whole set of tests might be an overly extensive, but after applying 
patch v2 [1/5]
tests are:
sbauer@metamini ~/devel/kernel-works/net-next.git lan743x_virtual_phy$ ifmtu 
eth7 500
mtu =  500
sbauer@metamini ~/devel/kernel-works/net-next.git lan743x_virtual_phy$ sudo 
test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
...
number of sent packets  = 100
number of received packets  = 747411
number of lost packets = 252589
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 252589
bit error rate = 0.252589
average speed: 408.0757 Mbit/s

...
number of sent packets  = 100
number of received packets  = 738377
number of lost packets = 261623
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 261623
bit error rate = 0.261623
average speed: 413.1470 Mbit/s

...
number of sent packets  = 100
number of received packets  = 738142
number of lost packets = 261858
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 261858
bit error rate = 0.261858
average speed: 413.2262 Mbit/s

...
number of sent packets  = 100
number of received packets  = 708973
number of lost packets = 291027
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 291027
bit error rate = 0.291027
average speed: 430.6224 Mbit/s

...
number of sent packets  = 100
number of received packets  = 725452
number of lost packets = 274548
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 274548
bit error rate = 0.274548
average speed: 420.7341 Mbit/s

sbauer@metamini ~/devel/kernel-works/net-next.git lan743x_virtual_phy$ ifmtu 
eth7 1500
mtu =  1500
sbauer@metamini ~/devel/kernel-works/net-next.git lan743x_virtual_phy$ sudo 
test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
...
number of sent packets  = 100
number of received packets  = 714228
number of lost packets = 285772
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 285772
bit error rate = 0.285772
average speed: 427.1300 Mbit/s

...
number of sent packets  = 100
number of received packets  = 750055
number of lost packets = 249945
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 249945
bit error rate = 0.249945
average speed: 405.0383 Mbit/s

...
number of sent packets  = 100
number of received packets  = 689458
number of lost packets = 310542
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 310542
bit error rate = 0.310542
average speed: 442.5301 Mbit/s

number of sent packets  = 100
number of received packets  = 676830
number of lost packets = 323170
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 323170
bit error rate = 0.32317
average speed: 450.9439 Mbit/s

number of sent packets  = 100
number of received packets  = 701719
number of lost packets = 29828

Re: [PATCH net-next v2 1/5] lan743x: boost performance on cpu archs w/o dma cache snooping

[Sergej Bauer]

On Friday, February 12, 2021 3:27:40 AM MSK you wrote:
> Hi Sergej, thank you for testing this !
Don't mention it, it's just a small assistance
 
> On Thu, Feb 11, 2021 at 7:18 PM Sergej Bauer  wrote:
> > although whole set of tests might be an overly extensive, but after
> > applying patch v2 [1/5]
> > tests are:
> I am unfamiliar with the test_ber tool. Does this patch improve things?
v1 does a great job
number of lost packets decreased by 2.5-3 times
except of this, without the patch I have bit error rate about 0.423531 with 
MTU=1500
and now with this patch BER=0.

resuls of v2 are about the same as results of v1
tomorrow I can test it in more wide range of frame sizes.

tomorrow I can test v2 again, if it needs to be tested again.

Re: [PATCH net-next v1 1/6] lan743x: boost performance on cpu archs w/o dma cache snooping

[Sergej Bauer]

Hi Sven
I can confirm great stability improvement after your patch
"lan743x: boost performance on cpu archs w/o dma cache snooping".
Please note, that test_ber opens raw sockets as
s = socket(AF_PACKET, SOCK_RAW, ETH_P_ALL)
and resulting 'average speed' is a average egress speed.

Test machine is Intel Pentium G4560 3.50GHz
lan743x with rejected virtual phy 'inside'

What I had before:
$ ifmtu eth7 500
$ test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
...
number of sent packets  = 100
number of received packets  = 289017
number of lost packets = 710983
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 710983
bit error rate = 0.710983
average speed: 429.3799 Mbit/s

$ ifmtu eth7 1500
$ sudo test_ber -l eth7 -c 1000 -n 100 -f1500 --no-conf
...
number of sent packets  = 100
number of received packets  = 577194
number of lost packets = 422806
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 422806
bit error rate = 0.422806
average speed: 644.6557 Mbit/s
---

and what I had after your patch:
$ ifmtu eth7 500
$ test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
...
number of sent packets  = 100
number of received packets  = 711329
number of lost packets = 288671
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 288671
bit error rate = 0.288671
average speed: 429.2263 Mbit/s

$ ifmtu eth7 1500
$ test_ber -l eth7 -c 1000 -n 100 -f1500 --no-conf
...
number of sent packets  = 100
number of received packets  = 100
number of lost packets = 0
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 0
bit error rate = 0
average speed: 644.5405 Mbit/s

Re: [PATCH net-next v1 1/6] lan743x: boost performance on cpu archs w/o dma cache snooping

[Sergej Bauer]

sOn Friday, February 5, 2021 7:39:40 PM MSK Sven Van Asbroeck wrote:
> Hi Sergej,
> 
> On Fri, Feb 5, 2021 at 10:09 AM Sergej Bauer  wrote:
> > Tests after applying patches [2/6] and [3/6] are:
> > $ ifmtu eth7 500
> > $ sudo test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
> 
> Thank you! Is there a way for me to run test_ber myself?
> Is this a standard, or a bespoke testing tool?
It's kind of bespoke... A part of framework to assist HW guys in developing
PHY-device. But the project is finished, so I could ask for permission to send
the source to you.

Re: [PATCH net-next v1 1/6] lan743x: boost performance on cpu archs w/o dma cache snooping

[Sergej Bauer]

On Friday, February 5, 2021 5:07:22 PM MSK you wrote:
> Hi Sergej,
> 
> On Fri, Feb 5, 2021 at 7:44 AM Sergej Bauer  wrote:
> > Hi Sven
> > I can confirm great stability improvement after your patch
> > "lan743x: boost performance on cpu archs w/o dma cache snooping".
> > 
> > Test machine is Intel Pentium G4560 3.50GHz
> > lan743x with rejected virtual phy 'inside'
> 
> Interesting, so the speed boost patch seems to improve things even on
> Intel...
> 
> Would you be able to apply and test the multi-buffer patch as well?
> To do that, you can simply apply patches [2/6] and [3/6] on top of
> what you already have.
> 

Hi Sven
Tests after applying patches [2/6] and [3/6] are:
$ ifmtu eth7 500
$ sudo test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
...
number of sent packets  = 100
number of received packets  = 713288
number of lost packets = 286712
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 286712
bit error rate = 0.286712
average speed: 427.8043 Mbit/s

$ ifmtu eth7 1500
$ sudo test_ber -l eth7 -c 1000 -n 100 -f500 --no-conf
...
number of sent packets  = 100
number of received packets  = 707869
number of lost packets = 292131
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 292131
bit error rate = 0.292131
average speed: 431.0163 Mbit/s

$ sudo test_ber -l eth7 -c 1000 -n 100 -f1500 --no-conf
...
number of sent packets  = 100
number of received packets  = 100
number of lost packets = 0
number of out of order packets = 0
number of bit errors   = 0
total errors detected  = 0
bit error rate = 0
average speed: 646.4932 Mbit/s

> Keeping in mind that Bryan has identified an issue with the above
> patch, which will get fixed in v2. So YMMV.
I'll perform tests with v2 as well.

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> > > > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > > > lan743x_adapter *adapter)>
> > > > 
> > > > struct net_device *netdev = adapter->netdev;
> > > > 
> > > > phy_stop(netdev->phydev);
> > > > 
> > > > -   phy_disconnect(netdev->phydev);
> > > > -   netdev->phydev = NULL;
> > > > +   if (phy_is_pseudo_fixed_link(netdev->phydev))
> > > > +   lan743x_virtual_phy_disconnect(netdev->phydev);
> > > > +   else
> > > > +   phy_disconnect(netdev->phydev);
> > > 
> > > phy_disconnect() should work. You might want to call
> 
> There are drivers which call phy_disconnect() on a fixed_link. e.g.
> 
> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx.c#
> L3555.
> 
> It could be your missing call to fixed_phy_unregister() is leaving
> behind bad state.
> 
fixed_phy_unregister() were inside of lan743x_virtual_phy_disconnect()

> > It was to make ethtool show full set of supported speeds and MII only in
> > supported ports (without TP and the no any ports in the bare card).
> 
> But fixed link does not support the full set of speed. It is fixed. It
> supports only one speed it is configured with.  And by setting it
> wrongly, you are going to allow the user to do odd things, like use
> ethtool force the link speed to a speed which is not actually
> supported.
when writing virtual phy i have used "Microchip AN2948 Configuration Registers
of LAN743x" document and the lan743x is designed only for LAN7430 either
LAN7431 as it pointed in the document and in lan743x_pcidev_tbl. which both
support speeds 10/100/1000 Mbps.


-- 
Regards,
Sergej.

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 4:39:47 AM MSK Florian Fainelli wrote:
> On 1/22/2021 5:01 PM, Sergej Bauer wrote:
> > On Saturday, January 23, 2021 3:01:47 AM MSK Florian Fainelli wrote:
> >> On 1/22/2021 3:58 PM, Sergej Bauer wrote:
> >>> On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> >>>>>>> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> >>>>>>> lan743x_adapter *adapter)>
> >>>>>>> 
> >>>>>>>   struct net_device *netdev = adapter->netdev;
> >>>>>>>   
> >>>>>>>   phy_stop(netdev->phydev);
> >>>>>>> 
> >>>>>>> - phy_disconnect(netdev->phydev);
> >>>>>>> - netdev->phydev = NULL;
> >>>>>>> + if (phy_is_pseudo_fixed_link(netdev->phydev))
> >>>>>>> + lan743x_virtual_phy_disconnect(netdev->phydev);
> >>>>>>> + else
> >>>>>>> + phy_disconnect(netdev->phydev);
> >>>>>> 
> >>>>>> phy_disconnect() should work. You might want to call
> >>>> 
> >>>> There are drivers which call phy_disconnect() on a fixed_link. e.g.
> >>>> 
> >>>> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78
> >>>> xx
> >>>> .c# L3555.
> >>>> 
> >>>> 
> >>>> It could be your missing call to fixed_phy_unregister() is leaving
> >>>> behind bad state.
> >>> 
> >>> lan743x_virtual_phy_disconnect removes sysfs-links and calls
> >>> fixed_phy_unregister()
> >>> and the reason was phydev in sysfs.
> >>> 
> >>>>> It was to make ethtool show full set of supported speeds and MII only
> >>>>> in
> >>>>> supported ports (without TP and the no any ports in the bare card).
> >>>> 
> >>>> But fixed link does not support the full set of speed. It is fixed. It
> >>>> supports only one speed it is configured with.
> >>> 
> >>> That's why I "re-implemented the fixed PHY driver" as Florian said.
> >>> The goal of virtual phy was to make an illusion of real device working
> >>> in
> >>> loopback mode. So I could use ethtool and ioctl's to switch speed of
> >>> device.>
> >>> 
> >>>> And by setting it
> >>>> wrongly, you are going to allow the user to do odd things, like use
> >>>> ethtool force the link speed to a speed which is not actually
> >>>> supported.
> >>> 
> >>> I have lan743x only and in loopback mode it allows to use speeds
> >>> 10/100/1000MBps
> >>> in full-duplex mode only. But the highest speed I have achived was
> >>> something near
> >>> 752Mbps...
> >>> And I can switch speed on the fly, without reloading the module.
> >>> 
> >>> May by I should limit the list of acceptable devices?
> >> 
> >> It is not clear what your use case is so maybe start with explaining it
> >> and we can help you define something that may be acceptable for upstream
> >> inclusion.
> > 
> > it migth be helpful for developers work on userspace networking tools with
> > PHY-less lan743x (the interface even could not be brought up)
> > of course, there nothing much to do without TP port but the difference is
> > representative.
> 
> You are still not explaining why fixed PHY is not a suitable emulation
> and what is different, providing the output of ethtool does not tell me
> how you are using it.
> 
> Literally everyone using Ethernet switches or MAC to MAC Ethernet
> connections uses fixed PHY and is reasonably happy with it.

Florian, the key idea is to make virtual phy device which acts as real 802.11
standard  phy.

original fixed_phy and swphy are little bit useless as they do not support
write operation. in contrast of them virtual_phy supports write to all
registers. and can change the speed of interface by means of SIOCSMIIREG ioctl
call either ethtool.
changing of appropriate bits in register 0 will change speed reflecting in 
ethtool
and vise versa.


-- 
Regards,
Sergej

[PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

From: sba...@blackbox.su

v1->v2:
switch to using of fixed_phy as was suggested by Andrew and Florian
also features-related parts are removed

Previous versions can be found at:
v1:
initial version
https://lkml.org/lkml/2020/9/17/1272

Signed-off-by: Sergej Bauer 

diff --git a/MAINTAINERS b/MAINTAINERS
index 650deb973913..86304efd7691 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11699,6 +11699,12 @@ L: net...@vger.kernel.org
 S: Maintained
 F: drivers/net/ethernet/microchip/lan743x_*
 
+MICROCHIP LAN743X VIRTUAL PHY
+M:     Sergej Bauer 
+L: net...@vger.kernel.org
+S: Maintained
+F: drivers/net/ethernet/microchip/lan743x_virtual_phy.*
+
 MICROCHIP LCDFB DRIVER
 M: Nicolas Ferre 
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/net/ethernet/microchip/Kconfig 
b/drivers/net/ethernet/microchip/Kconfig
index d0f6dfe0dcf3..fbc94cf115bd 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -48,6 +48,7 @@ config LAN743X
select PHYLIB
select CRC16
select CRC32
+   select FIXED_PHY
help
  Support for the Microchip LAN743x PCI Express Gigabit Ethernet chip
 
diff --git a/drivers/net/ethernet/microchip/Makefile 
b/drivers/net/ethernet/microchip/Makefile
index da603540ca57..dc3a2d66c286 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_ENC28J60) += enc28j60.o
 obj-$(CONFIG_ENCX24J600) += encx24j600.o encx24j600-regmap.o
 obj-$(CONFIG_LAN743X) += lan743x.o
 
-lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
+lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o 
lan743x_virtual_phy.o
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c 
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index c5de8f46cdd3..22636366d0db 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -816,6 +816,17 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
 }
 #endif /* CONFIG_PM */
 
+int lan743x_set_link_ksettings(struct net_device *netdev,
+  const struct ethtool_link_ksettings *cmd)
+{
+   if (!netdev->phydev)
+   return -ENETDOWN;
+
+   return phy_is_pseudo_fixed_link(netdev->phydev) ?
+   lan743x_set_virtual_link_ksettings(netdev, cmd)
+   : phy_ethtool_set_link_ksettings(netdev, cmd);
+}
+
 const struct ethtool_ops lan743x_ethtool_ops = {
.get_drvinfo = lan743x_ethtool_get_drvinfo,
.get_msglevel = lan743x_ethtool_get_msglevel,
@@ -839,7 +850,7 @@ const struct ethtool_ops lan743x_ethtool_ops = {
.get_eee = lan743x_ethtool_get_eee,
.set_eee = lan743x_ethtool_set_eee,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
-   .set_link_ksettings = phy_ethtool_set_link_ksettings,
+   .set_link_ksettings = lan743x_set_link_ksettings,
 #ifdef CONFIG_PM
.get_wol = lan743x_ethtool_get_wol,
.set_wol = lan743x_ethtool_set_wol,
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.h 
b/drivers/net/ethernet/microchip/lan743x_ethtool.h
index d0d11a777a58..7287474d4a5c 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.h
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.h
@@ -8,4 +8,7 @@
 
 extern const struct ethtool_ops lan743x_ethtool_ops;
 
+int lan743x_set_virtual_link_ksettings(struct net_device *netdev,
+  const struct ethtool_link_ksettings 
*cmd);
+
 #endif /* _LAN743X_ETHTOOL_H */
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
b/drivers/net/ethernet/microchip/lan743x_main.c
index 3804310c853a..d8c00fa298d0 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -17,6 +17,11 @@
 #include 
 #include "lan743x_main.h"
 #include "lan743x_ethtool.h"
+#include "lan743x_virtual_phy.h"
+
+static char *mii_regs[32];
+static int mii_regs_count;
+module_param_array(mii_regs, charp, &mii_regs_count, 0644);
 
 static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
 {
@@ -821,7 +826,7 @@ static int lan743x_mac_init(struct lan743x_adapter *adapter)
return 0;
 }
 
-static int lan743x_mac_open(struct lan743x_adapter *adapter)
+int lan743x_mac_open(struct lan743x_adapter *adapter)
 {
u32 temp;
 
@@ -832,7 +837,7 @@ static int lan743x_mac_open(struct lan743x_adapter *adapter)
return 0;
 }
 
-static void lan743x_mac_close(struct lan743x_adapter *adapter)
+void lan743x_mac_close(struct lan743x_adapter *adapter)
 {
u32 temp;
 
@@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct lan743x_adapter 
*adapter)
struct net_device *netdev = adapter->netdev;
 
phy_stop(netdev->phydev);
-   phy_disconnect(netdev->phydev);
-   netdev-

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 12:52:48 AM MSK Florian Fainelli wrote:
> On 1/22/2021 1:42 PM, Sergej Bauer wrote:
> > From: sba...@blackbox.su
> > 
> > v1->v2:
> > switch to using of fixed_phy as was suggested by Andrew and Florian
> > also features-related parts are removed
> > 
> > Previous versions can be found at:
> > v1:
> > initial version
> > 
> > https://lkml.org/lkml/2020/9/17/1272
> > 
> > Signed-off-by: Sergej Bauer 
> 
> You are not explaining why you need this and why you are second guessing
> the fixed PHY MII emulation that already exists. You really need to do a
> better job at describing your changes and why the emulation offered by
> swphy.c is not enough for your use case.

ok, I'll try to accomplish it with swphy.

-- 
Regards,
   Sergej.

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 1:08:03 AM MSK Andrew Lunn wrote:
> On Sat, Jan 23, 2021 at 12:42:41AM +0300, Sergej Bauer wrote:
> > From: sba...@blackbox.su
> > 
> > v1->v2:
> > switch to using of fixed_phy as was suggested by Andrew and Florian
> > also features-related parts are removed
> 
> This is not using fixed_phy, at least not in the normal way.
> 
> Take a look at bgmac_phy_connect_direct() for example. Call
> fixed_phy_register(), and then phy_connect_direct() with the
> phydev. End of story. Done.
> 
> > +int lan743x_set_link_ksettings(struct net_device *netdev,
> > +  const struct ethtool_link_ksettings *cmd)
> > +{
> > +   if (!netdev->phydev)
> > +   return -ENETDOWN;
> > +
> > +   return phy_is_pseudo_fixed_link(netdev->phydev) ?
> > +   lan743x_set_virtual_link_ksettings(netdev, cmd)
> > +   : phy_ethtool_set_link_ksettings(netdev, cmd);
> > +}
> 
> There should not be any need to do something different. The whole
> point of fixed_phy is it looks like a PHY. So calling
> phy_ethtool_set_link_ksettings() should work, nothing special needed.
> 
> > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > lan743x_adapter *adapter)> 
> > struct net_device *netdev = adapter->netdev;
> > 
> > phy_stop(netdev->phydev);
> > 
> > -   phy_disconnect(netdev->phydev);
> > -   netdev->phydev = NULL;
> > +   if (phy_is_pseudo_fixed_link(netdev->phydev))
> > +   lan743x_virtual_phy_disconnect(netdev->phydev);
> > +   else
> > +   phy_disconnect(netdev->phydev);
> 
> phy_disconnect() should work. You might want to call
Andrew, this is why I was playing with my own _connect/_disconnect 
realizations
It should work but it don't.
modprobe lan743x
rmmod lan743x
modprobe lan743x
leads to
"net eth7: could not add device link to fixed-0:06 err -17"
in dmesg (it does not removes corresponding phydev file in /sysfs)
moreover phy_disconnect leads to kernel panic
[82363.976726] BUG: kernel NULL pointer dereference, address: 03c4
[82363.977402] #PF: supervisor read access in kernel mode
[82363.978077] #PF: error_code(0x) - not-present page
[82363.978588] PGD 0 P4D 0 
[82363.978588] Oops:  [#1] SMP NOPTI
[82363.978588] CPU: 3 PID: 2634 Comm: ifconfig Tainted: G   O  
5.11.0-rc4net-next-virtual_phy+ #3
[82363.978588] Hardware name: Gigabyte Technology Co., Ltd. H110-D3/H110-D3-
CF, BIOS F24 04/11/2018
[82363.978588] RIP: 0010:lan743x_phy_close.isra.26+0x28/0x40 [lan743x]
[82363.978588] Code: c3 90 0f 1f 44 00 00 53 48 89 fb 48 8b bf 28 08 00 00 e8 
ab 5e 86 ff 48 8b bb 28 08 00 00 e8 4f 92 86 ff 48 8b bb 28 08 00 00  87 c4 
03 00 00 04 75 02 5b c3 5b e9 f7 35 ea ff 0f 1f 80 00 00
[82363.982448] RSP: 0018:a528c04c7c38 EFLAGS: 00010246
[82363.982448] RAX: 000f RBX: 991a47e38000 RCX: 0027
[82363.982448] RDX:  RSI: 991c76d98b30 RDI: 
[82363.982448] RBP: 0001 R08:  R09: c000efff
[82363.982448] R10: 0001 R11: a528c04c7a48 R12: 991a47e388c0
[82363.986855] R13: 991a47e390b8 R14: 991a47e39050 R15: 991a47e388c0
[82363.986855] FS:  7f7c3ebd6540() GS:991c76d8() knlGS:

[82363.986855] CS:  0010 DS:  ES:  CR0: 80050033
[82363.986855] CR2: 03c4 CR3: 1b2ac005 CR4: 
003706e0
[82363.986855] Call Trace:
[82363.986855]  lan743x_netdev_close+0x223/0x250 [lan743x]
...

> fixed_phy_unregister() afterwards, so you do not leak memory.
> 
> > +   if (phy_is_pseudo_fixed_link(phydev)) {
> > +   ret = phy_connect_direct(netdev, phydev,
> > +
> > lan743x_virtual_phy_status_change,
> > +PHY_INTERFACE_MODE_MII);
> > +   } else {
> > +   ret = phy_connect_direct(netdev, phydev,
> > +lan743x_phy_link_status_change,
> 
> There should not be any need for a special link change
> callback. lan743x_phy_link_status_change() should work fine, the MAC
> should have no idea it is using a fixed_phy.
> 
> > +PHY_INTERFACE_MODE_GMII);
> > +   }
> > +
> > 
> > if (ret)
> > 
> > goto return_error;
> > 
> > }
> > 
> > @@ -1031,6 +1049,15 @@ static int lan743x_phy_open(struct lan743x_adapter
> > *adapter)> 
> > /* MAC doesn't sup

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> > > > @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> > > > lan743x_adapter *adapter)>
> > > > 
> > > > struct net_device *netdev = adapter->netdev;
> > > > 
> > > > phy_stop(netdev->phydev);
> > > > 
> > > > -   phy_disconnect(netdev->phydev);
> > > > -   netdev->phydev = NULL;
> > > > +   if (phy_is_pseudo_fixed_link(netdev->phydev))
> > > > +   lan743x_virtual_phy_disconnect(netdev->phydev);
> > > > +   else
> > > > +   phy_disconnect(netdev->phydev);
> > > 
> > > phy_disconnect() should work. You might want to call
> 
> There are drivers which call phy_disconnect() on a fixed_link. e.g.
> 
> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx.c#
> L3555.
> 

> It could be your missing call to fixed_phy_unregister() is leaving
> behind bad state.
>
lan743x_virtual_phy_disconnect removes sysfs-links and calls 
fixed_phy_unregister()
and the reason was phydev in sysfs.

> > It was to make ethtool show full set of supported speeds and MII only in
> > supported ports (without TP and the no any ports in the bare card).
> 
> But fixed link does not support the full set of speed. It is fixed. It
> supports only one speed it is configured with.
That's why I "re-implemented the fixed PHY driver" as Florian said.
The goal of virtual phy was to make an illusion of real device working in
loopback mode. So I could use ethtool and ioctl's to switch speed of device.

> And by setting it
> wrongly, you are going to allow the user to do odd things, like use
> ethtool force the link speed to a speed which is not actually
> supported.
I have lan743x only and in loopback mode it allows to use speeds 
10/100/1000MBps
in full-duplex mode only. But the highest speed I have achived was something 
near
752Mbps...
And I can switch speed on the fly, without reloading the module.

May by I should limit the list of acceptable devices?

> 
> Andrew

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 3:01:47 AM MSK Florian Fainelli wrote:
> On 1/22/2021 3:58 PM, Sergej Bauer wrote:
> > On Saturday, January 23, 2021 2:23:25 AM MSK Andrew Lunn wrote:
> >>>>> @@ -1000,8 +1005,10 @@ static void lan743x_phy_close(struct
> >>>>> lan743x_adapter *adapter)>
> >>>>> 
> >>>>> struct net_device *netdev = adapter->netdev;
> >>>>> 
> >>>>> phy_stop(netdev->phydev);
> >>>>> 
> >>>>> -   phy_disconnect(netdev->phydev);
> >>>>> -   netdev->phydev = NULL;
> >>>>> +   if (phy_is_pseudo_fixed_link(netdev->phydev))
> >>>>> +   lan743x_virtual_phy_disconnect(netdev->phydev);
> >>>>> +   else
> >>>>> +   phy_disconnect(netdev->phydev);
> >>>> 
> >>>> phy_disconnect() should work. You might want to call
> >> 
> >> There are drivers which call phy_disconnect() on a fixed_link. e.g.
> >> 
> >> https://elixir.bootlin.com/linux/v5.11-rc4/source/drivers/net/usb/lan78xx
> >> .c# L3555.
> >> 
> >> 
> >> It could be your missing call to fixed_phy_unregister() is leaving
> >> behind bad state.
> > 
> > lan743x_virtual_phy_disconnect removes sysfs-links and calls
> > fixed_phy_unregister()
> > and the reason was phydev in sysfs.
> > 
> >>> It was to make ethtool show full set of supported speeds and MII only in
> >>> supported ports (without TP and the no any ports in the bare card).
> >> 
> >> But fixed link does not support the full set of speed. It is fixed. It
> >> supports only one speed it is configured with.
> > 
> > That's why I "re-implemented the fixed PHY driver" as Florian said.
> > The goal of virtual phy was to make an illusion of real device working in
> > loopback mode. So I could use ethtool and ioctl's to switch speed of
> > device.> 
> >> And by setting it
> >> wrongly, you are going to allow the user to do odd things, like use
> >> ethtool force the link speed to a speed which is not actually
> >> supported.
> > 
> > I have lan743x only and in loopback mode it allows to use speeds
> > 10/100/1000MBps
> > in full-duplex mode only. But the highest speed I have achived was
> > something near
> > 752Mbps...
> > And I can switch speed on the fly, without reloading the module.
> > 
> > May by I should limit the list of acceptable devices?
> 
> It is not clear what your use case is so maybe start with explaining it
> and we can help you define something that may be acceptable for upstream
> inclusion.
it migth be helpful for developers work on userspace networking tools with
PHY-less lan743x (the interface even could not be brought up)
of course, there nothing much to do without TP port but the difference is
representative.

sbauer@metamini ~$ sudo ethtool eth7
Settings for eth7:
Cannot get device settings: No such device
Supports Wake-on: pumbag
Wake-on: d
Current message level: 0x0137 (311)
   drv probe link ifdown ifup tx_queued
Link detected: no
sbauer@metamini ~$ sudo ifup eth7
sbauer@metamini ~$ sudo ethtool eth7
Settings for eth7:
Supported ports: [ MII ]
Supported link modes:   10baseT/Full 
100baseT/Full 
1000baseT/Full 
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes:  10baseT/Full 
100baseT/Full 
1000baseT/Full 
Advertised pause frame use: Symmetric Receive-only
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Supports Wake-on: pumbag
Wake-on: d
Current message level: 0x0137 (311)
   drv probe link ifdown ifup tx_queued
Link detected: yes
sbauer@metamini ~$ sudo mii-tool -vv eth7
Using SIOCGMIIPHY=0x8947
eth7: negotiated 1000baseT-FD, link ok
  registers for MII PHY 0: 
5140 512d 7431 0011 4140 4140 000d 
 0200 7800     2000
       
       
  product info: vendor 1d:0c:40, model 1 rev 1
  basic mode:   loopback, autonegotiation enabled
  basic status: autonegotiation complete, link ok
  capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
  advertising:  1000baseT-FD 100baseTx-FD 10baseT-FD
  link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD

   Regards,
   Sergej.

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

resending answer to all:
On Saturday, January 23, 2021 4:32:38 AM MSK Andrew Lunn wrote:
> > it migth be helpful for developers work on userspace networking tools with
> > PHY-less lan743x
> 
> (the interface even could not be brought up)
> 
> > of course, there nothing much to do without TP port but the difference is
> > representative.
> > 
> > sbauer@metamini ~$ sudo ethtool eth7
> > Settings for eth7:
> > Cannot get device settings: No such device
> > 
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x0137 (311)
> > 
> >drv probe link ifdown ifup tx_queued
> > 
> > Link detected: no
> > 
> > sbauer@metamini ~$ sudo ifup eth7
> > sbauer@metamini ~$ sudo ethtool eth7
> > 
> > Settings for eth7:
> > Supported ports: [ MII ]
> > Supported link modes:   10baseT/Full
> > 
> > 100baseT/Full
> > 1000baseT/Full
> > 
> > Supported pause frame use: Symmetric Receive-only
> > Supports auto-negotiation: Yes
> > Supported FEC modes: Not reported
> > Advertised link modes:  10baseT/Full
> > 
> > 100baseT/Full
> > 1000baseT/Full
> > 
> > Advertised pause frame use: Symmetric Receive-only
> > Advertised auto-negotiation: Yes
> > Advertised FEC modes: Not reported
> > Speed: 1000Mb/s
> > Duplex: Full
> > Port: MII
> > PHYAD: 0
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x0137 (311)
> > 
> >drv probe link ifdown ifup tx_queued
> > 
> > Link detected: yes
> > 
> > sbauer@metamini ~$ sudo mii-tool -vv eth7
> > Using SIOCGMIIPHY=0x8947
> > eth7: negotiated 1000baseT-FD, link ok
> > 
> >   registers for MII PHY 0:
> > 5140 512d 7431 0011 4140 4140 000d 
> >  0200 7800     2000
> >        
> >        
> >   
> >   product info: vendor 1d:0c:40, model 1 rev 1
> >   basic mode:   loopback, autonegotiation enabled
> >   basic status: autonegotiation complete, link ok
> >   capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
> >   advertising:  1000baseT-FD 100baseTx-FD 10baseT-FD
> >   link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD
> 
> You have not shown anything i cannot do with the ethernet interfaces i
> have in my laptop. And since ethtool is pretty standardized, what
> lan743x offers should be pretty much the same as any 1G Ethernet MAC
> using most 1G PHYs.
> 
>   Andrew

Andrew, I can reproduce segfault with following changes:
sbauer@metamini ~/devel/kernel-works/net-next.git master$ git diff .
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/
ethernet/microchip/lan743x_main.c
index 3804310c853a..6e2961f47211 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1001,6 +1001,8 @@ static void lan743x_phy_close(struct lan743x_adapter 
*adapter)
 
phy_stop(netdev->phydev);
phy_disconnect(netdev->phydev);
+   if (phy_is_pseudo_fixed_link(netdev->phydev))
+   fixed_phy_unregister(netdev->phydev);
netdev->phydev = NULL;
 }
 
@@ -1018,9 +1020,21 @@ static int lan743x_phy_open(struct lan743x_adapter 
*adapter)
if (!phydev) {
/* try internal phy */
phydev = phy_find_first(adapter->mdiobus);
-   if (!phydev)
-   goto return_error;
-
+   if (!phydev) {
+   struct fixed_phy_status status = {
+   .link = 1,
+   .speed = SPEED_1000,
+   .duplex = DUPLEX_FULL,
+   .asym_pause = 1,
+   };
+
+   phydev = fixed_phy_register(PHY_POLL, &status, NULL);
+   if (!phydev || IS_ERR(phydev)) {
+   netif_err(adapter, probe, netdev,
+ "Failed to register fixed PHY 
device\n");
+   goto return_error;
+   }
+   }
ret = phy_connect_direct(netdev, phydev,
 lan743x_phy_link_status_change,
 PHY_INTERFACE_MODE_GMII);

sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo modprobe 
lan743x
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo ifup eth7
sbauer@metamini ~/devel/kernel-works/net-next.git master$ ethtool eth7
Settings for eth7:
Supported ports: [ TP MII ]
 

Re: [PATCH v2] lan743x: add virtual PHY for PHY-less devices

[Sergej Bauer]

On Saturday, January 23, 2021 4:32:38 AM MSK Andrew Lunn wrote:
> > it migth be helpful for developers work on userspace networking tools with
> > PHY-less lan743x
> 
> (the interface even could not be brought up)
> 
> > of course, there nothing much to do without TP port but the difference is
> > representative.
> > 
> > sbauer@metamini ~$ sudo ethtool eth7
> > Settings for eth7:
> > Cannot get device settings: No such device
> > 
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x0137 (311)
> > 
> >drv probe link ifdown ifup tx_queued
> > 
> > Link detected: no
> > 
> > sbauer@metamini ~$ sudo ifup eth7
> > sbauer@metamini ~$ sudo ethtool eth7
> > 
> > Settings for eth7:
> > Supported ports: [ MII ]
> > Supported link modes:   10baseT/Full
> > 
> > 100baseT/Full
> > 1000baseT/Full
> > 
> > Supported pause frame use: Symmetric Receive-only
> > Supports auto-negotiation: Yes
> > Supported FEC modes: Not reported
> > Advertised link modes:  10baseT/Full
> > 
> > 100baseT/Full
> > 1000baseT/Full
> > 
> > Advertised pause frame use: Symmetric Receive-only
> > Advertised auto-negotiation: Yes
> > Advertised FEC modes: Not reported
> > Speed: 1000Mb/s
> > Duplex: Full
> > Port: MII
> > PHYAD: 0
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: pumbag
> > Wake-on: d
> > Current message level: 0x0137 (311)
> > 
> >drv probe link ifdown ifup tx_queued
> > 
> > Link detected: yes
> > 
> > sbauer@metamini ~$ sudo mii-tool -vv eth7
> > Using SIOCGMIIPHY=0x8947
> > eth7: negotiated 1000baseT-FD, link ok
> > 
> >   registers for MII PHY 0:
> > 5140 512d 7431 0011 4140 4140 000d 
> >  0200 7800     2000
> >        
> >        
> >   
> >   product info: vendor 1d:0c:40, model 1 rev 1
> >   basic mode:   loopback, autonegotiation enabled
> >   basic status: autonegotiation complete, link ok
> >   capabilities: 1000baseT-FD 100baseTx-FD 10baseT-FD
> >   advertising:  1000baseT-FD 100baseTx-FD 10baseT-FD
> >   link partner: 1000baseT-FD 100baseTx-FD 10baseT-FD
> 
> You have not shown anything i cannot do with the ethernet interfaces i
> have in my laptop. And since ethtool is pretty standardized, what
> lan743x offers should be pretty much the same as any 1G Ethernet MAC
> using most 1G PHYs.
> 
>   Andrew

Andrew, for this moment with lan743x we can get only this:
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo ethtool eth7
Settings for eth7:
Cannot get device settings: No such device
Supports Wake-on: pumbag
Wake-on: d
Current message level: 0x0137 (311)
   drv probe link ifdown ifup tx_queued
Link detected: no
sbauer@metamini ~/devel/kernel-works/net-next.git master$ sudo mii-tool -vv 
eth7
Using SIOCGMIIPHY=0x8947
SIOCGMIIPHY on 'eth7' failed: Invalid argument


-- 
Regards,
Sergej.

[PATCH v4] lan743x: fix for potential NULL pointer dereference with bare card

[Sergej Bauer]

This is the 4th revision of the patch fix for potential null pointer dereference
with lan743x card.

The simpliest way to reproduce: boot with bare lan743x and issue "ethtool ethN"
command where ethN is the interface with lan743x card. Example:

$ sudo ethtool eth7
dmesg:
[  103.510336] BUG: kernel NULL pointer dereference, address: 0340
...
[  103.510836] RIP: 0010:phy_ethtool_get_wol+0x5/0x30 [libphy]
...
[  103.511629] Call Trace:
[  103.511666]  lan743x_ethtool_get_wol+0x21/0x40 [lan743x]
[  103.511724]  dev_ethtool+0x1507/0x29d0
[  103.511769]  ? avc_has_extended_perms+0x17f/0x440
[  103.511820]  ? tomoyo_init_request_info+0x84/0x90
[  103.511870]  ? tomoyo_path_number_perm+0x68/0x1e0
[  103.511919]  ? tty_insert_flip_string_fixed_flag+0x82/0xe0
[  103.511973]  ? inet_ioctl+0x187/0x1d0
[  103.512016]  dev_ioctl+0xb5/0x560
[  103.512055]  sock_do_ioctl+0xa0/0x140
[  103.512098]  sock_ioctl+0x2cb/0x3c0
[  103.512139]  __x64_sys_ioctl+0x84/0xc0
[  103.512183]  do_syscall_64+0x33/0x80
[  103.512224]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  103.512274] RIP: 0033:0x7f54a9cba427
...

Previous versions can be found at:
v1:
initial version
https://lkml.org/lkml/2020/10/28/921

v2:
do not return from lan743x_ethtool_set_wol if netdev->phydev == NULL, just skip
the call of phy_ethtool_set_wol() instead.
https://lkml.org/lkml/2020/10/31/380

v3:
in function lan743x_ethtool_set_wol:
use ternary operator instead of if-else sentence (review by Markus Elfring)
return -ENETDOWN instead of -EIO (review by Andrew Lunn)

v4:
Sven Van Asbruck noticed that the patch was being applied cleanly to the 5.9
branch, so the tag “Fixes” was added as Jakub suggested.

Signed-off-by: Sergej Bauer 
Reviewed-by: Andrew Lunn 
Reviewed-by: Jakub Kicinski 
Fixes: 4d94282afd95 ("lan743x: Add power management support")
---
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c 
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index dcde496da7fb..c5de8f46cdd3 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -780,7 +780,9 @@ static void lan743x_ethtool_get_wol(struct net_device 
*netdev,
 
wol->supported = 0;
wol->wolopts = 0;
-   phy_ethtool_get_wol(netdev->phydev, wol);
+
+   if (netdev->phydev)
+   phy_ethtool_get_wol(netdev->phydev, wol);
 
wol->supported |= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
@@ -809,9 +811,8 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
 
device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
 
-   phy_ethtool_set_wol(netdev->phydev, wol);
-
-   return 0;
+   return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
+   : -ENETDOWN;
 }
 #endif /* CONFIG_PM */
 

Re: [PATCH v4] lan743x: fix for potential NULL pointer dereference with bare card

[Sergej Bauer]

On Wednesday, December 16, 2020 4:12:42 AM MSK Jakub Kicinski wrote:
> On Tue, 15 Dec 2020 23:39:14 + patchwork-bot+netdev...@kernel.org
> 
> wrote:
> > Hello:
> > 
> > This patch was applied to bpf/bpf.git (refs/heads/master):
> > 
> > On Tue, 15 Dec 2020 19:12:45 +0300 you wrote:
> > > This is the 4th revision of the patch fix for potential null pointer
> > > dereference with lan743x card.
> > > 
> > > The simpliest way to reproduce: boot with bare lan743x and issue
> > > "ethtool ethN" command where ethN is the interface with lan743x card.
> > > Example:
> > > 
> > > $ sudo ethtool eth7
> > > dmesg:
...
> > > [...]
> > 
> > Here is the summary with links:
> >   - [v4] lan743x: fix for potential NULL pointer dereference with bare
> >   card
> >   
> > https://git.kernel.org/bpf/bpf/c/e9e13b6adc33
> > 
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> Heh the bot got confused, I think.
> 
> What I meant when I said "let's wait for the merge window" was that
> the patch will not hit upstream until the merge window. It's now in
> Linus's tree. I'll make a submission of stable patches to Greg at the
> end of the week and I'll include this patch.
> 
> Thanks!

I think, firstly the bot was confused by me :-\
I should have asked you what exactly did you mean with "let's wait for the 
merge window"...
That's completely my fault, sorry for that.

Regards,
Sergej.

Re: [PATCH v3] lan743x: fix for potential NULL pointer dereference with bare card

[Sergej Bauer]

Hi Sven.

> Hi Jakub, Sergej,
> 
> On Tue, Nov 3, 2020 at 8:41 PM Jakub Kicinski  wrote:
> > On Mon,  2 Nov 2020 01:35:55 +0300 Sergej Bauer wrote:
> > > This is the 3rd revision of the patch fix for potential null pointer
> > > dereference with lan743x card.
> > 
> > Applied, thanks!
> 
> I noticed that this went into net-next. Is there a chance that it could also
> be added to net? It's a real issue on 5.9, and the patch applies cleanly
> there.

It's my mistake - I missed pointing of stable branch.
Should I resend the patch (with tag Cc:sta...@vger.kernel.org)?

Regards,
Sergej.

[PATCH] fix for potential NULL pointer dereference with bare lan743x

[Sergej Bauer]

  This is just a minor fix which prevents a kernel NULL pointer
dereference when using phy-less lan743x.

Signed-off-by: Sergej Bauer 
---
 drivers/net/ethernet/microchip/lan743x_ethtool.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c 
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index dcde496da7fb..354d72d550f2 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -780,7 +780,9 @@ static void lan743x_ethtool_get_wol(struct net_device 
*netdev,
 
wol->supported = 0;
wol->wolopts = 0;
-   phy_ethtool_get_wol(netdev->phydev, wol);
+
+   if (netdev->phydev)
+   phy_ethtool_get_wol(netdev->phydev, wol);
 
wol->supported |= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
@@ -793,6 +795,9 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
 {
struct lan743x_adapter *adapter = netdev_priv(netdev);
 
+   if (!netdev->phydev)
+   return -EIO;
+
adapter->wolopts = 0;
if (wol->wolopts & WAKE_UCAST)
adapter->wolopts |= WAKE_UCAST;
-- 
2.20.1

[PATCH v2] fix for potential NULL pointer dereference with bare lan743x

[Sergej Bauer]

Signed-off-by: Sergej Bauer 
---
 drivers/net/ethernet/microchip/lan743x_ethtool.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c 
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index dcde496da7fb..ad38fc9e1468 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -780,7 +780,9 @@ static void lan743x_ethtool_get_wol(struct net_device 
*netdev,
 
wol->supported = 0;
wol->wolopts = 0;
-   phy_ethtool_get_wol(netdev->phydev, wol);
+
+   if (netdev->phydev)
+   phy_ethtool_get_wol(netdev->phydev, wol);
 
wol->supported |= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
@@ -792,6 +794,7 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
   struct ethtool_wolinfo *wol)
 {
struct lan743x_adapter *adapter = netdev_priv(netdev);
+   int ret;
 
adapter->wolopts = 0;
if (wol->wolopts & WAKE_UCAST)
@@ -809,9 +812,12 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
 
device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
 
-   phy_ethtool_set_wol(netdev->phydev, wol);
+   if (netdev->phydev)
+   ret = phy_ethtool_set_wol(netdev->phydev, wol);
+   else
+   ret = -EIO;
 
-   return 0;
+   return ret;
 }
 #endif /* CONFIG_PM */
 
-- 
2.20.1

Re: [PATCH v2] lan743x: Fix for potential null pointer dereference

[Sergej Bauer]

> > Signed-off-by: Sergej Bauer 
> 
> * I miss a change description here.
The reason for the fix is when the device is down netdev->phydev will be NULL 
and there is no checking for this situation. So 'ethtool ethN' leads to kernel 
panic.

$ sudo ethtool eth7

[  103.510336] BUG: kernel NULL pointer dereference, address: 0340
[  103.510454] #PF: supervisor read access in kernel mode
[  103.510530] #PF: error_code(0x) - not-present page
[  103.510600] PGD 0 P4D 0 
[  103.510635] Oops:  [#1] SMP PTI
[  103.510675] CPU: 1 PID: 7182 Comm: ethtool Not tainted 5.9.0upstream+ #5
[  103.510737] Hardware name: Gigabyte Technology Co., Ltd. H110-D3/H110-D3-
CF, BIOS F24 04/11/2018
[  103.510836] RIP: 0010:phy_ethtool_get_wol+0x5/0x30 [libphy]
[  103.510892] Code: 00 48 85 c0 74 11 48 8b 80 40 01 00 00 48 85 c0 74 05 e9 
8e 7a 6f dd b8 a1 ff ff ff c3 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 <48> 8b 87 
40 03 00 00 48 85 c0 74 11 48 8b 80 48 01 00 00 48 85 c0
[  103.511054] RSP: 0018:b6cd85123cf0 EFLAGS: 00010286
[  103.511106] RAX: c03f0d00 RBX: b6cd85123d90 RCX: 9e6fdd20
[  103.511171] RDX: 0001 RSI: b6cd85123d90 RDI: 
[  103.511237] RBP: 946f811b4000 R08: 1000 R09: 
[  103.511302] R10:  R11: 0089 R12: 
7ffde92be040
[  103.511367] R13: 0005 R14: 946f811b4000 R15: 
[  103.511434] FS:  7f54a9bc7740() GS:9470b6c8() knlGS:

[  103.511508] CS:  0010 DS:  ES:  CR0: 80050033
[  103.511564] CR2: 0340 CR3: 00011d366001 CR4: 
003706e0
[  103.511629] Call Trace:
[  103.511666]  lan743x_ethtool_get_wol+0x21/0x40 [lan743x]
[  103.511724]  dev_ethtool+0x1507/0x29d0
[  103.511769]  ? avc_has_extended_perms+0x17f/0x440
[  103.511820]  ? tomoyo_init_request_info+0x84/0x90
[  103.511870]  ? tomoyo_path_number_perm+0x68/0x1e0
[  103.511919]  ? tty_insert_flip_string_fixed_flag+0x82/0xe0
[  103.511973]  ? inet_ioctl+0x187/0x1d0
[  103.512016]  dev_ioctl+0xb5/0x560
[  103.512055]  sock_do_ioctl+0xa0/0x140
[  103.512098]  sock_ioctl+0x2cb/0x3c0
[  103.512139]  __x64_sys_ioctl+0x84/0xc0
[  103.512183]  do_syscall_64+0x33/0x80
[  103.512224]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  103.512274] RIP: 0033:0x7f54a9cba427
[  103.512313] Code: 00 00 90 48 8b 05 69 aa 0c 00 64 c7 00 26 00 00 00 48 c7 
c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 
f0 
ff ff 73 01 c3 48 8b 0d 39 aa 0c 00 f7 d8 64 89 01 48
...
---
So changes - is just to check a pointer for NULL;

> * Should a prefix be specified in the patch subject?
> 
as far as I understand subject should be "[PATCH v2] lan743x: fix for potential 
NULL pointer dereference with bare lan743x"?

ok, I've got it.

> 
> …
> 
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> 
> …
> 
> > @@ -809,9 +812,12 @@ static int lan743x_ethtool_set_wol(struct net_device
> > *netdev,> 
> > device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
> > 
> > -   phy_ethtool_set_wol(netdev->phydev, wol);
> > +   if (netdev->phydev)
> > +   ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > +   else
> > +   ret = -EIO;
> > 
> > -   return 0;
> > +   return ret;
> > 
> >  }
> >  #endif /* CONFIG_PM */
> 
> How do you think about to use the following code variant?
> 
> + return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol) : -EIO;
> 
It will be quite shorter, thanks.

> Regards,
> Markus

Regards.
Sergej.

Re: [PATCH v2] lan743x: Fix for potential null pointer dereference

[Sergej Bauer]

On Sunday, November 1, 2020 11:38:20 PM MSK Andrew Lunn wrote:
> On Sun, Nov 01, 2020 at 10:54:38PM +0300, Sergej Bauer wrote:
> > > > Signed-off-by: Sergej Bauer 
> > > 
> > > * I miss a change description here.
> > 
> > The reason for the fix is when the device is down netdev->phydev will be
> > NULL and there is no checking for this situation. So 'ethtool ethN' leads
> > to kernel panic.
> > 
> > > > @@ -809,9 +812,12 @@ static int lan743x_ethtool_set_wol(struct
> > > > net_device
> > > > *netdev,>
> > > > 
> > > > device_set_wakeup_enable(&adapter->pdev->dev, 
> > > > (bool)wol->wolopts);
> > > > 
> > > > -   phy_ethtool_set_wol(netdev->phydev, wol);
> > > > +   if (netdev->phydev)
> > > > +   ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > > > +   else
> > > > +   ret = -EIO;
> 
> -ENETDOWN would be better, it gives a hit that WoL can be configured
> when the interface is configured up.
> 
>  Andrew

Ok, thank you, Andrew! I was doubted in the correctness of returning -EIO.

Regards,
Sergej.

[PATCH v3] lan743x: fix for potential NULL pointer dereference with bare card

[Sergej Bauer]

This is the 3rd revision of the patch fix for potential null pointer dereference
with lan743x card.

The simpliest way to reproduce: boot with bare lan743x and issue "ethtool ethN"
commant where ethN is the interface with lan743x card. Example:

$ sudo ethtool eth7
dmesg:
[  103.510336] BUG: kernel NULL pointer dereference, address: 0340
...
[  103.510836] RIP: 0010:phy_ethtool_get_wol+0x5/0x30 [libphy]
...
[  103.511629] Call Trace:
[  103.511666]  lan743x_ethtool_get_wol+0x21/0x40 [lan743x]
[  103.511724]  dev_ethtool+0x1507/0x29d0
[  103.511769]  ? avc_has_extended_perms+0x17f/0x440
[  103.511820]  ? tomoyo_init_request_info+0x84/0x90
[  103.511870]  ? tomoyo_path_number_perm+0x68/0x1e0
[  103.511919]  ? tty_insert_flip_string_fixed_flag+0x82/0xe0
[  103.511973]  ? inet_ioctl+0x187/0x1d0
[  103.512016]  dev_ioctl+0xb5/0x560
[  103.512055]  sock_do_ioctl+0xa0/0x140
[  103.512098]  sock_ioctl+0x2cb/0x3c0
[  103.512139]  __x64_sys_ioctl+0x84/0xc0
[  103.512183]  do_syscall_64+0x33/0x80
[  103.512224]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  103.512274] RIP: 0033:0x7f54a9cba427
...

Previous versions can be found at:
v1:
initial version
https://lkml.org/lkml/2020/10/28/921

v2:
do not return from lan743x_ethtool_set_wol if netdev->phydev == NULL, just skip
the call of phy_ethtool_set_wol() instead.
https://lkml.org/lkml/2020/10/31/380

v3:
in function lan743x_ethtool_set_wol:
use ternary operator instead of if-else sentence (review by Markus Elfring)
return -ENETDOWN insted of -EIO (review by Andrew Lunn)

Signed-off-by: Sergej Bauer 
---
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c 
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index dcde496da7fb..c5de8f46cdd3 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -780,7 +780,9 @@ static void lan743x_ethtool_get_wol(struct net_device 
*netdev,
 
wol->supported = 0;
wol->wolopts = 0;
-   phy_ethtool_get_wol(netdev->phydev, wol);
+
+   if (netdev->phydev)
+   phy_ethtool_get_wol(netdev->phydev, wol);
 
wol->supported |= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
@@ -809,9 +811,8 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
 
device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
 
-   phy_ethtool_set_wol(netdev->phydev, wol);
-
-   return 0;
+   return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
+   : -ENETDOWN;
 }
 #endif /* CONFIG_PM */
 

Re: [PATCH v3] lan743x: fix for potential NULL pointer dereference with bare card

[Sergej Bauer]

On Wednesday, November 4, 2020 4:38:33 AM MSK Jakub Kicinski wrote:
> On Mon,  2 Nov 2020 01:35:55 +0300 Sergej Bauer wrote:
> > This is the 3rd revision of the patch fix for potential null pointer
> > dereference with lan743x card.
> > 
> > The simpliest way to reproduce: boot with bare lan743x and issue "ethtool
> > ethN" commant where ethN is the interface with lan743x card. Example:
> > 
> > $ sudo ethtool eth7
> > dmesg:
> > [  103.510336] BUG: kernel NULL pointer dereference, address:
> > 0340 ...
> > [  103.510836] RIP: 0010:phy_ethtool_get_wol+0x5/0x30 [libphy]
> > ...
> > [  103.511629] Call Trace:
> > [  103.511666]  lan743x_ethtool_get_wol+0x21/0x40 [lan743x]
> > [  103.511724]  dev_ethtool+0x1507/0x29d0
> > [  103.511769]  ? avc_has_extended_perms+0x17f/0x440
> > [  103.511820]  ? tomoyo_init_request_info+0x84/0x90
> > [  103.511870]  ? tomoyo_path_number_perm+0x68/0x1e0
> > [  103.511919]  ? tty_insert_flip_string_fixed_flag+0x82/0xe0
> > [  103.511973]  ? inet_ioctl+0x187/0x1d0
> > [  103.512016]  dev_ioctl+0xb5/0x560
> > [  103.512055]  sock_do_ioctl+0xa0/0x140
> > [  103.512098]  sock_ioctl+0x2cb/0x3c0
> > [  103.512139]  __x64_sys_ioctl+0x84/0xc0
> > [  103.512183]  do_syscall_64+0x33/0x80
> > [  103.512224]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [  103.512274] RIP: 0033:0x7f54a9cba427
> 
> Applied, thanks!

Hi, Jakub!
Thank you for taking the time to review my patch

Regards,
Sergej.

[PATCH] add virtual PHY for PHY-less devices

[Sergej Bauer]

From: sba...@blackbox.su

Here is a kernel related part of my work which was helps to develop brand
new PHY device.

It is migth be helpful for developers work with PHY-less lan743x
(7431:0011 in my case). It's just a fake virtual PHY which can change speed of
network card processing as a loopback device. Baud rate can be tuned with
ethtool from command line or by means of SIOCSMIIREG ioctl. Duplex mode not
configurable and it's allways DUPLEX_FULL.

It also provides module parameter mii_regs for setting initial values of
IEEE 802.3 Control Register.

diff --git a/MAINTAINERS b/MAINTAINERS
index f0068bceeb61..d4c10895aa13 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11423,6 +11448,12 @@ L: net...@vger.kernel.org
 S: Maintained
 F: drivers/net/ethernet/microchip/lan743x_*
 
+MICROCHIP LAN743X VIRTUAL PHY DRIVER
+M:     Sergej Bauer 
+L: net...@vger.kernel.org
+S: Maintained
+F: drivers/net/ethernet/microchip/lan743x_virtual_phy.*
+
 MICROCHIP LCDFB DRIVER
 M: Nicolas Ferre 
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/net/ethernet/microchip/Makefile 
b/drivers/net/ethernet/microchip/Makefile
index da603540ca57..8850ba5f5104 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -7,4 +7,5 @@ obj-$(CONFIG_ENC28J60) += enc28j60.o
 obj-$(CONFIG_ENCX24J600) += encx24j600.o encx24j600-regmap.o
 obj-$(CONFIG_LAN743X) += lan743x.o
 
-lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
+lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o \
+   lan743x_virtual_phy.o
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c 
b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index dcde496da7fb..0eaa20529ba2 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -815,6 +815,72 @@ static int lan743x_ethtool_set_wol(struct net_device 
*netdev,
 }
 #endif /* CONFIG_PM */
 
+int lan743x_set_link_ksettings(struct net_device *netdev,
+  const struct ethtool_link_ksettings *cmd)
+{
+   struct ethtool_link_ksettings *cmd_ref =
+   (struct ethtool_link_ksettings *)cmd;
+   struct lan743x_adapter *adapter = netdev_priv(netdev);
+   u8 duplex = cmd->base.duplex;
+   u32 speed = cmd->base.speed;
+   u16 v = 0, mac_rx, mac_tx;
+
+   if (speed &&
+   speed != SPEED_10 && speed != SPEED_100 && speed != SPEED_1000) {
+   netdev_err(netdev, "%s: unknown speed %d\n",
+  netdev->name, speed);
+   goto out;
+   }
+
+   mac_tx = lan743x_csr_read(adapter, MAC_TX);
+   mac_rx = lan743x_csr_read(adapter, MAC_RX);
+
+   if (adapter->virtual_phy) {
+   v = adapter->regs802p3[0];
+   v &= ~(BMCR_SPEED1000 | BMCR_SPEED100);
+
+   lan743x_netdev_set_bit(netdev, MAC_CR, 12, 0); // ADD
+   lan743x_netdev_set_bit(netdev, MAC_CR, 11, 0); // ASD
+   duplex = DUPLEX_FULL;
+   switch (speed) {
+   case 0:
+   break;
+   case SPEED_10:
+   lan743x_netdev_set_bit(netdev, MAC_CR, 2, 0);
+   lan743x_netdev_set_bit(netdev, MAC_CR, 1, 0);
+   netif_info(adapter, probe, adapter->netdev,
+  "lan743x: speed 10Mbps/%s\n",
+  duplex ? "Full" : "Half");
+   break;
+   case SPEED_100:
+   v |= BMCR_SPEED100;
+   lan743x_netdev_set_bit(netdev, MAC_CR, 2, 0);
+   lan743x_netdev_set_bit(netdev, MAC_CR, 1, 1);
+   netif_info(adapter, probe, adapter->netdev,
+  "lan743x: speed 100Mbps/%s\n",
+  duplex ? "Full" : "Half");
+   break;
+   case SPEED_1000:
+   v |= BMCR_SPEED1000;
+   lan743x_netdev_set_bit(netdev, MAC_CR, 2, 1);
+   lan743x_netdev_set_bit(netdev, MAC_CR, 1, 0);
+   netif_info(adapter, probe, adapter->netdev,
+  "lan743x: speed 1Gbps/%s\n",
+  duplex ? "Full" : "Half");
+   break;
+   default:
+   return -EINVAL;
+   };
+
+   v |= BMCR_LOOPBACK | BMCR_FULLDPLX;
+   adapter->regs802p3[0] = v;
+   cmd_ref->base.duplex = duplex;
+   }
+
+out:
+   return phy_ethtool_set_link_ksettings(netdev, cmd);
+}
+
 const struct ethtool_ops lan743x_ethtool_ops = {
.get_drvinfo = 

Re: [PATCH] add virtual PHY for PHY-less devices

[Sergej Bauer]

Hi Andrew

  To tell the truth, I thought that fixed_phy is only for devices with a Device
Trees and I never met DTS on x86 machines... 

So it looks like there realy no any significant advantage _except_ of
ability to use ethtool and ioctl to set speed and rx-all/fcs flags without
removing module. That was most wanted request from HW designers as they are
wanted to change registers of virtual PHY on-the-fly with ethtool either custom
tool (using SIOCSMIIREG ioctl) for controling PHY registers.

p.s. And that's my bad, the original driver was developed year ago (for 
linux-5.2.15),
but I had no time before this moment.


p.p.s. sorry for long time to answer but it's far behind the midnight in my 
region.

-- 
Sergej


On Friday, September 18, 2020 1:15:47 AM MSK Andrew Lunn wrote:
> On Fri, Sep 18, 2020 at 12:40:10AM +0300, Sergej Bauer wrote:
> > From: sba...@blackbox.su
> > 
> > Here is a kernel related part of my work which was helps to develop
> > brand
> > 
> > new PHY device.
> > 
> > It is migth be helpful for developers work with PHY-less lan743x
> > 
> > (7431:0011 in my case). It's just a fake virtual PHY which can change
> > speed of network card processing as a loopback device. Baud rate can be
> > tuned with ethtool from command line or by means of SIOCSMIIREG ioctl.
> > Duplex mode not configurable and it's allways DUPLEX_FULL.
> 
> Hi Sergej
> 
> What is the advantage of this over using driver/net/phy/fixed_phy.c
> which also emulates a standard PHY?
> 
>   Andrew

Re: [PATCH] add virtual PHY for PHY-less devices

[Sergej Bauer]

Hi Florian

  The fixed-PHY driver looks a little bit hardly tunable, please look at
https://www.lkml.org/lkml/2020/9/17/1424
  This is not my idea. This is a request of HW team, and I thnk it will be
useful for someone else. But I'll try to combine fixed-phy with virtual-PHY
tomorrow. It just to rearrange some lines in lan743x_phy_open.

-- 
Sergej

On Friday, September 18, 2020 1:16:30 AM MSK Florian Fainelli wrote:
> On 9/17/2020 2:40 PM, Sergej Bauer wrote:
> > From: sba...@blackbox.su
> > 
> >  Here is a kernel related part of my work which was helps to develop
> >  brand
> > 
> > new PHY device.
> > 
> >  It is migth be helpful for developers work with PHY-less lan743x
> > 
> > (7431:0011 in my case). It's just a fake virtual PHY which can change
> > speed of network card processing as a loopback device. Baud rate can be
> > tuned with ethtool from command line or by means of SIOCSMIIREG ioctl.
> > Duplex mode not configurable and it's allways DUPLEX_FULL.
> > 
> >  It also provides module parameter mii_regs for setting initial values
> >  of
> > 
> > IEEE 802.3 Control Register.
> 
> You appear to have re-implemented the fixed PHY driver, please use that
> instead of rolling your own.