| Message ID | 1390087466-9898-1-git-send-email-khoroshilov@ispras.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote: > There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev() > anywhere in the driver. > > The patch adds usb_get_dev() to failure handling code of gtco_probe() > and to gtco_disconnect((). Hmm, I think gtco should simply not use usb_get_dev() in the first place. Thanks.
On 21.01.2014 23:59, Dmitry Torokhov wrote: > On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote: >> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev() >> anywhere in the driver. >> >> The patch adds usb_get_dev() to failure handling code of gtco_probe() >> and to gtco_disconnect((). > Hmm, I think gtco should simply not use usb_get_dev() in the first > place. > > Thanks. Dear Dmitry, Could you please clarify why usb_get_dev() not needed here? We store reference to usb_dev in gtco structure, so we should refcount it. What is wrong in this reasoning? Thanks, Alexey -- 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
Hi Alexey, On Mon, Jan 27, 2014 at 10:31:36AM +0400, Alexey Khoroshilov wrote: > On 21.01.2014 23:59, Dmitry Torokhov wrote: > > On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote: > >> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev() > >> anywhere in the driver. > >> > >> The patch adds usb_get_dev() to failure handling code of gtco_probe() > >> and to gtco_disconnect((). > > Hmm, I think gtco should simply not use usb_get_dev() in the first > > place. > > > > Thanks. > Dear Dmitry, > > Could you please clarify why usb_get_dev() not needed here? > We store reference to usb_dev in gtco structure, so we should refcount it. > What is wrong in this reasoning? The lifetime of gtco structure is already directly tied to lifetime of usb_dev: when destroying usb_dev driver core will call remove() function of currently bound driver (in our case gtco) which will destroy gtco memory. Taking additional reference is not needed here. Hope this helps.
On 27.01.2014 10:54, Dmitry Torokhov wrote: > Hi Alexey, > > On Mon, Jan 27, 2014 at 10:31:36AM +0400, Alexey Khoroshilov wrote: >> On 21.01.2014 23:59, Dmitry Torokhov wrote: >>> On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote: >>>> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev() >>>> anywhere in the driver. >>>> >>>> The patch adds usb_get_dev() to failure handling code of gtco_probe() >>>> and to gtco_disconnect((). >>> Hmm, I think gtco should simply not use usb_get_dev() in the first >>> place. >>> >>> Thanks. >> Dear Dmitry, >> >> Could you please clarify why usb_get_dev() not needed here? >> We store reference to usb_dev in gtco structure, so we should refcount it. >> What is wrong in this reasoning? > The lifetime of gtco structure is already directly tied to lifetime of > usb_dev: when destroying usb_dev driver core will call remove() function > of currently bound driver (in our case gtco) which will destroy gtco > memory. > > Taking additional reference is not needed here. > > Hope this helps. Thank you, that helps a lot. By the way, usb_skeleton suggests to use usb_get_dev()/usb_put_dev() nevertheless. Greg, may be it makes sense to fix usb_skeleton as well? -- Alexey -- 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 Mon, Jan 27, 2014 at 02:29:23PM +0400, Alexey Khoroshilov wrote: > On 27.01.2014 10:54, Dmitry Torokhov wrote: > > Hi Alexey, > > > > On Mon, Jan 27, 2014 at 10:31:36AM +0400, Alexey Khoroshilov wrote: > >> On 21.01.2014 23:59, Dmitry Torokhov wrote: > >>> On Sun, Jan 19, 2014 at 03:24:26AM +0400, Alexey Khoroshilov wrote: > >>>> There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev() > >>>> anywhere in the driver. > >>>> > >>>> The patch adds usb_get_dev() to failure handling code of gtco_probe() > >>>> and to gtco_disconnect((). > >>> Hmm, I think gtco should simply not use usb_get_dev() in the first > >>> place. > >>> > >>> Thanks. > >> Dear Dmitry, > >> > >> Could you please clarify why usb_get_dev() not needed here? > >> We store reference to usb_dev in gtco structure, so we should refcount it. > >> What is wrong in this reasoning? > > The lifetime of gtco structure is already directly tied to lifetime of > > usb_dev: when destroying usb_dev driver core will call remove() function > > of currently bound driver (in our case gtco) which will destroy gtco > > memory. > > > > Taking additional reference is not needed here. > > > > Hope this helps. > Thank you, that helps a lot. > > By the way, usb_skeleton suggests to use usb_get_dev()/usb_put_dev() > nevertheless. > > Greg, may be it makes sense to fix usb_skeleton as well? Patches are always welcome. -- 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
diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c index 29e01ab6859f..6ec8a3a04672 100644 --- a/drivers/input/tablet/gtco.c +++ b/drivers/input/tablet/gtco.c @@ -858,7 +858,7 @@ static int gtco_probe(struct usb_interface *usbinterface, if (!gtco->buffer) { dev_err(&usbinterface->dev, "No more memory for us buffers\n"); error = -ENOMEM; - goto err_free_devs; + goto err_put_usb; } /* Allocate URB for reports */ @@ -990,6 +990,8 @@ static int gtco_probe(struct usb_interface *usbinterface, err_free_buf: usb_free_coherent(gtco->usbdev, REPORT_MAX_SIZE, gtco->buffer, gtco->buf_dma); + err_put_usb: + usb_put_dev(gtco->usbdev); err_free_devs: input_free_device(input_dev); kfree(gtco); @@ -1013,6 +1015,7 @@ static void gtco_disconnect(struct usb_interface *interface) usb_free_urb(gtco->urbinfo); usb_free_coherent(gtco->usbdev, REPORT_MAX_SIZE, gtco->buffer, gtco->buf_dma); + usb_put_dev(gtco->usbdev); kfree(gtco); }
There is usb_get_dev() in gtco_probe(), but there is no usb_put_dev() anywhere in the driver. The patch adds usb_get_dev() to failure handling code of gtco_probe() and to gtco_disconnect((). Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/input/tablet/gtco.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)