On Mar 3, 2015, at 8:44 AM, David Higgs <hig...@gmail.com> wrote:
> With much help from mpi@, I have made a first big step towards improving
> upd(4). I’m not sure when tree lock ends, but I’m still happy to accept
> feedback if right now isn’t the time to commit. There’s plenty more to do,
> but I’d like to get this ironed out before moving on.
>
> New behavior with this diff:
> - Leverage new USB async reporting (must have up-to-date tree)
> - Sensor dependencies ensure reports are gathered in useful order
> - Track pending reports to minimize USB queries
> - Relevant sensors immediately invalidated when battery is removed (instead
> of waiting for the next refresh)
>
> Things that may need sanity checks:
> - Simplified upd_attach; the old code seemed to have redundant / confusing
> logic
> - Integer promotion/truncation with batpres and hdata/adjust variables
Below is an overhauled diff with some additional considerations I came up with.
Improvements on the previous version:
- ACPresent no longer depends on BatteryPresent
- Sensor dependencies now handled manually, so dynamic behavior can be added
later.
- Avoid hypothetical cases where certain USB report layouts could trigger:
- Infinite loops in sensor dependencies.
- Updating sensor contents using stale information.
- Unnecessary sensor invalidation.
- Redundant USB transfers.
As before, comments, questions, and feedback are welcome.
--david
Index: upd.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.13
diff -u -p -r1.13 upd.c
--- upd.c 11 Jan 2015 03:08:38 -0000 1.13
+++ upd.c 5 Mar 2015 17:06:19 -0000
@@ -39,45 +39,17 @@
#define DPRINTF(x)
#endif
-struct upd_usage_entry {
- uint8_t usage_pg;
- uint8_t usage_id;
- enum sensor_type senstype;
- char *usage_name; /* sensor string */
-};
-
-static struct upd_usage_entry upd_usage_table[] = {
- { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
- SENSOR_PERCENT, "RelativeStateOfCharge" },
- { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
- SENSOR_PERCENT, "AbsoluteStateOfCharge" },
- { HUP_BATTERY, HUB_REM_CAPACITY,
- SENSOR_PERCENT, "RemainingCapacity" },
- { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
- SENSOR_PERCENT, "FullChargeCapacity" },
- { HUP_BATTERY, HUB_CHARGING,
- SENSOR_INDICATOR, "Charging" },
- { HUP_BATTERY, HUB_DISCHARGING,
- SENSOR_INDICATOR, "Discharging" },
- { HUP_BATTERY, HUB_BATTERY_PRESENT,
- SENSOR_INDICATOR, "BatteryPresent" },
- { HUP_POWER, HUP_SHUTDOWN_IMMINENT,
- SENSOR_INDICATOR, "ShutdownImminent" },
- { HUP_BATTERY, HUB_AC_PRESENT,
- SENSOR_INDICATOR, "ACPresent" },
- { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA, "AtRateTimeToFull" }
-};
-
struct upd_report {
size_t size;
- int enabled;
+ int pending;
};
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
+ struct upd_usage_entry *entry;
int attached;
+ int pending;
};
struct upd_softc {
@@ -85,6 +57,7 @@ struct upd_softc {
int sc_num_sensors;
u_int sc_max_repid;
u_int sc_max_sensors;
+ char sc_buf[256];
/* sensor framework */
struct ksensordev sc_sensordev;
@@ -93,15 +66,66 @@ struct upd_softc {
struct upd_sensor *sc_sensors;
};
+struct upd_usage_entry {
+ uint8_t usage_pg;
+ uint8_t usage_id;
+ enum sensor_type senstype;
+ char *usage_name; /* sensor string */
+ void (*update_sensor)(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+};
+
int upd_match(struct device *, void *, void *);
void upd_attach(struct device *, struct device *, void *);
int upd_detach(struct device *, int);
void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
-void upd_intr(struct uhidev *, void *, uint);
+void upd_request_sensor_refresh(struct upd_softc *, uint8_t, uint8_t, int);
+void upd_update_sensor_cb(void *, int, void *, int);
+void upd_mark_sensor_invalid(struct upd_softc *, struct upd_sensor *, int);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
+int upd_battery_present(struct upd_softc *);
+void upd_update_batpres_sensor(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+void upd_update_batdep_sensor(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+void upd_intr(struct uhidev *, void *, uint);
+
+static struct upd_usage_entry upd_usage_table[] = {
+ { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
+ SENSOR_PERCENT, "RelativeStateOfCharge",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
+ SENSOR_PERCENT, "AbsoluteStateOfCharge",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_REM_CAPACITY,
+ SENSOR_PERCENT, "RemainingCapacity",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
+ SENSOR_PERCENT, "FullChargeCapacity",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_CHARGING,
+ SENSOR_INDICATOR, "Charging",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_DISCHARGING,
+ SENSOR_INDICATOR, "Discharging",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_INDICATOR, "BatteryPresent",
+ upd_update_batpres_sensor },
+ { HUP_POWER, HUP_SHUTDOWN_IMMINENT,
+ SENSOR_INDICATOR, "ShutdownImminent",
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_AC_PRESENT,
+ SENSOR_INDICATOR, "ACPresent",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
+ SENSOR_TIMEDELTA, "AtRateTimeToFull",
+ upd_update_batdep_sensor }
+};
struct cfdriver upd_cd = {
NULL, "upd", DV_DULL
@@ -183,38 +207,30 @@ upd_attach(struct device *parent, struct
sc->sc_num_sensors < sc->sc_max_sensors; ) {
DPRINTF(("upd: repid=%d\n", item.report_ID));
if (item.kind != hid_feature ||
- item.report_ID < 0)
+ item.report_ID < 0 ||
+ item.report_ID >= sc->sc_max_repid)
continue;
-
if ((entry = upd_lookup_usage_entry(&item)) == NULL)
continue;
-
- /* filter repeated usages, avoid duplicated sensors */
- sensor = upd_lookup_sensor(sc, entry->usage_pg,
- entry->usage_id);
- if (sensor && sensor->attached)
+ if (upd_lookup_sensor(sc, entry->usage_pg, entry->usage_id))
continue;
sensor = &sc->sc_sensors[sc->sc_num_sensors];
- /* keep our copy of hid_item */
memcpy(&sensor->hitem, &item, sizeof(struct hid_item));
strlcpy(sensor->ksensor.desc, entry->usage_name,
sizeof(sensor->ksensor.desc));
sensor->ksensor.type = entry->senstype;
- sensor->ksensor.flags |= SENSOR_FINVALID;
- sensor->ksensor.status = SENSOR_S_UNKNOWN;
+ upd_mark_sensor_invalid(sc, sensor, 0);
sensor->ksensor.value = 0;
+ sensor->entry = entry;
sensor_attach(&sc->sc_sensordev, &sensor->ksensor);
sensor->attached = 1;
+ sensor->pending = 0;
sc->sc_num_sensors++;
- if (item.report_ID >= sc->sc_max_repid ||
- sc->sc_reports[item.report_ID].enabled)
- continue;
-
sc->sc_reports[item.report_ID].size = hid_report_size(desc,
size, item.kind, item.report_ID);
- sc->sc_reports[item.report_ID].enabled = 1;
+ sc->sc_reports[item.report_ID].pending = 0;
}
hid_end_parse(hdata);
DPRINTF(("upd: sc_num_sensors=%d\n", sc->sc_num_sensors));
@@ -262,32 +278,124 @@ void
upd_refresh(void *arg)
{
struct upd_softc *sc = (struct upd_softc *)arg;
+ struct upd_sensor *sensor;
struct upd_report *report;
- uint8_t buf[256];
- int repid, actlen;
+ int i, pending = 0;
- for (repid = 0; repid < sc->sc_max_repid; repid++) {
- report = &sc->sc_reports[repid];
- if (!report->enabled)
+ for (i = 0; i < sc->sc_num_sensors; i++) {
+ sensor = &sc->sc_sensors[i];
+ if (!sensor->attached)
continue;
+ report = &sc->sc_reports[sensor->hitem.report_ID];
+ if (sensor->pending || report->pending) {
+ pending++;
+ upd_mark_sensor_invalid(sc, sensor, 0);
+ DPRINTF(("%s: sensor %s (repid=%d) still pending\n",
+ sc->sc_sensordev.xname, sensor->ksensor.desc,
+ sensor->hitem.report_ID));
+ }
+ }
- memset(buf, 0x0, sizeof(buf));
- actlen = uhidev_get_report(sc->sc_hdev.sc_parent,
- UHID_FEATURE_REPORT, repid, buf, report->size);
+ if (pending > 0)
+ printf("%s: %d async sensors still pending\n",
+ sc->sc_sensordev.xname, pending);
+ else {
+ upd_request_sensor_refresh(sc, HUP_POWER,
+ HUP_SHUTDOWN_IMMINENT, 1);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_BATTERY_PRESENT, 1);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_AC_PRESENT, 1);
+ }
+}
- if (actlen == -1) {
- DPRINTF(("upd: failed to get report id=%02x\n", repid));
- continue;
- }
+void
+upd_request_sensor_refresh(struct upd_softc *sc, uint8_t usage_pg,
+ uint8_t usage_id, int valid)
+{
+ struct upd_sensor *sensor;
+ struct upd_report *report;
+ int repid;
- /* Deal with buggy firmwares. */
- if (actlen < report->size)
- report->size = actlen;
+ sensor = upd_lookup_sensor(sc, usage_pg, usage_id);
+ if (sensor != NULL && sensor->attached) {
+#ifdef DIAGNOSTIC
+ if (sensor->pending)
+ printf("%s: sensor %s is already pending!\n",
+ sc->sc_sensordev.xname, sensor->ksensor.desc);
+#endif
+ repid = sensor->hitem.report_ID;
+ report = &sc->sc_reports[repid];
- upd_update_sensors(sc, buf, report->size, repid);
+ if (!valid)
+ /* invalidate just one sensor */
+ upd_mark_sensor_invalid(sc, sensor, 1);
+ else if (report->pending)
+ /* already requested */
+ sensor->pending = 1;
+ else if (uhidev_get_report_async(sc->sc_hdev.sc_parent,
+ UHID_FEATURE_REPORT, repid, sc->sc_buf, report->size,
+ sc, upd_update_sensor_cb) > 0) {
+ DPRINTF(("%s: repid=%d requested by %s\n",
+ sc->sc_sensordev.xname, repid,
+ sensor->ksensor.desc));
+ report->pending = 1;
+ sensor->pending = 1;
+ } else
+ /* no other sensors can be pending on this repid */
+ upd_mark_sensor_invalid(sc, sensor, 1);
}
}
+void
+upd_update_sensor_cb(void *priv, int repid, void *data, int len)
+{
+ struct upd_softc *sc = priv;
+ struct upd_report *report;
+ struct upd_sensor *sensor;
+ int i, updated;
+
+ report = &sc->sc_reports[repid];
+ if (len > 0 && report->size != len) {
+ printf("%s: adjusting size of repid %d\n",
+ sc->sc_sensordev.xname, repid);
+ report->size = len;
+ }
+
+ /* sc_sensors may not be in dependency order */
+ do {
+ updated = 0;
+ for (i = 0; i < sc->sc_num_sensors; i++) {
+ sensor = &sc->sc_sensors[i];
+
+ /* prevent infinite dependency traversals */
+ if (!sensor->attached || !sensor->pending ||
+ sensor->hitem.report_ID != repid)
+ continue;
+
+ if (len > 0)
+ sensor->entry->update_sensor(sc,
+ sensor, data, len);
+ else
+ upd_mark_sensor_invalid(sc, sensor, 1);
+ sensor->pending = 0;
+ updated = 1;
+ }
+ } while (updated);
+
+ report->pending = 0;
+}
+
+void
+upd_mark_sensor_invalid(struct upd_softc *sc, struct upd_sensor *sensor,
+ int deps)
+{
+ sensor->ksensor.status = SENSOR_S_UNKNOWN;
+ sensor->ksensor.flags |= SENSOR_FINVALID;
+ if (deps)
+ sensor->entry->update_sensor(sc, sensor, NULL, -1);
+}
+
struct upd_usage_entry *
upd_lookup_usage_entry(const struct hid_item *hitem)
{
@@ -318,35 +426,61 @@ upd_lookup_sensor(struct upd_softc *sc,
return (NULL);
}
-void
-upd_update_sensors(struct upd_softc *sc, uint8_t *buf, unsigned int len,
- int repid)
+int
+upd_battery_present(struct upd_softc *sc)
{
struct upd_sensor *sensor;
- ulong hdata, batpres;
- ulong adjust;
- int i;
+ int batpres = 0;
sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
- batpres = sensor ? sensor->ksensor.value : -1;
+ if (sensor && !(sensor->ksensor.flags & SENSOR_FINVALID))
+ batpres = sensor->ksensor.value;
+ return (batpres);
+}
- for (i = 0; i < sc->sc_num_sensors; i++) {
- sensor = &sc->sc_sensors[i];
- if (!(sensor->hitem.report_ID == repid && sensor->attached))
- continue;
+void
+upd_update_batpres_sensor(struct upd_softc *sc, struct upd_sensor *sensor,
+ uint8_t *buf, int len)
+{
+ int batpres;
- /* invalidate battery dependent sensors */
- if (HID_GET_USAGE_PAGE(sensor->hitem.usage) == HUP_BATTERY &&
- batpres <= 0) {
- /* exception to the battery sensor itself */
- if (HID_GET_USAGE(sensor->hitem.usage) !=
- HUB_BATTERY_PRESENT) {
- sensor->ksensor.status = SENSOR_S_UNKNOWN;
- sensor->ksensor.flags |= SENSOR_FINVALID;
- continue;
- }
- }
+ upd_update_sensor_value(sc, sensor, buf, len);
+ batpres = upd_battery_present(sc);
+
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_REL_STATEOF_CHARGE, batpres);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_ABS_STATEOF_CHARGE, batpres);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_REM_CAPACITY, batpres);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_FULLCHARGE_CAPACITY, batpres);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_CHARGING, batpres);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_DISCHARGING, batpres);
+ upd_request_sensor_refresh(sc, HUP_BATTERY,
+ HUB_ATRATE_TIMETOFULL, batpres);
+}
+
+void
+upd_update_batdep_sensor(struct upd_softc *sc, struct upd_sensor *sensor,
+ uint8_t *buf, int len)
+{
+ if (upd_battery_present(sc))
+ upd_update_sensor_value(sc, sensor, buf, len);
+ else
+ upd_mark_sensor_invalid(sc, sensor, 0);
+}
+
+void
+upd_update_sensor_value(struct upd_softc *sc, struct upd_sensor *sensor,
+ uint8_t *buf, int len)
+{
+ int64_t hdata, adjust;
+ if (len > 0) {
+ /* XXX ignores units, CapacityMode, and AtRate */
switch (HID_GET_USAGE(sensor->hitem.usage)) {
case HUB_REL_STATEOF_CHARGE:
case HUB_ABS_STATEOF_CHARGE:
@@ -360,15 +494,13 @@ upd_update_sensors(struct upd_softc *sc,
}
hdata = hid_get_data(buf, len, &sensor->hitem.loc);
-
sensor->ksensor.value = hdata * adjust;
sensor->ksensor.status = SENSOR_S_OK;
sensor->ksensor.flags &= ~SENSOR_FINVALID;
- DPRINTF(("%s: hidget data: %lu\n",
- sc->sc_sensordev.xname, hdata));
+ DPRINTF(("%s: %s hidget data: %lld\n",
+ sc->sc_sensordev.xname, sensor->ksensor.desc, hdata));
}
}
-
void
upd_intr(struct uhidev *uh, void *p, uint len)
