| Message ID | fe8e9a96b29122876346fc98a6a9ede7e4f28707.1719990273.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
Hi Viresh, On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote: > This commit adds initial Rust bindings for the Operating performance > points (OPP) core. This adds bindings for `struct dev_pm_opp` and > `struct dev_pm_opp_data` to begin with. > > Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- [...] > + > +/// Operating performance point (OPP). > +/// > +/// # Invariants > +/// > +/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In > +/// particular, the ARef instance owns an increment on underlying object´s reference count. Since you use `ARef` pattern now, you may want to rewrite this "invariants". > +#[repr(transparent)] > +pub struct OPP(Opaque<bindings::dev_pm_opp>); > + > +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread. > +unsafe impl Send for OPP {} > + > +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any > +// thread. > +unsafe impl Sync for OPP {} > + Same for the above safety comments, as they are still based on the old implementation. > +// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted. > +unsafe impl AlwaysRefCounted for OPP { > + fn inc_ref(&self) { > + // SAFETY: The existence of a shared reference means that the refcount is nonzero. > + unsafe { bindings::dev_pm_opp_get(self.0.get()) }; > + } > + > + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > + unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) } > + } > +} > + > +impl OPP { [...] > + > +impl Drop for OPP { I don't think you need the `drop` implementation here, since it should be already handled by `impl AlwaysRefCounted`, could you try to a doc test for this? Something like: let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1> drop(opp); IIUC, this will result double-free with the current implementation. Overall, `OPP` is now representing to the actual device instead of the pointer to the device, so the `drop` function won't need to handle the refcounting. Regards, Boqun > + fn drop(&mut self) { > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > + unsafe { bindings::dev_pm_opp_put(self.as_raw()) } > + } > +} > -- > 2.31.1.272.g89b43f80a514 >
Hi Boqun, On 03-07-24, 08:34, Boqun Feng wrote: > On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote: > > +/// Operating performance point (OPP). > > +/// > > +/// # Invariants > > +/// > > +/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In > > +/// particular, the ARef instance owns an increment on underlying object´s reference count. > > Since you use `ARef` pattern now, you may want to rewrite this > "invariants". I copied it from the device's documentation. What all details should I be writing here ? A link to some other implementation would be useful. > > +impl Drop for OPP { > > I don't think you need the `drop` implementation here, since it should > be already handled by `impl AlwaysRefCounted`, Right. > could you try to a doc > test for this? Something like: > > let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1> > drop(opp); I now tested it with a kernel test to see what's going on internally > IIUC, this will result double-free with the current implementation. Quite the opposite actually. I am getting double get and a single put :) Thanks a lot for pointing me to this direction as I have found that my implementation was incorrect. This is how I understand it, I can be wrong since I am okayish with Rust: - What's getting returned from `from_raw_opp/from_raw_opp_owned` is a reference: `<&'a Self>`. - Since this is a reference, when it gets out of scope, nothing happens. i.e. the `drop()` fn of `struct OPP` never gets called for the OPP object, as there is no real OPP object, but just a reference. - When this gets converted to an `ARef` object (implicit typecasting), we increment the count. And when that gets dropped, we decrement it. But Apart from an `ARef` object, only the reference to the OPP gets dropped and hence again, drop() doesn't get called. - The important part here is that `from_raw_opp()` shouldn't be incrementing the refcount, as drop() will never get called. And since we reach here from the C implementation, the OPP will remain valid for the function call. - On the other hand, I can't return <&'a Self> from from_raw_opp_owned() anymore. In this case the OPP core has already incremented the refcount of the OPP (while it searched the OPP on behalf of the Rust code). Whatever is returned here, must drop the refcount when it goes out of scope. Also the returned OPP reference can live for a longer period of time in this case, since the call originates from the Rust side. So, it needs to be an explicit conversion to ARef which won't increment the refcount, but just decrement when the ARef gets out of scope. Here is the diff that I need: diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs index aaf220e6aeac..a99950b4d835 100644 --- a/rust/kernel/opp.rs +++ b/rust/kernel/opp.rs @@ -692,7 +692,7 @@ pub fn opp_from_freq( })?; // SAFETY: The `ptr` is guaranteed by the C code to be valid. - Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() }) + unsafe { OPP::from_raw_opp_owned(ptr) } } /// Finds OPP based on level. @@ -718,7 +718,7 @@ pub fn opp_from_level(&self, mut level: u32, stype: SearchType) -> Result<ARef<O })?; // SAFETY: The `ptr` is guaranteed by the C code to be valid. - Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() }) + unsafe { OPP::from_raw_opp_owned(ptr) } } /// Finds OPP based on bandwidth. @@ -743,7 +743,7 @@ pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result< })?; // SAFETY: The `ptr` is guaranteed by the C code to be valid. - Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() }) + unsafe { OPP::from_raw_opp_owned(ptr) } } /// Enable the OPP. @@ -834,31 +834,33 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) { } impl OPP { - /// Creates a reference to a [`OPP`] from a valid pointer. + /// Creates an owned reference to a [`OPP`] from a valid pointer. /// /// # Safety /// - /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the - /// returned [`OPP`] reference. - pub unsafe fn from_raw_opp_owned<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { - // SAFETY: The caller guarantees that the pointer is not dangling - // and stays valid for the duration of 'a. The cast is okay because - // `OPP` is `repr(transparent)`. - Ok(unsafe { &*ptr.cast() }) + /// The caller must ensure that `ptr` is valid and OPP's refcount is incremented. The refcount + /// will be decremented by `dec_ref` when the ARef object is dropped. + pub unsafe fn from_raw_opp_owned(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> { + let ptr = ptr::NonNull::new(ptr).ok_or(ENODEV)?; + + // SAFETY: The safety requirements guarantee the validity of the pointer. + // + // INVARIANT: The refcount is already incremented by the C API that returned the pointer, + // and we pass ownership of the refcount to the new `ARef<OPP>`. + Ok(unsafe { ARef::from_raw(ptr.cast()) }) } /// Creates a reference to a [`OPP`] from a valid pointer. /// /// # Safety /// - /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the - /// returned [`OPP`] reference. + /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a. The + /// refcount is not updated by the Rust API unless the returned reference is converted to an + /// ARef object. pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { - let opp = unsafe { Self::from_raw_opp_owned(ptr) }?; - - // Take an extra reference to the OPP since the caller didn't take it. - opp.inc_ref(); - Ok(opp) + // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the + // duration of 'a. The cast is okay because `OPP` is `repr(transparent)`. + Ok(unsafe { &*ptr.cast() }) } #[inline] @@ -910,10 +912,3 @@ pub fn is_turbo(&self) -> bool { unsafe { bindings::dev_pm_opp_is_turbo(self.as_raw()) } } } - -impl Drop for OPP { - fn drop(&mut self) { - // SAFETY: The safety requirements guarantee that the refcount is nonzero. - unsafe { bindings::dev_pm_opp_put(self.as_raw()) } - } -} Makes sense ?
On Fri, Jul 05, 2024 at 04:32:28PM +0530, Viresh Kumar wrote: > Hi Boqun, > > On 03-07-24, 08:34, Boqun Feng wrote: > > On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote: > > > +/// Operating performance point (OPP). > > > +/// > > > +/// # Invariants > > > +/// > > > +/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In > > > +/// particular, the ARef instance owns an increment on underlying object´s reference count. > > > > Since you use `ARef` pattern now, you may want to rewrite this > > "invariants". > > I copied it from the device's documentation. What all details should I > be writing here ? A link to some other implementation would be useful. > "Invariants" defines "what's a valid instance of a type", so here I think you could drop the "# Invariants" section at all, unless you need to use some of the invariants of fields in `dev_mp_opp` as a justification for safety. For example if you have a pointer in `dev_mp_opp`, and it's always pointing to a valid `T`, and in one method of `OPP`, you need to dereference the pointer. > > > +impl Drop for OPP { > > > > I don't think you need the `drop` implementation here, since it should > > be already handled by `impl AlwaysRefCounted`, > > Right. > > > could you try to a doc > > test for this? Something like: > > > > let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1> > > drop(opp); > > I now tested it with a kernel test to see what's going on internally > > > IIUC, this will result double-free with the current implementation. > > Quite the opposite actually. I am getting double get and a single put :) > > Thanks a lot for pointing me to this direction as I have found that my > implementation was incorrect. This is how I understand it, I can be > wrong since I am okayish with Rust: > > - What's getting returned from `from_raw_opp/from_raw_opp_owned` is a > reference: `<&'a Self>`. > > - Since this is a reference, when it gets out of scope, nothing > happens. i.e. the `drop()` fn of `struct OPP` never gets called for > the OPP object, as there is no real OPP object, but just a > reference. > > - When this gets converted to an `ARef` object (implicit typecasting), > we increment the count. And when that gets dropped, we decrement it. > But Apart from an `ARef` object, only the reference to the OPP gets > dropped and hence again, drop() doesn't get called. > > - The important part here is that `from_raw_opp()` shouldn't be > incrementing the refcount, as drop() will never get called. And > since we reach here from the C implementation, the OPP will remain > valid for the function call. > Right. I wasn't aware that you didn't return a `ARef<OPP>`, which mean the return value won't handle the refcounting automatically (and because of that, the refcount shouldn't be increased.) > - On the other hand, I can't return <&'a Self> from > from_raw_opp_owned() anymore. In this case the OPP core has already > incremented the refcount of the OPP (while it searched the OPP on > behalf of the Rust code). Whatever is returned here, must drop the > refcount when it goes out of scope. Also the returned OPP reference > can live for a longer period of time in this case, since the call > originates from the Rust side. So, it needs to be an explicit > conversion to ARef which won't increment the refcount, but just > decrement when the ARef gets out of scope. > I think you got it correct ;-) The takeaway is: there are different types of pointers/references, 1) if users only use a `struct dev_pm_opp *` shortly and the scope can be told at compile time, you can provide a `&OPP`, 2) if the scope of usage is somewhat dynamic, and the users should descrease the refcount after use it, the API should return a `ARef<OPP>`. And since the refcount inc/dec are already maintained in `ARef<_>`, `OPP::drop` shouldn't touch the refcount anymore. Also as you already noticed, calling `into` on a `&OPP` will give a `ARef<OPP>`, which includes an increment of the refcount, and usually should be used when the users want to switch to long-term usage after a quick short-term use (e.g. do a quick check of the status and decide some more work is needed, and maybe need to transfer the ownership of the pointer to a workqueue or something). > Here is the diff that I need: > > diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs > index aaf220e6aeac..a99950b4d835 100644 > --- a/rust/kernel/opp.rs > +++ b/rust/kernel/opp.rs > @@ -692,7 +692,7 @@ pub fn opp_from_freq( > })?; > > // SAFETY: The `ptr` is guaranteed by the C code to be valid. > - Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() }) > + unsafe { OPP::from_raw_opp_owned(ptr) } > } > > /// Finds OPP based on level. > @@ -718,7 +718,7 @@ pub fn opp_from_level(&self, mut level: u32, stype: SearchType) -> Result<ARef<O > })?; > > // SAFETY: The `ptr` is guaranteed by the C code to be valid. > - Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() }) > + unsafe { OPP::from_raw_opp_owned(ptr) } > } > > /// Finds OPP based on bandwidth. > @@ -743,7 +743,7 @@ pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result< > })?; > > // SAFETY: The `ptr` is guaranteed by the C code to be valid. > - Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() }) > + unsafe { OPP::from_raw_opp_owned(ptr) } > } > > /// Enable the OPP. > @@ -834,31 +834,33 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > } > > impl OPP { > - /// Creates a reference to a [`OPP`] from a valid pointer. > + /// Creates an owned reference to a [`OPP`] from a valid pointer. > /// > /// # Safety > /// > - /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the > - /// returned [`OPP`] reference. > - pub unsafe fn from_raw_opp_owned<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { > - // SAFETY: The caller guarantees that the pointer is not dangling > - // and stays valid for the duration of 'a. The cast is okay because > - // `OPP` is `repr(transparent)`. > - Ok(unsafe { &*ptr.cast() }) > + /// The caller must ensure that `ptr` is valid and OPP's refcount is incremented. The refcount > + /// will be decremented by `dec_ref` when the ARef object is dropped. Usually the second sentense "The refcount ..." won't need to be put in the safety requirement, as it just describes how ARef works. > + pub unsafe fn from_raw_opp_owned(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> { > + let ptr = ptr::NonNull::new(ptr).ok_or(ENODEV)?; > + > + // SAFETY: The safety requirements guarantee the validity of the pointer. > + // > + // INVARIANT: The refcount is already incremented by the C API that returned the pointer, > + // and we pass ownership of the refcount to the new `ARef<OPP>`. You can probably drop the "INVARIANT", as it's an invariant of `ARef` which already guarantees since the safety requirement of `ARef::from_raw()` meets. At least you can write them as "normal" comments. > + Ok(unsafe { ARef::from_raw(ptr.cast()) }) > } > > /// Creates a reference to a [`OPP`] from a valid pointer. > /// > /// # Safety > /// > - /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the > - /// returned [`OPP`] reference. > + /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a. The > + /// refcount is not updated by the Rust API unless the returned reference is converted to an > + /// ARef object. Again you could drop the second sentence, or you can put it somewhere outside the "Safety" section, if you want to. > pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { > - let opp = unsafe { Self::from_raw_opp_owned(ptr) }?; > - > - // Take an extra reference to the OPP since the caller didn't take it. > - opp.inc_ref(); > - Ok(opp) > + // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the > + // duration of 'a. The cast is okay because `OPP` is `repr(transparent)`. > + Ok(unsafe { &*ptr.cast() }) > } > > #[inline] > @@ -910,10 +912,3 @@ pub fn is_turbo(&self) -> bool { > unsafe { bindings::dev_pm_opp_is_turbo(self.as_raw()) } > } > } > - > -impl Drop for OPP { > - fn drop(&mut self) { > - // SAFETY: The safety requirements guarantee that the refcount is nonzero. > - unsafe { bindings::dev_pm_opp_put(self.as_raw()) } > - } > -} > > Makes sense ? > Yep, looks good to me! Regards, Boqun > -- > viresh
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index d8b54b9fa4d0..1bf8e053c8f4 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -15,6 +15,7 @@ #include <linux/of.h> #include <linux/phy.h> #include <linux/platform_device.h> +#include <linux/pm_opp.h> #include <linux/refcount.h> #include <linux/sched.h> #include <linux/slab.h> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 3bf1089b87a3..e309d7774cbd 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -45,6 +45,8 @@ #[cfg(CONFIG_NET)] pub mod net; pub mod of; +#[cfg(CONFIG_PM_OPP)] +pub mod opp; pub mod platform; pub mod prelude; pub mod print; diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs new file mode 100644 index 000000000000..b26e39a74635 --- /dev/null +++ b/rust/kernel/opp.rs @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Operating performance points. +//! +//! This module provides bindings for interacting with the OPP subsystem. +//! +//! C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h) + +use crate::{ + bindings, + device::Device, + error::{code::*, to_result, Result}, + types::{ARef, AlwaysRefCounted, Opaque}, +}; + +use core::ptr; + +/// Dynamically created Operating performance point (OPP). +pub struct Token { + dev: ARef<Device>, + freq: u64, +} + +impl Token { + /// Adds an OPP dynamically. + pub fn new(dev: &ARef<Device>, mut data: Data) -> Result<Self> { + // SAFETY: The requirements are satisfied by the existence of `Device` and its safety + // requirements. + to_result(unsafe { bindings::dev_pm_opp_add_dynamic(dev.as_raw(), &mut data.0) })?; + Ok(Self { + dev: dev.clone(), + freq: data.freq(), + }) + } +} + +impl Drop for Token { + fn drop(&mut self) { + // SAFETY: The requirements are satisfied by the existence of `Device` and its safety + // requirements. + unsafe { bindings::dev_pm_opp_remove(self.dev.as_raw(), self.freq) }; + } +} + +/// Equivalent to `struct dev_pm_opp_data` in the C Code. +#[repr(transparent)] +pub struct Data(bindings::dev_pm_opp_data); + +impl Data { + /// Creates new instance of [`Data`]. + pub fn new(freq: u64, u_volt: u64, level: u32, turbo: bool) -> Self { + Self(bindings::dev_pm_opp_data { + turbo, + freq, + u_volt, + level, + }) + } + + /// Adds an OPP dynamically. The OPP is freed once the [`Token`] gets freed. + pub fn add_opp(self, dev: &ARef<Device>) -> Result<Token> { + Token::new(dev, self) + } + + fn freq(&self) -> u64 { + self.0.freq + } +} + +/// Operating performance point (OPP). +/// +/// # Invariants +/// +/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In +/// particular, the ARef instance owns an increment on underlying object’s reference count. +#[repr(transparent)] +pub struct OPP(Opaque<bindings::dev_pm_opp>); + +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread. +unsafe impl Send for OPP {} + +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any +// thread. +unsafe impl Sync for OPP {} + +// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted. +unsafe impl AlwaysRefCounted for OPP { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference means that the refcount is nonzero. + unsafe { bindings::dev_pm_opp_get(self.0.get()) }; + } + + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { + // SAFETY: The safety requirements guarantee that the refcount is nonzero. + unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) } + } +} + +impl OPP { + /// Creates a reference to a [`OPP`] from a valid pointer. + /// + /// # Safety + /// + /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the + /// returned [`OPP`] reference. + pub unsafe fn from_raw_opp_owned<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { + // SAFETY: The caller guarantees that the pointer is not dangling + // and stays valid for the duration of 'a. The cast is okay because + // `OPP` is `repr(transparent)`. + Ok(unsafe { &*ptr.cast() }) + } + + /// Creates a reference to a [`OPP`] from a valid pointer. + /// + /// # Safety + /// + /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the + /// returned [`OPP`] reference. + pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { + let opp = unsafe { Self::from_raw_opp_owned(ptr) }?; + + // Take an extra reference to the OPP since the caller didn't take it. + opp.inc_ref(); + Ok(opp) + } + + #[inline] + fn as_raw(&self) -> *mut bindings::dev_pm_opp { + self.0.get() + } + + /// Returns the frequency of an OPP. + pub fn freq(&self, index: Option<u32>) -> u64 { + let index = index.unwrap_or(0); + + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // use it. + unsafe { bindings::dev_pm_opp_get_freq_indexed(self.as_raw(), index) } + } + + /// Returns the voltage of an OPP. + pub fn voltage(&self) -> u64 { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // use it. + unsafe { bindings::dev_pm_opp_get_voltage(self.as_raw()) } + } + + /// Returns the level of an OPP. + pub fn level(&self) -> u32 { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // use it. + unsafe { bindings::dev_pm_opp_get_level(self.as_raw()) } + } + + /// Returns the power of an OPP. + pub fn power(&self) -> u64 { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // use it. + unsafe { bindings::dev_pm_opp_get_power(self.as_raw()) } + } + + /// Returns the required pstate of an OPP. + pub fn required_pstate(&self, index: u32) -> u32 { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // use it. + unsafe { bindings::dev_pm_opp_get_required_pstate(self.as_raw(), index) } + } + + /// Returns true if the OPP is turbo. + pub fn is_turbo(&self) -> bool { + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to + // use it. + unsafe { bindings::dev_pm_opp_is_turbo(self.as_raw()) } + } +} + +impl Drop for OPP { + fn drop(&mut self) { + // SAFETY: The safety requirements guarantee that the refcount is nonzero. + unsafe { bindings::dev_pm_opp_put(self.as_raw()) } + } +}