linux-kernel - Re: [PATCH] amiflop: get rid of sleep

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 9 Dec 2008 09:31:11 +0100 (CET)
From:	Geert Uytterhoeven <geert@...ux-m68k.org>
To:	Andreas Bombe <aeb@...ian.org>
cc:	linux-m68k@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Mon, 8 Dec 2008, Andreas Bombe wrote:
> Apart from sleep_on() calls that could be easily converted to
> wait_event() and completion calls amiflop also used a flag in ms_delay()
> and ms_isr() as a custom mutex for ms_delay() without a need for
> explicit unlocking.  I converted that to a standard mutex semaphore.
> 
> The replacement for the unconditional sleep_on() in fd_motor_on() is a
> complete_all() together with a INIT_COMPLETION() before the mod_timer()
> call.  It appears to me that fd_motor_on() might be called concurrently
> and fd_select() does not guarantee mutual exclusivity in the case the
> same drive gets selected again.
> 
> Signed-off-by: Andreas Bombe <aeb@...ian.org>
> ---
> 
> Note: I haven't tested these changes beyond correct compilation.
> 
> The complete_all() with INIT_COMPLETION() was because I'm not sure about
> whether fd_motor_on() might run concurrently.  Better safe than sorry.

I'm afraid my Amiga floppy drive doesn't work anymore...
Any other testers available?

> --- a/drivers/block/amiflop.c
> +++ b/drivers/block/amiflop.c
> @@ -220,19 +218,17 @@ static irqreturn_t ms_isr(int irq, void *dummy)
>     A more generic routine would do a schedule a la timer.device */
>  static void ms_delay(int ms)
>  {
> -	unsigned long flags;
>  	int ticks;
> +	static DECLARE_MUTEX(mutex);
               ^^^^^^^^^^^^^
Shouldn't this be a real mutex (declared using DEFINE_MUTEX(mutex)) instead of
a semaphore used as a mutex?

> +
>  	if (ms > 0) {
> -		local_irq_save(flags);
> -		while (ms_busy == 0)
> -			sleep_on(&ms_wait);
> -		ms_busy = 0;
> -		local_irq_restore(flags);
> +		down(&mutex);
                ^^^^
		mutex_lock

>  		ticks = MS_TICKS*ms-1;
>  		ciaa.tblo=ticks%256;
>  		ciaa.tbhi=ticks/256;
>  		ciaa.crb=0x19; /*count eclock, force load, one-shoot, start */
> -		sleep_on(&ms_wait);
> +		wait_for_completion(&ms_wait_completion);
> +		up(&mutex);
		mutex_unlock

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/