From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6FC03C04A95 for ; Sat, 22 Oct 2022 10:42:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229926AbiJVKmE (ORCPT ); Sat, 22 Oct 2022 06:42:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37322 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230096AbiJVKlp (ORCPT ); Sat, 22 Oct 2022 06:41:45 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 43B04309D69; Sat, 22 Oct 2022 02:59:10 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2A68760B30; Sat, 22 Oct 2022 09:58:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7BF4CC433D6; Sat, 22 Oct 2022 09:58:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1666432730; bh=WP8PgJkZVHH/W2+YkNZq5BVxG5GyrvdYpbdg64fNj8U=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=CeOdEUJW8auDaOQX9iLqKzUnSDWqVR65dWfwkAX6e7qWQO4RRIQtfslDSNBovITcw EGTJXSYZunsXNlylJt5kkPJNnpIl7zaRtlx8b9u26ibddedNJWQJkULhZ5IFlgNN25 ke11IpSHU/Mzr9D6QehjJp3zxenL68KSg25g4vDXKsGy+Lg6Uga1o3aQO6OVR+Mz2y NBJs1AOGXTQD/unUTeSBsuM322qqNcBW22IVTDLcf0gdjvYnzgSEVtTSkl1d8deD6+ RkyE9/Ukv84iChKBPFh9ut+CEqX6eImRkrP3zR+TbOMnQIergIoSJFZj/TQqmcxR7C udzlt8jP/UGeA== Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1omBHE-000ka9-4n; Sat, 22 Oct 2022 10:58:48 +0100 Date: Sat, 22 Oct 2022 10:58:07 +0100 Message-ID: <87h6zwgoy8.wl-maz@kernel.org> From: Marc Zyngier To: Joe Korty Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] arm64: arch_timer: XGene-1 has 31 bit, not 32 bit, arch timer. In-Reply-To: <20221021194746.GA5830@zipoli.concurrent-rt.com> References: <20221021153424.GA25677@zipoli.concurrent-rt.com> <864jvxnj65.wl-maz@kernel.org> <20221021194746.GA5830@zipoli.concurrent-rt.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: joe.korty@concurrent-rt.com, linux-kernel@vger.kernel.org, stable@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Joe, On Fri, 21 Oct 2022 20:47:46 +0100, Joe Korty wrote: > > Hi Marc, > > On Fri, Oct 21, 2022 at 07:08:50PM +0100, Marc Zyngier wrote: > > Sorry, but you'll have to provide a bit more of an analysis here. As > > far as I can tell, you're just changing a parameter without properly > > describing what breaks and how. > > There isn't much to analyse. Actually, there is plenty to analyse. Starting with *why* 31 is the correct value (it actually is, see below) other than "hey, I reverted this and it's all good, just merge it". > For ages, 0x7fffffff (31 bits) was the > declared width of 'arch timer' for all arm architures, and that worked. > Your patch series made the declared width vary according to which chipset > was in use, which is good, but that rewrite changed the above mask for > the XGene-1 from 0x7fffffff to 0xffffffff. This isn't quite what my changes did, but hey, let's not get derailed. > That change broke timers > for the XGene-1 since it seems that, in actuality, it has only a 31 bit > wide arch timer. Thus declaring that arch timer has 32-bits is wrong. > This mismatch between the actual and declared sizes would cause arithmetic > errors in the calculation of timer deltas which more than accounts for > the hrtimer failures I am seeing when running 5.16+ on my Mustang XGene1. This is the important point, and the reason why it breaks: XGene implements CVAL (a 64bit comparator) in terms of TVAL (a countdown register) instead of the other way around. TVAL being a 32bit register, the width of the counter should equally be 32. However, TVAL is a *signed* value, and keeps counting down in the negative range once the timer fires. It means that any TVAL value with bit 31 set will fire immediately, as it cannot be distinguished from an already expired timer. Reducing the timer range back to a paltry 31 bits papers over the issue. Another problem cannot be fixed though, which is that the timer interrupt *must* be handled within the negative countdown period, or the interrupt will be lost (TVAL will rollover to a positive value, indicative of a new timer deadline). > Only one line need change, the rest are fluff: > > - return CLOCKSOURCE_MASK(32); > + return CLOCKSOURCE_MASK(31); Yes, and all you need is to send a proper patch, see below. > > > Also, this isn't much of a patch. > > I don't know what this means. The patch contains all that is needed for > the fix, no more. I could add more comments as to _why_ it is 31 bits > not 32, but I don't know why. I only know that the motherboard behaves > as if 31 bits is all that is available in the hardware. > > > Please see the documentation on how to properly submit one. > > AFAICS, the only submission mistake is that the 'Cc: stable@vger.kernel.org' > line is missing. What you have done here is to write an email with a diff appended to it, which isn't a proper kernel patch. I expect a patch to be formatted with "git format-patch" instead of "git diff" (i.e. something that is an actually commit instead of a local diff), with a proper commit message (feel free to nick some of the description above), with a Cc: stable@ and a Fixes: tag at the right spot, Cc'ing all the relevant maintainers. All of this is eloquently explained in the kernel documentation (Documentation/process/submitting-patches.rst), and I would definitely encourage you to read the sections titled "Describe your changes" and "The canonical patch format". You can also look at the previous commits to the same file to get a sense of the formatting that people use. Thanks, M. -- Without deviation from the norm, progress is not possible.