Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.

[Life is hard, and then you die]

  Hi Benjamin,

Sorry for the extremely late reply - RL etc.

On Fri, Apr 26, 2019 at 08:26:25AM +0200, Benjamin Tissoires wrote:
> On Fri, Apr 26, 2019 at 7:56 AM Life is hard, and then you die
> <ronald@xxxxxxxxxxxxx> wrote:
> >
> > On Thu, Apr 25, 2019 at 11:39:12AM +0200, Benjamin Tissoires wrote:
> > > On Thu, Apr 25, 2019 at 10:19 AM Life is hard, and then you die
> > > <ronald@xxxxxxxxxxxxx> wrote:
> > > >
> > > >   Hi Benjamin,
> > > >
> > > > Thank you for looking at this.
> > > >
> > > > On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote:
> > > > > On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@xxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > The iBridge device provides access to several devices, including:
> > > > > > - the Touch Bar
> > > > > > - the iSight webcam
> > > > > > - the light sensor
> > > > > > - the fingerprint sensor
> > > > > >
> > > > > > This driver provides the core support for managing the iBridge device
> > > > > > and the access to the underlying devices. In particular, since the
> > > > > > functionality for the touch bar and light sensor is exposed via USB HID
> > > > > > interfaces, and the same HID device is used for multiple functions, this
> > > > > > driver provides a multiplexing layer that allows multiple HID drivers to
> > > > > > be registered for a given HID device. This allows the touch bar and ALS
> > > > > > driver to be separated out into their own modules.
> > > > >
> > > > > Sorry for coming late to the party, but IMO this series is far too
> > > > > complex for what you need.
> > > > >
> > > > > As I read this and the first comment of drivers/mfd/apple-ibridge.c,
> > > > > you need to have a HID driver that multiplex 2 other sub drivers
> > > > > through one USB communication.
> > > > > For that, you are using MFD, platform driver and you own sauce instead
> > > > > of creating a bus.
> > > >
> > > > Basically correct. To be a bit more precise, there are currently two
> > > > hid-devices and two drivers (touchbar and als) involved, with
> > > > connections as follows (pardon the ugly ascii art):
> > > >
> > > >   hdev1  ---  tb-drv
> > > >            /
> > > >           /
> > > >          /
> > > >   hdev2  ---  als-drv
> > > >
> > > > i.e. the touchbar driver talks to both hdev's, and hdev2's events
> > > > (reports) are processed by both drivers (though each handles different
> > > > reports).
> > > >
> > > > > So, how about we reuse entirely the HID subsystem which already
> > > > > provides the capability you need (assuming I am correct above).
> > > > > hid-logitech-dj already does the same kind of stuff and you could:
> > > > > - create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE
> > > > > - hid-ibridge will then register itself to the hid subsystem with a
> > > > > call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and
> > > > > hid_device_io_start(hdev) to enable the events (so you don't create
> > > > > useless input nodes for it)
> > > > > - then you add your 2 new devices by calling hid_allocate_device() and
> > > > > then hid_add_device(). You can even create a new HID group
> > > > > APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them
> > > > > from the actual USB device.
> > > > > - then you have 2 brand new HID devices you can create their driver as
> > > > > a regular ones.
> > > > >
> > > > > hid-ibridge.c would just need to behave like any other hid transport
> > > > > driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and
> > > > > you can get rid of at least the MFD and the platform part of your
> > > > > drivers.
> > > > >
> > > > > Does it makes sense or am I missing something obvious in the middle?
> > > >
> > > > Yes, I think I understand, and I think this can work. Basically,
> > > > instead of demux'ing at the hid-driver level as I am doing now (i.e.
> > > > the iBridge hid-driver forwarding calls to the sub-hid-drivers), we
> > > > demux at the hid-device level (events forwarded from iBridge hdev to
> > > > all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to
> > > > the original hdev via an iBridge ll_driver attached to the
> > > > sub-hdev's).
> > > >
> > > > So I would need to create 3 new "virtual" hid-devices (instances) as
> > > > follows:
> > > >
> > > >   hdev1  ---  vhdev1  ---  tb-drv
> > > >                         /
> > > >           --  vhdev2  --
> > > >          /
> > > >   hdev2  ---  vhdev3  ---  als-drv
> > > >
> > > > (vhdev1 is probably not strictly necessary, but makes things more
> > > > consistent).
> > >
> > > Oh, ok.
> > >
> > > How about the following:
> > >
> > > hdev1 and hdev2 are merged together in hid-apple-ibridge.c, and then
> > > this driver creates 2 virtual hid drivers that are consistent
> > >
> > > like
> > >
> > > hdev1---ibridge-drv---vhdev1---tb-drv
> > > hdev2--/           \--vhdev2---als-drv
> >
> > I don't think this will work. The problem is when the sub-drivers need
> > to send a report or usb-command: how to they specify which hdev the
> > report/command is destined for? While we could store the original hdev
> > in each report (the hid_report's device field), that only works for
> > hid_hw_request(), but not for things like hid_hw_raw_request() or
> > hid_hw_output_report(). Now, currently I don't use the latter two; but
> > I do need to send raw usb control messages in the touchbar driver
> > (some commands are not proper hid reports), so it definitely breaks
> > down there.
> >
> > Or am I missing something?
> 
> I'd need to have a deeper look at the protocol, but you can emulate
> pure HID devices by having your ibridge handling a translation from
> set/get features/input to the usb control messages. Likewise, nothing
> prevents you to slightly rewrite the report descriptors you present to
> the als and touchbar to have a clear separation with the report ID.
> 
> For example, if both hdev1 and hdev2 use a report ID of 0x01, you
> could rewrite the report descriptor so that when you receive a report
> with an id of 0x01 you send this to hdev1, but you can also translate
> 0x11 to a report ID 0x01 to hdev2.
> Likewise, report ID 0x42 could be a raw USB control message to the USB
> under hdev2.
> 
> Note that you will have to write 2 report descriptors for your new
> devices, but you can take what makes sense from the original ones, and
> just add a new collection with a vendor application with with an
> opaque meaning (for the USB control messages).

A couple things here. First of all, I went and rewrote the mfd driver
with the hid-driver demultiplexer as a straight hid driver with 3
(well, 4 actually) virtual hid devices, as first discussed above.
This overall led to some simplifications, with only smaller
adjustments in the Touch Bar and ALS drivers (the diff stat shows 468
insertions, 825 deletions), so this looks good. Importantly (IMO),
this leaves the whole awareness of the fact that the Touch Bar driver
is talking to multiple usb interfaces and needs to coordinate
appropriately between them (including things like which order they are
accessed, sleep times between those accesses, and different power
management) clearly in the Touch Bar driver, and the ibridge driver is
still fairly generic unaware of any of the details that the
sub-drivers need to worry about.

Then I started looking more closely at your last suggestion above of
creating only 2 virtual hid devices, with report descriptor
merging/mangling and the addition of 3 custom reports (for the
set-power, io-wait, and usb-control functionality), and I'm having
trouble seeing the justification for it. AFAICT, the only advantage of
this approach is that there are fewer virtual hid devices. But the
disadvantages are significantly more code (especially in the ibridge
driver) and more leakage of knowledge from the Touch Bar driver into
the ibridge driver. In particular:

* this leads to additional work, synchronization, and state management
  in the ibridge driver to deal with the fact that we have to wait for
  all (real) hid devices to be probed before we can start creating the
  virtual hid devices (and visa versa on removal).
* merging and mangling those report descriptors requires re-parsing
  of the descriptors and dealing with various corner cases, which adds
  a bunch of code.
* while the custom set-power and io-wait reports are simple, the
  custom usb-control report is ugly, because it actually ends up
  needing to compute some of the parameters and rewrite the data, as
  both those have values that require knowledge of the real underlying
  reports and usb interfaces (i.e. it's not a report for a generic
  usb-control call, but a very Touch Bar specific one, i.e. leaking
  particular Touch Bar driver knowledge into the ibridge driver).
* In addition, while creating especially the custom usb-control
  report, I started to wonder if it's really worth serializing the
  parameters for the custom functions/report into an actual report
  buffer, just to be deserialized again two function calls down the
  stack. This became obvious when I was adding a helper function to
  the ibridge driver to serialize the function parameters, and at the
  same time writing a function right below it to deserialize those
  parameters again, all so we can call hid_hw_request() to pass them
  from the Touch Bar driver to the ibridge driver - but since the
  Touch Bar driver is already calling a custom function in the ibridge
  driver to serialize the parameters, it seems like that function
  might just as well make the desired underlying function call
  directly in the first place. Alternatively, the serializing
  functionality can be put in the Touch Bar driver instead, with the
  disadvantage that the implicit knowledge of the structure of the
  custom report is now spread over two modules. All of this seems
  somewhat ugly to me.

Lastly, as hinted earlier, while this tries to hide the fact that
there are actually multiple hid devices (aka usb interfaces) that are
being driven by the Touch Bar driver, the Touch Bar driver still needs
to be acutely aware of that fact because it cannot treat them equally.
So now instead it clearly dealing with two different devices, it now
has to do so indirectly by figuring out which reports (in the same
virtual hid device) belong to which underlying real hid devices so it
can treat those reports accordingly (e.g. when creating the reports to
trigger set-power or usb-ctrl, instead of the desired device/interface
being targeted directly, that info has to instead be passed somewhat
opaquely via a report-id of a report that it happens to know is mapped
to the desired device/interface).

Sorry for the long-winded response. I hope it isn't too cryptic. But
basically it boils down to: going for single virtual hid-device per
real device adds a bunch of complexity and knowledge leaking from the
Touch Bar driver the ibridge driver with AFAICT only a small advantage
(namely fewer virtual devices).


  Cheers,

  Ronald