| Message ID | 1398175209-9565-12-git-send-email-madcatxster@devoid-pointer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote: > static int holtekff_play(struct input_dev *dev, void *data, > - struct ff_effect *effect) > + const struct mlnx_effect_command *command) > { > struct hid_device *hid = input_get_drvdata(dev); > struct holtekff_device *holtekff = data; > + const struct mlnx_rumble_force *rumble_force = > &command->u.rumble_force; > int left, right; > /* effect type 1, length 65535 msec */ > u8 buf[HOLTEKFF_MSG_LENGTH] = > { 0x01, 0x01, 0xff, 0xff, 0x10, 0xe0, 0x00 }; On the kernel stack. > > - left = effect->u.rumble.strong_magnitude; > - right = effect->u.rumble.weak_magnitude; > - dbg_hid("called with 0x%04x 0x%04x\n", left, right); > + switch (command->cmd) { > + case MLNX_START_RUMBLE: > + left = rumble_force->strong; > + right = rumble_force->weak; > + dbg_hid("called with 0x%04x 0x%04x\n", left, right); > > - if (!left && !right) { > - holtekff_send(holtekff, hid, stop_all6); > - return 0; > - } > + if (!left && !right) { > + holtekff_send(holtekff, hid, stop_all6); > + return 0; > + } > > - if (left) > - buf[1] |= 0x80; > - if (right) > - buf[1] |= 0x40; > + if (left) > + buf[1] |= 0x80; > + if (right) > + buf[1] |= 0x40; > > - /* The device takes a single magnitude, so we just sum them > up. */ > - buf[6] = min(0xf, (left >> 12) + (right >> 12)); > + /* The device takes a single magnitude, so we just sum > them up. */ > + buf[6] = min(0xf, (left >> 12) + (right >> 12)); > > - holtekff_send(holtekff, hid, buf); > - holtekff_send(holtekff, hid, start_effect_1); > + holtekff_send(holtekff, hid, buf); > + holtekff_send(holtekff, hid, start_effect_1); > + return 0; > + case MLNX_STOP_RUMBLE: > + holtekff_send(holtekff, hid, stop_all6); > + return 0; > + default: > + return -EINVAL; > + } > > return 0; > } This looks very much like doing DMA on the kernel stack. That is very strictly forbidden. The bug is also in the current code, but would you care to fix it up? Regards Oliver -- 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 Wednesday 23 of April 2014 14:17:50 Oliver Neukum wrote: > On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote: > > static int holtekff_play(struct input_dev *dev, void *data, > > > > - struct ff_effect *effect) > > + const struct mlnx_effect_command *command) > > > > { > > > > struct hid_device *hid = input_get_drvdata(dev); > > struct holtekff_device *holtekff = data; > > > > + const struct mlnx_rumble_force *rumble_force = > > &command->u.rumble_force; > > > > int left, right; > > /* effect type 1, length 65535 msec */ > > u8 buf[HOLTEKFF_MSG_LENGTH] = > > > > { 0x01, 0x01, 0xff, 0xff, 0x10, 0xe0, 0x00 }; > > On the kernel stack. > > > - left = effect->u.rumble.strong_magnitude; > > - right = effect->u.rumble.weak_magnitude; > > - dbg_hid("called with 0x%04x 0x%04x\n", left, right); > > + switch (command->cmd) { > > + case MLNX_START_RUMBLE: > > + left = rumble_force->strong; > > + right = rumble_force->weak; > > + dbg_hid("called with 0x%04x 0x%04x\n", left, right); > > > > - if (!left && !right) { > > - holtekff_send(holtekff, hid, stop_all6); > > - return 0; > > - } > > + if (!left && !right) { > > + holtekff_send(holtekff, hid, stop_all6); > > + return 0; > > + } > > > > - if (left) > > - buf[1] |= 0x80; > > - if (right) > > - buf[1] |= 0x40; > > + if (left) > > + buf[1] |= 0x80; > > + if (right) > > + buf[1] |= 0x40; > > > > - /* The device takes a single magnitude, so we just sum them > > up. */ > > - buf[6] = min(0xf, (left >> 12) + (right >> 12)); > > + /* The device takes a single magnitude, so we just sum > > them up. */ > > + buf[6] = min(0xf, (left >> 12) + (right >> 12)); > > > > - holtekff_send(holtekff, hid, buf); > > - holtekff_send(holtekff, hid, start_effect_1); > > + holtekff_send(holtekff, hid, buf); > > + holtekff_send(holtekff, hid, start_effect_1); > > + return 0; > > + case MLNX_STOP_RUMBLE: > > + holtekff_send(holtekff, hid, stop_all6); > > + return 0; > > + default: > > + return -EINVAL; > > + } > > > > return 0; > > > > } > > This looks very much like doing DMA on the kernel stack. > That is very strictly forbidden. The bug is also in the current > code, but would you care to fix it up? Okay, I'll look into it. Michal -- 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 Wed, Apr 23, 2014 at 02:17:50PM +0200, Oliver Neukum wrote: > On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote: > > static int holtekff_play(struct input_dev *dev, void *data, > > - struct ff_effect *effect) > > + const struct mlnx_effect_command *command) > > { > > struct hid_device *hid = input_get_drvdata(dev); > > struct holtekff_device *holtekff = data; > > + const struct mlnx_rumble_force *rumble_force = > > &command->u.rumble_force; > > int left, right; > > /* effect type 1, length 65535 msec */ > > u8 buf[HOLTEKFF_MSG_LENGTH] = > > { 0x01, 0x01, 0xff, 0xff, 0x10, 0xe0, 0x00 }; > > On the kernel stack. > > > > > - left = effect->u.rumble.strong_magnitude; > > - right = effect->u.rumble.weak_magnitude; > > - dbg_hid("called with 0x%04x 0x%04x\n", left, right); > > + switch (command->cmd) { > > + case MLNX_START_RUMBLE: > > + left = rumble_force->strong; > > + right = rumble_force->weak; > > + dbg_hid("called with 0x%04x 0x%04x\n", left, right); > > > > - if (!left && !right) { > > - holtekff_send(holtekff, hid, stop_all6); > > - return 0; > > - } > > + if (!left && !right) { > > + holtekff_send(holtekff, hid, stop_all6); > > + return 0; > > + } > > > > - if (left) > > - buf[1] |= 0x80; > > - if (right) > > - buf[1] |= 0x40; > > + if (left) > > + buf[1] |= 0x80; > > + if (right) > > + buf[1] |= 0x40; > > > > - /* The device takes a single magnitude, so we just sum them > > up. */ > > - buf[6] = min(0xf, (left >> 12) + (right >> 12)); > > + /* The device takes a single magnitude, so we just sum > > them up. */ > > + buf[6] = min(0xf, (left >> 12) + (right >> 12)); > > > > - holtekff_send(holtekff, hid, buf); > > - holtekff_send(holtekff, hid, start_effect_1); > > + holtekff_send(holtekff, hid, buf); > > + holtekff_send(holtekff, hid, start_effect_1); > > + return 0; > > + case MLNX_STOP_RUMBLE: > > + holtekff_send(holtekff, hid, stop_all6); > > + return 0; > > + default: > > + return -EINVAL; > > + } > > > > return 0; > > } > > This looks very much like doing DMA on the kernel stack. It isn't: static void holtekff_send(struct holtekff_device *holtekff, struct hid_device *hid, const u8 data[HOLTEKFF_MSG_LENGTH]) { int i; for (i = 0; i < HOLTEKFF_MSG_LENGTH; i++) { holtekff->field->value[i] = data[i]; } dbg_hid("sending %7ph\n", data); hid_hw_request(hid, holtekff->field->report, HID_REQ_SET_REPORT); } And also hid layer copies data a bunch of times over into DMA-safe buffers. > That is very strictly forbidden. The bug is also in the current > code, but would you care to fix it up? > > Regards > Oliver > >
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 24b336e..d1d4e77 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -247,7 +247,7 @@ config HID_HOLTEK config HOLTEK_FF bool "Holtek On Line Grip force feedback support" depends on HID_HOLTEK - select INPUT_FF_MEMLESS + select INPUT_FF_MEMLESS_NEXT ---help--- Say Y here if you have a Holtek On Line Grip based game controller and want to have force feedback support for it. diff --git a/drivers/hid/hid-holtekff.c b/drivers/hid/hid-holtekff.c index 9325545..9c6064d 100644 --- a/drivers/hid/hid-holtekff.c +++ b/drivers/hid/hid-holtekff.c @@ -27,9 +27,12 @@ #include <linux/input.h> #include <linux/module.h> #include <linux/slab.h> +#include <linux/input/ff-memless-next.h> #include "hid-ids.h" +#define FF_UPDATE_RATE 50 + #ifdef CONFIG_HOLTEK_FF MODULE_LICENSE("GPL"); @@ -104,34 +107,44 @@ static void holtekff_send(struct holtekff_device *holtekff, } static int holtekff_play(struct input_dev *dev, void *data, - struct ff_effect *effect) + const struct mlnx_effect_command *command) { struct hid_device *hid = input_get_drvdata(dev); struct holtekff_device *holtekff = data; + const struct mlnx_rumble_force *rumble_force = &command->u.rumble_force; int left, right; /* effect type 1, length 65535 msec */ u8 buf[HOLTEKFF_MSG_LENGTH] = { 0x01, 0x01, 0xff, 0xff, 0x10, 0xe0, 0x00 }; - left = effect->u.rumble.strong_magnitude; - right = effect->u.rumble.weak_magnitude; - dbg_hid("called with 0x%04x 0x%04x\n", left, right); + switch (command->cmd) { + case MLNX_START_RUMBLE: + left = rumble_force->strong; + right = rumble_force->weak; + dbg_hid("called with 0x%04x 0x%04x\n", left, right); - if (!left && !right) { - holtekff_send(holtekff, hid, stop_all6); - return 0; - } + if (!left && !right) { + holtekff_send(holtekff, hid, stop_all6); + return 0; + } - if (left) - buf[1] |= 0x80; - if (right) - buf[1] |= 0x40; + if (left) + buf[1] |= 0x80; + if (right) + buf[1] |= 0x40; - /* The device takes a single magnitude, so we just sum them up. */ - buf[6] = min(0xf, (left >> 12) + (right >> 12)); + /* The device takes a single magnitude, so we just sum them up. */ + buf[6] = min(0xf, (left >> 12) + (right >> 12)); - holtekff_send(holtekff, hid, buf); - holtekff_send(holtekff, hid, start_effect_1); + holtekff_send(holtekff, hid, buf); + holtekff_send(holtekff, hid, start_effect_1); + return 0; + case MLNX_STOP_RUMBLE: + holtekff_send(holtekff, hid, stop_all6); + return 0; + default: + return -EINVAL; + } return 0; } @@ -171,7 +184,7 @@ static int holtekff_init(struct hid_device *hid) holtekff_send(holtekff, hid, stop_all4); holtekff_send(holtekff, hid, stop_all6); - error = input_ff_create_memless(dev, holtekff, holtekff_play); + error = input_ff_create_mlnx(dev, holtekff, holtekff_play, FF_UPDATE_RATE); if (error) { kfree(holtekff); return error;
Port hid-holtekff to ff-memless-next Signed-off-by: Michal Malý <madcatxster@devoid-pointer.net> --- drivers/hid/Kconfig | 2 +- drivers/hid/hid-holtekff.c | 47 +++++++++++++++++++++++++++++----------------- 2 files changed, 31 insertions(+), 18 deletions(-)