Protect code from pollers

Protect code from pollers

[Sascha Hauer]

barebox runs code in pollers. This works reasonably well when the
pollers do not touch any resources that are used by code outside the
pollers as well. Currently we also have fastboot running in a poller
and this is where things become dangerous. This series aims to provide a
way for solving resource conflicts with pollers. It is currently enough
to safely run the networking receive loop inside of pollers, but there
are more possible issues, one of them described below. There are several
more places that have to be sprinkled with:

slices, the barebox idea of locking
===================================

barebox has pollers which execute code in the background whenever one of the
delay functions (udelay, mdelay, ...) or is_timeout() are called. This
introduces resource problems when some device triggers a poller by calling
a delay function and then the poller code calls into the same device again.

As an example consider a I2C GPIO expander which drives a LED which shall
be used as a heartbeat LED:

poller -> LED on/off -> GPIO high/low -> I2C transfer

The I2C controller has a timeout loop using is_timeout() and thus can trigger
a poller run. With this the following can happen during an unrelated I2C
transfer:

I2C transfer -> is_timeout() -> poller -> LED on/off -> GPIO high/low -> I2C transfer

We end up with issuing an I2C transfer during another I2C transfer and
things go downhill.

Due to the lack of interrupts we can't do real locking in barebox. We use
a mechanism called slices instead. A slice describes a resource to which
other slices can be attached. Whenever a slice is needed it must be acquired.
Acquiring a slice never fails, it just increases the acquired counter of
the slice and its dependent slices. when a slice shall be used inside a
poller it must first be tested if the slice is already in use. If it is,
we can't do the operation on the slice now and must return and hope that
we have more luck in the next poller call.

slices can be attached other slices as dependencies. In the example above
LED driver would add a dependency to the GPIO controller and the GPIO driver
would add a dependency to the I2C bus:

GPIO driver probe:

slice_add(&gpio->slice, i2c_device_slice(i2cdev));

LED driver probe:

slice_add(&led->slice, gpio_slice(gpio));

The GPIO code would call slice_acquire(&gpio->slice) before doing any
operation on the GPIO chip providing this GPIO, likewise the I2C core
would call slice_acquire(&i2cbus->slice) before doing an operation on
this I2C bus.

The heartbeat poller code would call slice_acquired(led_slice(led)) and
only continue when the slice is not acquired.

Sascha

Sascha Hauer (8):
  Introduce slices
  net: Add a slice to struct eth_device
  net: mdiobus: Add slice
  usb: Add a slice to usb host controllers
  usbnet: Add slice
  net: Call net_poll() in a poller
  net: reply to ping requests
  usbnet: Be more friendly in the receive path

 common/Makefile            |   1 +
 common/slice.c             | 316 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |  40 +++++
 drivers/net/phy/phy.c      |   2 +
 drivers/net/usb/usbnet.c   |  22 ++-
 drivers/usb/core/usb.c     |   6 +
 fs/nfs.c                   |   2 -
 fs/tftp.c                  |   2 -
 include/linux/phy.h        |  32 ++--
 include/net.h              |  11 +-
 include/slice.h            |  31 ++++
 include/usb/usb.h          |   7 +
 include/usb/usbnet.h       |   3 +
 net/dhcp.c                 |   1 -
 net/dns.c                  |   1 -
 net/eth.c                  |  29 +++-
 net/net.c                  |  60 ++++++-
 net/netconsole.c           |   4 +-
 net/nfs.c                  |   1 -
 net/ping.c                 |   2 -
 net/sntp.c                 |   2 -
 21 files changed, 524 insertions(+), 51 deletions(-)
 create mode 100644 common/slice.c
 create mode 100644 include/slice.h

-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/8] Introduce slices

[Sascha Hauer]

slices, the barebox idea of locking

barebox has pollers which execute code in the background whenever one of the
delay functions (udelay, mdelay, ...) or is_timeout() are called. This
introduces resource problems when some device triggers a poller by calling
a delay function and then the poller code calls into the same device again.

As an example consider a I2C GPIO expander which drives a LED which shall
be used as a heartbeat LED:

poller -> LED on/off -> GPIO high/low -> I2C transfer

The I2C controller has a timeout loop using is_timeout() and thus can trigger
a poller run. With this the following can happen during an unrelated I2C
transfer:

I2C transfer -> is_timeout() -> poller -> LED on/off -> GPIO high/low -> I2C transfer

We end up with issuing an I2C transfer during another I2C transfer and
things go downhill.

Due to the lack of interrupts we can't do real locking in barebox. We use
a mechanism called slices instead. A slice describes a resource to which
other slices can be attached. Whenever a slice is needed it must be acquired.
Acquiring a slice never fails, it just increases the acquired counter of
the slice and its dependent slices. when a slice shall be used inside a
poller it must first be tested if the slice is already in use. If it is,
we can't do the operation on the slice now and must return and hope that
we have more luck in the next poller call.

slices can be attached other slices as dependencies. In the example above
LED driver would add a dependency to the GPIO controller and the GPIO driver
would add a dependency to the I2C bus:

GPIO driver probe:

slice_add(&gpio->slice, i2c_device_slice(i2cdev));

LED driver probe:

slice_add(&led->slice, gpio_slice(gpio));

The GPIO code would call slice_acquire(&gpio->slice) before doing any
operation on the GPIO chip providing this GPIO, likewise the I2C core
would call slice_acquire(&i2cbus->slice) before doing an operation on
this I2C bus.

The heartbeat poller code would call slice_acquired(led_slice(led)) and
only continue when the slice is not acquired.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/Makefile |   1 +
 common/slice.c  | 316 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/slice.h |  31 +++++
 3 files changed, 348 insertions(+)
 create mode 100644 common/slice.c
 create mode 100644 include/slice.h

diff --git a/common/Makefile b/common/Makefile
index 84463b4d48..16f14db41c 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -11,6 +11,7 @@ obj-y				+= bootsource.o
 obj-$(CONFIG_ELF)		+= elf.o
 obj-y				+= restart.o
 obj-y				+= poweroff.o
+obj-y				+= slice.o
 obj-$(CONFIG_MACHINE_ID)	+= machine_id.o
 obj-$(CONFIG_AUTO_COMPLETE)	+= complete.o
 obj-y				+= version.o
diff --git a/common/slice.c b/common/slice.c
new file mode 100644
index 0000000000..2c3b46fbda
--- /dev/null
+++ b/common/slice.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) "slice: " fmt
+
+#include <common.h>
+#include <slice.h>
+
+/*
+ * slices, the barebox idea of locking
+ *
+ * barebox has pollers which execute code in the background whenever one of the
+ * delay functions (udelay, mdelay, ...) or is_timeout() are called. This
+ * introduces resource problems when some device triggers a poller by calling
+ * a delay function and then the poller code calls into the same device again.
+ *
+ * As an example consider a I2C GPIO expander which drives a LED which shall
+ * be used as a heartbeat LED:
+ *
+ * poller -> LED on/off -> GPIO high/low -> I2C transfer
+ *
+ * The I2C controller has a timeout loop using is_timeout() and thus can trigger
+ * a poller run. With this the following can happen during an unrelated I2C
+ * transfer:
+ *
+ * I2C transfer -> is_timeout() -> poller -> LED on/off -> GPIO high/low -> I2C transfer
+ *
+ * We end up with issuing an I2C transfer during another I2C transfer and
+ * things go downhill.
+ *
+ * Due to the lack of interrupts we can't do real locking in barebox. We use
+ * a mechanism called slices instead. A slice describes a resource to which
+ * other slices can be attached. Whenever a slice is needed it must be acquired.
+ * Acquiring a slice never fails, it just increases the acquired counter of
+ * the slice and its dependent slices. when a slice shall be used inside a
+ * poller it must first be tested if the slice is already in use. If it is,
+ * we can't do the operation on the slice now and must return and hope that
+ * we have more luck in the next poller call.
+ *
+ * slices can be attached other slices as dependencies. In the example above
+ * LED driver would add a dependency to the GPIO controller and the GPIO driver
+ * would add a dependency to the I2C bus:
+ *
+ * GPIO driver probe:
+ *
+ * slice_add(&gpio->slice, i2c_device_slice(i2cdev));
+ *
+ * LED driver probe:
+ *
+ * slice_add(&led->slice, gpio_slice(gpio));
+ *
+ * The GPIO code would call slice_acquire(&gpio->slice) before doing any
+ * operation on the GPIO chip providing this GPIO, likewise the I2C core
+ * would call slice_acquire(&i2cbus->slice) before doing an operation on
+ * this I2C bus.
+ *
+ * The heartbeat poller code would call slice_acquired(led_slice(led)) and
+ * only continue when the slice is not acquired.
+ */
+
+/*
+ * slices are not required to have a device and thus a name, but it really
+ * helps debugging when it has one.
+ */
+static const char *slice_name(struct slice *slice)
+{
+	return slice->dev ? dev_name(slice->dev) : "<unknown>";
+}
+
+static void __slice_acquire(struct slice *slice, int add)
+{
+	struct slice_entry *se;
+
+	slice->acquired += add;
+
+	if (slice->acquired < 0) {
+		pr_err("Slice %s acquired count drops below zero\n",
+			slice_name(slice));
+		dump_stack();
+		slice->acquired = 0;
+		return;
+	}
+
+	list_for_each_entry(se, &slice->deps, list)
+		__slice_acquire(se->slice, add);
+}
+
+/**
+ * slice_acquire: acquire a slice
+ * @slice: The slice to acquire
+ *
+ * This acquires a slice and its dependencies
+ */
+void slice_acquire(struct slice *slice)
+{
+	__slice_acquire(slice, 1);
+}
+
+/**
+ * slice_release: release a slice
+ * @slice: The slice to release
+ *
+ * This releases a slice and its dependencies
+ */
+void slice_release(struct slice *slice)
+{
+	__slice_acquire(slice, -1);
+}
+
+/**
+ * slice_acquired: test if a slice is acquired
+ * @slice: The slice to test
+ *
+ * This tests if a slice is acquired. Returns true if it is, false otherwise
+ */
+bool slice_acquired(struct slice *slice)
+{
+	struct slice_entry *se;
+
+	if (slice->acquired)
+		return true;
+
+	list_for_each_entry(se, &slice->deps, list)
+		if (slice_acquired(se->slice))
+			return true;
+
+	return false;
+}
+
+static void __slice_debug_acquired(struct slice *slice, int level)
+{
+	struct slice_entry *se;
+
+	pr_debug("%*s%s: %d\n", level * 4, "",
+		 slice_name(slice),
+		 slice->acquired);
+	
+	list_for_each_entry(se, &slice->deps, list)
+		__slice_debug_acquired(se->slice, level + 1);
+}
+
+/**
+ * slice_debug_acquired: print the acquired state of a slice
+ *
+ * @slice: The slice to print
+ *
+ * This prints the acquired state of a slice and its dependencies.
+ */
+void slice_debug_acquired(struct slice *slice)
+{
+	if (!slice_acquired(slice))
+		return;
+
+	__slice_debug_acquired(slice, 0);
+}
+
+/**
+ * slice_add: Add a dependency to a slice
+ *
+ * @slice: The slice to add the dependency to
+ * @dep:   The slice @slice depends on
+ *
+ * This makes @slice dependent on @dep. In other words, acquiring @slice
+ * will lead to also acquiring @dep.
+ */
+void slice_add(struct slice *slice, struct slice *dep)
+{
+	struct slice_entry *se = xzalloc(sizeof(*se));
+
+	pr_debug("Adding dependency %s (%d) to slice %s (%d)\n",
+		 slice_name(dep), dep->acquired,
+		 slice_name(slice), slice->acquired);
+
+	se->slice = dep;
+
+	__slice_acquire(dep, slice->acquired);
+
+	list_add_tail(&se->list, &slice->deps);
+}
+
+static LIST_HEAD(slices);
+
+/**
+ * slice_del: Remove a dependency previously added with slice_add
+ * @slice: The slice to remove the dependency from
+ * @dep:   The slice @slice depended on
+ */
+void slice_del(struct slice *slice, struct slice *dep)
+{
+	struct slice_entry *se;
+
+	list_del(&slice->list);
+
+	list_for_each_entry(se, &slice->deps, list) {
+		if (se->slice == dep) {
+			list_del(&se->list);
+			free(se);
+			return;
+		}
+	}
+}
+
+/**
+ * slice_init - initialize a slice before usage
+ * @slice: The slice to initialize
+ * @dev:   a device as context pointer
+ *
+ * This initializes a slice before usage. Passing a nonzero @dev pointer
+ * is optional but strongly recommended to allow printing some context
+ * when dumping the dependencies.
+ */
+void slice_init(struct slice *slice, struct device_d *dev)
+{
+	INIT_LIST_HEAD(&slice->deps);
+	slice->dev = dev;
+	list_add_tail(&slice->list, &slices);
+}
+
+#ifdef DEBUG
+#include <command.h>
+#include <getopt.h>
+
+static void slice_info(void)
+{
+	struct slice *slice;
+	struct slice_entry *se;
+
+	list_for_each_entry(slice, &slices, list) {
+		printf("slice %s: acquired=%d\n",
+		       slice_name(slice), slice->acquired);
+		list_for_each_entry(se, &slice->deps, list)
+			printf("    dep: %s\n", slice_name(se->slice));
+	}
+}
+
+static int __dev_acquire(const char *devname, int add)
+{
+	struct device_d *dev;
+	struct slice *slice;
+
+	dev = get_device_by_name(optarg);
+	if (!dev) {
+		pr_err("No such device: %s\n", optarg);
+		return 1;
+	}
+
+	list_for_each_entry(slice, &slices, list) {
+		if (slice->dev == dev) {
+			__slice_acquire(slice, add);
+			return 0;
+		}
+	}
+
+	printf("No slice for %s\n", optarg);
+
+	return 1;
+}
+
+static void slice_time(void)
+{
+	uint64_t start = get_time_ns();
+	int i = 0;
+
+	/*
+	 * Not really slice related, but useful to know in this context:
+	 * How many times we can run the registered pollers in one second?
+	 *
+	 * A low number here may point to problems with pollers taking too
+	 * much time.
+	 */
+	while (!is_timeout(start, SECOND))
+		i++;
+
+	printf("%d poller calls in 1s\n", i);
+}
+
+BAREBOX_CMD_HELP_START(slice)
+BAREBOX_CMD_HELP_TEXT("slice debugging command")
+BAREBOX_CMD_HELP_TEXT("")
+BAREBOX_CMD_HELP_TEXT("Options:")
+BAREBOX_CMD_HELP_OPT ("-i", "Print information about slices")
+BAREBOX_CMD_HELP_OPT ("-a <devname>", "acquire a slice")
+BAREBOX_CMD_HELP_OPT ("-r <devname>", "release a slice")
+BAREBOX_CMD_HELP_OPT ("-t", "measure how many pollers we run in 1s")
+BAREBOX_CMD_HELP_END
+
+static int do_slice(int argc, char *argv[])
+{
+	int opt;
+
+	while ((opt = getopt(argc, argv, "a:r:it")) > 0) {
+		switch (opt) {
+		case 'a':
+			return __dev_acquire(optarg, 1);
+		case 'r':
+			return __dev_acquire(optarg, -1);
+		case 'i':
+			slice_info();
+			return 0;
+		case 't':
+			slice_time();
+			return 0;
+		}
+	}
+
+	return COMMAND_ERROR_USAGE;
+}
+
+BAREBOX_CMD_START(slice)
+	.cmd = do_slice,
+	BAREBOX_CMD_DESC("debug slices")
+        BAREBOX_CMD_OPTS("[-ari]")
+        BAREBOX_CMD_GROUP(CMD_GRP_MISC)
+        BAREBOX_CMD_HELP(cmd_slice_help)
+
+BAREBOX_CMD_END
+#endif
diff --git a/include/slice.h b/include/slice.h
new file mode 100644
index 0000000000..e8fdde17c4
--- /dev/null
+++ b/include/slice.h
@@ -0,0 +1,31 @@
+#ifndef __SLICE_H
+#define __SLICE_H
+
+enum slice_action {
+	SLICE_ACQUIRE = 1,
+	SLICE_RELEASE = -1,
+	SLICE_TEST = 0,
+};
+
+struct slice {
+	int acquired;
+	struct list_head deps;
+	struct device_d *dev;
+	struct list_head list;
+};
+
+struct slice_entry {
+	struct slice *slice;
+	struct list_head list;
+};
+
+void slice_acquire(struct slice *slice);
+void slice_release(struct slice *slice);
+bool slice_acquired(struct slice *slice);
+void slice_add(struct slice *slice, struct slice *dep);
+void slice_del(struct slice *slice, struct slice *dep);
+void slice_init(struct slice *slice, struct device_d *dev);
+
+void slice_debug_acquired(struct slice *slice);
+
+#endif /* __SLICE_H */
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/8] net: Add a slice to struct eth_device

[Sascha Hauer]

Add ethernet code safe for being called from a poller.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/net.h |  8 ++++++++
 net/eth.c     | 26 ++++++++++++++++++++------
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/include/net.h b/include/net.h
index 6912a557b5..d7a1f4aaea 100644
--- a/include/net.h
+++ b/include/net.h
@@ -19,6 +19,7 @@
 #include <stdlib.h>
 #include <clock.h>
 #include <led.h>
+#include <slice.h>
 #include <xfuncs.h>
 #include <linux/phy.h>
 #include <linux/string.h>	/* memcpy */
@@ -63,6 +64,8 @@ struct eth_device {
 	char *bootarg;
 	char *linuxdevname;
 
+	struct slice slice;
+
 	bool ifup;
 #define ETH_MODE_DHCP 0
 #define ETH_MODE_STATIC 1
@@ -72,6 +75,11 @@ struct eth_device {
 
 #define dev_to_edev(d) container_of(d, struct eth_device, dev)
 
+static inline struct slice *eth_device_slice(struct eth_device *edev)
+{
+	return &edev->slice;
+}
+
 static inline const char *eth_name(struct eth_device *edev)
 {
 	return edev->devname;
diff --git a/net/eth.c b/net/eth.c
index 53d24baa16..6d36d05a6f 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -235,32 +235,44 @@ int eth_send(struct eth_device *edev, void *packet, int length)
 {
 	int ret;
 
+	slice_acquire(eth_device_slice(edev));
+
 	ret = eth_check_open(edev);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = eth_carrier_check(edev, 0);
 	if (ret)
-		return ret;
+		goto out;
 
 	led_trigger_network(LED_TRIGGER_NET_TX);
 
-	return edev->send(edev, packet, length);
+	ret = edev->send(edev, packet, length);
+out:
+	slice_release(eth_device_slice(edev));
+
+	return ret;
 }
 
 static int __eth_rx(struct eth_device *edev)
 {
 	int ret;
 
+	slice_acquire(eth_device_slice(edev));
+
 	ret = eth_check_open(edev);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = eth_carrier_check(edev, 0);
 	if (ret)
-		return ret;
+		goto out;
 
-	return edev->recv(edev);
+	ret = edev->recv(edev);
+out:
+	slice_release(eth_device_slice(edev));
+
+	return ret;
 }
 
 int eth_rx(void)
@@ -377,6 +389,8 @@ int eth_register(struct eth_device *edev)
 		edev->dev.id = DEVICE_ID_DYNAMIC;
 	}
 
+	slice_init(&edev->slice, dev);
+
 	ret = register_device(&edev->dev);
 	if (ret)
 		return ret;
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/8] net: mdiobus: Add slice

[Sascha Hauer]

By adding a slice to the mdio bus we make the mdio code safe for being
called in a poller.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/phy/mdio_bus.c | 40 ++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy.c      |  2 ++
 include/linux/phy.h        | 32 ++++++++++--------------------
 3 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 3480e2ffb4..30c2e1f409 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -232,6 +232,7 @@ int mdiobus_register(struct mii_bus *bus)
 	dev_set_name(&bus->dev, "miibus");
 	bus->dev.parent = bus->parent;
 	bus->dev.detect = mdiobus_detect;
+	slice_init(&bus->slice, &bus->dev);
 
 	err = register_device(&bus->dev);
 	if (err) {
@@ -357,6 +358,45 @@ static int mdio_bus_match(struct device_d *dev, struct driver_d *drv)
 	return 1;
 }
 
+/**
+ * mdiobus_read - Convenience function for reading a given MII mgmt register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to read
+ */
+int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
+{
+	int ret;
+
+	slice_acquire(&bus->slice);
+
+	ret = bus->read(bus, addr, regnum);
+
+	slice_release(&bus->slice);
+
+	return ret;
+}
+
+/**
+ * mdiobus_write - Convenience function for writing a given MII mgmt register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ */
+int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
+{
+	int ret;
+
+	slice_acquire(&bus->slice);
+
+	ret = bus->write(bus, addr, regnum, val);
+
+	slice_release(&bus->slice);
+
+	return ret;
+}
+
 static ssize_t phydev_read(struct cdev *cdev, void *_buf, size_t count, loff_t offset, ulong flags)
 {
 	int i = count;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ccdc9f3716..3425f02721 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -389,6 +389,8 @@ static int phy_device_attach(struct phy_device *phy, struct eth_device *edev,
 	if (ret)
 		return ret;
 
+	slice_add(eth_device_slice(edev), &phy->bus->slice);
+
 	/* Sanitize settings based on PHY capabilities */
 	if ((phy->supported & SUPPORTED_Autoneg) == 0)
 		phy->autoneg = AUTONEG_DISABLE;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a9fdf44f1a..48502caffa 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -16,6 +16,7 @@
 #define __PHY_H
 
 #include <driver.h>
+#include <slice.h>
 #include <linux/list.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
@@ -116,9 +117,16 @@ struct mii_bus {
 	struct list_head list;
 
 	bool is_multiplexed;
+
+	struct slice slice;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
+static inline struct slice *mdiobus_slice(struct mii_bus *bus)
+{
+	return &bus->slice;
+}
+
 int mdiobus_register(struct mii_bus *bus);
 void mdiobus_unregister(struct mii_bus *bus);
 struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
@@ -134,28 +142,8 @@ struct mii_bus *mdiobus_get_bus(int busnum);
 
 struct mii_bus *of_mdio_find_bus(struct device_node *mdio_bus_np);
 
-/**
- * mdiobus_read - Convenience function for reading a given MII mgmt register
- * @bus: the mii_bus struct
- * @addr: the phy address
- * @regnum: register number to read
- */
-static inline int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
-{
-	return bus->read(bus, addr, regnum);
-}
-
-/**
- * mdiobus_write - Convenience function for writing a given MII mgmt register
- * @bus: the mii_bus struct
- * @addr: the phy address
- * @regnum: register number to write
- * @val: value to write to @regnum
- */
-static inline int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
-{
-	return bus->write(bus, addr, regnum, val);
-}
+int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
+int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
 
 /* phy_device: An instance of a PHY
  *
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/8] usb: Add a slice to usb host controllers

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/core/usb.c | 6 ++++++
 include/usb/usb.h      | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 1c3dcb79a8..da7afb5a32 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -83,11 +83,16 @@ static inline int usb_host_acquire(struct usb_host *host)
 	if (host->sem)
 		return -EAGAIN;
 	host->sem++;
+
+	slice_acquire(&host->slice);
+
 	return 0;
 }
 
 static inline void usb_host_release(struct usb_host *host)
 {
+	slice_release(&host->slice);
+
 	if (host->sem > 0)
 		host->sem--;
 }
@@ -97,6 +102,7 @@ int usb_register_host(struct usb_host *host)
 	list_add_tail(&host->list, &host_list);
 	host->busnum = host_busnum++;
 	host->sem = 0;
+	slice_init(&host->slice, host->hw_dev);
 	return 0;
 }
 
diff --git a/include/usb/usb.h b/include/usb/usb.h
index 95dedfd5b7..d730578fd7 100644
--- a/include/usb/usb.h
+++ b/include/usb/usb.h
@@ -23,6 +23,7 @@
 #define _USB_H_
 
 #include <driver.h>
+#include <slice.h>
 #include <usb/ch9.h>
 #include <usb/ch11.h>
 #include <usb/usb_defs.h>
@@ -154,11 +155,17 @@ struct usb_host {
 	struct usb_device *root_dev;
 	int sem;
 	struct usb_phy *usbphy;
+	struct slice slice;
 };
 
 int usb_register_host(struct usb_host *);
 void usb_unregister_host(struct usb_host *host);
 
+static inline struct slice *usb_device_slice(struct usb_device *udev)
+{
+	return &udev->host->slice;
+}
+
 int usb_host_detect(struct usb_host *host);
 
 int usb_set_protocol(struct usb_device *dev, int ifnum, int protocol);
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/8] usbnet: Add slice

[Sascha Hauer]

Both the ethernet device and the mdio bus of a USB network controller
need the USB bus. Add dependencies to it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/usb/usbnet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 60e67ff1a2..f1a49a142c 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -213,6 +213,9 @@ int usbnet_probe(struct usb_device *usbdev, const struct usb_device_id *prod)
 
 	eth_register(edev);
 
+	slice_add(eth_device_slice(edev), usb_device_slice(usbdev));
+	slice_add(mdiobus_slice(&undev->miibus), usb_device_slice(usbdev));
+
 	return 0;
 out1:
 	dev_dbg(&edev->dev, "err: %d\n", status);
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/8] net: Call net_poll() in a poller

[Sascha Hauer]

This patch moves the ethernet receive loop into a poller. With this
network protocols no longer have to call net_loop() explicitly but
can do it implicitly by calling is_timeout() when waiting for packets.

Having the network receive loop running in a poller has the advantage
that we have it running continuously and not only when we explicitly
expect packets to come in. With this we can reasonably well answer to
ping requests which is implemented in the next patch. This also helps
protocols that run in the background like fastboot over UDP.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c         |  2 --
 fs/tftp.c        |  2 --
 include/net.h    |  3 ---
 net/dhcp.c       |  1 -
 net/dns.c        |  1 -
 net/eth.c        |  3 +++
 net/net.c        | 14 +++++++++++---
 net/netconsole.c |  4 +---
 net/nfs.c        |  1 -
 net/ping.c       |  2 --
 net/sntp.c       |  2 --
 11 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index 0ad07aa3f2..355202df45 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -459,8 +459,6 @@ again:
 		if (ctrlc())
 			return ERR_PTR(-EINTR);
 
-		net_poll();
-
 		if (is_timeout(nfs_timer_start, NFS_TIMEOUT)) {
 			tries++;
 			if (tries == NFS_MAX_RESEND)
diff --git a/fs/tftp.c b/fs/tftp.c
index 1f36c56511..11f289584d 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -210,8 +210,6 @@ static int tftp_poll(struct file_priv *priv)
 		return -ETIMEDOUT;
 	}
 
-	net_poll();
-
 	return 0;
 }
 
diff --git a/include/net.h b/include/net.h
index d7a1f4aaea..0faa684e7f 100644
--- a/include/net.h
+++ b/include/net.h
@@ -243,9 +243,6 @@ IPaddr_t net_get_nameserver(void);
 const char *net_get_domainname(void);
 struct eth_device *net_route(IPaddr_t ip);
 
-/* Do the work */
-void net_poll(void);
-
 static inline struct iphdr *net_eth_to_iphdr(char *pkt)
 {
 	return (struct iphdr *)(pkt + ETHER_HDR_SIZE);
diff --git a/net/dhcp.c b/net/dhcp.c
index 670a6a6422..b37a09944d 100644
--- a/net/dhcp.c
+++ b/net/dhcp.c
@@ -513,7 +513,6 @@ int dhcp_request(struct eth_device *edev, const struct dhcp_req_param *param,
 			ret = -ETIMEDOUT;
 			goto out1;
 		}
-		net_poll();
 		if (is_timeout(dhcp_start, 3 * SECOND)) {
 			dhcp_start = get_time_ns();
 			printf("T ");
diff --git a/net/dns.c b/net/dns.c
index ffe98ef9e3..851bf3722e 100644
--- a/net/dns.c
+++ b/net/dns.c
@@ -231,7 +231,6 @@ int resolv(const char *host, IPaddr_t *ip)
 		if (ctrlc()) {
 			break;
 		}
-		net_poll();
 		if (is_timeout(dns_timer_start, SECOND)) {
 			dns_timer_start = get_time_ns();
 			printf("T ");
diff --git a/net/eth.c b/net/eth.c
index 6d36d05a6f..e3c1ff30c7 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -258,6 +258,9 @@ static int __eth_rx(struct eth_device *edev)
 {
 	int ret;
 
+	if (slice_acquired(eth_device_slice(edev)))
+		return 0;
+
 	slice_acquire(eth_device_slice(edev));
 
 	ret = eth_check_open(edev);
diff --git a/net/net.c b/net/net.c
index 0d889ddb52..a5c7fb5211 100644
--- a/net/net.c
+++ b/net/net.c
@@ -242,8 +242,6 @@ static int arp_request(struct eth_device *edev, IPaddr_t dest, unsigned char *et
 
 		if (retries > PKT_NUM_RETRIES)
 			return -ETIMEDOUT;
-
-		net_poll();
 	}
 
 	pr_debug("Got ARP REPLY for %pI4: %02x:%02x:%02x:%02x:%02x:%02x\n",
@@ -252,11 +250,21 @@ static int arp_request(struct eth_device *edev, IPaddr_t dest, unsigned char *et
 	return 0;
 }
 
-void net_poll(void)
+static void net_poll(struct poller_struct *poller)
 {
 	eth_rx();
 }
 
+static struct poller_struct net_poller = {
+	.func = net_poll,
+};
+
+static int init_net_poll(void)
+{
+	return poller_register(&net_poller);
+}
+device_initcall(init_net_poll);
+
 static uint16_t net_udp_new_localport(void)
 {
 	static uint16_t localport;
diff --git a/net/netconsole.c b/net/netconsole.c
index ef8b1856b6..457a3c2971 100644
--- a/net/netconsole.c
+++ b/net/netconsole.c
@@ -62,7 +62,7 @@ static int nc_getc(struct console_device *cdev)
 		return 0;
 
 	while (!kfifo_len(priv->fifo))
-		net_poll();
+		udelay(1);
 
 	kfifo_getc(priv->fifo, &c);
 
@@ -80,8 +80,6 @@ static int nc_tstc(struct console_device *cdev)
 	if (priv->busy)
 		return kfifo_len(priv->fifo) ? 1 : 0;
 
-	net_poll();
-
 	return kfifo_len(priv->fifo) ? 1 : 0;
 }
 
diff --git a/net/nfs.c b/net/nfs.c
index 63573098d7..e0479ef692 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -708,7 +708,6 @@ static int do_nfs(int argc, char *argv[])
 			nfs_err = -EINTR;
 			break;
 		}
-		net_poll();
 		if (is_timeout(nfs_timer_start, NFS_TIMEOUT * SECOND)) {
 			show_progress(-1);
 			nfs_send();
diff --git a/net/ping.c b/net/ping.c
index a71f59a805..f3de0c27a4 100644
--- a/net/ping.c
+++ b/net/ping.c
@@ -89,8 +89,6 @@ static int do_ping(int argc, char *argv[])
 			break;
 		}
 
-		net_poll();
-
 		if (is_timeout(ping_start, SECOND)) {
 			/* No answer, send another packet */
 			ping_start = get_time_ns();
diff --git a/net/sntp.c b/net/sntp.c
index b4e6d6439c..f693a2e8af 100644
--- a/net/sntp.c
+++ b/net/sntp.c
@@ -145,8 +145,6 @@ s64 sntp(const char *server)
 			break;
 		}
 
-		net_poll();
-
 		if (is_timeout(sntp_start, 1 * SECOND)) {
 			sntp_start = get_time_ns();
 			ret = sntp_send();
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/8] net: reply to ping requests

[Sascha Hauer]

Now that we have the network receive function running in a poller we
can reasonably well answer to ping requests. Implement this feature.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 net/net.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index a5c7fb5211..abb0a1e5e7 100644
--- a/net/net.c
+++ b/net/net.c
@@ -573,12 +573,54 @@ static int net_handle_udp(unsigned char *pkt, int len)
 	return -EINVAL;
 }
 
-static int net_handle_icmp(unsigned char *pkt, int len)
+static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
+{
+	struct ethernet *et = (struct ethernet *)pkt;
+	struct icmphdr *icmp;
+	struct iphdr *ip = (struct iphdr *)(pkt + ETHER_HDR_SIZE);
+	unsigned char *packet;
+	int ret;
+
+	memcpy(et->et_dest, et->et_src, 6);
+	memcpy(et->et_src, edev->ethaddr, 6);
+	et->et_protlen = htons(PROT_IP);
+
+	icmp = net_eth_to_icmphdr(pkt);
+
+	icmp->type = ICMP_ECHO_REPLY;
+	icmp->checksum = 0;
+	icmp->checksum = ~net_checksum((unsigned char *)icmp,
+				       len - sizeof(struct iphdr) - ETHER_HDR_SIZE);
+	ip->check = 0;
+	ip->frag_off = 0;
+	net_copy_ip((void *)&ip->daddr, &ip->saddr);
+	net_copy_ip((void *)&ip->saddr, &edev->ipaddr);
+	ip->check = ~net_checksum((unsigned char *)ip, sizeof(struct iphdr));
+
+	packet = net_alloc_packet();
+	if (!packet)
+		return 0;
+
+	memcpy(packet, pkt, ETHER_HDR_SIZE + len);
+
+	ret = eth_send(edev, packet, ETHER_HDR_SIZE + len);
+
+	free(packet);
+
+	return 0;
+}
+
+static int net_handle_icmp(struct eth_device *edev, unsigned char *pkt, int len)
 {
 	struct net_connection *con;
+	struct icmphdr *icmp;
 
 	pr_debug("%s\n", __func__);
 
+	icmp = net_eth_to_icmphdr(pkt);
+	if (icmp->type == ICMP_ECHO_REQUEST)
+		ping_reply(edev, pkt, len);
+
 	list_for_each_entry(con, &connection_list, list) {
 		if (con->proto == IPPROTO_ICMP) {
 			con->handler(con->priv, pkt, len);
@@ -615,7 +657,7 @@ static int net_handle_ip(struct eth_device *edev, unsigned char *pkt, int len)
 
 	switch (ip->protocol) {
 	case IPPROTO_ICMP:
-		return net_handle_icmp(pkt, len);
+		return net_handle_icmp(edev, pkt, len);
 	case IPPROTO_UDP:
 		return net_handle_udp(pkt, len);
 	}
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 8/8] usbnet: Be more friendly in the receive path

[Sascha Hauer]

To recognize if we have a receive packet pending we must set up a USB
bulk transfer. When there's no incoming packet we must wait until the
transfer times out. We do this with every poller call which can
considerably slow down the system. With this patch we do two things
against this:

- lower the timeout for the bulk transfer
- When we haven't received a packet for longer then lower the frequency
  of calling into the USB stack to once every 100ms

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/usb/usbnet.c | 19 ++++++++++++++++++-
 include/usb/usbnet.h     |  3 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f1a49a142c..38aa73047e 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -123,12 +123,29 @@ static int usbnet_recv(struct eth_device *edev)
 
 	dev_dbg(&edev->dev, "%s\n",__func__);
 
+	/*
+	 * we must let the usb_bulk_msg below timeout before we realize
+	 * that we have no packet received. Since this function runs
+	 * inside a poller we considerably slow down barebox when we
+	 * wait for the timeout too often. To improve this we only poll
+	 * with full speed when we actually have received a packet in the
+	 * last 100ms.
+	 */
+	if (is_timeout(dev->last_pkt_received, 100 * MSECOND) &&
+	    !is_timeout(dev->last_recv_call, 100 * MSECOND)) {
+		return 0;
+	}
+
+	dev->last_recv_call = get_time_ns();
+
 	len = dev->rx_urb_size;
 
-	ret = usb_bulk_msg(dev->udev, dev->in, rx_buf, len, &alen, 100);
+	ret = usb_bulk_msg(dev->udev, dev->in, rx_buf, len, &alen, 2);
 	if (ret)
 		return ret;
 
+	dev->last_pkt_received = get_time_ns();
+
 	if (alen) {
 		if (info->rx_fixup)
 			return info->rx_fixup(dev, rx_buf, alen);
diff --git a/include/usb/usbnet.h b/include/usb/usbnet.h
index 386c2164bd..ee578b81ed 100644
--- a/include/usb/usbnet.h
+++ b/include/usb/usbnet.h
@@ -52,6 +52,9 @@ struct usbnet {
 #		define EVENT_RX_MEMORY	2
 #		define EVENT_STS_SPLIT	3
 #		define EVENT_LINK_RESET	4
+
+	uint64_t last_pkt_received;
+	uint64_t last_recv_call;
 };
 
 #if 0
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] Introduce slices

[Daniel Glöckner]

Hello Sascha,

Am 03/06/20 um 20:33 schrieb Sascha Hauer:
> +static void __slice_acquire(struct slice *slice, int add)
> +{
> +	struct slice_entry *se;
> +
> +	slice->acquired += add;
> +
> +	if (slice->acquired < 0) {
> +		pr_err("Slice %s acquired count drops below zero\n",
> +			slice_name(slice));
> +		dump_stack();
> +		slice->acquired = 0;
> +		return;
> +	}
> +
> +	list_for_each_entry(se, &slice->deps, list)
> +		__slice_acquire(se->slice, add);

I don't think we need that loop. slice_acquired will already recurse into
the dependency tree. That loop would also mark resources as used which
are still unused. The network packet handlers can safely do MDIO transfers
because the network stack does not talk to the PHY while checking for
received packets.

> +bool slice_acquired(struct slice *slice)
> +{
> +	struct slice_entry *se;
> +
> +	if (slice->acquired)
> +		return true;
> +
> +	list_for_each_entry(se, &slice->deps, list)
> +		if (slice_acquired(se->slice))
> +			return true;
> +
> +	return false;
> +}

It would be nice if this function was able to detect cyclic dependencies
if DEBUG. This can be done by setting slice->acquired to INT_MIN while
recursing and checking if any of the values found are negative.

Best regards,

  Daniel

-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke
Ust-IdNr.: DE 205 198 055

emlix - your embedded linux partner

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] Introduce slices

[Sascha Hauer]

On Mon, Mar 09, 2020 at 03:49:53PM +0100, Daniel Glöckner wrote:
> Hello Sascha,
> 
> Am 03/06/20 um 20:33 schrieb Sascha Hauer:
> > +static void __slice_acquire(struct slice *slice, int add)
> > +{
> > +	struct slice_entry *se;
> > +
> > +	slice->acquired += add;
> > +
> > +	if (slice->acquired < 0) {
> > +		pr_err("Slice %s acquired count drops below zero\n",
> > +			slice_name(slice));
> > +		dump_stack();
> > +		slice->acquired = 0;
> > +		return;
> > +	}
> > +
> > +	list_for_each_entry(se, &slice->deps, list)
> > +		__slice_acquire(se->slice, add);
> 
> I don't think we need that loop. slice_acquired will already recurse into
> the dependency tree. That loop would also mark resources as used which
> are still unused. The network packet handlers can safely do MDIO transfers
> because the network stack does not talk to the PHY while checking for
> received packets.

Agreed.

> 
> > +bool slice_acquired(struct slice *slice)
> > +{
> > +	struct slice_entry *se;
> > +
> > +	if (slice->acquired)
> > +		return true;
> > +
> > +	list_for_each_entry(se, &slice->deps, list)
> > +		if (slice_acquired(se->slice))
> > +			return true;
> > +
> > +	return false;
> > +}
> 
> It would be nice if this function was able to detect cyclic dependencies
> if DEBUG. This can be done by setting slice->acquired to INT_MIN while
> recursing and checking if any of the values found are negative.

Yes, good idea.

I'll send an update for this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox