Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752522AbdIARxj (ORCPT ); Fri, 1 Sep 2017 13:53:39 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:36244 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752489AbdIARxi (ORCPT ); Fri, 1 Sep 2017 13:53:38 -0400 X-Google-Smtp-Source: ADKCNb6U15VdtuMugYNGoTngTPIj/7O1hd9V0fXe6zXH2FeBOW1akWHQmkgvXaR0tCaF2Hpoxah6TOzkRawjFBq/9Ow= MIME-Version: 1.0 In-Reply-To: References: <1504222183-61202-1-git-send-email-keescook@chromium.org> <1504222183-61202-27-git-send-email-keescook@chromium.org> <5d865f87-4aa5-6a72-ce8a-c0232b362adc@mev.co.uk> From: Kees Cook Date: Fri, 1 Sep 2017 10:53:37 -0700 X-Google-Sender-Auth: 1jDD3HfInSwJDD9QOfzG7x-82nc Message-ID: Subject: Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional To: Ian Abbott Cc: Thomas Gleixner , H Hartley Sweeten , Greg Kroah-Hartman , devel@driverdev.osuosl.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: 2538 Lines: 70 On Fri, Sep 1, 2017 at 2:38 AM, Ian Abbott wrote: > On 01/09/17 10:29, Ian Abbott wrote: >> >> On 01/09/17 00:29, Kees Cook wrote: >>> >>> With timer initialization made unconditional, there is no reason to >>> make del_timer_sync() calls conditionally, there by removing the test >>> of the .data field. >>> >>> Cc: Ian Abbott >>> Cc: H Hartley Sweeten >>> Cc: Greg Kroah-Hartman >>> Cc: devel@driverdev.osuosl.org >>> Signed-off-by: Kees Cook >>> --- >>> drivers/staging/comedi/drivers/das16.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/staging/comedi/drivers/das16.c >>> b/drivers/staging/comedi/drivers/das16.c >>> index 5d157951f63f..4514179b2007 100644 >>> --- a/drivers/staging/comedi/drivers/das16.c >>> +++ b/drivers/staging/comedi/drivers/das16.c >>> @@ -941,10 +941,8 @@ static void das16_alloc_dma(struct comedi_device >>> *dev, unsigned int dma_chan) >>> /* DMA uses two buffers */ >>> devpriv->dma = comedi_isadma_alloc(dev, 2, dma_chan, dma_chan, >>> DAS16_DMA_SIZE, COMEDI_ISADMA_READ); >>> - if (devpriv->dma) { >>> - setup_timer(&devpriv->timer, das16_timer_interrupt, >>> - (unsigned long)dev); >>> - } >>> + setup_timer(&devpriv->timer, das16_timer_interrupt, >>> + (unsigned long)dev); >>> } >> >> >> das16_alloc_dma() returns before the call to comedi_isadma_alloc() if the >> dma_chan parameter is not one of the values 1 or 3, so setup_timer() will >> not be called in that case. >> >>> static void das16_free_dma(struct comedi_device *dev) >>> @@ -952,8 +950,7 @@ static void das16_free_dma(struct comedi_device *dev) >>> struct das16_private_struct *devpriv = dev->private; >>> if (devpriv) { >>> - if (devpriv->timer.data) >>> - del_timer_sync(&devpriv->timer); >>> + del_timer_sync(&devpriv->timer); >> >> >> If setup_timer() has not been called (see remark above), this change will >> break. >> >>> comedi_isadma_free(devpriv->dma); >>> } >>> } >>> > > If you want to avoid testing devpriv->timer.data for some reason, you could > make the calls to setup_timer() and del_timer_sync() depend on devpriv->dma. Thanks for checking this! I think the cleanest would be to just move setup_timer() to the start of das16_alloc_dma(). I'll make that adjustment. -Kees -- Kees Cook Pixel Security