* [PATCH] input: pxrc - do not store USB device in private struct
@ 2018-07-16 14:40 Marcus Folkesson
2018-07-24 2:38 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Marcus Folkesson @ 2018-07-16 14:40 UTC (permalink / raw)
To: Dmitry Torokhov, Marcus Folkesson, Alexey Khoroshilov
Cc: linux-input, linux-kernel
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.
So not keep a refcount on the device is safe.
Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
drivers/input/joystick/pxrc.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index 07a0dbd3ced2..46a7acb747bf 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;
@@ -120,7 +119,7 @@ static void pxrc_close(struct input_dev *input)
mutex_unlock(&pxrc->pm_mutex);
}
-static int pxrc_usb_init(struct pxrc *pxrc)
+static int pxrc_usb_init(struct pxrc *pxrc, struct usb_device *udev)
{
struct usb_endpoint_descriptor *epirq;
unsigned int pipe;
@@ -145,7 +144,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,8 +153,8 @@ 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,
+ pipe = usb_rcvintpipe(udev, pxrc->epaddr),
+ usb_fill_int_urb(pxrc->urb, udev, pipe, pxrc->data, pxrc->bsize,
pxrc_usb_irq, pxrc, 1);
error:
@@ -164,7 +163,7 @@ static int pxrc_usb_init(struct pxrc *pxrc)
}
-static int pxrc_input_init(struct pxrc *pxrc)
+static int pxrc_input_init(struct pxrc *pxrc, struct usb_device *udev)
{
pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
if (pxrc->input == NULL) {
@@ -174,7 +173,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(udev, &pxrc->input->id);
pxrc->input->open = pxrc_open;
pxrc->input->close = pxrc_close;
@@ -197,6 +196,7 @@ static int pxrc_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct pxrc *pxrc;
+ struct usb_device *udev;
int retval;
pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
@@ -204,23 +204,25 @@ static int pxrc_probe(struct usb_interface *intf,
return -ENOMEM;
mutex_init(&pxrc->pm_mutex);
- pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
+ udev = usb_get_dev(interface_to_usbdev(intf));
pxrc->intf = intf;
- retval = pxrc_usb_init(pxrc);
+ retval = pxrc_usb_init(pxrc, udev);
if (retval)
goto error;
- retval = pxrc_input_init(pxrc);
+ retval = pxrc_input_init(pxrc, udev);
if (retval)
goto err_free_urb;
+ usb_put_dev(udev);
return 0;
err_free_urb:
usb_free_urb(pxrc->urb);
error:
+ usb_put_dev(udev);
return retval;
}
--
2.18.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] input: pxrc - do not store USB device in private struct
2018-07-16 14:40 [PATCH] input: pxrc - do not store USB device in private struct Marcus Folkesson
@ 2018-07-24 2:38 ` Dmitry Torokhov
2018-07-24 18:09 ` Marcus Folkesson
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2018-07-24 2:38 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Alexey Khoroshilov, linux-input, linux-kernel
Hi Marcus,
On Mon, Jul 16, 2018 at 04:40:14PM +0200, Marcus Folkesson wrote:
> 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.
> So not keep a refcount on the device is safe.
>
> Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
> drivers/input/joystick/pxrc.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> index 07a0dbd3ced2..46a7acb747bf 100644
> --- a/drivers/input/joystick/pxrc.c
> +++ b/drivers/input/joystick/pxrc.c
...
> @@ -204,23 +204,25 @@ static int pxrc_probe(struct usb_interface *intf,
> return -ENOMEM;
>
> mutex_init(&pxrc->pm_mutex);
> - pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> + udev = usb_get_dev(interface_to_usbdev(intf));
There is really no need to "get" device for the probe duration, or in
general, when you are not storing the reference to it.
I posted series with an updated version of this patch plus couple more
cleanups/fixes, and would appreciate if you could give it a spin.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] input: pxrc - do not store USB device in private struct
2018-07-24 2:38 ` Dmitry Torokhov
@ 2018-07-24 18:09 ` Marcus Folkesson
2018-07-25 23:37 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Marcus Folkesson @ 2018-07-24 18:09 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Alexey Khoroshilov, linux-input, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]
Hello Dmitry,
On Tue, Jul 24, 2018 at 02:38:04AM +0000, Dmitry Torokhov wrote:
> Hi Marcus,
>
> On Mon, Jul 16, 2018 at 04:40:14PM +0200, Marcus Folkesson wrote:
> > 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.
> > So not keep a refcount on the device is safe.
> >
> > Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> > drivers/input/joystick/pxrc.c | 22 ++++++++++++----------
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> > index 07a0dbd3ced2..46a7acb747bf 100644
> > --- a/drivers/input/joystick/pxrc.c
> > +++ b/drivers/input/joystick/pxrc.c
> ...
>
> > @@ -204,23 +204,25 @@ static int pxrc_probe(struct usb_interface *intf,
> > return -ENOMEM;
> >
> > mutex_init(&pxrc->pm_mutex);
> > - pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> > + udev = usb_get_dev(interface_to_usbdev(intf));
>
> There is really no need to "get" device for the probe duration, or in
> general, when you are not storing the reference to it.
>
> I posted series with an updated version of this patch plus couple more
> cleanups/fixes, and would appreciate if you could give it a spin.
Thank you for doing this.
I have reviewed the patchset and tested on real hardware, and it looks good
to me.
For what it's worth:
Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Tested-by: Marcus Folkesson <marcus.folkesson@gmail.com>
On the whole patchset.
>
> Thanks.
>
> --
> Dmitry
Best regards
Marcus Folkesson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] input: pxrc - do not store USB device in private struct
2018-07-24 18:09 ` Marcus Folkesson
@ 2018-07-25 23:37 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2018-07-25 23:37 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Alexey Khoroshilov, linux-input, linux-kernel
On Tue, Jul 24, 2018 at 08:09:59PM +0200, Marcus Folkesson wrote:
> Hello Dmitry,
>
> On Tue, Jul 24, 2018 at 02:38:04AM +0000, Dmitry Torokhov wrote:
> > Hi Marcus,
> >
> > On Mon, Jul 16, 2018 at 04:40:14PM +0200, Marcus Folkesson wrote:
> > > 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.
> > > So not keep a refcount on the device is safe.
> > >
> > > Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > ---
> > > drivers/input/joystick/pxrc.c | 22 ++++++++++++----------
> > > 1 file changed, 12 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> > > index 07a0dbd3ced2..46a7acb747bf 100644
> > > --- a/drivers/input/joystick/pxrc.c
> > > +++ b/drivers/input/joystick/pxrc.c
> > ...
> >
> > > @@ -204,23 +204,25 @@ static int pxrc_probe(struct usb_interface *intf,
> > > return -ENOMEM;
> > >
> > > mutex_init(&pxrc->pm_mutex);
> > > - pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> > > + udev = usb_get_dev(interface_to_usbdev(intf));
> >
> > There is really no need to "get" device for the probe duration, or in
> > general, when you are not storing the reference to it.
> >
> > I posted series with an updated version of this patch plus couple more
> > cleanups/fixes, and would appreciate if you could give it a spin.
>
> Thank you for doing this.
>
> I have reviewed the patchset and tested on real hardware, and it looks good
> to me.
>
> For what it's worth:
>
> Reviewed-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Tested-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>
> On the whole patchset.
Excellent, thank you.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-07-25 23:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-16 14:40 [PATCH] input: pxrc - do not store USB device in private struct Marcus Folkesson
2018-07-24 2:38 ` Dmitry Torokhov
2018-07-24 18:09 ` Marcus Folkesson
2018-07-25 23:37 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).