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;
