The single user wants this the sane way around.  Write it as a normal static
inline just like rspin_lock().

Fixes: cc3e8df542ed ("xen/spinlock: add rspin_[un]lock_irq[save|restore]()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Juergen Gross <jgross@suse.com>
CC: George Dunlap <George.Dunlap@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
---
 xen/drivers/char/console.c |  6 +-----
 xen/include/xen/spinlock.h | 14 ++++++++------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index ccd5f8cc149f..349ce2a50d96 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1159,11 +1159,7 @@ void console_end_log_everything(void)
 
 unsigned long console_lock_recursive_irqsave(void)
 {
-    unsigned long flags;
-
-    rspin_lock_irqsave(&console_lock, flags);
-
-    return flags;
+    return rspin_lock_irqsave(&console_lock);
 }
 
 void console_unlock_recursive_irqrestore(unsigned long flags)
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 593cba640ee6..f210aa77f581 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -278,12 +278,6 @@ static always_inline void spin_lock_if(bool condition, spinlock_t *l)
  */
 bool _rspin_trylock(rspinlock_t *lock);
 void _rspin_lock(rspinlock_t *lock);
-#define rspin_lock_irqsave(l, f)                                \
-    ({                                                          \
-        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
-        (f) = _rspin_lock_irqsave(l);                           \
-        block_lock_speculation();                               \
-    })
 unsigned long _rspin_lock_irqsave(rspinlock_t *lock);
 void _rspin_unlock(rspinlock_t *lock);
 void _rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
@@ -294,6 +288,14 @@ static always_inline void rspin_lock(rspinlock_t *lock)
     block_lock_speculation();
 }
 
+static always_inline unsigned long rspin_lock_irqsave(rspinlock_t *lock)
+{
+    unsigned long flags = _rspin_lock_irqsave(lock);
+
+    block_lock_speculation();
+    return flags;
+}
+
 #define rspin_trylock(l)              lock_evaluate_nospec(_rspin_trylock(l))
 #define rspin_unlock(l)               _rspin_unlock(l)
 #define rspin_unlock_irqrestore(l, f) _rspin_unlock_irqrestore(l, f)

base-commit: d2276b86e5eb8dd2617d917f7b49cdd1f29ac299
-- 
2.30.2

On 19.03.2024 12:30, Andrew Cooper wrote:
> The single user wants this the sane way around.  Write it as a normal static
> inline just like rspin_lock().
> 
> Fixes: cc3e8df542ed ("xen/spinlock: add rspin_[un]lock_irq[save|restore]()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Notwithstanding Jürgen's R-b I'd be quite a bit happier if (a) this and
spin_lock_irqsave() remained consistent with one another or at least
(b) the implications of doing the necessary transformation for the
latter towards Linux compatibility were visible to have been considered,
in particular with it in mind that Misra won't like

#define spin_lock_irqsave(l, f)                               \
    ({                                                        \
        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));     \
        (f) = spin_lock_irqsave(l);                           \
    })

in linux-compat.h (and obviously with xen/spinlock.h included ahead of
this).

Jan

On 19.03.24 12:30, Andrew Cooper wrote:
> The single user wants this the sane way around.  Write it as a normal static
> inline just like rspin_lock().
> 
> Fixes: cc3e8df542ed ("xen/spinlock: add rspin_[un]lock_irq[save|restore]()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

Maybe with the subject fixed (s/rwlock/spinlock/).


Juergen

On 20/03/2024 06:35, Jürgen Groß wrote:
> On 19.03.24 12:30, Andrew Cooper wrote:
>> The single user wants this the sane way around.  Write it as a normal 
>> static
>> inline just like rspin_lock().
>>
>> Fixes: cc3e8df542ed ("xen/spinlock: add 
>> rspin_[un]lock_irq[save|restore]()")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Maybe with the subject fixed (s/rwlock/spinlock/).

And s/perpeuatite/perpetuate/ :).

Cheers,

-- 
Julien Grall