On 05/03/15(Thu) 12:25, David Higgs wrote: > > 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.
That's great, some comments inline. > 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 } > +}; I see that all the HUP_BATTERY sensors have the same dependency. Why not have a table for items without dependency (the parents) and a child table per parent? This way you would have only one "upd_update_sensor" function. This is really the key of this driver. Why a flat list like you have right now you need much more code, flags and customs functions. > > 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->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; I know that this is not new but is there an advantage of having a table rather than a LIST_? Could you get rid of the "attached" flag by using a list? > + 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)); Does this happen very often? I'm not sure if marking the sensor invalide here makes sense, what about simply skipping the update? > + } > + } > > - 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); Have you seen this message? I'm not sure users care about this, I'd not print anything since this is redundant with your debugging printf. > + 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) { If you need to keep the "attached" flag, I'd suggest returning NULL if there's no (attached) sensor. But that's another argument for using a LIST_ 8) > +#ifdef DIAGNOSTIC > + if (sensor->pending) > + printf("%s: sensor %s is already pending!\n", > + sc->sc_sensordev.xname, sensor->ksensor.desc); This should never happen and be a KASSERT(). > +#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; Please do not print anything, this is clearly a firmware problem, not something we want to see in any USB diver :) > + } > + > + /* 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) > >