* [PATCH 2/5] Input: pxrc - fix freeing URB on device teardown
2018-07-24 2:35 [PATCH 1/5] Input: pxrc - do not store USB device in private struct Dmitry Torokhov
@ 2018-07-24 2:35 ` Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 3/5] Input: pxrc - move module device table closer to where it is used Dmitry Torokhov
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2018-07-24 2:35 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: linux-input, linux-kernel, Alexey Khoroshilov
URB is the only resource that is not managed, and thus is destroyed too early,
before we unregister input device and stop URB in pxrc_close(). To fix it let's
install custom devm handler to free the URB at the right time in devm unwind
sequence.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/pxrc.c | 66 ++++++++++++++++++++-----------------------
1 file changed, 30 insertions(+), 36 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 000b9b7da744..1560f0e39c34 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -119,49 +119,52 @@ static void pxrc_close(struct input_dev *input)
mutex_unlock(&pxrc->pm_mutex);
}
+static void pxrc_free_urb(void *_pxrc)
+{
+ struct pxrc *pxrc = _pxrc;
+
+ usb_free_urb(pxrc->urb);
+}
+
static int pxrc_usb_init(struct pxrc *pxrc)
{
struct usb_device *udev = interface_to_usbdev(pxrc->intf);
struct usb_endpoint_descriptor *epirq;
unsigned int pipe;
- int retval;
+ int error;
/* Set up the endpoint information */
/* This device only has an interrupt endpoint */
- retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
- NULL, NULL, &epirq, NULL);
- if (retval) {
- dev_err(&pxrc->intf->dev,
- "Could not find endpoint\n");
- goto error;
+ error = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
+ NULL, NULL, &epirq, NULL);
+ if (error) {
+ dev_err(&pxrc->intf->dev, "Could not find endpoint\n");
+ return error;
}
pxrc->bsize = usb_endpoint_maxp(epirq);
pxrc->epaddr = epirq->bEndpointAddress;
pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
- if (!pxrc->data) {
- retval = -ENOMEM;
- goto error;
- }
+ if (!pxrc->data)
+ return -ENOMEM;
usb_set_intfdata(pxrc->intf, pxrc);
usb_make_path(udev, pxrc->phys, sizeof(pxrc->phys));
strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!pxrc->urb) {
- retval = -ENOMEM;
- goto error;
- }
+ if (!pxrc->urb)
+ return -ENOMEM;
+
+ error = devm_add_action_or_reset(&pxrc->intf->dev, pxrc_free_urb, pxrc);
+ if (error)
+ return error;
pipe = usb_rcvintpipe(udev, pxrc->epaddr),
usb_fill_int_urb(pxrc->urb, udev, pipe, pxrc->data, pxrc->bsize,
pxrc_usb_irq, pxrc, 1);
-error:
- return retval;
-
-
+ return 0;
}
static int pxrc_input_init(struct pxrc *pxrc)
@@ -197,7 +200,7 @@ static int pxrc_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct pxrc *pxrc;
- int retval;
+ int error;
pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
if (!pxrc)
@@ -206,29 +209,20 @@ static int pxrc_probe(struct usb_interface *intf,
mutex_init(&pxrc->pm_mutex);
pxrc->intf = intf;
- retval = pxrc_usb_init(pxrc);
- if (retval)
- goto error;
+ error = pxrc_usb_init(pxrc);
+ if (error)
+ return error;
- retval = pxrc_input_init(pxrc);
- if (retval)
- goto err_free_urb;
+ error = pxrc_input_init(pxrc);
+ if (error)
+ return error;
return 0;
-
-err_free_urb:
- usb_free_urb(pxrc->urb);
-
-error:
- return retval;
}
static void pxrc_disconnect(struct usb_interface *intf)
{
- struct pxrc *pxrc = usb_get_intfdata(intf);
-
- usb_free_urb(pxrc->urb);
- usb_set_intfdata(intf, NULL);
+ /* All driver resources are devm-managed. */
}
static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/5] Input: pxrc - move module device table closer to where it is used
2018-07-24 2:35 [PATCH 1/5] Input: pxrc - do not store USB device in private struct Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 2/5] Input: pxrc - fix freeing URB on device teardown Dmitry Torokhov
@ 2018-07-24 2:35 ` Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 4/5] Input: pxrc - do not store unneeded data in driver structure Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 5/5] Input: pxrc - flatten probe code Dmitry Torokhov
3 siblings, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2018-07-24 2:35 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: linux-input, linux-kernel, Alexey Khoroshilov
There is no need to have the device table first in the file.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/pxrc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 1560f0e39c34..327b5ef8515f 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -19,12 +19,6 @@
#define PXRC_VENDOR_ID (0x1781)
#define PXRC_PRODUCT_ID (0x0898)
-static const struct usb_device_id pxrc_table[] = {
- { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
- { }
-};
-MODULE_DEVICE_TABLE(usb, pxrc_table);
-
struct pxrc {
struct input_dev *input;
struct usb_interface *intf;
@@ -277,6 +271,12 @@ static int pxrc_reset_resume(struct usb_interface *intf)
return pxrc_resume(intf);
}
+static const struct usb_device_id pxrc_table[] = {
+ { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
+ { }
+};
+MODULE_DEVICE_TABLE(usb, pxrc_table);
+
static struct usb_driver pxrc_driver = {
.name = "pxrc",
.probe = pxrc_probe,
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 4/5] Input: pxrc - do not store unneeded data in driver structure
2018-07-24 2:35 [PATCH 1/5] Input: pxrc - do not store USB device in private struct Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 2/5] Input: pxrc - fix freeing URB on device teardown Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 3/5] Input: pxrc - move module device table closer to where it is used Dmitry Torokhov
@ 2018-07-24 2:35 ` Dmitry Torokhov
2018-07-24 2:35 ` [PATCH 5/5] Input: pxrc - flatten probe code Dmitry Torokhov
3 siblings, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2018-07-24 2:35 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: linux-input, linux-kernel, Alexey Khoroshilov
There is no need to store data buffer size, pointer to the buffer, or endpoint
address in pxrc structure, as they are either only needed during setup, or are
available from elsewhere.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/pxrc.c | 35 +++++++++++++++++------------------
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 327b5ef8515f..887a0df9d9a7 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -25,15 +25,13 @@ struct pxrc {
struct urb *urb;
struct mutex pm_mutex;
bool is_open;
- __u8 epaddr;
char phys[64];
- unsigned char *data;
- size_t bsize;
};
static void pxrc_usb_irq(struct urb *urb)
{
struct pxrc *pxrc = urb->context;
+ u8 *data = urb->transfer_buffer;
int error;
switch (urb->status) {
@@ -61,15 +59,15 @@ static void pxrc_usb_irq(struct urb *urb)
}
if (urb->actual_length == 8) {
- input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
- input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
- input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
- input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
- input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
- input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
- input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
-
- input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
+ input_report_abs(pxrc->input, ABS_X, data[0]);
+ input_report_abs(pxrc->input, ABS_Y, data[2]);
+ input_report_abs(pxrc->input, ABS_RX, data[3]);
+ input_report_abs(pxrc->input, ABS_RY, data[4]);
+ input_report_abs(pxrc->input, ABS_RUDDER, data[5]);
+ input_report_abs(pxrc->input, ABS_THROTTLE, data[6]);
+ input_report_abs(pxrc->input, ABS_MISC, data[7]);
+
+ input_report_key(pxrc->input, BTN_A, data[1]);
}
exit:
@@ -124,6 +122,8 @@ static int pxrc_usb_init(struct pxrc *pxrc)
{
struct usb_device *udev = interface_to_usbdev(pxrc->intf);
struct usb_endpoint_descriptor *epirq;
+ size_t xfer_size;
+ void *xfer_buf;
unsigned int pipe;
int error;
@@ -136,10 +136,9 @@ static int pxrc_usb_init(struct pxrc *pxrc)
return error;
}
- pxrc->bsize = usb_endpoint_maxp(epirq);
- pxrc->epaddr = epirq->bEndpointAddress;
- pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
- if (!pxrc->data)
+ xfer_size = usb_endpoint_maxp(epirq);
+ xfer_buf = devm_kmalloc(&pxrc->intf->dev, xfer_size, GFP_KERNEL);
+ if (!xfer_buf)
return -ENOMEM;
usb_set_intfdata(pxrc->intf, pxrc);
@@ -154,8 +153,8 @@ static int pxrc_usb_init(struct pxrc *pxrc)
if (error)
return error;
- pipe = usb_rcvintpipe(udev, pxrc->epaddr),
- usb_fill_int_urb(pxrc->urb, udev, pipe, pxrc->data, pxrc->bsize,
+ pipe = usb_rcvintpipe(udev, epirq->bEndpointAddress),
+ usb_fill_int_urb(pxrc->urb, udev, pipe, xfer_buf, xfer_size,
pxrc_usb_irq, pxrc, 1);
return 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 5/5] Input: pxrc - flatten probe code
2018-07-24 2:35 [PATCH 1/5] Input: pxrc - do not store USB device in private struct Dmitry Torokhov
` (2 preceding siblings ...)
2018-07-24 2:35 ` [PATCH 4/5] Input: pxrc - do not store unneeded data in driver structure Dmitry Torokhov
@ 2018-07-24 2:35 ` Dmitry Torokhov
3 siblings, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2018-07-24 2:35 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: linux-input, linux-kernel, Alexey Khoroshilov
Instead of splitting probe code into separate USB and input setup, flatten it.
This allows for easier inspection of order of set up steps, since the probe code
is reasonably small.
Move input-related initialization (phys) from USB block to input block.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/pxrc.c | 84 ++++++++++++++++++-------------------------
1 file changed, 35 insertions(+), 49 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 887a0df9d9a7..ea2bf5951d67 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -3,7 +3,6 @@
* Driver for Phoenix RC Flight Controller Adapter
*
* Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
- *
*/
#include <linux/kernel.h>
@@ -16,8 +15,8 @@
#include <linux/mutex.h>
#include <linux/input.h>
-#define PXRC_VENDOR_ID (0x1781)
-#define PXRC_PRODUCT_ID (0x0898)
+#define PXRC_VENDOR_ID 0x1781
+#define PXRC_PRODUCT_ID 0x0898
struct pxrc {
struct input_dev *input;
@@ -118,59 +117,66 @@ static void pxrc_free_urb(void *_pxrc)
usb_free_urb(pxrc->urb);
}
-static int pxrc_usb_init(struct pxrc *pxrc)
+static int pxrc_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
{
- struct usb_device *udev = interface_to_usbdev(pxrc->intf);
+ struct usb_device *udev = interface_to_usbdev(intf);
+ struct pxrc *pxrc;
struct usb_endpoint_descriptor *epirq;
size_t xfer_size;
void *xfer_buf;
- unsigned int pipe;
int error;
- /* Set up the endpoint information */
- /* This device only has an interrupt endpoint */
- error = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
+ /*
+ * Locate the endpoint information. This device only has an
+ * interrupt endpoint.
+ */
+ error = usb_find_common_endpoints(intf->cur_altsetting,
NULL, NULL, &epirq, NULL);
if (error) {
- dev_err(&pxrc->intf->dev, "Could not find endpoint\n");
+ dev_err(&intf->dev, "Could not find endpoint\n");
return error;
}
- xfer_size = usb_endpoint_maxp(epirq);
- xfer_buf = devm_kmalloc(&pxrc->intf->dev, xfer_size, GFP_KERNEL);
- if (!xfer_buf)
+ pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
+ if (!pxrc)
return -ENOMEM;
+ mutex_init(&pxrc->pm_mutex);
+ pxrc->intf = intf;
+
usb_set_intfdata(pxrc->intf, pxrc);
- usb_make_path(udev, pxrc->phys, sizeof(pxrc->phys));
- strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
+
+ xfer_size = usb_endpoint_maxp(epirq);
+ xfer_buf = devm_kmalloc(&intf->dev, xfer_size, GFP_KERNEL);
+ if (!xfer_buf)
+ return -ENOMEM;
pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
if (!pxrc->urb)
return -ENOMEM;
- error = devm_add_action_or_reset(&pxrc->intf->dev, pxrc_free_urb, pxrc);
+ error = devm_add_action_or_reset(&intf->dev, pxrc_free_urb, pxrc);
if (error)
return error;
- pipe = usb_rcvintpipe(udev, epirq->bEndpointAddress),
- usb_fill_int_urb(pxrc->urb, udev, pipe, xfer_buf, xfer_size,
- pxrc_usb_irq, pxrc, 1);
+ usb_fill_int_urb(pxrc->urb, udev,
+ usb_rcvintpipe(udev, epirq->bEndpointAddress),
+ xfer_buf, xfer_size, pxrc_usb_irq, pxrc, 1);
- return 0;
-}
-
-static int pxrc_input_init(struct pxrc *pxrc)
-{
- pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
- if (pxrc->input == NULL) {
- dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
+ pxrc->input = devm_input_allocate_device(&intf->dev);
+ if (!pxrc->input) {
+ dev_err(&intf->dev, "couldn't allocate input device\n");
return -ENOMEM;
}
pxrc->input->name = "PXRC Flight Controller Adapter";
+
+ usb_make_path(udev, pxrc->phys, sizeof(pxrc->phys));
+ strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
pxrc->input->phys = pxrc->phys;
- usb_to_input_id(interface_to_usbdev(pxrc->intf), &pxrc->input->id);
+
+ usb_to_input_id(udev, &pxrc->input->id);
pxrc->input->open = pxrc_open;
pxrc->input->close = pxrc_close;
@@ -186,27 +192,7 @@ static int pxrc_input_init(struct pxrc *pxrc)
input_set_drvdata(pxrc->input, pxrc);
- return input_register_device(pxrc->input);
-}
-
-static int pxrc_probe(struct usb_interface *intf,
- const struct usb_device_id *id)
-{
- struct pxrc *pxrc;
- int error;
-
- pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
- if (!pxrc)
- return -ENOMEM;
-
- mutex_init(&pxrc->pm_mutex);
- pxrc->intf = intf;
-
- error = pxrc_usb_init(pxrc);
- if (error)
- return error;
-
- error = pxrc_input_init(pxrc);
+ error = input_register_device(pxrc->input);
if (error)
return error;
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread