Input: pxrc - do not store USB device in private struct

PATCH New Dmitry Torokhov 1 participant 5 messages Jul 24, 2018

Subsystems (3)

HID Input drivers Phoenix RC flight controller adapter

lore

Share & tools

lore

Dmitry Torokhov

author maintainer

2018-07-24 02:35 UTC #0 lore raw

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

From: Marcus Folkesson <[email protected]>

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 <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

Patchwork-Id: 10526903

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/input/joystick/pxrc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

drivers/input/joystick/pxrc.c +6 -7

index 07a0dbd3ced2..000b9b7da744 100644
@@ -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);

Signature · 1 line

2.11.0

Dmitry Torokhov

author maintainer

2018-07-24 02:35 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/input/joystick/pxrc.c | 66 ++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

drivers/input/joystick/pxrc.c +30 ~~-36~~

index 000b9b7da744..1560f0e39c34 100644
@@ -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)

Signature · 1 line

2.11.0

Dmitry Torokhov

author maintainer

2018-07-24 02:35 UTC #2 lore raw

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

In reply to Dmitry Torokhov

There is no need to have the device table first in the file.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/input/joystick/pxrc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/input/joystick/pxrc.c +6 -6

index 1560f0e39c34..327b5ef8515f 100644
@@ -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,

Signature · 1 line

2.11.0

Dmitry Torokhov

author maintainer

2018-07-24 02:35 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/input/joystick/pxrc.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

drivers/input/joystick/pxrc.c +17 ~~-18~~

index 327b5ef8515f..887a0df9d9a7 100644
@@ -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;

Signature · 1 line

2.11.0

Dmitry Torokhov

author maintainer

2018-07-24 02:35 UTC #4 lore raw

PATCH 5/5 Input: pxrc - flatten probe code

In reply to 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 <[email protected]>

---
 drivers/input/joystick/pxrc.c | 84 ++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 49 deletions(-)

drivers/input/joystick/pxrc.c +35 ~~-49~~

index 887a0df9d9a7..ea2bf5951d67 100644
@@ -3,7 +3,6 @@
  * Driver for Phoenix RC Flight Controller Adapter
  *
  * Copyright (C) 2018 Marcus Folkesson <[email protected]>
- *
  */
 
 #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;

Signature · 1 line

2.11.0