[PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Boqun Feng]

To provide examples on usage of `Guard::lock_ref()` along with the unit
test, an "assert a lock is held by a guard" example is added.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
This depends on Alice's patch:

	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/

I'm also OK to fold this in if Alice thinks it's fine.

 rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 3701fac6ebf6..6d868e35b0a3 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
 
 impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
     /// Returns the lock that this guard originates from.
+    ///
+    /// # Examples
+    ///
+    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
+    /// lock is held.
+    ///
+    /// ```
+    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
+    ///
+    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
+    ///     // Address-equal means the same lock.
+    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
+    /// }
+    ///
+    /// // Creates a new lock on stack.
+    /// stack_pin_init!{
+    ///     let l = new_spinlock!(42)
+    /// }
+    ///
+    /// let g = l.lock();
+    ///
+    /// // `g` originates from `l`.
+    /// assert_held(&g, &l);
+    /// ```
     pub fn lock_ref(&self) -> &'a Lock<T, B> {
         self.lock
     }
-- 
2.39.5 (Apple Git-154)

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Benno Lossin]

On 23.02.25 08:21, Boqun Feng wrote:
> To provide examples on usage of `Guard::lock_ref()` along with the unit
> test, an "assert a lock is held by a guard" example is added.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
> This depends on Alice's patch:
> 
> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
> 
> I'm also OK to fold this in if Alice thinks it's fine.
> 
>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 3701fac6ebf6..6d868e35b0a3 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
> 
>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>      /// Returns the lock that this guard originates from.
> +    ///
> +    /// # Examples
> +    ///
> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
> +    /// lock is held.
> +    ///
> +    /// ```
> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
> +    ///
> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
> +    ///     // Address-equal means the same lock.
> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
> +    /// }
> +    ///
> +    /// // Creates a new lock on stack.

I would be inclined to write "new lock on the stack.", but maybe that is
incorrect.

---
Cheers,
Benno

> +    /// stack_pin_init!{
> +    ///     let l = new_spinlock!(42)
> +    /// }
> +    ///
> +    /// let g = l.lock();
> +    ///
> +    /// // `g` originates from `l`.
> +    /// assert_held(&g, &l);
> +    /// ```
>      pub fn lock_ref(&self) -> &'a Lock<T, B> {
>          self.lock
>      }
> --
> 2.39.5 (Apple Git-154)
> 

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Boqun Feng]

On Sun, Feb 23, 2025 at 10:54:59AM +0000, Benno Lossin wrote:
> On 23.02.25 08:21, Boqun Feng wrote:
> > To provide examples on usage of `Guard::lock_ref()` along with the unit
> > test, an "assert a lock is held by a guard" example is added.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 

Thanks!

> > ---
> > This depends on Alice's patch:
> > 
> > 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
> > 
> > I'm also OK to fold this in if Alice thinks it's fine.
> > 
> >  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 3701fac6ebf6..6d868e35b0a3 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
> > 
> >  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
> >      /// Returns the lock that this guard originates from.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
> > +    /// lock is held.
> > +    ///
> > +    /// ```
> > +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
> > +    ///
> > +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
> > +    ///     // Address-equal means the same lock.
> > +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
> > +    /// }
> > +    ///
> > +    /// // Creates a new lock on stack.
> 
> I would be inclined to write "new lock on the stack.", but maybe that is
> incorrect.
> 

Yes, "on the stack" is better.

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > +    /// stack_pin_init!{
> > +    ///     let l = new_spinlock!(42)
> > +    /// }
> > +    ///
> > +    /// let g = l.lock();
> > +    ///
> > +    /// // `g` originates from `l`.
> > +    /// assert_held(&g, &l);
> > +    /// ```
> >      pub fn lock_ref(&self) -> &'a Lock<T, B> {
> >          self.lock
> >      }
> > --
> > 2.39.5 (Apple Git-154)
> > 
> 

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Andreas Hindborg]

Boqun Feng <boqun.feng@gmail.com> writes:

> To provide examples on usage of `Guard::lock_ref()` along with the unit
> test, an "assert a lock is held by a guard" example is added.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> This depends on Alice's patch:
>
> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>
> I'm also OK to fold this in if Alice thinks it's fine.
>
>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 3701fac6ebf6..6d868e35b0a3 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>  
>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>      /// Returns the lock that this guard originates from.
> +    ///
> +    /// # Examples
> +    ///
> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
> +    /// lock is held.
> +    ///
> +    /// ```
> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
> +    ///
> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
> +    ///     // Address-equal means the same lock.
> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
> +    /// }

This seems super useful. Perhaps add this method as part of the lock api
instead of just having it in the example?


Best regards,
Andreas Hindborg

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Benno Lossin]

On 24.02.25 09:08, Andreas Hindborg wrote:
> Boqun Feng <boqun.feng@gmail.com> writes:
> 
>> To provide examples on usage of `Guard::lock_ref()` along with the unit
>> test, an "assert a lock is held by a guard" example is added.
>>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> ---
>> This depends on Alice's patch:
>>
>> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>>
>> I'm also OK to fold this in if Alice thinks it's fine.
>>
>>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>> index 3701fac6ebf6..6d868e35b0a3 100644
>> --- a/rust/kernel/sync/lock.rs
>> +++ b/rust/kernel/sync/lock.rs
>> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>>
>>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>>      /// Returns the lock that this guard originates from.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
>> +    /// lock is held.
>> +    ///
>> +    /// ```
>> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
>> +    ///
>> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
>> +    ///     // Address-equal means the same lock.
>> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
>> +    /// }
> 
> This seems super useful. Perhaps add this method as part of the lock api
> instead of just having it in the example?

I don't think it should be an assert. Instead make it return a
`Result<(), ()>`. (or create better named unit error types)

---
Cheers,
Benno

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 24.02.25 09:08, Andreas Hindborg wrote:
>> Boqun Feng <boqun.feng@gmail.com> writes:
>>
>>> To provide examples on usage of `Guard::lock_ref()` along with the unit
>>> test, an "assert a lock is held by a guard" example is added.
>>>
>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>> ---
>>> This depends on Alice's patch:
>>>
>>> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>>>
>>> I'm also OK to fold this in if Alice thinks it's fine.
>>>
>>>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>> index 3701fac6ebf6..6d868e35b0a3 100644
>>> --- a/rust/kernel/sync/lock.rs
>>> +++ b/rust/kernel/sync/lock.rs
>>> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>>>
>>>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>>>      /// Returns the lock that this guard originates from.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
>>> +    /// lock is held.
>>> +    ///
>>> +    /// ```
>>> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
>>> +    ///
>>> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
>>> +    ///     // Address-equal means the same lock.
>>> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
>>> +    /// }
>>
>> This seems super useful. Perhaps add this method as part of the lock api
>> instead of just having it in the example?
>
> I don't think it should be an assert. Instead make it return a
> `Result<(), ()>`. (or create better named unit error types)

No, this should not be part of usual control flow, and developers should
not make control flow decisions based on this. It would always be an
assertion. But you are right that `assert!` is probably not what we
want. `debug_assert!` might be fine though.


Best regards,
Andreas Hindborg

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Alice Ryhl]

On Sun, Feb 23, 2025 at 8:21 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> To provide examples on usage of `Guard::lock_ref()` along with the unit
> test, an "assert a lock is held by a guard" example is added.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> This depends on Alice's patch:
>
>         https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>
> I'm also OK to fold this in if Alice thinks it's fine.

I think we can just keep two patches :)

Alice

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Benno Lossin]

On 24.02.25 12:15, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
>> On 24.02.25 09:08, Andreas Hindborg wrote:
>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>
>>>> To provide examples on usage of `Guard::lock_ref()` along with the unit
>>>> test, an "assert a lock is held by a guard" example is added.
>>>>
>>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>>> ---
>>>> This depends on Alice's patch:
>>>>
>>>> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>>>>
>>>> I'm also OK to fold this in if Alice thinks it's fine.
>>>>
>>>>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>>>>  1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>>> index 3701fac6ebf6..6d868e35b0a3 100644
>>>> --- a/rust/kernel/sync/lock.rs
>>>> +++ b/rust/kernel/sync/lock.rs
>>>> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>>>>
>>>>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>>>>      /// Returns the lock that this guard originates from.
>>>> +    ///
>>>> +    /// # Examples
>>>> +    ///
>>>> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
>>>> +    /// lock is held.
>>>> +    ///
>>>> +    /// ```
>>>> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
>>>> +    ///
>>>> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
>>>> +    ///     // Address-equal means the same lock.
>>>> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
>>>> +    /// }
>>>
>>> This seems super useful. Perhaps add this method as part of the lock api
>>> instead of just having it in the example?
>>
>> I don't think it should be an assert. Instead make it return a
>> `Result<(), ()>`. (or create better named unit error types)
> 
> No, this should not be part of usual control flow, and developers should
> not make control flow decisions based on this. It would always be an
> assertion. But you are right that `assert!` is probably not what we
> want. `debug_assert!` might be fine though.

I agree, that it shouldn't be used for driver logic, but you still might
want to warn/warn_once instead of panic (or debug_assert).

---
Cheers,
Benno

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Boqun Feng]

On Mon, Feb 24, 2025 at 09:08:09AM +0100, Andreas Hindborg wrote:
> Boqun Feng <boqun.feng@gmail.com> writes:
> 
> > To provide examples on usage of `Guard::lock_ref()` along with the unit
> > test, an "assert a lock is held by a guard" example is added.
> >
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> > This depends on Alice's patch:
> >
> > 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
> >
> > I'm also OK to fold this in if Alice thinks it's fine.
> >
> >  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 3701fac6ebf6..6d868e35b0a3 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
> >  
> >  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
> >      /// Returns the lock that this guard originates from.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
> > +    /// lock is held.
> > +    ///
> > +    /// ```
> > +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
> > +    ///
> > +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
> > +    ///     // Address-equal means the same lock.
> > +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
> > +    /// }
> 
> This seems super useful. Perhaps add this method as part of the lock api
> instead of just having it in the example?
> 

I would like to have this (along with Alice's lock_ref()) for the
upcoming Rust locking PR. So I'm going to keep it as a test now, but
happy to see a patch (or write one if needed). Thoughts?

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 
> 
> 

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 24.02.25 12:15, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On 24.02.25 09:08, Andreas Hindborg wrote:
>>>> Boqun Feng <boqun.feng@gmail.com> writes:
>>>>
>>>>> To provide examples on usage of `Guard::lock_ref()` along with the unit
>>>>> test, an "assert a lock is held by a guard" example is added.
>>>>>
>>>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>>>> ---
>>>>> This depends on Alice's patch:
>>>>>
>>>>> 	https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
>>>>>
>>>>> I'm also OK to fold this in if Alice thinks it's fine.
>>>>>
>>>>>  rust/kernel/sync/lock.rs | 24 ++++++++++++++++++++++++
>>>>>  1 file changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>>>> index 3701fac6ebf6..6d868e35b0a3 100644
>>>>> --- a/rust/kernel/sync/lock.rs
>>>>> +++ b/rust/kernel/sync/lock.rs
>>>>> @@ -201,6 +201,30 @@ unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
>>>>>
>>>>>  impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
>>>>>      /// Returns the lock that this guard originates from.
>>>>> +    ///
>>>>> +    /// # Examples
>>>>> +    ///
>>>>> +    /// The following example shows how to use [`Guard::lock_ref()`] to assert the corresponding
>>>>> +    /// lock is held.
>>>>> +    ///
>>>>> +    /// ```
>>>>> +    /// # use kernel::{new_spinlock, stack_pin_init, sync::lock::{Backend, Guard, Lock}};
>>>>> +    ///
>>>>> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
>>>>> +    ///     // Address-equal means the same lock.
>>>>> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
>>>>> +    /// }
>>>>
>>>> This seems super useful. Perhaps add this method as part of the lock api
>>>> instead of just having it in the example?
>>>
>>> I don't think it should be an assert. Instead make it return a
>>> `Result<(), ()>`. (or create better named unit error types)
>>
>> No, this should not be part of usual control flow, and developers should
>> not make control flow decisions based on this. It would always be an
>> assertion. But you are right that `assert!` is probably not what we
>> want. `debug_assert!` might be fine though.
>
> I agree, that it shouldn't be used for driver logic, but you still might
> want to warn/warn_once instead of panic (or debug_assert).

It might be useful to have an `assert!` that just does `pr_once!` on
failed assertion. I sort of said I would pick up the `pr_once!` patches,
so perhaps I should add that?


Best regards,
Andreas Hindborg

Re: [PATCH] rust: sync: lock: Add an example for Guard::lock_ref()

[Boqun Feng]

On Mon, Feb 24, 2025 at 12:19:27PM +0100, Alice Ryhl wrote:
> On Sun, Feb 23, 2025 at 8:21 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > To provide examples on usage of `Guard::lock_ref()` along with the unit
> > test, an "assert a lock is held by a guard" example is added.
> >
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 

Thanks!

> > This depends on Alice's patch:
> >
> >         https://lore.kernel.org/all/20250130-guard-get-lock-v1-1-8ed87899920a@google.com/
> >
> > I'm also OK to fold this in if Alice thinks it's fine.
> 
> I think we can just keep two patches :)
> 

Fine by me.

Regards,
Boqun

> Alice