| Message ID | 1475869180-26757-2-git-send-email-roderick@gaikai.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
We have been developing some new code building on top of this hid-sony patchset.
However during additional testing we noticed some issues between
swapping devices
between USB and Bluetooth. There is some code as part of 'sony_input_configured'
preventing two connections from the same device. The code is kind of
working as it
rejects the device, but there is some memory corruption due to 'sony_probe'
succeeding on this failure (later on 'sony_remove' double frees memory), which
was introduced by this patch. We intend to fix this, but I'm not sure
where the fix
should be. So far I'm leaning towards outside of this driver, but let
me explain.
On close inspection, the problem is that 'hid_hw_start' as called by
'sony_probe'
isn't returning an error on a 'sony_input_configured' failure.
Personally, I expected
it to behave like that, but it doesn't. As an outsider to the HID subsystem, I'm
not sure what expected behavior is.
I looked over some other drivers, which are emitting errors from
'input_configured'.
There are only 2 drivers returning errors and catching them through a
custom mechanism.
The drivers are 'hid-magicmouse' and 'hid-rmi'. The first kind of works around
'hid_hw_start' returning errors by setting some state (msc->input) from within
'input_configured' which 'probe' can pick up.
The 'hid-rmi' driver kind of relies on hid_hw_start not returning
errors. It leaves
the device in some in-between state with hidraw operational, so
someone could flash
a new firmware. For reference I have attached these code snippets to
the bottom of this
email.
I'm not exactly sure on what the best way to fix the issue I'm
experiencing is. My
feeling is that it should be fixed in a general way within
'hid_hw_start' and related
logic; not in a driver specific workaround. Outside of hid_hw_start
feels unclean
to me for various including keeping a device node around in some undefined state
for a short period of time.
The root cause seems to be that 'hid_connect' doesn't return a failure upon
'hidinput_connect' and only sets a flag:
if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
connect_mask & HID_CONNECT_HIDINPUT_FORCE))
hdev->claimed |= HID_CLAIMED_INPUT;
I think it should return an error when hidinput_connect fails, which
would ultimately
bubble up to a return error in 'hid_hw_start'. I don't know the
intimate details of
all the HID code well, so I'm not sure if this is the best solution.
With such a change made, the 'if (!msc->input)' logic in magicmouse_probe could
be removed. The RMI driver would need some fixing. I would say it should call
'hid_hw_start' again on the first hid_hw_start failure, but with a different
connect_mask, so only hidraw is activated.
For reference two pieces of code, which are dealing with the same kind
of problem,
I'm seeing in hid-sony. I put some comments '<--' to show relevant pieces.
magicmouse_input_configured:
ret = magicmouse_setup_input(msc->input, hdev);
if (ret) {
hid_err(hdev, "magicmouse setup input failed (%d)\n", ret);
/* clean msc->input to notify probe() of the failure */
msc->input = NULL; <-- Used to notify magicmouse_probe on
hid_hw_start failure
return ret;
}
magicmouse_probe:
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret) {
hid_err(hdev, "magicmouse hw start failed\n");
return ret;
}
if (!msc->input) { <-- workaround to detect
magicmouse_input_configured failure
hid_err(hdev, "magicmouse input not registered\n");
ret = -ENOMEM;
goto err_stop_hw;
}
rmi_probe:
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret) {
hid_err(hdev, "hw start failed\n");
return ret;
}
if ((data->device_flags & RMI_DEVICE) &&
!test_bit(RMI_STARTED, &data->flags)) <-- input_configure sets
this bit on success
/*
* The device maybe in the bootloader if rmi_input_configured
* failed to find F11 in the PDT. Print an error, but don't
* return an error from rmi_probe so that hidraw will be
* accessible from userspace. That way a userspace tool
* can be used to reload working firmware on the touchpad.
*/
hid_err(hdev, "Device failed to be properly configured\n");
Thanks,
Roderick
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 10 2016 or thereabouts, Roderick Colenbrander wrote: > Hi, > > We have been developing some new code building on top of this hid-sony patchset. > However during additional testing we noticed some issues between > swapping devices > between USB and Bluetooth. There is some code as part of 'sony_input_configured' > preventing two connections from the same device. The code is kind of > working as it > rejects the device, but there is some memory corruption due to 'sony_probe' > succeeding on this failure (later on 'sony_remove' double frees memory), which > was introduced by this patch. We intend to fix this, but I'm not sure > where the fix > should be. So far I'm leaning towards outside of this driver, but let > me explain. > > On close inspection, the problem is that 'hid_hw_start' as called by > 'sony_probe' > isn't returning an error on a 'sony_input_configured' failure. > Personally, I expected > it to behave like that, but it doesn't. As an outsider to the HID subsystem, I'm > not sure what expected behavior is. > > I looked over some other drivers, which are emitting errors from > 'input_configured'. > There are only 2 drivers returning errors and catching them through a > custom mechanism. > The drivers are 'hid-magicmouse' and 'hid-rmi'. The first kind of works around > 'hid_hw_start' returning errors by setting some state (msc->input) from within > 'input_configured' which 'probe' can pick up. Well, hid-magicmouse is not the best of the art in term of HID driver. But this quirk works OK-ish. > The 'hid-rmi' driver kind of relies on hid_hw_start not returning > errors. It leaves > the device in some in-between state with hidraw operational, so > someone could flash > a new firmware. For reference I have attached these code snippets to > the bottom of this > email. And hid-rmi is also a "temporary" driver and we (Andrew mostly) are trying to clean it up and forward all the RMI4 logic to RMI4 core now upstream in the drivers/input/rmi4 directory. > > I'm not exactly sure on what the best way to fix the issue I'm > experiencing is. My > feeling is that it should be fixed in a general way within > 'hid_hw_start' and related > logic; not in a driver specific workaround. Outside of hid_hw_start > feels unclean > to me for various including keeping a device node around in some undefined state > for a short period of time. I am not entirely sure having hidinput_connect failing being an actual error. It can be useful to still present the HID node through hidraw if the device fails for whatever reasons to bind on the input side. If I am not wrong, you can simply check after hid_hw_start() if hdev->claimed matches HID_CLAIMED_INPUT, and if not just bail out. > > The root cause seems to be that 'hid_connect' doesn't return a failure upon > 'hidinput_connect' and only sets a flag: > if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev, > connect_mask & HID_CONNECT_HIDINPUT_FORCE)) > hdev->claimed |= HID_CLAIMED_INPUT; > > I think it should return an error when hidinput_connect fails, which > would ultimately > bubble up to a return error in 'hid_hw_start'. I don't know the > intimate details of > all the HID code well, so I'm not sure if this is the best solution. > > With such a change made, the 'if (!msc->input)' logic in magicmouse_probe could > be removed. The RMI driver would need some fixing. I would say it should call > 'hid_hw_start' again on the first hid_hw_start failure, but with a different > connect_mask, so only hidraw is activated. Again, I wouldn't bother much for these 2 drivers. The first one works and this wouldn't add much, and the second one is doomed to be changed, so as long as it ain't broken... :) Cheers, Benjamin > > > For reference two pieces of code, which are dealing with the same kind > of problem, > I'm seeing in hid-sony. I put some comments '<--' to show relevant pieces. > > magicmouse_input_configured: > ret = magicmouse_setup_input(msc->input, hdev); > if (ret) { > hid_err(hdev, "magicmouse setup input failed (%d)\n", ret); > /* clean msc->input to notify probe() of the failure */ > msc->input = NULL; <-- Used to notify magicmouse_probe on > hid_hw_start failure > return ret; > } > > magicmouse_probe: > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > if (ret) { > hid_err(hdev, "magicmouse hw start failed\n"); > return ret; > } > > if (!msc->input) { <-- workaround to detect > magicmouse_input_configured failure > hid_err(hdev, "magicmouse input not registered\n"); > ret = -ENOMEM; > goto err_stop_hw; > } > > > rmi_probe: > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > if (ret) { > hid_err(hdev, "hw start failed\n"); > return ret; > } > > if ((data->device_flags & RMI_DEVICE) && > !test_bit(RMI_STARTED, &data->flags)) <-- input_configure sets > this bit on success > /* > * The device maybe in the bootloader if rmi_input_configured > * failed to find F11 in the PDT. Print an error, but don't > * return an error from rmi_probe so that hidraw will be > * accessible from userspace. That way a userspace tool > * can be used to reload working firmware on the touchpad. > */ > hid_err(hdev, "Device failed to be properly configured\n"); > > > Thanks, > Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 11, 2016 at 8:43 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Oct 10 2016 or thereabouts, Roderick Colenbrander wrote: >> Hi, >> >> We have been developing some new code building on top of this hid-sony patchset. >> However during additional testing we noticed some issues between >> swapping devices >> between USB and Bluetooth. There is some code as part of 'sony_input_configured' >> preventing two connections from the same device. The code is kind of >> working as it >> rejects the device, but there is some memory corruption due to 'sony_probe' >> succeeding on this failure (later on 'sony_remove' double frees memory), which >> was introduced by this patch. We intend to fix this, but I'm not sure >> where the fix >> should be. So far I'm leaning towards outside of this driver, but let >> me explain. >> >> On close inspection, the problem is that 'hid_hw_start' as called by >> 'sony_probe' >> isn't returning an error on a 'sony_input_configured' failure. >> Personally, I expected >> it to behave like that, but it doesn't. As an outsider to the HID subsystem, I'm >> not sure what expected behavior is. >> >> I looked over some other drivers, which are emitting errors from >> 'input_configured'. >> There are only 2 drivers returning errors and catching them through a >> custom mechanism. >> The drivers are 'hid-magicmouse' and 'hid-rmi'. The first kind of works around >> 'hid_hw_start' returning errors by setting some state (msc->input) from within >> 'input_configured' which 'probe' can pick up. > > Well, hid-magicmouse is not the best of the art in term of HID driver. > But this quirk works OK-ish. > >> The 'hid-rmi' driver kind of relies on hid_hw_start not returning >> errors. It leaves >> the device in some in-between state with hidraw operational, so >> someone could flash >> a new firmware. For reference I have attached these code snippets to >> the bottom of this >> email. > > And hid-rmi is also a "temporary" driver and we (Andrew mostly) are > trying to clean it up and forward all the RMI4 logic to RMI4 core now > upstream in the drivers/input/rmi4 directory. > >> >> I'm not exactly sure on what the best way to fix the issue I'm >> experiencing is. My >> feeling is that it should be fixed in a general way within >> 'hid_hw_start' and related >> logic; not in a driver specific workaround. Outside of hid_hw_start >> feels unclean >> to me for various including keeping a device node around in some undefined state >> for a short period of time. > > I am not entirely sure having hidinput_connect failing being an actual > error. It can be useful to still present the HID node through hidraw if > the device fails for whatever reasons to bind on the input side. > > If I am not wrong, you can simply check after hid_hw_start() if > hdev->claimed matches HID_CLAIMED_INPUT, and if not just bail out. Yep that's correct. We actually implemented that method already, but weren't sure if it was a workaround or a solution. What concerned me is that doing it this way still leaves a 'transient' device node around (although for a very short while until probe really fails). If you think something like this is fine over, maybe modifying the other HID code, I'm fine with that. Thanks, Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index b0bb99a..afa8219 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -1387,28 +1387,6 @@ static int sony_register_touchpad(struct hid_input *hi, int touch_count, return 0; } -static int sony_input_configured(struct hid_device *hdev, - struct hid_input *hidinput) -{ - struct sony_sc *sc = hid_get_drvdata(hdev); - int ret; - - /* - * The Dualshock 4 touchpad supports 2 touches and has a - * resolution of 1920x942 (44.86 dots/mm). - */ - if (sc->quirks & DUALSHOCK4_CONTROLLER) { - ret = sony_register_touchpad(hidinput, 2, 1920, 942); - if (ret) { - hid_err(sc->hdev, - "Unable to initialize multi-touch slots: %d\n", - ret); - return ret; - } - } - - return 0; -} /* * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller @@ -2329,45 +2307,12 @@ static inline void sony_cancel_work_sync(struct sony_sc *sc) cancel_work_sync(&sc->state_worker); } -static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) +static int sony_input_configured(struct hid_device *hdev, + struct hid_input *hidinput) { - int ret; + struct sony_sc *sc = hid_get_drvdata(hdev); int append_dev_id; - unsigned long quirks = id->driver_data; - struct sony_sc *sc; - unsigned int connect_mask = HID_CONNECT_DEFAULT; - - if (!strcmp(hdev->name, "FutureMax Dance Mat")) - quirks |= FUTUREMAX_DANCE_MAT; - - sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL); - if (sc == NULL) { - hid_err(hdev, "can't alloc sony descriptor\n"); - return -ENOMEM; - } - - spin_lock_init(&sc->lock); - - sc->quirks = quirks; - hid_set_drvdata(hdev, sc); - sc->hdev = hdev; - - ret = hid_parse(hdev); - if (ret) { - hid_err(hdev, "parse failed\n"); - return ret; - } - - if (sc->quirks & VAIO_RDESC_CONSTANT) - connect_mask |= HID_CONNECT_HIDDEV_FORCE; - else if (sc->quirks & SIXAXIS_CONTROLLER) - connect_mask |= HID_CONNECT_HIDDEV_FORCE; - - ret = hid_hw_start(hdev, connect_mask); - if (ret) { - hid_err(hdev, "hw start failed\n"); - return ret; - } + int ret; ret = sony_set_device_id(sc); if (ret < 0) { @@ -2427,6 +2372,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) } } + /* + * The Dualshock 4 touchpad supports 2 touches and has a + * resolution of 1920x942 (44.86 dots/mm). + */ + ret = sony_register_touchpad(hidinput, 2, 1920, 942); + if (ret) { + hid_err(sc->hdev, + "Unable to initialize multi-touch slots: %d\n", + ret); + return ret; + } + sony_init_output_report(sc, dualshock4_send_output_report); } else if (sc->quirks & MOTION_CONTROLLER) { sony_init_output_report(sc, motion_send_output_report); @@ -2482,6 +2439,48 @@ err_stop: return ret; } +static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) +{ + int ret; + unsigned long quirks = id->driver_data; + struct sony_sc *sc; + unsigned int connect_mask = HID_CONNECT_DEFAULT; + + if (!strcmp(hdev->name, "FutureMax Dance Mat")) + quirks |= FUTUREMAX_DANCE_MAT; + + sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL); + if (sc == NULL) { + hid_err(hdev, "can't alloc sony descriptor\n"); + return -ENOMEM; + } + + spin_lock_init(&sc->lock); + + sc->quirks = quirks; + hid_set_drvdata(hdev, sc); + sc->hdev = hdev; + + ret = hid_parse(hdev); + if (ret) { + hid_err(hdev, "parse failed\n"); + return ret; + } + + if (sc->quirks & VAIO_RDESC_CONSTANT) + connect_mask |= HID_CONNECT_HIDDEV_FORCE; + else if (sc->quirks & SIXAXIS_CONTROLLER) + connect_mask |= HID_CONNECT_HIDDEV_FORCE; + + ret = hid_hw_start(hdev, connect_mask); + if (ret) { + hid_err(hdev, "hw start failed\n"); + return ret; + } + + return ret; +} + static void sony_remove(struct hid_device *hdev) { struct sony_sc *sc = hid_get_drvdata(hdev);