Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754831AbdIFXXz (ORCPT ); Wed, 6 Sep 2017 19:23:55 -0400 Received: from mail-io0-f177.google.com ([209.85.223.177]:37710 "EHLO mail-io0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752351AbdIFXXx (ORCPT ); Wed, 6 Sep 2017 19:23:53 -0400 X-Google-Smtp-Source: AOwi7QBxpeB+0Aos18ArtWk1uFbfX9GPfKiNeU9mz/SgahUDfQXlp11k9NV+8M5HeWOZrMkmsfL7SsuzyrH5PloV/qo= MIME-Version: 1.0 In-Reply-To: References: <1504222183-61202-1-git-send-email-keescook@chromium.org> <1504222183-61202-28-git-send-email-keescook@chromium.org> From: Kees Cook Date: Wed, 6 Sep 2017 16:23:51 -0700 X-Google-Sender-Auth: pqMpWrE6_lBP8r1XwmWUaPMLOnc Message-ID: Subject: Re: [PATCH 27/31] usb/gadget/snps_udc_core: Move timer initialization earlier To: Michal Nazarewicz Cc: Thomas Gleixner , Felipe Balbi , Greg Kroah-Hartman , Raviteja Garimella , "Gustavo A. R. Silva" , linux-usb@vger.kernel.org, LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4306 Lines: 119 On Sun, Sep 3, 2017 at 2:12 PM, Michal Nazarewicz wrote: > On Thu, Aug 31 2017, Kees Cook wrote: >> With timer initialization made earlier at the start, there is no reason >> to make del_timer_sync() calls conditionally, there by removing the >> assignments and tests of the .data field. >> >> Cc: Felipe Balbi >> Cc: Greg Kroah-Hartman >> Cc: Raviteja Garimella >> Cc: Michal Nazarewicz >> Cc: "Gustavo A. R. Silva" >> Cc: linux-usb@vger.kernel.org >> Signed-off-by: Kees Cook >> --- >> drivers/usb/gadget/udc/snps_udc_core.c | 16 +++++----------- >> 1 file changed, 5 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c >> index 5460e5ba1c3c..1607e901e16b 100644 >> --- a/drivers/usb/gadget/udc/snps_udc_core.c >> +++ b/drivers/usb/gadget/udc/snps_udc_core.c >> @@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev) >> stop_timer++; >> if (timer_pending(&udc_timer)) >> wait_for_completion(&on_exit); >> - if (udc_timer.data) >> - del_timer_sync(&udc_timer); >> + del_timer_sync(&udc_timer); >> /* remove pollstall timer */ >> stop_pollstall_timer++; >> if (timer_pending(&udc_pollstall_timer)) >> wait_for_completion(&on_pollstall_exit); >> - if (udc_pollstall_timer.data) >> - del_timer_sync(&udc_pollstall_timer); >> + del_timer_sync(&udc_pollstall_timer); >> udc = NULL; >> } >> EXPORT_SYMBOL_GPL(udc_remove); >> @@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev) >> u32 reg; >> int retval; >> >> - /* mark timer as not initialized */ >> - udc_timer.data = 0; >> - udc_pollstall_timer.data = 0; >> + /* timer init */ >> + setup_timer(&udc_timer, udc_timer_function, 0); >> + setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 0); >> >> /* device struct setup */ >> dev->gadget.ops = &udc_ops; >> @@ -3206,10 +3204,6 @@ int udc_probe(struct udc *dev) >> if (retval) >> goto finished; >> >> - /* timer init */ >> - setup_timer(&udc_timer, udc_timer_function, 1); >> - setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 1); >> - >> /* set SD */ >> reg = readl(&dev->regs->ctl); >> reg |= AMD_BIT(UDC_DEVCTL_SD); > > > Stupid question, is the check in udc_remove even necessary? > > udc_probe is called from udc_plat_probe: > > if (udc_probe(udc)) { > ret = -ENODEV; > goto exit_dma; > } > > If the call fails, udc_plat_probe cleans up after itself and noticeably > *does not* call udc_remove. As far as I understand, if probe callback > fails, remove callback is *not* called. Meanwhile, udc_remove is called > from the remove callback which is udc_plat_remove. So, udc_remove can > be called only if udc_probe succeeds. > > It seems to me that a better patch is: > > --- a/drivers/usb/gadget/udc/snps_udc_core.c > +++ b/drivers/usb/gadget/udc/snps_udc_core.c > @@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev) > stop_timer++; > if (timer_pending(&udc_timer)) > wait_for_completion(&on_exit); > - if (udc_timer.data) > - del_timer_sync(&udc_timer); > + del_timer_sync(&udc_timer); > /* remove pollstall timer */ > stop_pollstall_timer++; > if (timer_pending(&udc_pollstall_timer)) > wait_for_completion(&on_pollstall_exit); > - if (udc_pollstall_timer.data) > - del_timer_sync(&udc_pollstall_timer); > + del_timer_sync(&udc_pollstall_timer); > udc = NULL; > } > EXPORT_SYMBOL_GPL(udc_remove); > @@ -3164,9 +3162,9 @@ int udc_probe(struct udc *dev) > u32 reg; > int retval; > > - /* mark timer as not initialized */ > - udc_timer.data = 0; > - udc_pollstall_timer.data = 0; > - > /* device struct setup */ > dev->gadget.ops = &udc_ops; > Okay, sounds good. I'll adjust the patch. Thanks! -Kees -- Kees Cook Pixel Security