From: Denis Mukhin <dmukhin@ford.com>
Replace VPL011_{LOCK,UNLOCK} macros with raw spinlock calls to improve
readability.
No functional change.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
xen/arch/arm/include/asm/vpl011.h | 4 ---
xen/arch/arm/vpl011.c | 50 +++++++++++++++----------------
2 files changed, 25 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/include/asm/vpl011.h b/xen/arch/arm/include/asm/vpl011.h
index 5c308cc8c148..8f6ea0005e72 100644
--- a/xen/arch/arm/include/asm/vpl011.h
+++ b/xen/arch/arm/include/asm/vpl011.h
@@ -24,10 +24,6 @@
#include <public/io/console.h>
#include <xen/mm.h>
-/* helper macros */
-#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
-#define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
-
#define SBSA_UART_FIFO_SIZE 32
/* Same size as VUART_BUF_SIZE, used in vuart.c */
#define SBSA_UART_OUT_BUF_SIZE 128
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 2cf88a70ecdb..a97c3b74208c 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -85,7 +85,7 @@ static void vpl011_write_data_xen(struct domain *d, uint8_t data)
struct vpl011_xen_backend *intf = vpl011->backend.xen;
struct domain *input = console_get_domain();
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
intf->out[intf->out_prod++] = data;
if ( d == input )
@@ -127,7 +127,7 @@ static void vpl011_write_data_xen(struct domain *d, uint8_t data)
vpl011->uartfr |= TXFE;
vpl011_update_interrupt_status(d);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
console_put_domain(input);
}
@@ -144,7 +144,7 @@ static uint8_t vpl011_read_data_xen(struct domain *d)
struct vpl011_xen_backend *intf = vpl011->backend.xen;
XENCONS_RING_IDX in_cons, in_prod;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
in_cons = intf->in_cons;
in_prod = intf->in_prod;
@@ -190,7 +190,7 @@ static uint8_t vpl011_read_data_xen(struct domain *d)
*/
vpl011->uartfr &= ~RXFF;
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return data;
}
@@ -203,7 +203,7 @@ static uint8_t vpl011_read_data(struct domain *d)
struct xencons_interface *intf = vpl011->backend.dom.ring_buf;
XENCONS_RING_IDX in_cons, in_prod;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
in_cons = intf->in_cons;
in_prod = intf->in_prod;
@@ -249,7 +249,7 @@ static uint8_t vpl011_read_data(struct domain *d)
*/
vpl011->uartfr &= ~RXFF;
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
/*
* Send an event to console backend to indicate that data has been
@@ -288,7 +288,7 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
struct xencons_interface *intf = vpl011->backend.dom.ring_buf;
XENCONS_RING_IDX out_cons, out_prod;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
out_cons = intf->out_cons;
out_prod = intf->out_prod;
@@ -336,7 +336,7 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
vpl011->uartfr &= ~TXFE;
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
/*
* Send an event to console backend to indicate that there is
@@ -378,34 +378,34 @@ static int vpl011_mmio_read(struct vcpu *v,
case FR:
if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
*r = vreg_reg32_extract(vpl011->uartfr, info);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 1;
case RIS:
if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
*r = vreg_reg32_extract(vpl011->uartris, info);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 1;
case MIS:
if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
*r = vreg_reg32_extract(vpl011->uartris & vpl011->uartimsc,
info);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 1;
case IMSC:
if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
*r = vreg_reg32_extract(vpl011->uartimsc, info);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 1;
case ICR:
@@ -476,19 +476,19 @@ static int vpl011_mmio_write(struct vcpu *v,
case IMSC:
if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
vreg_reg32_update(&vpl011->uartimsc, r, info);
vpl011_update_interrupt_status(v->domain);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 1;
case ICR:
if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
vreg_reg32_clearbits(&vpl011->uartris, r, info);
vpl011_update_interrupt_status(d);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 1;
default:
@@ -587,13 +587,13 @@ int vuart_putchar(struct domain *d, char c)
if ( intf == NULL )
return -ENODEV;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
in_cons = intf->in_cons;
in_prod = intf->in_prod;
if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == sizeof(intf->in) )
{
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return -ENOSPC;
}
@@ -605,7 +605,7 @@ int vuart_putchar(struct domain *d, char c)
sizeof(intf->in));
vpl011_data_avail(d, in_fifo_level, sizeof(intf->in), 0, SBSA_UART_FIFO_SIZE);
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
return 0;
}
@@ -619,7 +619,7 @@ static void vpl011_notification(struct vcpu *v, unsigned int port)
XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
XENCONS_RING_IDX in_fifo_level, out_fifo_level;
- VPL011_LOCK(d, flags);
+ spin_lock_irqsave(&vpl011->lock, flags);
in_cons = intf->in_cons;
in_prod = intf->in_prod;
@@ -639,7 +639,7 @@ static void vpl011_notification(struct vcpu *v, unsigned int port)
vpl011_data_avail(v->domain, in_fifo_level, sizeof(intf->in),
out_fifo_level, sizeof(intf->out));
- VPL011_UNLOCK(d, flags);
+ spin_unlock_irqrestore(&vpl011->lock, flags);
}
int vuart_init(struct domain *d, struct vuart_params *params)
--
2.34.1On 24.06.2025 05:55, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
>
> Replace VPL011_{LOCK,UNLOCK} macros with raw spinlock calls to improve
> readability.
I'm not an Arm maintainer, so I have limited say here, but: How is this
improving readability? It better utilizes available local variables, yes,
so this may be a little bit of an optimization, but otherwise to me this
looks to rather hamper readability.
Jan
On 24/06/2025 07:46, Jan Beulich wrote:
> On 24.06.2025 05:55, dmkhn@proton.me wrote:
>> From: Denis Mukhin <dmukhin@ford.com>
>>
>> Replace VPL011_{LOCK,UNLOCK} macros with raw spinlock calls to improve
>> readability.
>
> I'm not an Arm maintainer, so I have limited say here, but: How is this
> improving readability? It better utilizes available local variables, yes,
> so this may be a little bit of an optimization, but otherwise to me this
> looks to rather hamper readability.
I agree with Jan here. I don't think it improves readability, therefore I don't
think such change is needed.
~Michal
On Tue, Jun 24, 2025 at 09:50:54AM +0200, Orzel, Michal wrote:
>
>
> On 24/06/2025 07:46, Jan Beulich wrote:
> > On 24.06.2025 05:55, dmkhn@proton.me wrote:
> >> From: Denis Mukhin <dmukhin@ford.com>
> >>
> >> Replace VPL011_{LOCK,UNLOCK} macros with raw spinlock calls to improve
> >> readability.
> >
> > I'm not an Arm maintainer, so I have limited say here, but: How is this
> > improving readability? It better utilizes available local variables, yes,
> > so this may be a little bit of an optimization, but otherwise to me this
> > looks to rather hamper readability.
> I agree with Jan here. I don't think it improves readability, therefore I don't
> think such change is needed.
I think exdanding macros helps to understand the code since is explicitly
shows what kind of locking *really* used, so this aspect is actually getting
more readable; yes, that's a bit of more text.
But, MMIO-based flavor does not define such helpers for example, so now vUARTs
follow similar coding pattern which is easy to read/follow.
>
> ~Michal
>
On 24/06/2025 23:46, dmkhn@proton.me wrote:
> On Tue, Jun 24, 2025 at 09:50:54AM +0200, Orzel, Michal wrote:
>>
>>
>> On 24/06/2025 07:46, Jan Beulich wrote:
>>> On 24.06.2025 05:55, dmkhn@proton.me wrote:
>>>> From: Denis Mukhin <dmukhin@ford.com>
>>>>
>>>> Replace VPL011_{LOCK,UNLOCK} macros with raw spinlock calls to improve
>>>> readability.
>>>
>>> I'm not an Arm maintainer, so I have limited say here, but: How is this
>>> improving readability? It better utilizes available local variables, yes,
>>> so this may be a little bit of an optimization, but otherwise to me this
>>> looks to rather hamper readability.
>> I agree with Jan here. I don't think it improves readability, therefore I don't
>> think such change is needed.
>
> I think exdanding macros helps to understand the code since is explicitly
> shows what kind of locking *really* used, so this aspect is actually getting
> more readable; yes, that's a bit of more text.
>
> But, MMIO-based flavor does not define such helpers for example, so now vUARTs
> follow similar coding pattern which is easy to read/follow.
I understand your point of view. It's more like a matter of taste here, so I
won't oppose to it. Others may chime in.
~Michal
On Wed, Jun 25, 2025 at 08:52:39AM +0200, Orzel, Michal wrote:
>
>
> On 24/06/2025 23:46, dmkhn@proton.me wrote:
> > On Tue, Jun 24, 2025 at 09:50:54AM +0200, Orzel, Michal wrote:
> >>
> >>
> >> On 24/06/2025 07:46, Jan Beulich wrote:
> >>> On 24.06.2025 05:55, dmkhn@proton.me wrote:
> >>>> From: Denis Mukhin <dmukhin@ford.com>
> >>>>
> >>>> Replace VPL011_{LOCK,UNLOCK} macros with raw spinlock calls to improve
> >>>> readability.
> >>>
> >>> I'm not an Arm maintainer, so I have limited say here, but: How is this
> >>> improving readability? It better utilizes available local variables, yes,
> >>> so this may be a little bit of an optimization, but otherwise to me this
> >>> looks to rather hamper readability.
> >> I agree with Jan here. I don't think it improves readability, therefore I don't
> >> think such change is needed.
> >
> > I think exdanding macros helps to understand the code since is explicitly
> > shows what kind of locking *really* used, so this aspect is actually getting
> > more readable; yes, that's a bit of more text.
> >
> > But, MMIO-based flavor does not define such helpers for example, so now vUARTs
> > follow similar coding pattern which is easy to read/follow.
> I understand your point of view. It's more like a matter of taste here, so I
> won't oppose to it. Others may chime in.
Thank you.
>
> ~Michal
>
© 2016 - 2025 Red Hat, Inc.