Re: Async upd(4) - patch 7/7
On 2015-05-07 04:19, David Higgs wrote: On Apr 30, 2015, at 7:09 AM, Martin Pieuchot m...@openbsd.org wrote: [...] Your tweaks were good, so I tweaked it further: - When submit fails, invalidate affected sensors as described above. - When invalidating sensors, do it recursively. - When battery is not present, invalidate children but not BatteryPresent. Let me know what you think. Committed thanks!. I'm looking forward to see more supported sensors added to upd(4) :) M.
Re: Async upd(4) - patch 7/7
On Apr 30, 2015, at 7:09 AM, Martin Pieuchot m...@openbsd.org wrote:
On 24/04/15(Fri) 20:48, David Higgs wrote:
This is the final patch in the series.
Utilize the pending flags and report callback for their intended purpose -
to process async behavior.
Apply splusb() to ensure report callbacks can't fire before their data
structures have been properly updated. This only needs to happen in
upd_refresh(); all other calls to upd_request_children() are from a report
callback.
This is some good work. I don't think your patch #6 makes sense without
#7, so I merged them.
I don't think you need a pending flags for the sensors, since what you
are querying are the reports. You basically do not want to send to
requests for the same report at once.
I still disagree. However, the fix for this isn’t difficult and can be handled
in a future diff.
I'd also take the simpler approach of not deactivating all sensors if
you fail to *submit* a transfer. When such thing happens it's either
because the device/bus is going away or because we cannot allocate
memory.
Even in these situations, invalidating all affected sensors seems an easy and
consistent thing to do.
I tweaked your diff below, included some comments and your copyright.
This is untested but I hope you'll correct/improve it.
Your tweaks were good, so I tweaked it further:
- When submit fails, invalidate affected sensors as described above.
- When invalidating sensors, do it recursively.
- When battery is not present, invalidate children but not BatteryPresent.
Let me know what you think.
--david
--- a/upd.c
+++ b/upd.c
@@ -1,6 +1,7 @@
/* $OpenBSD: upd.c,v 1.19 2015/04/30 10:09:31 mpi Exp $ */
/*
+ * Copyright (c) 2015 David Higgs hig...@gmail.com
* Copyright (c) 2014 Andre de Oliveira an...@openbsd.org
*
* Permission to use, copy, modify, and distribute this software for any
@@ -78,25 +79,28 @@ static struct upd_usage_entry upd_usage_
};
#define UPD_MAX_SENSORS(nitems(upd_usage_batdep) +
nitems(upd_usage_roots))
+SLIST_HEAD(upd_sensor_head, upd_sensor);
+
struct upd_report {
- size_t size;
- SLIST_HEAD(, upd_sensor)sensors;
+ size_t size; /* Size of the report */
+ struct upd_sensor_head sensors;/* List in dependency order */
+ int pending;/* Waiting for an answer */
};
-SLIST_HEAD(upd_sensor_head, upd_sensor);
struct upd_sensor {
- struct ksensor ksensor;
- struct hid_item hitem;
- int attached;
- struct upd_sensor_head children;
- SLIST_ENTRY(upd_sensor) dep_next;
- SLIST_ENTRY(upd_sensor) rep_next;
+ struct ksensor ksensor;
+ struct hid_item hitem;
+ int attached; /* Is there a matching report */
+ struct upd_sensor_head children; /* list of children sensors */
+ SLIST_ENTRY(upd_sensor) dep_next; /* next in the child list */
+ SLIST_ENTRY(upd_sensor) rep_next; /* next in the report list */
};
struct upd_softc {
struct uhidevsc_hdev;
int sc_num_sensors;
u_intsc_max_repid;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -112,11 +116,13 @@ void upd_attach_sensor_tree(struct upd_s
struct upd_usage_entry *, struct upd_sensor_head *);
int upd_detach(struct device *, int);
-void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
-void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
-uint8_t *, int);
void upd_intr(struct uhidev *, void *, uint);
+void upd_refresh(void *);
+void upd_request_children(struct upd_softc *, struct upd_sensor_head *);
+void upd_update_report_cb(void *, int, void *, int);
+
+void upd_sensor_invalidate(struct upd_softc *, struct upd_sensor *);
+void upd_sensor_update(struct upd_softc *, struct upd_sensor *, uint8_t *,
int);
int upd_lookup_usage_entry(void *, int, struct upd_usage_entry *,
struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
@@ -126,10 +132,7 @@ struct cfdriver upd_cd = {
};
const struct cfattach upd_ca = {
- sizeof(struct upd_softc),
- upd_match,
- upd_attach,
- upd_detach
+ sizeof(struct upd_softc), upd_match, upd_attach, upd_detach
};
int
@@ -273,42 +276,50 @@ upd_detach(struct device *self, int flag
sensor = sc-sc_sensors[i];
if (sensor-attached)
sensor_detach(sc-sc_sensordev, sensor-ksensor);
- DPRINTF((upd_detach: %s\n, sensor-ksensor.desc));
}
free(sc-sc_reports, M_USBDEV, 0);
Re: Async upd(4) - patch 7/7
On 24/04/15(Fri) 20:48, David Higgs wrote:
This is the final patch in the series.
Utilize the pending flags and report callback for their intended purpose - to
process async behavior.
Apply splusb() to ensure report callbacks can't fire before their data
structures have been properly updated. This only needs to happen in
upd_refresh(); all other calls to upd_request_children() are from a report
callback.
This is some good work. I don't think your patch #6 makes sense without
#7, so I merged them.
I don't think you need a pending flags for the sensors, since what you
are querying are the reports. You basically do not want to send to
requests for the same report at once.
I'd also take the simpler approach of not deactivating all sensors if
you fail to *submit* a transfer. When such thing happens it's either
because the device/bus is going away or because we cannot allocate
memory.
I tweaked your diff below, included some comments and your copyright.
This is untested but I hope you'll correct/improve it.
I'm also curious, which new reports/sensors do you want to add?
Cheers,
Martin
Index: upd.c
===
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.19
diff -u -p -r1.19 upd.c
--- upd.c 30 Apr 2015 10:09:31 - 1.19
+++ upd.c 30 Apr 2015 11:02:04 -
@@ -1,6 +1,7 @@
/* $OpenBSD: upd.c,v 1.19 2015/04/30 10:09:31 mpi Exp $ */
/*
+ * Copyright (c) 2015 David Higgs hig...@gmail.com
* Copyright (c) 2014 Andre de Oliveira an...@openbsd.org
*
* Permission to use, copy, modify, and distribute this software for any
@@ -78,25 +79,28 @@ static struct upd_usage_entry upd_usage_
};
#define UPD_MAX_SENSORS(nitems(upd_usage_batdep) +
nitems(upd_usage_roots))
+SLIST_HEAD(upd_sensor_head, upd_sensor);
+
struct upd_report {
- size_t size;
- SLIST_HEAD(, upd_sensor)sensors;
+ size_t size; /* Size of the report */
+ struct upd_sensor_head sensors;/* List in dependency order */
+ int pending;/* Waiting for an answer */
};
-SLIST_HEAD(upd_sensor_head, upd_sensor);
struct upd_sensor {
- struct ksensor ksensor;
- struct hid_item hitem;
- int attached;
- struct upd_sensor_head children;
- SLIST_ENTRY(upd_sensor) dep_next;
- SLIST_ENTRY(upd_sensor) rep_next;
+ struct ksensor ksensor;
+ struct hid_item hitem;
+ int attached; /* Is there a matching report */
+ struct upd_sensor_head children; /* list of children sensors */
+ SLIST_ENTRY(upd_sensor) dep_next; /* next in the child list */
+ SLIST_ENTRY(upd_sensor) rep_next; /* next in the report list */
};
struct upd_softc {
struct uhidevsc_hdev;
int sc_num_sensors;
u_intsc_max_repid;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -112,11 +116,13 @@ void upd_attach_sensor_tree(struct upd_s
struct upd_usage_entry *, struct upd_sensor_head *);
int upd_detach(struct device *, int);
-void upd_refresh(void *);
-void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
-void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
-uint8_t *, int);
void upd_intr(struct uhidev *, void *, uint);
+void upd_refresh(void *);
+void upd_request_children(struct upd_softc *, struct upd_sensor_head *);
+void upd_update_report_cb(void *, int, void *, int);
+
+void upd_sensor_invalidate(struct upd_softc *, struct upd_sensor *);
+void upd_sensor_update(struct upd_softc *, struct upd_sensor *, uint8_t *,
int);
int upd_lookup_usage_entry(void *, int, struct upd_usage_entry *,
struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
@@ -126,10 +132,7 @@ struct cfdriver upd_cd = {
};
const struct cfattach upd_ca = {
- sizeof(struct upd_softc),
- upd_match,
- upd_attach,
- upd_detach
+ sizeof(struct upd_softc), upd_match, upd_attach, upd_detach
};
int
@@ -273,42 +276,51 @@ upd_detach(struct device *self, int flag
sensor = sc-sc_sensors[i];
if (sensor-attached)
sensor_detach(sc-sc_sensordev, sensor-ksensor);
- DPRINTF((upd_detach: %s\n, sensor-ksensor.desc));
}
free(sc-sc_reports, M_USBDEV, 0);
free(sc-sc_sensors, M_USBDEV, 0);
- DPRINTF((upd_detach: complete\n));
return (0);
}
void
upd_refresh(void *arg)
{
- struct upd_softc*sc = (struct upd_softc *)arg;
+ struct upd_softc*sc = arg;
+ int
Re: Async upd(4) - patch 7/7
On Thu, Apr 30, 2015 at 7:09 AM, Martin Pieuchot m...@openbsd.org wrote: On 24/04/15(Fri) 20:48, David Higgs wrote: This is the final patch in the series. Utilize the pending flags and report callback for their intended purpose - to process async behavior. Apply splusb() to ensure report callbacks can't fire before their data structures have been properly updated. This only needs to happen in upd_refresh(); all other calls to upd_request_children() are from a report callback. This is some good work. I don't think your patch #6 makes sense without #7, so I merged them. Sounds good, thanks for all the help. I don't think you need a pending flags for the sensors, since what you are querying are the reports. You basically do not want to send to requests for the same report at once. In general the code tries to avoid requesting reports more than once, but this prevents sensor dependencies from causing some nasty problems. In the example below, I don't want the update of C to re-update A and trigger an infinite loop by requesting B. Sensor A (report 1) - Sensor B (report 2) - Sensor C (report 1) The alternative is to allow inconsistent sensor values. If the value of B affects the value of C, I would rather perform a 2nd request for report 1 once I have the value of B, than to put garbage/bogus values into C. I prefer to keep sensor values consistent, to ensure nobody's sensorsd starts to panic and because I doubt many real devices have this kind of annoying HID descriptor layout. I'd also take the simpler approach of not deactivating all sensors if you fail to *submit* a transfer. When such thing happens it's either because the device/bus is going away or because we cannot allocate memory. This sounds fine for the bus/device case or if M_NOWAIT would trigger a panic. How about if the usb device itself runs out of memory/buffers; will the rest of the system stay happy? Again, I prefer to err on the side of consistency and invalidate child sensors so sensorsd isn't looking at stale values. I tweaked your diff below, included some comments and your copyright. This is untested but I hope you'll correct/improve it. I'll take a look over the next week. I'm also curious, which new reports/sensors do you want to add? I haven't looked at this in some months. My consumer-grade device had several useful sensors that aren't currently handled. There are some internal reports (CapacityMode, AtRate) that aren't suitable as sensors, but might fix other sensor values being mis-reported. And there are interactive things that can be done via reports - silencing the audible alarm, requesting self-tests - that would be good as sysctls or ioctls someday. I'll be happy to look at lsusb output if anyone has a device in mind. Thanks. --david
Async upd(4) - patch 7/7
This is the final patch in the series.
Utilize the pending flags and report callback for their intended purpose - to
process async behavior.
Apply splusb() to ensure report callbacks can't fire before their data
structures have been properly updated. This only needs to happen in
upd_refresh(); all other calls to upd_request_children() are from a report
callback.
--david
--- a/upd.c
+++ b/upd.c
@@ -99,6 +99,7 @@ struct upd_softc {
struct uhidevsc_hdev;
int sc_num_sensors;
u_intsc_max_repid;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -290,27 +291,12 @@ void
upd_refresh(void *arg)
{
struct upd_softc*sc = arg;
- struct upd_report *report;
- uint8_t buf[256];
- int repid, actlen, done;
+ int s;
- /* request root sensors */
+ /* request root sensors, do not let async handlers fire yet */
+ s = splusb();
upd_request_children(sc, sc-sc_root_sensors, 1);
-
- /* repeat until all reports queried */
- do {
- done = 1;
- for (repid = 0; repid sc-sc_max_repid; repid++) {
- report = sc-sc_reports[repid];
- if (!report-pending)
- continue;
- memset(buf, 0x0, sizeof(buf));
- actlen = uhidev_get_report(sc-sc_hdev.sc_parent,
- UHID_FEATURE_REPORT, repid, buf, report-size);
- upd_update_report_cb(sc, repid, buf, actlen);
- done = 0;
- }
- } while (!done);
+ splx(s);
}
void
@@ -336,7 +322,14 @@ upd_request_children(struct upd_softc *s
} else if (report-pending)
/* already requested */
sensor-pending = 1;
- else {
+ else if (uhidev_get_report_async(sc-sc_hdev.sc_parent,
+ UHID_FEATURE_REPORT, repid, sc-sc_buf,
+ report-size, sc, upd_update_report_cb) 0) {
+ DPRINTF((%s: %s request of repid %d failed\n,
+ DEVNAME(sc), sensor-ksensor.desc, repid));
+ sensor-pending = 1;
+ upd_update_report_cb(sc, repid, NULL, -1);
+ } else {
DPRINTF((%s: %s requests repid %d\n,
DEVNAME(sc), sensor-ksensor.desc, repid));
sensor-pending = 1;
Re: Async upd(4)
On Mon, Mar 9, 2015 at 6:04 AM, Martin Pieuchot mpieuc...@nolizard.org
wrote:
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.
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 =
Re: Async upd(4)
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,
-
Re: Async upd(4)
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 - 1.13
+++ upd.c 5 Mar 2015 17:06:19 -
@@ -39,45 +39,17 @@
#define DPRINTF(x)
#endif
-struct upd_usage_entry {
- uint8_t usage_pg;
- uint8_t usage_id;
- enum sensor_typesenstype;
- 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_intsc_max_repid;
u_intsc_max_sensors;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_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_typesenstype;
+ 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
Async upd(4)
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
Suggestions, bug reports, and feedback 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 - 1.13
+++ upd.c 20 Feb 2015 02:28:04 -
@@ -39,44 +39,15 @@
#define DPRINTF(x)
#endif
-struct upd_usage_entry {
- uint8_t usage_pg;
- uint8_t usage_id;
- enum sensor_typesenstype;
- 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;
};
@@ -85,6 +56,7 @@ struct upd_softc {
int sc_num_sensors;
u_intsc_max_repid;
u_intsc_max_sensors;
+ char sc_buf[256];
/* sensor framework */
struct ksensordevsc_sensordev;
@@ -93,6 +65,17 @@ struct upd_softc {
struct upd_sensor *sc_sensors;
};
+struct upd_usage_entry {
+ uint8_t usage_pg;
+ uint8_t usage_id;
+ uint8_t parent_pg;
+ uint8_t parent_id;
+ enum sensor_typesenstype;
+ 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);
@@ -100,8 +83,58 @@ 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_refresh_sensor_tree(struct upd_softc *, uint8_t, uint8_t, int);
+void upd_get_report_async_cb(void *, int, void *, int);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, 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);
+
+/* dependency order only for developer sanity */
+static struct upd_usage_entry upd_usage_table[] = {
+ { HUP_POWER,HUP_SHUTDOWN_IMMINENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, ShutdownImminent,
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_BATTERY_PRESENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, BatteryPresent,
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_AC_PRESENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, ACPresent,
+ upd_update_sensor_value },
+ { HUP_BATTERY,
