Re: CVS commit: src/sys/dev/usb

[Taylor R Campbell]

   Date: Fri, 18 Jul 2014 11:58:33 +1000
   from: matthew green 

   "Taylor R Campbell" writes:
   > Fix usb task queue locking.

   why does this take the lock and then use atomic operations?

Reading task->queue is not covered by a lock: until you know what it
is you don't know what lock to hold.  usb_task_rem needs to read it
and use it without holding any queue's lock, and by the time
usb_task_rem takes a lock, the value it read may be out of date.

   such strange usage should have a comment..

Better yet, we should have a better task queue system.  More on that
soon, perhaps...

re: CVS commit: src/sys/dev/usb

[matthew green]

"Taylor R Campbell" writes:
> Module Name:  src
> Committed By: riastradh
> Date: Thu Jul 17 18:42:38 UTC 2014
> 
> Modified Files:
>   src/sys/dev/usb: usb.c usbdi.h
> 
> Log Message:
> Fix usb task queue locking.

why does this take the lock and then use atomic operations?

such strange usage should have a comment..

thanks.


.mrg.

Re: CVS commit: src/sys/sys

[Lourival Vieira Neto]

On Thu, Jul 17, 2014 at 3:14 PM, Marc Balmer  wrote:
>
> Am 17.07.2014 um 16:55 schrieb Taylor R Campbell :
>
>> Module Name:  src
>> Committed By: riastradh
>> Date: Thu Jul 17 14:55:32 UTC 2014
>>
>> Modified Files:
>>   src/sys/sys: systm.h
>>
>> Log Message:
>> Expand null macros to `do {} while (0)', not to nothing.
>>
>>
>
> Not that I object, but out of curiosity, what is this good for, or, why is 
> this need/a good thing?
>
>> Index: src/sys/sys/systm.h
>> diff -u src/sys/sys/systm.h:1.263 src/sys/sys/systm.h:1.264
>> --- src/sys/sys/systm.h:1.263 Thu Apr  3 15:22:57 2014
>> +++ src/sys/sys/systm.h   Thu Jul 17 14:55:32 2014
>> @@ -1,4 +1,4 @@
>> -/*   $NetBSD: systm.h,v 1.263 2014/04/03 15:22:57 christos Exp $ */
>> +/*   $NetBSD: systm.h,v 1.264 2014/07/17 14:55:32 riastradh Exp $*/
>>
>> /*-
>>  * Copyright (c) 1982, 1988, 1991, 1993
>> @@ -527,7 +527,7 @@ void assert_sleepable(void);
>> #if defined(DEBUG)
>> #define   ASSERT_SLEEPABLE()  assert_sleepable()
>> #else /* defined(DEBUG) */
>> -#define  ASSERT_SLEEPABLE()  /* nothing */
>> +#define  ASSERT_SLEEPABLE()  do {} while (0)
>> #endif /* defined(DEBUG) */
>>
>> vaddr_t calc_cache_size(vsize_t , int, int);
>>

Wouldn't be '((void)0)' better?
Regards,
-- 
Lourival Vieira Neto

Re: CVS commit: src/sys/sys

[Marc Balmer]

Am 17.07.2014 um 16:55 schrieb Taylor R Campbell :

> Module Name:  src
> Committed By: riastradh
> Date: Thu Jul 17 14:55:32 UTC 2014
> 
> Modified Files:
>   src/sys/sys: systm.h
> 
> Log Message:
> Expand null macros to `do {} while (0)', not to nothing.
> 
> 

Not that I object, but out of curiosity, what is this good for, or, why is this 
need/a good thing?

> Index: src/sys/sys/systm.h
> diff -u src/sys/sys/systm.h:1.263 src/sys/sys/systm.h:1.264
> --- src/sys/sys/systm.h:1.263 Thu Apr  3 15:22:57 2014
> +++ src/sys/sys/systm.h   Thu Jul 17 14:55:32 2014
> @@ -1,4 +1,4 @@
> -/*   $NetBSD: systm.h,v 1.263 2014/04/03 15:22:57 christos Exp $ */
> +/*   $NetBSD: systm.h,v 1.264 2014/07/17 14:55:32 riastradh Exp $*/
> 
> /*-
>  * Copyright (c) 1982, 1988, 1991, 1993
> @@ -527,7 +527,7 @@ void assert_sleepable(void);
> #if defined(DEBUG)
> #define   ASSERT_SLEEPABLE()  assert_sleepable()
> #else /* defined(DEBUG) */
> -#define  ASSERT_SLEEPABLE()  /* nothing */
> +#define  ASSERT_SLEEPABLE()  do {} while (0)
> #endif /* defined(DEBUG) */
> 
> vaddr_t calc_cache_size(vsize_t , int, int);
>