[PATCH DRAFT RFC] upoe: tiny daemon for PoE devices

Robert Marko robert.marko at sartura.hr
Thu May 13 09:34:02 PDT 2021


On Thu, May 13, 2021 at 12:14 PM Bjørn Mork <bjorn at mork.no> wrote:
>
> [ CCing robimarko as the author and maintainer of the tps23861 driver ]
>
> Rafał Miłecki <zajec5 at gmail.com> writes:
>
> > From: Rafał Miłecki <rafal at milecki.pl>
> >
> > This is a tiny app that reads list of PoE devices and initializes them
> > using built-in drivers. PoE status can be read using ubus.
> >
> > Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
> > ---
> > This is a *** DRAFT *** for how we could handle PoE devices in OpenWrt.
> > Please feel free to comment if this EARLY & INCOMPLETE design looks sane
> > enough.
> > ---
> >  CMakeLists.txt |   9 ++++
> >  dummy.c        |  24 +++++++++
> >  upoe.c         | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  upoe.h         |  36 +++++++++++++
> >  4 files changed, 213 insertions(+)
> >  create mode 100644 CMakeLists.txt
> >  create mode 100644 dummy.c
> >  create mode 100644 upoe.c
> >  create mode 100644 upoe.h
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > new file mode 100644
> > index 0000000..6474663
> > --- /dev/null
> > +++ b/CMakeLists.txt
> > @@ -0,0 +1,9 @@
> > +cmake_minimum_required(VERSION 2.6)
> > +
> > +project(upoe)
> > +set(CMAKE_C_FLAGS "-std=c99 -D_GNU_SOURCE -Wall")
> > +
> > +add_executable(upoe upoe.c dummy.c)
> > +target_link_libraries(upoe ubox ubus uci)
> > +
> > +install(TARGETS upoe RUNTIME DESTINATION bin)
> > diff --git a/dummy.c b/dummy.c
> > new file mode 100644
> > index 0000000..de0abfb
> > --- /dev/null
> > +++ b/dummy.c
> > @@ -0,0 +1,24 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "upoe.h"
> > +
> > +static int upoe_dummy_probe_device(struct upoe *upoe, struct uci_element *e)
> > +{
> > +     struct device *dev = calloc(1, sizeof(*dev));
> > +
> > +     dev->drv = &dummy;
> > +     upoe_device_register(upoe, dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int upoe_dummy_get_status(struct upoe *upoe, struct device *dev)
> > +{
> > +     return 0;
> > +}
> > +
> > +struct driver dummy = {
> > +     .type = "dummy",
> > +     .probe_device = upoe_dummy_probe_device,
> > +     .get_status = upoe_dummy_get_status,
> > +};
> > diff --git a/upoe.c b/upoe.c
> > new file mode 100644
> > index 0000000..3dfa834
> > --- /dev/null
> > +++ b/upoe.c
> > @@ -0,0 +1,144 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "upoe.h"
> > +
> > +/**************************************************
> > + * Device
> > + **************************************************/
> > +
> > +static int upoe_device_status(struct ubus_context *ctx,
> > +                           struct ubus_object *obj,
> > +                           struct ubus_request_data *req,
> > +                           const char *method,
> > +                           struct blob_attr *msg)
> > +{
> > +     struct upoe *upoe = container_of(ctx, struct upoe, ubus);
> > +     struct device *dev = container_of(obj, struct device, ubus_obj);
> > +     struct blob_buf b = { };
> > +     //void *c;
> > +
> > +     blob_buf_init(&b, 0);
> > +
> > +     dev->drv->get_status(upoe, dev);
> > +     blobmsg_add_string(&b, "foo", "bar");
> > +
> > +     ubus_send_reply(ctx, req, b.head);
> > +
> > +     blob_buf_free(&b);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct ubus_method upoe_ubus_device_methods[] = {
> > +     UBUS_METHOD_NOARG("status", upoe_device_status),
> > +};
> > +
> > +static struct ubus_object_type upoe_ubus_device_object_type =
> > +     UBUS_OBJECT_TYPE("upoe-device", upoe_ubus_device_methods);
> > +
> > +int upoe_device_register(struct upoe *upoe, struct device *dev)
> > +{
> > +        struct ubus_object *obj = &dev->ubus_obj;
> > +     char *name = NULL;
> > +     int err;
> > +
> > +     if (asprintf(&name, "upoe.%s", "dummy0") == -1)
> > +             return -ENOMEM;
> > +
> > +     obj->name = name;
> > +     obj->type = &upoe_ubus_device_object_type;
> > +     obj->methods = upoe_ubus_device_methods;
> > +     obj->n_methods = ARRAY_SIZE(upoe_ubus_device_methods);
> > +
> > +     err = ubus_add_object(&upoe->ubus, obj);
> > +     if (err) {
> > +             fprintf(stderr, "Failed to add ubus object\n");
> > +             free(name);
> > +     }
> > +
> > +     return err;
> > +}
> > +
> > +/**************************************************
> > + * main()
> > + **************************************************/
> > +
> > +static struct driver *drivers[] = {
> > +     &dummy,
> > +};
> > +
> > +static int upoe_probe_devices(struct upoe *upoe) {
> > +     struct uci_package *p;
> > +     struct uci_element *e;
> > +
> > +     uci_load(upoe->uci, "upoe", &p);
> > +     if (!p) {
> > +             fprintf(stderr, "Failed to load upoe config\n");
> > +             return -ENOENT;
> > +     }
> > +
> > +     uci_foreach_element(&p->sections, e) {
> > +             struct uci_section *s;
> > +             const char *type;
> > +             int i;
> > +
> > +             s = uci_to_section(e);
> > +
> > +             if (strcmp(s->type, "device"))
> > +                     continue;
> > +
> > +             type = uci_lookup_option_string(upoe->uci, s, "type");
> > +             if (!type)
> > +                     continue;
> > +
> > +             for (i = 0; i < ARRAY_SIZE(drivers); i++) {
> > +                     struct driver *drv = drivers[i];
> > +
> > +                     if (!strcmp(drv->type, type)) {
> > +                             drv->probe_device(upoe, e);
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +
> > +     uci_unload(upoe->uci, p);
> > +
> > +     return 0;
> > +}
> > +
> > +int main(int argc, char **argv) {
> > +     struct upoe *upoe = calloc(1, sizeof(*upoe));
> > +     int err;
> > +
> > +     if (!upoe) {
> > +             err = -ENOMEM;
> > +             goto err_out;
> > +     }
> > +
> > +     upoe->uci = uci_alloc_context();
> > +     if (!upoe->uci) {
> > +             err = -ENOMEM;
> > +             goto err_free_upoe;
> > +     }
> > +
> > +     if (ubus_connect_ctx(&upoe->ubus, NULL)) {
> > +             fprintf(stderr, "Failed to connect to ubus\n");
> > +             err = -ENXIO;
> > +             goto err_uci_free;
> > +     }
> > +
> > +     upoe_probe_devices(upoe);
> > +
> > +     uloop_init();
> > +     ubus_add_uloop(&upoe->ubus);
> > +     uloop_run();
> > +     uloop_done();
> > +
> > +     ubus_shutdown(&upoe->ubus);
> > +err_uci_free:
> > +     uci_free_context(upoe->uci);
> > +err_free_upoe:
> > +     free(upoe);
> > +err_out:
> > +     return err;
> > +}
> > diff --git a/upoe.h b/upoe.h
> > new file mode 100644
> > index 0000000..c3fd18d
> > --- /dev/null
> > +++ b/upoe.h
> > @@ -0,0 +1,36 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#ifndef UPOE_H_
> > +#define UPOE_H_
> > +
> > +#include <uci.h>
> > +#include <libubus.h>
> > +
> > +struct upoe;
> > +struct device;
> > +
> > +struct driver {
> > +     const char              *type;
> > +     int                     (*probe_device)(struct upoe *upoe, struct uci_element *e);
> > +     int                     (*get_status)(struct upoe *upoe, struct device *dev);
> > +};
> > +
> > +struct device {
> > +     struct driver           *drv;
> > +     struct ubus_object      ubus_obj;
> > +     void                    (*get_status)(struct device *dev);
> > +
> > +     struct device           *next;
> > +};
> > +
> > +struct upoe {
> > +     struct uci_context      *uci;
> > +     struct ubus_context     ubus;
> > +     struct device           *devices;
> > +};
> > +
> > +int upoe_device_register(struct upoe *upoe, struct device *dev);
> > +
> > +struct driver dummy;
> > +
> > +#endif
>
>
> I don't have strong opinions either way here.  I am pretty sure we'll
> have to redesign this at some point no matter how much we try to think
> about it right now.  We just don't have enough experience with different
> hardware interfaces and use cases to make a proper design.  At least, I
> don't have...
>
> Hoping Robert will chime in, obviously having thought about this when
> designing the tps23861 hwmon interface.

Thanks for including me.
As far as my decisions for TPS23861 go, well the choice was to keep it
as simple as possible.
This PSE controller is not advanced, it's pretty much smart enough to
do 802.3at/af in auto mode
just fine.
So, I decided to pretty much only support auto mode and expose a basic
port on/off, current, voltage
and temperature measurements as those have standard hwmon properties.
Operating mode, whether PD is detected, class that is detected, and
PoE+ status are exposed in debugfs.
This controller does not really expose much more.

On the other hand, I am now working regularly with a
Microsemi(Microchip) PD69200 which is way more
advanced, it runs field upgradable FW and they got this controller
from 2015 to do 802.3bt in 4 pair mode.

It speaks a 15-byte protocol via UART or I2C, thankfully the docs for
the protocol per firmware release
and the firmware itself is freely available.
This thing is a whole another level, handling up to 48 ports while
being able to set power budgets per group,
matrix etc, priorities, measure power per port, limit power per port,
measure temperature per port,
monitor the number of PSU-s online and set power budgets according to
that, LLDP, and many many more.

I got some basic stuff working in the kernel driver due to lack of
time, but soon it should progress much more
and be posted for mainlining.

So, basically with the amount of things that these controllers support
what you want to expose depends solely
on what will you use as otherwise it will take too much time.
>
> This is diffult.  Just take a look at the existing PoE MIBs. The IETF
> experts should have solved this problem for us a long time ago, defining
> the statistiscs and settings required for any PoE managment.  But RFC3621
> was never sufficient, so every vendor made their own PoE MIBs.
>
> Do we want this daemon to be a low-level or high-level interface?  Will
> this be the interface used by some future lldp plugin, implementing the
> full layer2 802.3at/bt stuff?  If so, then we should define the suitable
> per-port hooks for that.  Maybe just use the lldp TLVs directly, or
> almost directy?
>
> Or is this just a high-level thing for UCI/LUCI only?  Then I guess the
> above example just needs a per-port enable, and it would be fine.

To be honest, I think that a PoE daemon is a must-have now with switch
targets becoming more common
and with stuff like cheap MikroTik routerboards having multi-port PoE.

That being said, I think that we should start small and implement a
UCI/LuCI based thing will port on/off
and some basic monitoring exposed via ubus or something like that.
And then we can build on that.

Regards,
Robert
>
> Sorry, I just have plenty of questions and no answers.  Not very
> helpful.  Will shut up now.
>
>
>
> Bjørn



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko at sartura.hr
Web: www.sartura.hr



More information about the openwrt-devel mailing list