linux-kernel - Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

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:	Fri, 29 May 2015 11:07:15 +0200
From:	Heiko Carstens <heiko.carstens@...ibm.com>
To:	Nicholas Mc Guire <hofrat@...dl.org>,
	Michael Holzheu <holzheu@...ux.vnet.ibm.com>
Cc:	Martin Schwidefsky <schwidefsky@...ibm.com>, linux390@...ibm.com,
	linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] s390/sclp: pass timeout as HZ independent value

On Wed, May 27, 2015 at 07:04:43PM +0200, Nicholas Mc Guire wrote:
> schedule_timeout takes a timeout in jiffies but the code currently is
> passing in a constant SDIAS_SLEEP_TICKS which sounds like it should be
> in jiffies but it is actually not and thus makes this timeout HZ
> dependent, to fix this it is passed through msecs_to_jiffies().
> 
> patch was not even compile tested as s390 toolchain from kernel.org 
> failed for me (on a debian wheezy x86_64) with:
> cc1: error: unrecognized command line option '-mtune=zEC12'
> 
> Patch is against 4.0-rc5 (localversion-next is -next-20150527)
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> ---
> 
> As there is no documentation of the intended timeout it might be wrong
> to convert it with msecs_to_jiffies() since the effective timeout is 
> at least a factor 10 smaller than it is now - so someone that knows this
> driver needs to check on the actual value - but in any case it needs to
> be passed in a HZ independent way.

Yes, the orginal code seems to be broken. Since I've no idea what the intended
timeout value should be, let's simply ask Michael, who wrote this code eight
years ago ;)
While these lines get touched anyway, it would make sense to use
schedule_timeout_interruptible() instead, and get rid of set_current_state().


>  drivers/s390/char/sclp_sdias.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/char/sclp_sdias.c b/drivers/s390/char/sclp_sdias.c
> index eb7cb07..ab92a75 100644
> --- a/drivers/s390/char/sclp_sdias.c
> +++ b/drivers/s390/char/sclp_sdias.c
> @@ -68,7 +68,7 @@ static int sdias_sclp_send(struct sclp_req *req)
>  			/* not initiated, wait some time and retry */
>  			set_current_state(TASK_INTERRUPTIBLE);
>  			TRACE("add request failed: rc = %i\n",rc);
> -			schedule_timeout(SDIAS_SLEEP_TICKS);
> +			schedule_timeout(msecs_to_jiffies(SDIAS_SLEEP_TICKS));
>  			continue;
>  		}
>  		/* initiated, wait for completion of service call */
> -- 

Thanks,
Heiko

--
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/