From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 36C3F14A4D2; Thu, 19 Sep 2024 06:01:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1726725700; cv=none; b=fOZGA0Qw/QBfre4hVwjAojMunVQudLoLFdmWANffil7hUpiqTeXHHr4kEryeNTcvCFBiKliuJ9fz1DBMQouNNwg4SKDBlzENCDJP/gBFBW61lg1dX8Q2j7Mxb0IxS81G4vfjVztU88twXTMe+sTtdPBOm1cJvfu9mAQa/p/k6D4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1726725700; c=relaxed/simple; bh=Tavix2DjYUpx/L6KLOj9yAdZF5jGIMZhgXaAgOdqhy0=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=lm0CkcaKDCUyFCznvWAQeDukg0FTwS2nxsvPSScMuYOuISgcIyqTuoBpqFvQjDDBLol310yTs9ZIgxVqRe2DavINpB5V7phcdlY2QBQkPduEcYBBUKGzoJ1RrNjBIHdkkvW0Gpqa6Fow6so9Yn5lz22L6gqMAPGbeOPNnZ74mBY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=q6+IMQEY; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="q6+IMQEY" Received: by smtp.kernel.org (Postfix) with ESMTPSA id ABC78C4CEC4; Thu, 19 Sep 2024 06:01:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1726725699; bh=Tavix2DjYUpx/L6KLOj9yAdZF5jGIMZhgXaAgOdqhy0=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=q6+IMQEYsIWaRUXZimA6J9n8+NQK6W5laZfVkHahJRlJMnBNn883iGbYWd4EksTwF It3Ypy+hKFJ50K6k5mynJ1qmGnxzan+YIoFOeHnXHJW29xYdinDvjVgfy06PaHPw+q N0S4gs1pk5t0mtaCdzEmMHsmYY4PgrfIYupOZVxOwx+bjanrbyy/ueifgvhLv1VROW cfGTHvh9i544pDFXmb5UXMuJnlmNYlsb6ocla8gZPHN6aqZiuDrCI7UAHc5fmtgIey tiwzdYKSr9/MGGUr6p8nZCB0kCTdJWxPd1B/j3nVBW1qsN1gBvLMVStmISncyDx9IO 3y2t41Uekc8lw== From: Andreas Hindborg To: "Benno Lossin" Cc: "Miguel Ojeda" , "Alex Gaynor" , "Anna-Maria Behnsen" , "Frederic Weisbecker" , "Thomas Gleixner" , "Boqun Feng" , "Gary Guo" , =?utf-8?Q?Bj=C3=B6rn?= Roy Baron , "Alice Ryhl" , , Subject: Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support In-Reply-To: (Benno Lossin's message of "Wed, 18 Sep 2024 18:13:11 +0000") References: <20240917222739.1298275-1-a.hindborg@kernel.org> <20240917222739.1298275-3-a.hindborg@kernel.org> Date: Thu, 19 Sep 2024 07:43:19 +0200 Message-ID: <878qvojj0o.fsf@kernel.org> Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain Hi Benno, Thanks for the feedback. I will incorporate all the whitespace suggestions you have =F0=9F=91=8D "Benno Lossin" writes: > On 18.09.24 00:27, Andreas Hindborg wrote: [...] >> + >> +impl Timer { >> + /// Return an initializer for a new timer instance. >> + pub fn new() -> impl PinInit >> + where >> + T: TimerCallback, >> + { >> + pin_init!( Self { > > I would remove the space after the `(`. > Would be great if we had rustfmt support for custom macros. Yes, that would be great! > >> + // INVARIANTS: We initialize `timer` with `hrtimer_init` be= low. >> + timer <- Opaque::ffi_init(move |place: *mut bindings::hrtim= er| { >> + // SAFETY: By design of `pin_init!`, `place` is a point= er live >> + // allocation. hrtimer_init will initialize `place` and= does not >> + // require `place` to be initialized prior to the call. >> + unsafe { >> + bindings::hrtimer_init( >> + place, >> + bindings::CLOCK_MONOTONIC as i32, >> + bindings::hrtimer_mode_HRTIMER_MODE_REL, >> + ); >> + } >> + >> + // SAFETY: `place` is pointing to a live allocation, so= the deref >> + // is safe. >> + let function: *mut Option<_> =3D > > Do you really need this type hint? Apparently not! [...] >> +pub trait TimerPointer: Sync + Sized { >> + /// A handle representing a scheduled timer. >> + /// >> + /// If the timer is armed or if the timer callback is running when = the >> + /// handle is dropped, the drop method of `TimerHandle` should not = return >> + /// until the timer is unarmed and the callback has completed. >> + /// >> + /// Note: It must be safe to leak the handle. >> + type TimerHandle: TimerHandle; > > Why does this need to be an associated type? Couldn't we have a > `TimerHandle` struct? The schedule function below could then return > `TimerHandle`. At one point, I had some cycles in trait resolution. Moving generics to associated types solved that issue. Maybe this can be changed to a generic. But are generics preferred over associated types for some reason? Best regards, Andreas