Supporting 64-bit Digitizer Serial Numbers?

Hi folks,

I'm looking at being able to retrieve an accurate (not mangled or
truncated) Digitizer.Transducer Serial Number from user-space, primarily
through the power_supply node constructed for HID devices which expose
Battery Strength.

For the digitizers I'm looking at (USI) the Serial Number is a 64-bit
report field, making it not fit directly into evdev, or the hid-input
core.

I'm appending a proof-of-concept which implements this; I describe the
details below. (This is based on 5.4, but it looks clean for 5.10 as
well as relatively straightforward to backport.)

I'd appreciate any feedback on this approach.

My thought is to perform minimal modifications to allow hid-core to
transfer reports up to 64-bits in size to hid-input, so that it can
process this field and emit it to the power_supply integration, as well
as through classic MSC_SERIAL (for the low 32 bits) and a new
MSC_SERIAL2 (the upper bits) for applications which want it inline with
stylus event data.

hid_field.value is expanded to an s64 array from s32, and there are
a few knock-on effects to code that holds pointers to these fields.
No other interfaces, in-kernel, or user-space, are modified to support
HID reports/evdev events > 32 bits.

The Transducer Serial Number would show up as in the power_supply as a
serial_number of "DG:0123456789ABCDEF" (or however many hex digits are
warranted by the report size), and "DG:" being a fixed prefix -- we do
know it is a digitizer.

I realize this doesn't provide a scalable solution in the
hid-core/hid-input code for (hypothetical) fields larger than 64 bits,
although extending evdev with MSC_SERIAL3, 4, etc. would be trivial. (I
don't have a good sense of how common these larger fields are, although
obviously the code has survived until now with only 32-bit support, and
one comment saying that anything larger than 16-bit is unusual.)

78 more lines

diff --git a/Documentation/hid/hiddev.rst b/Documentation/hid/hiddev.rst
index 209e6ba4e019e..06eee00847e50 100644
--- a/Documentation/hid/hiddev.rst
+++ b/Documentation/hid/hiddev.rst

@@ -72,8 +72,10 @@ The hiddev API uses a read() interface, and a set

of ioctl() calls.

 HID devices exchange data with the host computer using data
 bundles called "reports".  Each report is divided into "fields",
-each of which can have one or more "usages".  In the hid-core,
-each one of these usages has a single signed 32 bit value.
+each of which can have one or more "usages". Each of these usages
+is a single value, usually a signed 32 bit value, which is what
+the hiddev API supports. (hid-core can process 64 bit values, but
+these are not currently exposed through hiddev.)

 read():
 -------

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1489f49dc53d0..5f4c7f06d0e96 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c

@@ -101,14 +101,14 @@ static struct hid_field

*hid_register_field(struct hid_report *report, unsigned

  field = kzalloc((sizeof(struct hid_field) +
  usages * sizeof(struct hid_usage) +
- usages * sizeof(unsigned)), GFP_KERNEL);
+ usages * sizeof(s64)), GFP_KERNEL);
  if (!field)
  return NULL;

  field->index = report->maxfield++;
  report->field[field->index] = field;
  field->usage = (struct hid_usage *)(field + 1);
- field->value = (s32 *)(field->usage + usages);
+ field->value = (s64 *)(field->usage + usages);
  field->report = report;

  return field;

@@ -337,7 +337,8 @@ static int hid_add_field(struct hid_parser

*parser, unsigned report_type, unsign
 }

 /*
- * Read data value from item.
+ * Read data value from global items, which are
+ * a maximum of 32 bits in size.
  */

 static u32 item_udata(struct hid_item *item)

@@ -763,7 +764,7 @@ static u8 *fetch_item(__u8 *start, __u8 *end,

struct hid_item *item)
  return start;

  case 3:
- item->size++;
+ item->size = 4;
  if ((end - start) < 4)
  return NULL;
  item->data.u32 = get_unaligned_le32(start);

@@ -1016,7 +1017,7 @@ static int hid_calculate_multiplier(struct

hid_device *hid,
      struct hid_field *multiplier)
 {
  int m;
- __s32 v = *multiplier->value;
+ __s64 v = *multiplier->value;
  __s32 lmin = multiplier->logical_minimum;
  __s32 lmax = multiplier->logical_maximum;
  __s32 pmin = multiplier->physical_minimum;

@@ -1299,27 +1300,17 @@ int hid_open_report(struct hid_device *device)
 }
 EXPORT_SYMBOL_GPL(hid_open_report);

-/*
- * Convert a signed n-bit integer to signed 32-bit integer. Common
- * cases are done through the compiler, the screwed things has to be
- * done by hand.
- */
-
-static s32 snto32(__u32 value, unsigned n)
+s32 hid_snto32(__u32 value, unsigned n)
 {
- switch (n) {
- case 8:  return ((__s8)value);
- case 16: return ((__s16)value);
- case 32: return ((__s32)value);
- }
- return value & (1 << (n - 1)) ? value | (~0U << n) : value;
+ return sign_extend32(value, n);
 }
+EXPORT_SYMBOL_GPL(hid_snto32);

-s32 hid_snto32(__u32 value, unsigned n)
+s64 hid_snto64(__u64 value, unsigned n)
 {
- return snto32(value, n);
+ return sign_extend64(value, n);
 }
-EXPORT_SYMBOL_GPL(hid_snto32);
+EXPORT_SYMBOL_GPL(hid_snto64);

 /*
  * Convert a signed 32-bit integer to a signed n-bit integer.

@@ -1342,20 +1333,21 @@ static u32 s32ton(__s32 value, unsigned n)
  * While the USB HID spec allows unlimited length bit fields in "report
  * descriptors", most devices never use more than 16 bits.
  * One model of UPS is claimed to report "LINEV" as a 32-bit field.
+ * Some digitizers report stylus transducer IDs as 64-bit fields.
  * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
  */

-static u32 __extract(u8 *report, unsigned offset, int n)
+static u64 __extract(u8 *report, unsigned offset, int n)
 {
  unsigned int idx = offset / 8;
  unsigned int bit_nr = 0;
  unsigned int bit_shift = offset % 8;
  int bits_to_copy = 8 - bit_shift;
- u32 value = 0;
- u32 mask = n < 32 ? (1U << n) - 1 : ~0U;
+ u64 value = 0;
+ u64 mask = n < 64 ? (1ULL << n) - 1 : ~0ULL;

  while (n > 0) {
- value |= ((u32)report[idx] >> bit_shift) << bit_nr;
+ value |= ((u64)report[idx] >> bit_shift) << bit_nr;
  n -= bits_to_copy;
  bit_nr += bits_to_copy;
  bits_to_copy = 8;

@@ -1366,6 +1358,10 @@ static u32 __extract(u8 *report, unsigned offset, int n)
  return value & mask;
 }

+/*
+ * Classic HID code assumes 32-bit truncation for fields. Provided
+ * for compatibility and consistency of warnings.
+ */
 u32 hid_field_extract(const struct hid_device *hid, u8 *report,
  unsigned offset, unsigned n)
 {

@@ -1379,6 +1375,22 @@ u32 hid_field_extract(const struct hid_device

*hid, u8 *report,
 }
 EXPORT_SYMBOL_GPL(hid_field_extract);

+/*
+ * Extract larger, 64-bit fields.
+ */
+u64 hid_field_extract64(const struct hid_device *hid, u8 *report,
+ unsigned offset, unsigned n)
+{
+ if (n > 64) {
+ hid_warn_once(hid, "%s() called with n (%d) > 64! (%s)\n",
+      __func__, n, current->comm);
+ n = 64;
+ }
+
+ return __extract(report, offset, n);
+}
+EXPORT_SYMBOL_GPL(hid_field_extract64);
+
 /*
  * "implement" : set bits in a little endian bit stream.
  * Same concepts as "extract" (see comments above).

@@ -1438,7 +1450,7 @@ static void implement(const struct hid_device

*hid, u8 *report,
  * Search an array for a value.
  */

-static int search(__s32 *array, __s32 value, unsigned n)
+static int search(__s64 *array, __s64 value, unsigned n)
 {
  while (n--) {
  if (*array++ == value)

@@ -1497,7 +1509,7 @@ static int hid_match_usage(struct hid_device

*hid, struct hid_usage *usage)
 }

 static void hid_process_event(struct hid_device *hid, struct hid_field *field,
- struct hid_usage *usage, __s32 value, int interrupt)
+ struct hid_usage *usage, __s64 value, int interrupt)
 {
  struct hid_driver *hdrv = hid->driver;
  int ret;

@@ -1506,7 +1518,7 @@ static void hid_process_event(struct hid_device

*hid, struct hid_field *field,
  hid_dump_input(hid, usage, value);

  if (hdrv && hdrv->event && hid_match_usage(hid, usage)) {
- ret = hdrv->event(hid, field, usage, value);
+ ret = hdrv->event(hid, field, usage, (__s32)value);
  if (ret != 0) {
  if (ret < 0)
  hid_err(hid, "%s's event failed with %d\n",

@@ -1536,18 +1548,18 @@ static void hid_input_field(struct hid_device

*hid, struct hid_field *field,
  unsigned size = field->report_size;
  __s32 min = field->logical_minimum;
  __s32 max = field->logical_maximum;
- __s32 *value;
+ __s64 *value;

- value = kmalloc_array(count, sizeof(__s32), GFP_ATOMIC);
+ value = kmalloc_array(count, sizeof(__s64), GFP_ATOMIC);
  if (!value)
  return;

  for (n = 0; n < count; n++) {

  value[n] = min < 0 ?
- snto32(hid_field_extract(hid, data, offset + n * size,
+ sign_extend64(hid_field_extract64(hid, data, offset + n * size,
        size), size) :
- hid_field_extract(hid, data, offset + n * size, size);
+ hid_field_extract64(hid, data, offset + n * size, size);

  /* Ignore report if ErrorRollOver */
  if (!(field->flags & HID_MAIN_ITEM_VARIABLE) &&

@@ -1577,7 +1589,7 @@ static void hid_input_field(struct hid_device

*hid, struct hid_field *field,
  hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
  }

- memcpy(field->value, value, count * sizeof(__s32));
+ memcpy(field->value, value, count * sizeof(__s64));
 exit:
  kfree(value);
 }

@@ -1672,7 +1684,7 @@ int hid_set_field(struct hid_field *field,

unsigned offset, __s32 value)
  return -1;
  }
  if (field->logical_minimum < 0) {
- if (value != snto32(s32ton(value, size), size)) {
+ if (value != hid_snto32(s32ton(value, size), size)) {
  hid_err(field->report->device, "value %d is out of range\n", value);
  return -1;
  }

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 9453147d020db..badd51a8ea4ba 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c

@@ -692,7 +692,7 @@ void hid_dump_report(struct hid_device *hid, int

type, u8 *data,
 }
 EXPORT_SYMBOL_GPL(hid_dump_report);

-void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage,
__s32 value)
+void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage,
__s64 value)
 {
  char *buf;
  int len;

@@ -701,7 +701,7 @@ void hid_dump_input(struct hid_device *hdev,

struct hid_usage *usage, __s32 valu
  if (!buf)
  return;
  len = strlen(buf);
- snprintf(buf + len, HID_DEBUG_BUFSIZE - len - 1, " = %d\n", value);
+ snprintf(buf + len, HID_DEBUG_BUFSIZE - len - 1, " = %lld\n", value);

  hid_debug_event(hdev, buf);

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index bceccd75b488e..87ade0d6963cf 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c

@@ -52,6 +52,7 @@ static const struct {
 #define map_rel(c) hid_map_usage(hidinput, usage, &bit, &max, EV_REL, (c))
 #define map_key(c) hid_map_usage(hidinput, usage, &bit, &max, EV_KEY, (c))
 #define map_led(c) hid_map_usage(hidinput, usage, &bit, &max, EV_LED, (c))
+#define map_msc(c) hid_map_usage(hidinput, usage, &bit, &max, EV_MSC, (c))

 #define map_abs_clear(c) hid_map_usage_clear(hidinput, usage, &bit, \
  &max, EV_ABS, (c))

@@ -286,6 +287,7 @@ static enum power_supply_property

hidinput_battery_props[] = {
  POWER_SUPPLY_PROP_ONLINE,
  POWER_SUPPLY_PROP_CAPACITY,
  POWER_SUPPLY_PROP_MODEL_NAME,
+ POWER_SUPPLY_PROP_SERIAL_NUMBER,
  POWER_SUPPLY_PROP_STATUS,
  POWER_SUPPLY_PROP_SCOPE,
 };

@@ -402,6 +404,22 @@ static int hidinput_get_battery_property(struct

power_supply *psy,
  val->strval = dev->name;
  break;

+ case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+ /* Serial number does not have an active HID query
+ * mechanism like hidinput_query_battery_capacity, as the
+ * only devices expected to have serial numbers are digitizers,
+ * which are unlikely to be able to pull the serial number from
+ * an untethered pen on demand.
+ */
+ if (dev->battery_serial_number == 0) {
+ /* Make no claims about S/N format if we haven't actually seen a value yet. */
+ strcpy(dev->battery_serial_number_str, "");
+ } else {
+ snprintf(dev->battery_serial_number_str,
sizeof(dev->battery_serial_number_str), "DG:%0*llX",
(dev->battery_serial_number_bits+3)/4, dev->battery_serial_number);
+ }
+ val->strval = dev->battery_serial_number_str;
+ break;
+
  case POWER_SUPPLY_PROP_STATUS:
  if (dev->battery_status != HID_BATTERY_REPORTED &&
     !dev->battery_avoid_query) {

@@ -485,6 +503,8 @@ static int hidinput_setup_battery(struct

hid_device *dev, unsigned report_type,
  dev->battery_max = max;
  dev->battery_report_type = report_type;
  dev->battery_report_id = field->report->id;
+ dev->battery_changed = false;
+ dev->battery_reported = false;

  /*
  * Stylus is normally not connected to the device and thus we

@@ -526,7 +546,7 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
  dev->battery = NULL;
 }

-static void hidinput_update_battery(struct hid_device *dev, int value)
+static void hidinput_update_battery_capacity(struct hid_device *dev,

__s32 value)
 {
  int capacity;

@@ -538,11 +558,47 @@ static void hidinput_update_battery(struct

hid_device *dev, int value)

  capacity = hidinput_scale_battery_capacity(dev, value);

+ if (capacity != dev->battery_capacity) {
+ dev->battery_capacity = capacity;
+ dev->battery_changed = true;
+ }
+ dev->battery_reported = true;
+}
+
+static void hidinput_update_battery_serial(struct hid_device *dev,
__u64 value, int bits)
+{
+ if (!dev->battery)
+ return;
+
+ if (value == 0)
+ return;
+
+ if (value != dev->battery_serial_number) {
+ dev->battery_serial_number = value;
+ dev->battery_serial_number_bits = bits;
+ dev->battery_changed = true;
+ }
+ dev->battery_reported = true;
+}
+
+static void hidinput_flush_battery(struct hid_device *dev)
+{
+ if (!dev->battery)
+ return;
+
+ /* Only consider pushing a battery change if there is a
+ * battery field in this report.
+ */
+ if (!dev->battery_reported)
+ return;
+
+ dev->battery_reported = false;

11 more lines

@@ -559,7 +615,15 @@ static void hidinput_cleanup_battery(struct

hid_device *dev)
 {
 }

-static void hidinput_update_battery(struct hid_device *dev, int value)
+static void hidinput_update_battery_capacity(struct hid_device *dev,
__s32 value)
+{
+}
+
+static void hidinput_update_battery_serial(struct hid_device *dev,
__u64 value, int bits)
+{
+}
+
+static void hidinput_flush_battery(struct hid_device *dev)
 {
 }
 #endif /* CONFIG_HID_BATTERY_STRENGTH */

@@ -850,6 +914,13 @@ static void hidinput_configure_usage(struct

hid_input *hidinput, struct hid_fiel
  usage->code = MSC_SERIAL;
  bit = input->mscbit;
  max = MSC_MAX;
+ if (field->report_size > 32) {
+ /* Deliver up to 64 bits of TransducerSerialNumber via
+ * MSC_SERIAL and MSC_SERIAL2.
+ */
+ set_bit(MSC_SERIAL2, input->mscbit);
+ set_bit(EV_MSC, input->evbit);
+ }
  break;

  default:  goto unknown;

@@ -1243,7 +1314,7 @@ static void hidinput_configure_usage(struct

hid_input *hidinput, struct hid_fiel

 static void hidinput_handle_scroll(struct hid_usage *usage,
    struct input_dev *input,
-   __s32 value)
+   __s64 value)
 {
  int code;
  int hi_res, lo_res;

@@ -1273,8 +1344,9 @@ static void hidinput_handle_scroll(struct

hid_usage *usage,
  input_event(input, EV_REL, usage->code, hi_res);
 }

-void hidinput_hid_event(struct hid_device *hid, struct hid_field
*field, struct hid_usage *usage, __s32 value)
+void hidinput_hid_event(struct hid_device *hid, struct hid_field
*field, struct hid_usage *usage, __s64 value64)
 {
+ __s32 value = value64;
  struct input_dev *input;
  unsigned *quirks = &hid->quirks;

@@ -1282,15 +1354,28 @@ void hidinput_hid_event(struct hid_device

*hid, struct hid_field *field, struct
  return;

  if (usage->type == EV_PWR) {
- hidinput_update_battery(hid, value);
+ hidinput_update_battery_capacity(hid, value);
  return;
  }
+ if (usage->type == EV_MSC && usage->code == MSC_SERIAL) {
+ hidinput_update_battery_serial(hid, value64, field->report_size);
+ }

  if (!field->hidinput)
  return;

  input = field->hidinput->input;

+ if (usage->type == EV_MSC && usage->code == MSC_SERIAL &&
field->report_size > 32) {
+ /* min/max are limited to 32-bits by spec and cannot be relevant if field is
+ * this large, ignore them and send 64 bits through. We leave fields <= 32 bits
+ * to be processed later with normal clamping, for compatibility.
+ */
+ input_event(input, usage->type, MSC_SERIAL, (__u32)value64);
+ input_event(input, usage->type, MSC_SERIAL2, value64 >> 32);
+ return;
+ }
+
  if (usage->hat_min < usage->hat_max || usage->hat_dir) {
  int hat_dir = usage->hat_dir;
  if (!hat_dir)

@@ -1415,6 +1500,8 @@ void hidinput_report_event(struct hid_device

*hid, struct hid_report *report)
 {
  struct hid_input *hidinput;

+ hidinput_flush_battery(hid);
+
  if (hid->quirks & HID_QUIRK_NO_INPUT_SYNC)
  return;

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index d91e6679afb18..23868a2e31187 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -95,11 +95,11 @@ static const __s32 mzero; /* default for 0 */

 struct mt_usages {
  struct list_head list;
- __s32 *x, *y, *cx, *cy, *p, *w, *h, *a;
- __s32 *contactid; /* the device ContactID assigned to this slot */
- bool *tip_state; /* is the touch valid? */
- bool *inrange_state; /* is the finger in proximity of the sensor? */
- bool *confidence_state; /* is the touch made by a finger? */
+ __s64 *x, *y, *cx, *cy, *p, *w, *h, *a;
+ __s64 *contactid; /* the device ContactID assigned to this slot */
+ __s64 *tip_state; /* is the touch valid? */
+ __s64 *inrange_state; /* is the finger in proximity of the sensor? */
+ __s64 *confidence_state; /* is the touch made by a finger? */
 };

 struct mt_application {

@@ -110,10 +110,10 @@ struct mt_application {

  __s32 quirks;

- __s32 *scantime; /* scantime reported */
+ __s64 *scantime; /* scantime reported */
  __s32 scantime_logical_max; /* max value for raw scantime */

- __s32 *raw_cc; /* contact count in the report */
+ __s64 *raw_cc; /* contact count in the report */
  int left_button_state; /* left button state */
  unsigned int mt_flags; /* flags to pass to input-mt */

@@ -642,11 +642,11 @@ static struct mt_report_data

*mt_find_report_data(struct mt_device *td,

 static void mt_store_field(struct hid_device *hdev,
    struct mt_application *application,
-   __s32 *value,
+   __s64 *value,
    size_t offset)
 {
  struct mt_usages *usage;
- __s32 **target;
+ __s64 **target;

  if (list_empty(&application->mt_usages))
  usage = mt_allocate_usage(hdev, application);

@@ -658,7 +658,7 @@ static void mt_store_field(struct hid_device *hdev,
  if (!usage)
  return;

- target = (__s32 **)((char *)usage + offset);
+ target = (__s64 **)((char *)usage + offset);

  /* the value has already been filled, create a new slot */
  if (*target != DEFAULT_TRUE &&

@@ -675,7 +675,7 @@ static void mt_store_field(struct hid_device *hdev,
  if (!usage)
  return;

- target = (__s32 **)((char *)usage + offset);
+ target = (__s64 **)((char *)usage + offset);
  }

  *target = value;

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 2f073f5360706..6501f33dbc725 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c

@@ -1827,7 +1827,7 @@ static void buzz_set_leds(struct sony_sc *sc)
  &hdev->report_enum[HID_OUTPUT_REPORT].report_list;
  struct hid_report *report = list_entry(report_list->next,
  struct hid_report, list);
- s32 *value = report->field[0]->value;
+ s64 *value = report->field[0]->value;

  BUILD_BUG_ON(MAX_LEDS < 4);

diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index fddac7c72f645..a8fb81cccb981 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c

@@ -138,7 +138,7 @@ static const u8 pidff_effect_operation_status[] =

{ 0x79, 0x7b };

 struct pidff_usage {
  struct hid_field *field;
- s32 *value;
+ s64 *value;
 };

 struct pidff_device {

diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
index ea7b23d13bfdf..280de6b7904e7 100644
--- a/include/linux/hid-debug.h
+++ b/include/linux/hid-debug.h

@@ -16,7 +16,7 @@
 #define HID_DEBUG_BUFSIZE 512
 #define HID_DEBUG_FIFOSIZE 512

-void hid_dump_input(struct hid_device *, struct hid_usage *, __s32);
+void hid_dump_input(struct hid_device *, struct hid_usage *, __s64);
 void hid_dump_report(struct hid_device *, int , u8 *, int);
 void hid_dump_device(struct hid_device *, struct seq_file *);
 void hid_dump_field(struct hid_field *, int, struct seq_file *);

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 58e635bf4ed09..f0520417a91d5 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -389,6 +389,7 @@ struct hid_item {

 struct hid_global {
  unsigned usage_page;
+ /* HID Global fields are constrained by spec to 32-bits */
  __s32    logical_minimum;
  __s32    logical_maximum;
  __s32    physical_minimum;

@@ -458,7 +459,7 @@ struct hid_field {
  unsigned  report_size; /* size of this field in the report */
  unsigned  report_count; /* number of this field in the report */
  unsigned  report_type; /* (input,output,feature) */
- __s32    *value; /* last known value(s) */
+ __s64    *value; /* last known value(s); 64-bit values are the

largest we process */
  __s32     logical_minimum;
  __s32     logical_maximum;
  __s32     physical_minimum;

@@ -584,8 +585,13 @@ struct hid_device { /* device report descriptor */
  __s32 battery_max;
  __s32 battery_report_type;
  __s32 battery_report_id;
+ __u64 battery_serial_number;
+ int battery_serial_number_bits; /* Actual number of digits in SN */
+ char battery_serial_number_str[20]; /* Space for "DG:" + max 16 hex digits */
  enum hid_battery_status battery_status;
  bool battery_avoid_query;
+ bool battery_changed;
+ bool battery_reported;
  ktime_t battery_ratelimit_time;
 #endif

@@ -875,7 +881,7 @@ extern void hid_unregister_driver(struct hid_driver *);
  module_driver(__hid_driver, hid_register_driver, \
       hid_unregister_driver)

-extern void hidinput_hid_event(struct hid_device *, struct hid_field

*, struct hid_usage *, __s32);
+extern void hidinput_hid_event(struct hid_device *, struct hid_field
*, struct hid_usage *, __s64);
 extern void hidinput_report_event(struct hid_device *hid, struct
hid_report *report);
 extern int hidinput_connect(struct hid_device *hid, unsigned int force);
 extern void hidinput_disconnect(struct hid_device *);

@@ -913,8 +919,11 @@ const struct hid_device_id

*hid_match_device(struct hid_device *hdev,
 bool hid_compare_device_paths(struct hid_device *hdev_a,
       struct hid_device *hdev_b, char separator);
 s32 hid_snto32(__u32 value, unsigned n);
+s64 hid_snto64(__u64 value, unsigned n);
 __u32 hid_field_extract(const struct hid_device *hid, __u8 *report,
      unsigned offset, unsigned n);
+__u64 hid_field_extract64(const struct hid_device *hid, __u8 *report,
+     unsigned offset, unsigned n);

 /**
  * hid_device_io_start - enable HID input during probe, remove

diff --git a/include/uapi/linux/input-event-codes.h

b/include/uapi/linux/input-event-codes.h
index 472cd5bc55676..612ca91a87326 100644

--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h

@@ -816,12 +816,13 @@
  * Misc events
  */

-#define MSC_SERIAL 0x00
+#define MSC_SERIAL 0x00 /* First 32-bits of serial number */
 #define MSC_PULSELED 0x01
 #define MSC_GESTURE 0x02
 #define MSC_RAW 0x03
 #define MSC_SCAN 0x04
 #define MSC_TIMESTAMP 0x05
+#define MSC_SERIAL2 0x06 /* Second 32-bits of serial number */
 #define MSC_MAX 0x07
 #define MSC_CNT (MSC_MAX+1)

-- 
2.29.2

Hi Kenneth,

On Wed, Mar 24, 2021 at 8:53 PM Kenneth Albanowski [off-list ref] wrote:

Hi folks,

I'm looking at being able to retrieve an accurate (not mangled or
truncated) Digitizer.Transducer Serial Number from user-space, primarily
through the power_supply node constructed for HID devices which expose
Battery Strength.

For the digitizers I'm looking at (USI) the Serial Number is a 64-bit
report field, making it not fit directly into evdev, or the hid-input
core.

Thanks for raising that issue. Before I jump into commenting on your
PoC, I'd like to ask you to provide the report descriptor of this
particular device (ideally with hid-recorder from the hid-tolls repo
below).
AFAICT, the HID spec we currently implement doesn't allow for data of
more than 32 bits (section 6.2.2.2 of
https://www.usb.org/sites/default/files/hid1_11.pdf).
The main problem is that a field is usually associated with a logical
max, and this part makes us limit the field up to 32bits.

There is a "long items" section right after (6.2.2.3) that allows for
longer data, and I don't think the current implementation supports it.

One other thing: this whole problem is very much a good candidate for
adding regression tests on
https://gitlab.freedesktop.org/libevdev/hid-tools.

So I'd be glad if we can work around the problem in the kernel *and*
add the matching test in hid-tools so we don't break this in the
future.

More comments inlined...

I'm appending a proof-of-concept which implements this; I describe the
details below. (This is based on 5.4, but it looks clean for 5.10 as
well as relatively straightforward to backport.)

I'd appreciate any feedback on this approach.

My thought is to perform minimal modifications to allow hid-core to
transfer reports up to 64-bits in size to hid-input, so that it can
process this field and emit it to the power_supply integration, as well
as through classic MSC_SERIAL (for the low 32 bits) and a new
MSC_SERIAL2 (the upper bits) for applications which want it inline with
stylus event data.

The wacom driver already has this problem and handles it in a creative
way (as usual ;-P)
If I am not mistaken, it splits the 64 bits serial on 2 events as
well: MSC_SERIAL and ABS_MISC (Jason might correct me here).

I am not saying this is what needs to be done, but just what some
userspace tools already expect.

hid_field.value is expanded to an s64 array from s32, and there are
a few knock-on effects to code that holds pointers to these fields.
No other interfaces, in-kernel, or user-space, are modified to support
HID reports/evdev events > 32 bits.

I also think both problems are orthogonal (which is why you haven't
tried to change the evdev protocol).

I don't think we can do much on the evdev side to support 64 bits or
bigger data.
Dmitry and Peter might find a good way to work around that, but I'm OK
with adding MSC_SERIAL_2 from my point of view.

The Transducer Serial Number would show up as in the power_supply as a
serial_number of "DG:0123456789ABCDEF" (or however many hex digits are
warranted by the report size), and "DG:" being a fixed prefix -- we do
know it is a digitizer.

I realize this doesn't provide a scalable solution in the
hid-core/hid-input code for (hypothetical) fields larger than 64 bits,
although extending evdev with MSC_SERIAL3, 4, etc. would be trivial. (I
don't have a good sense of how common these larger fields are, although
obviously the code has survived until now with only 32-bit support, and
one comment saying that anything larger than 16-bit is unusual.)

I would prefer having a generic solution for items longer than 32 bits
instead of having a special case for 64 bits only.

The spec allows for items longer than 32 bits (still section 6.2.2.3
of the HID spec) so we should probably handle that case in a generic
way to not have to deal with 128 bits next time a creative device
maker wants to use it.
The nice thing about the hid-tools test suite is that we can be
creative and produce this 128 bits new device, and have the tests for
it already :)

(The other approach I was looking at was leaving hid_field.value
unchanged, and instead just having hid-core recognize this particular
64 bit field, edit the hid_usage to turn it into a 2-count 32-bit field,
apply a unique usage code to the second half, and then have hid-input
recognize these usages and paste everything back together. This is
scalable to larger fields, but is putting an unprecedented amount of
magic in hid-core.)

Not really. There is already a lot of magic in hid-core, and handling
longer-than-32bits doesn't seem that far away. The report descriptor
should give us all the information we need, so we can surely have a
special case for long items and data. Also hid-input doesn't have to
be tied to evdev. So nothing prevents us to mark the field as being a
long one, and change the internal representation of the various 32
bits chunks of its data.

The other change is MSC_SERIAL2 would be added: in the specific case of
Usage(Digitizer.Transducer Serial Number) being present with
report_size > 32 then MSC_SERIAL is reliably the full lowest 32 bits,
and MSC_SERIAL2 is the next set of 32 bits; no clamping or scaling
applied, regardless of what the descriptor says. Both MSC_SERIAL* are
emitted for every hid record, there is no caching. If report_size <=
32, then there is no difference to existing behaviour, only MSC_SERIAL
is emitted. I'm assuming the cost of one additional event per sync, for
stylus data, is negligible.

I am not in the best position to comment, but as mentioned previously,
it works for me.

This does potentially alter the generation of MSC_SERIAL on existing
devices, but only ones where the report_size > 32 bits and the min/max
is not 0/~0 -- user space would see unclamped values and conceivably
could be surprised with a larger value in MSC_SERIAL than previously
seen.

Honestly it shouldn't introduce any regressions. I still have to work
on the tablet side of the regression tests, but so far only Wacom was
reliably sending serial numbers (and bigger than 32 bits). So I am not
worried from this side of things.

If changing MSC_SERIAL is deemed too risky for compatibility, then it's
easy to just leave MSC_SERIAL as-is, and put in a separate MSC_SERIAL1
(MSC_SERIAL0?) that has the pristine low bits -- just at a cost of yet
one more MSC_ field per report, and using up the last MSC_ bit before
we need to bump MSC_MAX.

This approach doesn't affect Wacom evdev events at all, all of that
logic is separate from hid-input.

There is a whole ecosystem for wacom, but we are pretty much in
control of all the pieces. So I would prefer involving wacom from the
beginning, and have a common representation to the user space.

Besides that, I skimmed through the  patch below. I do have comments,
but in addition to the above, they are mostly "hey this hunk should be
in its own separate patch". But I guess the final submission will have
this separation, so there is no real point in going too deep in the
details IMO.

Cheers,
Benjamin

Kind regards and good health,
- Kenneth Albanowski

From 9948f77d758c5688d5ac5dee8f7218aa36cbd20d Mon Sep 17 00:00:00 2001
From: Kenneth Albanowski <redacted>
Date: Mon, 22 Mar 2021 11:40:13 -0700
Subject: [PATCH] input: limited 64-bit usages; MSC_SERIAL2,
 POWER_SUPPLY_SERIAL_NUMBER

Extend hid-core to process 64-bit usages, updating hid-input
to recognize digitizer transducer serial numbers up to 64 bits,
emitting MSC_SERIAL and MSC_SERIAL2 (new) for second 32-bit word.

64-bit usages are only fed to hid-input, all other existing
frameworks and interfaces only see the traditional 32-bit
truncated values; the majority of hid-input still only uses
traditional 32-bit clamped values.

Behaviour is changed only for HID reports that have
Usage(Digitizer.TransducerSerialNumber) where size > 32 bits;
logical min/max are no longer used to clamp the value, so
MSC_SERIAL may be different from prior logic and now emits
entire lowest 32-bits, instead of clamped value. Upper bits
(32-64) are emitted to MSC_SERIAL2, also unclamped.

power_supply integration extended to emit
Digitizer.TransducerSerialNumber as POWER_SUPPLY_SERIAL_NUMBER
in format: "DG:0123456789ABCDEF", with appropriate number of
digits for actual field length.

Change-Id: Ic8f9c90081f324ff76f34b0906bca1fcd5eac06a
---
 Documentation/hid/hiddev.rst           |   6 +-
 drivers/hid/hid-core.c                 |  80 +++++++++++---------
 drivers/hid/hid-debug.c                |   4 +-
 drivers/hid/hid-input.c                | 101 +++++++++++++++++++++++--
 drivers/hid/hid-multitouch.c           |  22 +++---
 drivers/hid/hid-sony.c                 |   2 +-
 drivers/hid/usbhid/hid-pidff.c         |   2 +-

682 more quoted lines

On Thu, Mar 25, 2021 at 09:35:33AM +0100, Benjamin Tissoires wrote:
[...]

quoted

I'm appending a proof-of-concept which implements this; I describe the
details below. (This is based on 5.4, but it looks clean for 5.10 as
well as relatively straightforward to backport.)

I'd appreciate any feedback on this approach.

My thought is to perform minimal modifications to allow hid-core to
transfer reports up to 64-bits in size to hid-input, so that it can
process this field and emit it to the power_supply integration, as well
as through classic MSC_SERIAL (for the low 32 bits) and a new
MSC_SERIAL2 (the upper bits) for applications which want it inline with
stylus event data.

The wacom driver already has this problem and handles it in a creative
way (as usual ;-P)
If I am not mistaken, it splits the 64 bits serial on 2 events as
well: MSC_SERIAL and ABS_MISC (Jason might correct me here).

ABS_MISC is the tool ID, MSC_SERIAL is the 32-bit serial. Quick check
suggests the upper bits of the 64-bit serial number are simply dropped.
So afaict this situation is still officially unprecedented :)
 

I am not saying this is what needs to be done, but just what some
userspace tools already expect.

quoted

hid_field.value is expanded to an s64 array from s32, and there are
a few knock-on effects to code that holds pointers to these fields.
No other interfaces, in-kernel, or user-space, are modified to support
HID reports/evdev events > 32 bits.

I also think both problems are orthogonal (which is why you haven't
tried to change the evdev protocol).

I don't think we can do much on the evdev side to support 64 bits or
bigger data.
Dmitry and Peter might find a good way to work around that, but I'm OK
with adding MSC_SERIAL_2 from my point of view.

As long as the driver behaves well (i.e. it'll send *both* MSC_SERIAL and
MSC_SERIAL2 in the same frame) it'll be easy to deal with in userspace.

There's some theoretical option to have a special evdev event that works to
combine the values of multiple events (similar to utf8) but I think that's
something to consider when the problem is more wide-spread than a serial
number, it's a bit overkill here.

Cheers,
   Peter

On Thu, Mar 25, 2021 at 1:35 AM Benjamin Tissoires
[off-list ref] wrote:

Hi Kenneth,

On Wed, Mar 24, 2021 at 8:53 PM Kenneth Albanowski [off-list ref] wrote:

quoted

Hi folks,

I'm looking at being able to retrieve an accurate (not mangled or
truncated) Digitizer.Transducer Serial Number from user-space, primarily
through the power_supply node constructed for HID devices which expose
Battery Strength.

For the digitizers I'm looking at (USI) the Serial Number is a 64-bit
report field, making it not fit directly into evdev, or the hid-input
core.

Thanks for raising that issue. Before I jump into commenting on your
PoC, I'd like to ask you to provide the report descriptor of this
particular device (ideally with hid-recorder from the hid-tolls repo
below).
AFAICT, the HID spec we currently implement doesn't allow for data of
more than 32 bits (section 6.2.2.2 of
https://www.usb.org/sites/default/files/hid1_11.pdf).
The main problem is that a field is usually associated with a logical
max, and this part makes us limit the field up to 32bits.

Appended find the HID descriptor for a Lenovo Duet Chromebook with USI
digitizer -- I'm confident the S/N report size will be consistent
across other USI devices and touchscreen controllers.

Here's the section in question:

# 0x05, 0x0d,                    //   Usage Page (Digitizers)           653
# 0x09, 0x3b,                    //   Usage (Battery Strength)          655
# 0x15, 0x00,                    //   Logical Minimum (0)               657
# 0x25, 0x64,                    //   Logical Maximum (100)             659
# 0x75, 0x08,                    //   Report Size (8)                   661
# 0x81, 0x02,                    //   Input (Data,Var,Abs)              663
# 0x09, 0x5b,                    //   Usage (Transducer Serial Number)  665
# 0x25, 0xff,                    //   Logical Maximum (255)             667
# 0x75, 0x40,                    //   Report Size (64)                  669
# 0x81, 0x02,                    //   Input (Data,Var,Abs)              671

I've seen a few variations of min/max, but as you say the logical max can't
go over 32 bits in any case.

There is a "long items" section right after (6.2.2.3) that allows for
longer data, and I don't think the current implementation supports it.

I'm a bit puzzled as to the direction of the spec here: items within the
HID descriptor are clearly limited to 32 bit values (short items, 6.2.2.2),
and long items are specifically unimplemented, e.g. "No long item tags are
defined in this document. These tags are reserved for future use." (6.2.2.3)

So, not only is there nothing in our code for long items, it doesn't seem
there is a spec definition as to what we would do with them anyway.

However that size limit on items pertains to the HID descriptor, not the
HID report: the Report Size for a field can definitely be larger than 64
bits, and there are a few examples, such as this for USI, some web searches
bring up instances of "Report Size (64)" and "Report Size (128)", and we
also had the example of a sensor hub with a 192 bit report leading to
"HID: Increase maximum report size allowed by hid_field_extract()" (and its
subsequent reversion.)

Clearly this is logically inconsistent (the min/max cannot accurately
describe a > 32 bit field) but hey, that's life. Presumably anyone using
fields that large is just reading them raw and decoding them manually
(or using them as a bulk data pipe).

(If I'm misinterpreting this distinction between descriptor items and
report items, please correct me.)

Incidentally, the spec also says "An item field cannot span more than 4
bytes in a report. For example, a 32-bit item must start on a byte
boundary to satisfy this condition." (first bullet, 8.4) which to my
reading does completely preclude report fields over 32 bits, but
apparently nobody else is interpreting it that way, or at least being
strict about it.

To get back to reality, grabbing a handy Wacom digitizer (Intuos BT S):

# 0x09, 0x5b,                    //   Usage (Transducer Serial Number)  119
# 0x09, 0x5c,                    //   Usage (Wacom SerialHi)            121
# 0x17, 0x00, 0x00, 0x00, 0x80,  //   Logical Minimum (-2147483648)     123
# 0x27, 0xff, 0xff, 0xff, 0x7f,  //   Logical Maximum (2147483647)      128
# 0x75, 0x20,                    //   Report Size (32)                  133
# 0x95, 0x02,                    //   Report Count (2)                  135

4 more lines

One other thing: this whole problem is very much a good candidate for
adding regression tests on
https://gitlab.freedesktop.org/libevdev/hid-tools.

So I'd be glad if we can work around the problem in the kernel *and*
add the matching test in hid-tools so we don't break this in the
future.

Gladly. Thank you for mentioning those regression tests, I'd very much
like to aim towards high consistency and some form of documentation.

More comments inlined...

quoted

I'm appending a proof-of-concept which implements this; I describe the
details below. (This is based on 5.4, but it looks clean for 5.10 as
well as relatively straightforward to backport.)

I'd appreciate any feedback on this approach.

My thought is to perform minimal modifications to allow hid-core to
transfer reports up to 64-bits in size to hid-input, so that it can
process this field and emit it to the power_supply integration, as well
as through classic MSC_SERIAL (for the low 32 bits) and a new
MSC_SERIAL2 (the upper bits) for applications which want it inline with
stylus event data.

The wacom driver already has this problem and handles it in a creative
way (as usual ;-P)
If I am not mistaken, it splits the 64 bits serial on 2 events as
well: MSC_SERIAL and ABS_MISC (Jason might correct me here).

I am not saying this is what needs to be done, but just what some
userspace tools already expect.

As Peter mentions, this seems separate, ABS_MISC being a sort of tool
type identifier, quite different from the serial number. (They apparently
get extracted from the same data report in the wacom formats, but the
intention seems distinct.)

As far as I can see, the Wacom S/N (stuffed into MSC_SERIAL) is indeed
always truncated to 32 bits; introducing MSC_SERIAL2 should be suitable
for the wacom driver as well if they want to publish the high bits.

quoted

hid_field.value is expanded to an s64 array from s32, and there are
a few knock-on effects to code that holds pointers to these fields.
No other interfaces, in-kernel, or user-space, are modified to support
HID reports/evdev events > 32 bits.

I also think both problems are orthogonal (which is why you haven't
tried to change the evdev protocol).

I don't think we can do much on the evdev side to support 64 bits or
bigger data.
Dmitry and Peter might find a good way to work around that, but I'm OK
with adding MSC_SERIAL_2 from my point of view.

Thanks.

quoted

The Transducer Serial Number would show up as in the power_supply as a
serial_number of "DG:0123456789ABCDEF" (or however many hex digits are
warranted by the report size), and "DG:" being a fixed prefix -- we do
know it is a digitizer.

I realize this doesn't provide a scalable solution in the
hid-core/hid-input code for (hypothetical) fields larger than 64 bits,
although extending evdev with MSC_SERIAL3, 4, etc. would be trivial. (I
don't have a good sense of how common these larger fields are, although
obviously the code has survived until now with only 32-bit support, and
one comment saying that anything larger than 16-bit is unusual.)

I would prefer having a generic solution for items longer than 32 bits
instead of having a special case for 64 bits only.

The spec allows for items longer than 32 bits (still section 6.2.2.3
of the HID spec) so we should probably handle that case in a generic
way to not have to deal with 128 bits next time a creative device
maker wants to use it.

I'm glad to look at more generalized long-field support.

The nice thing about the hid-tools test suite is that we can be
creative and produce this 128 bits new device, and have the tests for
it already :)

Agreed, glad to reduce the headache involved with doing this again in
the future.

quoted

(The other approach I was looking at was leaving hid_field.value
unchanged, and instead just having hid-core recognize this particular
64 bit field, edit the hid_usage to turn it into a 2-count 32-bit field,
apply a unique usage code to the second half, and then have hid-input
recognize these usages and paste everything back together. This is
scalable to larger fields, but is putting an unprecedented amount of
magic in hid-core.)

Not really. There is already a lot of magic in hid-core, and handling
longer-than-32bits doesn't seem that far away. The report descriptor
should give us all the information we need, so we can surely have a
special case for long items and data. Also hid-input doesn't have to
be tied to evdev. So nothing prevents us to mark the field as being a
long one, and change the internal representation of the various 32
bits chunks of its data.

So, let me propose some specific alternate implementations:

1) Have hid-core notice a usage for a 64-bit Usage(Digitizer.Transducer
Serial Number) and split it up in the hid_field into a repeated field,
with the usage of the second set to, perhaps, Wacom.SerialHi.

Downside: modified hid_field definitions, which will show up in
hid-debug and hiddev -- there will be disagreement between the rdesc,
the kernel hid-fields, and a descriptor parsed by hid-tools. Also,
hidinput_hid_event() has to maintain state within one of the hid_*
structures if it needs to recombine these split-up reports.

2) All hid report fields with sizes greater than 32 bits (and a
multiple of 32 bits) get modified to be split into repeated fields.
The hid_usage definitions are retained, but a new 'split' attribute is
added allowing hid_process_event to understand which 32 bit slice of
a field is being processed, and better understand when it is getting
the last slice, if it needs to perform an action upon combining
everything.

Downside: same as #1, plus now a new concept to explain, and several
places need to reject split>0 to maintain prior low 32 bit behaviour,
and it can only handle 32 bit multiples unless we start splitting
one field into multiple fields in that scenario.

3) Same as #1 or #2, but give hid-input a function,
hidinput_should_split_field() that allows it to choose which repeated
fields opt-in to this new behaviour.

Downside: same as #1 or #2, but the disruption to the hid_fields is
constrained to these very specific cases. (Not sure that's much better
for someone trying to understand what is going on.)

4) Same as #1 or #2 or #3, but introduce a HID_QUIRK to trigger the
behaviour.

Downside: same as above, but at least there's clearly a quirk to blame.

5) Modify interface to hidinput_hid_event() to take u32* and a length

17 more lines

quoted

The other change is MSC_SERIAL2 would be added: in the specific case of
Usage(Digitizer.Transducer Serial Number) being present with
report_size > 32 then MSC_SERIAL is reliably the full lowest 32 bits,
and MSC_SERIAL2 is the next set of 32 bits; no clamping or scaling
applied, regardless of what the descriptor says. Both MSC_SERIAL* are
emitted for every hid record, there is no caching. If report_size <=
32, then there is no difference to existing behaviour, only MSC_SERIAL
is emitted. I'm assuming the cost of one additional event per sync, for
stylus data, is negligible.

I am not in the best position to comment, but as mentioned previously,
it works for me.

quoted

This does potentially alter the generation of MSC_SERIAL on existing
devices, but only ones where the report_size > 32 bits and the min/max
is not 0/~0 -- user space would see unclamped values and conceivably
could be surprised with a larger value in MSC_SERIAL than previously
seen.

Honestly it shouldn't introduce any regressions. I still have to work
on the tablet side of the regression tests, but so far only Wacom was
reliably sending serial numbers (and bigger than 32 bits). So I am not
worried from this side of things.

Ok, that feels reasonable -- any risk is low, as there already exist
devices that can reasonably emit a full 32 bit range on MSC_SERIAL,
combined with MSC_SERIAL actually being quite rare.

(I thought it would be nice to see a Wacom digitizer delivering a full
32 bit MSC_SERIAL in action, but so far I haven't found a combination of
tablet, pen, and kernel ready-to-hand that that actually get me a non-zero
field.)

quoted

If changing MSC_SERIAL is deemed too risky for compatibility, then it's
easy to just leave MSC_SERIAL as-is, and put in a separate MSC_SERIAL1
(MSC_SERIAL0?) that has the pristine low bits -- just at a cost of yet
one more MSC_ field per report, and using up the last MSC_ bit before
we need to bump MSC_MAX.

This approach doesn't affect Wacom evdev events at all, all of that
logic is separate from hid-input.

There is a whole ecosystem for wacom, but we are pretty much in
control of all the pieces. So I would prefer involving wacom from the
beginning, and have a common representation to the user space.

Besides that, I skimmed through the  patch below. I do have comments,
but in addition to the above, they are mostly "hey this hunk should be
in its own separate patch". But I guess the final submission will have
this separation, so there is no real point in going too deep in the
details IMO.

Cool, thanks.

Kind regards and good health,
- Kenneth Albanowski

localhost ~ # hid-recorder
Available devices:
/dev/hidraw0: hid-over-i2c 27C6:0E30
/dev/hidraw1: Google Inc. Hammer
/dev/hidraw2: Google Inc. Hammer
Select the device event number [0-2]: 0
# hid-over-i2c 27C6:0E30
# 0x05, 0x0d,                    // Usage Page (Digitizers)             0
# 0x09, 0x04,                    // Usage (Touch Screen)                2
# 0xa1, 0x01,                    // Collection (Application)            4
# 0x85, 0x01,                    //  Report ID (1)                      6
# 0x09, 0x22,                    //  Usage (Finger)                     8
# 0xa1, 0x02,                    //  Collection (Logical)               10
# 0x55, 0x0e,                    //   Unit Exponent (-2)                12
# 0x65, 0x11,                    //   Unit (Centimeter,SILinear)        14
# 0x35, 0x00,                    //   Physical Minimum (0)              16
# 0x15, 0x00,                    //   Logical Minimum (0)               18
# 0x09, 0x42,                    //   Usage (Tip Switch)                20
# 0x25, 0x01,                    //   Logical Maximum (1)               22
# 0x75, 0x01,                    //   Report Size (1)                   24
# 0x95, 0x01,                    //   Report Count (1)                  26
# 0x81, 0x02,                    //   Input (Data,Var,Abs)              28
# 0x25, 0x7f,                    //   Logical Maximum (127)             30
# 0x09, 0x30,                    //   Usage (Tip Pressure)              32
# 0x75, 0x07,                    //   Report Size (7)                   34
# 0x81, 0x42,                    //   Input (Data,Var,Abs,Null)         36
# 0x95, 0x01,                    //   Report Count (1)                  38
# 0x75, 0x08,                    //   Report Size (8)                   40
# 0x09, 0x51,                    //   Usage (Contact Id)                42
# 0x81, 0x02,                    //   Input (Data,Var,Abs)              44
# 0x75, 0x10,                    //   Report Size (16)                  46
# 0x05, 0x01,                    //   Usage Page (Generic Desktop)      48
# 0x26, 0x20, 0x1c,              //   Logical Maximum (7200)            50
# 0x46, 0x49, 0x05,              //   Physical Maximum (1353)           53

711 more lines

First of all, my apologies for the likely-wonky email formatting. For some reason I'm missing messages from linux-input in my Gmail so I'm forced to reply from Outlook (which doesn't exactly make inline plain-text replies easy).

quoted

quoted

My thought is to perform minimal modifications to allow hid-core to
transfer reports up to 64-bits in size to hid-input, so that it can
process this field and emit it to the power_supply integration, as well
as through classic MSC_SERIAL (for the low 32 bits) and a new
MSC_SERIAL2 (the upper bits) for applications which want it inline with
stylus event data.

The wacom driver already has this problem and handles it in a creative
way (as usual ;-P)
If I am not mistaken, it splits the 64 bits serial on 2 events as
well: MSC_SERIAL and ABS_MISC (Jason might correct me here).

I am not saying this is what needs to be done, but just what some

userspace tools already expect.

As Peter mentions, this seems separate, ABS_MISC being a sort of tool
type identifier, quite different from the serial number. (They apparently
get extracted from the same data report in the wacom formats, but the
intention seems distinct.)

As far as I can see, the Wacom S/N (stuffed into MSC_SERIAL) is indeed
always truncated to 32 bits; introducing MSC_SERIAL2 should be suitable
for the wacom driver as well if they want to publish the high bits.

ABS_MISC and MSC_SERIAL are nominally the tool type and serial number fields. A given serial number may be re-used across tool types, however, and so the two fields must be combined to get a globally unique ID. It's not *quite* that simple though:

    * The ABS_MISC field is sometimes intentionally mangled for compatibility with an ancient wacom.ko bug
    * A 32-bit MSC_SERIAL is sometimes not quite large enough to store the nominal serial number
    * The way the values are assembled varies between devices (i.e. on some devices ABS_MISC is the high "n"-bits, on others it is the low "n"-bits, and "n" may vary).

I'd love to have a way for our driver to report an unmangled 64-bit unique ID which matches what you'd get on Windows and OSX, but I don't think MSC_SERIAL2 quite solves that problem for us. We could use it with devices which have MSC_SERIAL as the low 32 bits of the unique ID, but not those that have ABS_MISC as the low "n"-bits).

quoted

If changing MSC_SERIAL is deemed too risky for compatibility, then it's
easy to just leave MSC_SERIAL as-is, and put in a separate MSC_SERIAL1
(MSC_SERIAL0?) that has the pristine low bits -- just at a cost of yet
one more MSC_ field per report, and using up the last MSC_ bit before
we need to bump MSC_MAX.

This approach doesn't affect Wacom evdev events at all, all of that
logic is separate from hid-input.

There is a whole ecosystem for wacom, but we are pretty much in
control of all the pieces. So I would prefer involving wacom from the
beginning, and have a common representation to the user space.

Providing a separate MSC_SERIAL1 (or zero) in addition to MSC_SERIAL2 could be the solution to the problems above. Our driver can be taught how to assemble the 64-bit IDs and then split them into events for the high- and low-order bytes. We might want to consider using a different prefix than MSC_SERIAL if we go this route, however. It could be very confusing to understand how all three events do (and don't) relate to each other if their names are too similar.

Of course, it'd also be great if the input structs could fit 64-bit integers, but that's probably not possible (and also doesn't help with devices that want to report e.g. 128-bit numbers).

On Thu, Apr 8, 2021 at 3:19 PM Gerecke, Jason [off-list ref] wrote:

First of all, my apologies for the likely-wonky email formatting. For some reason I'm missing messages from linux-input in my Gmail so I'm forced to reply from Outlook (which doesn't exactly make inline plain-text replies easy).

No worries.

quoted

As far as I can see, the Wacom S/N (stuffed into MSC_SERIAL) is indeed
always truncated to 32 bits; introducing MSC_SERIAL2 should be suitable
for the wacom driver as well if they want to publish the high bits.

ABS_MISC and MSC_SERIAL are nominally the tool type and serial number fields. A given serial number may be re-used across tool types, however, and so the two fields must be combined to get a globally unique ID. It's not *quite* that simple though:

    * The ABS_MISC field is sometimes intentionally mangled for compatibility with an ancient wacom.ko bug
    * A 32-bit MSC_SERIAL is sometimes not quite large enough to store the nominal serial number
    * The way the values are assembled varies between devices (i.e. on some devices ABS_MISC is the high "n"-bits, on others it is the low "n"-bits, and "n" may vary).

I'd love to have a way for our driver to report an unmangled 64-bit unique ID which matches what you'd get on Windows and OSX, but I don't think MSC_SERIAL2 quite solves that problem for us. We could use it with devices which have MSC_SERIAL as the low 32 bits of the unique ID, but not those that have ABS_MISC as the low "n"-bits).

Noted, thanks for explaining.

quoted

quoted

If changing MSC_SERIAL is deemed too risky for compatibility, then it's
easy to just leave MSC_SERIAL as-is, and put in a separate MSC_SERIAL1
(MSC_SERIAL0?) that has the pristine low bits -- just at a cost of yet
one more MSC_ field per report, and using up the last MSC_ bit before
we need to bump MSC_MAX.

This approach doesn't affect Wacom evdev events at all, all of that
logic is separate from hid-input.

There is a whole ecosystem for wacom, but we are pretty much in
control of all the pieces. So I would prefer involving wacom from the
beginning, and have a common representation to the user space.

Providing a separate MSC_SERIAL1 (or zero) in addition to MSC_SERIAL2 could be the solution to the problems above. Our driver can be taught how to assemble the 64-bit IDs and then split them into events for the high- and low-order bytes. We might want to consider using a different prefix than MSC_SERIAL if we go this route, however. It could be very confusing to understand how all three events do (and don't) relate to each other if their names are too similar.

Yes, we have just enough free slots within MSC_MAX to keep MSC_SERIAL
and set up new MSC_TRANSDUCER_ID0 and MSC_TRANSDUCER_ID1 -- better
names welcome. Since MSC_SERIAL has been weird and probably is often
logically clamped in strange ways, I'm happy with keeping it entirely
separate from the new clean ID events.

(Any guess how much pain we are in for if we want to expand MSC_MAX?)

Of course, it'd also be great if the input structs could fit 64-bit integers, but that's probably not possible (and also doesn't help with devices that want to report e.g. 128-bit numbers).

For what it's worth, the variation I'm looking at now involves
handling the longer fields in an arbitrary number of 32-bit slices, so
it's not 64-bit specific on the hid-core side. Obviously it
(currently) needs to fit back through evdev, so that is the
bottleneck.

- Kenneth


- Kenneth