On Mon, Mar 9, 2015 at 6:04 AM, Martin Pieuchot <[email protected]> wrote:
> 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. This does seem simpler; I'll give it a shot. > > > > 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? Sounds good, and I'll see if it makes sense to do the same for the reports too. > > + 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? > Hasn't happened on my UPS yet. I thought that some action was better than nothing if the requests somehow became wedged, but this probably isn't the right behavior. > > + } > > + } > > > > - 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. Again, haven't even seen the one above. > > + 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(). Roger. > +#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 :) I still think it useful to call out buggy firmware as such, but I'll change it to DPRINTF(). Thanks for all the feedback. I'm also going to try to split it into discrete patches for easier reviewing/committing. Please let me know if you have any particular suggestions in this vein, like tools to use (e.g. quilt) or preferred commit order. Off-list is fine. --david
