On 05/03/15(Thu) 12:25, David Higgs wrote:
>
> On Mar 3, 2015, at 8:44 AM, David Higgs <[email protected]> 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)
>
>
