[PATCH 1/5] Input: pxrc - do not store USB device in private struct

[PATCH 1/5] Input: pxrc - do not store USB device in private struct

[Dmitry Torokhov]

From: Marcus Folkesson <marcus.folkesson@gmail.com>

The USB device is only needed during setup, so put it back after
initialization and do not store it in our private struct.

Also, the USB device is a parent of USB interface so our driver
model rules ensure that USB device should not disappear while
interface device is still there; there is no need to take reference
to the USB device.

Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Patchwork-Id: 10526903
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/joystick/pxrc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 07a0dbd3ced2..000b9b7da744 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -27,7 +27,6 @@ MODULE_DEVICE_TABLE(usb, pxrc_table);
 
 struct pxrc {
 	struct input_dev	*input;
-	struct usb_device	*udev;
 	struct usb_interface	*intf;
 	struct urb		*urb;
 	struct mutex		pm_mutex;
@@ -122,6 +121,7 @@ static void pxrc_close(struct input_dev *input)
 
 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;
@@ -145,7 +145,7 @@ static int pxrc_usb_init(struct pxrc *pxrc)
 	}
 
 	usb_set_intfdata(pxrc->intf, pxrc);
-	usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
+	usb_make_path(udev, pxrc->phys, sizeof(pxrc->phys));
 	strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
 
 	pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
@@ -154,9 +154,9 @@ static int pxrc_usb_init(struct pxrc *pxrc)
 		goto error;
 	}
 
-	pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
-	usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
-						pxrc_usb_irq, pxrc, 1);
+	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;
@@ -174,7 +174,7 @@ static int pxrc_input_init(struct pxrc *pxrc)
 
 	pxrc->input->name = "PXRC Flight Controller Adapter";
 	pxrc->input->phys = pxrc->phys;
-	usb_to_input_id(pxrc->udev, &pxrc->input->id);
+	usb_to_input_id(interface_to_usbdev(pxrc->intf), &pxrc->input->id);
 
 	pxrc->input->open = pxrc_open;
 	pxrc->input->close = pxrc_close;
@@ -204,7 +204,6 @@ static int pxrc_probe(struct usb_interface *intf,
 		return -ENOMEM;
 
 	mutex_init(&pxrc->pm_mutex);
-	pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
 	pxrc->intf = intf;
 
 	retval = pxrc_usb_init(pxrc);
-- 
2.11.0

[PATCH 2/5] Input: pxrc - fix freeing URB on device teardown

[Dmitry Torokhov]

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

[PATCH 3/5] Input: pxrc - move module device table closer to where it is used

[Dmitry Torokhov]

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

[PATCH 4/5] Input: pxrc - do not store unneeded data in driver structure

[Dmitry Torokhov]

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

[PATCH 5/5] Input: pxrc - flatten probe code

[Dmitry Torokhov]

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