Re: [PATCH v5] hid-mcp2200: added driver for GPIOs of MCP2200

[Rahul Rameshbabu <sergeantsagara@protonmail.com>]

On Thu, 31 Aug, 2023 20:53:43 +0200 "Johannes Roith" <johannes@gnu-linux.rocks> wrote:
> Hi Rahul,
>
> thanks for the feedback, I will add it to the driver.
>
>> My personal recommendation is to just have a single DMA buffer allocated
>> for the mcp2200 instance rather than having to call the allocator and
>> release the memory per command.
>
> I added an 16-byte Array hid_report to the mcp2000 struct. When I need the
> report, I do the following:
>
> struct mcp_set_clear_outputs *cmd;
>
> mutex_lock(&mcp->lock);
> cmd = (struct mcp_set_clear_outputs *) mcp->hid_report
>
> Do you think this is a valid implementation or do I really have to add a
> pointer to the mcp2200 struct instead of the 16 byte array and allocate
> another 16 byte for it in the probe function?

This works fine since the mcp2000 struct will be dynamically allocated.
The reason I went with a separate allocation for the buffer was just to
make it explicitly clear that no matter how a thunderstrike instance is
set up, the buffer will need to be dynamically allocated for hid
requests.

>> The reason you run into this is likely because of the action added to
>> devm conflicting with hid_device_remove....
>>
>> I recommend not depending on devm for teardown rather than making a stub
>> remove function to work around the issue.

I have reinserted the relevant code from the core hid stack in my
previous email since it's important for this discussion.

    static void hid_device_remove(struct device *dev)
    {
      struct hid_device *hdev = to_hid_device(dev);
      struct hid_driver *hdrv;

      down(&hdev->driver_input_lock);
      hdev->io_started = false;

      hdrv = hdev->driver;
      if (hdrv) {
        if (hdrv->remove)
          hdrv->remove(hdev);
        else /* default remove */
          hid_hw_stop(hdev);

  hid_device_remove will call hid_hw_stop and so will
  mcp2200_hid_unregister because of the devm action you added.

>
> I am not sure, if I have understand this correctly, but basically I already
> have a stub remove function (which is empty). First the remove function gets
> called, then the unregister function and everything is cleaned up correctly.
> Did I get this right or do you have any other recommendation for me?

Let me try to break down the problem first.

1. You add mcp2200_hid_unregister to the devm actions for clean up the
   device.
2. mcp2200_hid_unregister will call hid_hw_close and hid_hw_stop,
   tearing down the device.
3. hid_device_remove is invoked when the device is removed, which
   already calls hid_hw_stop when no remove function is registered (the
   expectation is the device is simple when this is the case)
4. This leads to the device already being torn down, which leads to the
   exception seen when the devm kicks in and mcp_hid_unregister is then
   triggered.
5. Using an empty remove function resolves this but indicates the driver
   has an inappropriate devm action in my opinion/has problematic
   design.

I am saying that using an empty remove function to work
around the problem is not an upstream-able solution in my opinion.

Given this, I think its best to not use devm in this can and manually
handle cleanup, so you do not have a stub remove function and take
control of the teardown.

> So, do I need any adaptions, or can we go with the empty remove function?

That said, maybe someone else can chime in on this to see if this aligns
with others' preferences? At the very least though if others feel using
an empty remove function is ok, I think the comment in the remove
function needs to updated to add clear detail about the issue than what
is currently provided. That said, I am very much against using an empty
remove function to work around problematic devm practices.

 	/*
 	 * With no remove function you sometimes get a segmentation fault when
 	 * unloading the module or disconnecting the USB device
 	 */

--
Thanks,

Rahul Rameshbabu