Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-09-01 Thread Kees Cook 
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: de...@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(>timer, das16_timer_interrupt,
>>> -(unsigned long)dev);
>>> -}
>>> +setup_timer(>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(>timer);
>>> +del_timer_sync(>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

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-09-01 Thread Kees Cook 
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: de...@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(>timer, das16_timer_interrupt,
>>> -(unsigned long)dev);
>>> -}
>>> +setup_timer(>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(>timer);
>>> +del_timer_sync(>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

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-09-01 Thread Ian Abbott 

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: de...@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(>timer, das16_timer_interrupt,
-(unsigned long)dev);
-}
+setup_timer(>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(>timer);
+del_timer_sync(>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.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-09-01 Thread Ian Abbott 

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: de...@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(>timer, das16_timer_interrupt,
-(unsigned long)dev);
-}
+setup_timer(>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(>timer);
+del_timer_sync(>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.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-09-01 Thread Ian Abbott 

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: de...@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(>timer, das16_timer_interrupt,
-   (unsigned long)dev);
-   }
+   setup_timer(>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(>timer);
+   del_timer_sync(>timer);


If setup_timer() has not been called (see remark above), this change 
will break.



comedi_isadma_free(devpriv->dma);
}
  }




--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-09-01 Thread Ian Abbott 

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: de...@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(>timer, das16_timer_interrupt,
-   (unsigned long)dev);
-   }
+   setup_timer(>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(>timer);
+   del_timer_sync(>timer);


If setup_timer() has not been called (see remark above), this change 
will break.



comedi_isadma_free(devpriv->dma);
}
  }




--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-08-31 Thread Greg Kroah-Hartman 
On Thu, Aug 31, 2017 at 04:29:38PM -0700, 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: de...@driverdev.osuosl.org
> Signed-off-by: Kees Cook 
> ---
>  drivers/staging/comedi/drivers/das16.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)

Acked-by: Greg Kroah-Hartman

Re: [PATCH 26/31] staging/comedi/das16: Make timer initialization unconditional

2017-08-31 Thread Greg Kroah-Hartman 
On Thu, Aug 31, 2017 at 04:29:38PM -0700, 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: de...@driverdev.osuosl.org
> Signed-off-by: Kees Cook 
> ---
>  drivers/staging/comedi/drivers/das16.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)

Acked-by: Greg Kroah-Hartman