On 31/03/15(Tue) 23:06, David Higgs wrote:
> This was much more straightforward than expected.
>
> - Replace an array with a LIST of allocated sensors.
> - Remove or rescope variables counting sensors.
> - Allocated sensors are always attached.
> - Drop an unnecessary size calculation.
Do you need this for an upcoming change? I fail to see the benefit to
have a malloc(9) call per item, afaik theses items are really small and
you have at most 10 of them...
> Thanks.
>
> --david
>
> --- a/upd.c
> +++ b/upd.c
> @@ -23,6 +23,7 @@
> #include <sys/kernel.h>
> #include <sys/malloc.h>
> #include <sys/device.h>
> +#include <sys/queue.h>
> #include <sys/sensors.h>
>
> #include <dev/usb/hid.h>
> @@ -77,20 +78,18 @@ struct upd_report {
> struct upd_sensor {
> struct ksensor ksensor;
> struct hid_item hitem;
> - int attached;
> + LIST_ENTRY(upd_sensor) next;
> };
>
> struct upd_softc {
> struct uhidev sc_hdev;
> - int sc_num_sensors;
> u_int sc_max_repid;
> - u_int sc_max_sensors;
>
> /* sensor framework */
> struct ksensordev sc_sensordev;
> struct sensor_task *sc_sensortask;
> struct upd_report *sc_reports;
> - struct upd_sensor *sc_sensors;
> + LIST_HEAD(, upd_sensor) sc_sensors;
> };
>
> int upd_match(struct device *, void *, void *);
> @@ -155,14 +154,14 @@ upd_attach(struct device *parent, struct
> struct hid_data *hdata;
> struct upd_usage_entry *entry;
> struct upd_sensor *sensor;
> + int num_sensors;
> int size;
> void *desc;
>
> sc->sc_hdev.sc_intr = upd_intr;
> sc->sc_hdev.sc_parent = uha->parent;
> sc->sc_reports = NULL;
> - sc->sc_sensors = NULL;
> - sc->sc_max_sensors = nitems(upd_usage_table);
> + LIST_INIT(&sc->sc_sensors);
>
> strlcpy(sc->sc_sensordev.xname, sc->sc_hdev.sc_dev.dv_xname,
> sizeof(sc->sc_sensordev.xname));
> @@ -173,14 +172,11 @@ upd_attach(struct device *parent, struct
>
> sc->sc_reports = mallocarray(sc->sc_max_repid,
> sizeof(struct upd_report), M_USBDEV, M_WAITOK | M_ZERO);
> - sc->sc_sensors = mallocarray(sc->sc_max_sensors,
> - sizeof(struct upd_sensor), M_USBDEV, M_WAITOK | M_ZERO);
> - size = sc->sc_max_sensors * sizeof(struct upd_sensor);
> - sc->sc_num_sensors = 0;
> + num_sensors = 0;
> uhidev_get_report_desc(uha->parent, &desc, &size);
> for (hdata = hid_start_parse(desc, size, hid_feature);
> hid_get_item(hdata, &item) &&
> - sc->sc_num_sensors < sc->sc_max_sensors; ) {
> + num_sensors < nitems(upd_usage_table); ) {
> DPRINTF(("upd: repid=%d\n", item.report_ID));
> if (item.kind != hid_feature ||
> item.report_ID < 0 ||
> @@ -196,7 +192,8 @@ upd_attach(struct device *parent, struct
> if (sensor != NULL)
> continue;
>
> - sensor = &sc->sc_sensors[sc->sc_num_sensors];
> + sensor = malloc(sizeof(struct upd_sensor), M_USBDEV,
> + M_WAITOK | M_ZERO);
> memcpy(&sensor->hitem, &item, sizeof(struct hid_item));
> strlcpy(sensor->ksensor.desc, entry->usage_name,
> sizeof(sensor->ksensor.desc));
> @@ -205,8 +202,8 @@ upd_attach(struct device *parent, struct
> sensor->ksensor.status = SENSOR_S_UNKNOWN;
> sensor->ksensor.value = 0;
> sensor_attach(&sc->sc_sensordev, &sensor->ksensor);
> - sensor->attached = 1;
> - sc->sc_num_sensors++;
> + LIST_INSERT_HEAD(&sc->sc_sensors, sensor, next);
> + num_sensors++;
>
> if (sc->sc_reports[item.report_ID].enabled)
> continue;
> @@ -216,7 +213,7 @@ upd_attach(struct device *parent, struct
> sc->sc_reports[item.report_ID].enabled = 1;
> }
> hid_end_parse(hdata);
> - DPRINTF(("upd: sc_num_sensors=%d\n", sc->sc_num_sensors));
> + DPRINTF(("upd: num_sensors=%d\n", num_sensors));
>
> sc->sc_sensortask = sensor_task_register(sc, upd_refresh, 6);
> if (sc->sc_sensortask == NULL) {
> @@ -235,7 +232,7 @@ upd_detach(struct device *self, int flag
> {
> struct upd_softc *sc = (struct upd_softc *)self;
> struct upd_sensor *sensor;
> - int i;
> + struct upd_sensor *t;
>
> if (sc->sc_sensortask != NULL) {
> wakeup(&sc->sc_sensortask);
> @@ -244,15 +241,14 @@ upd_detach(struct device *self, int flag
>
> sensordev_deinstall(&sc->sc_sensordev);
>
> - for (i = 0; i < sc->sc_num_sensors; i++) {
> - sensor = &sc->sc_sensors[i];
> - if (sensor->attached)
> - sensor_detach(&sc->sc_sensordev, &sensor->ksensor);
> + LIST_FOREACH_SAFE(sensor, &sc->sc_sensors, next, t) {
> + sensor_detach(&sc->sc_sensordev, &sensor->ksensor);
> DPRINTF(("upd_detach: %s\n", sensor->ksensor.desc));
> + LIST_REMOVE(sensor, next);
> + free(sensor, M_USBDEV, sizeof(struct upd_sensor));
> }
>
> free(sc->sc_reports, M_USBDEV, 0);
> - free(sc->sc_sensors, M_USBDEV, 0);
> DPRINTF(("upd_detach: complete\n"));
> return (0);
> }
> @@ -306,10 +302,8 @@ struct upd_sensor *
> upd_lookup_sensor(struct upd_softc *sc, int page, int usage)
> {
> struct upd_sensor *sensor = NULL;
> - int i;
>
> - for (i = 0; i < sc->sc_num_sensors; i++) {
> - sensor = &sc->sc_sensors[i];
> + LIST_FOREACH(sensor, &sc->sc_sensors, next) {
> if (page == HID_GET_USAGE_PAGE(sensor->hitem.usage) &&
> usage == HID_GET_USAGE(sensor->hitem.usage))
> return (sensor);
> @@ -324,14 +318,12 @@ upd_update_sensors(struct upd_softc *sc,
> struct upd_sensor *sensor;
> ulong hdata, batpres;
> ulong adjust;
> - int i;
>
> sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
> batpres = sensor ? sensor->ksensor.value : -1;
>
> - for (i = 0; i < sc->sc_num_sensors; i++) {
> - sensor = &sc->sc_sensors[i];
> - if (!(sensor->hitem.report_ID == repid && sensor->attached))
> + LIST_FOREACH(sensor, &sc->sc_sensors, next) {
> + if (sensor->hitem.report_ID != repid)
> continue;
>
> /* invalidate battery dependent sensors */
>
