The local variables 'irq_desc' shadow the homonymous global variable,
declared in 'xen/arch/x86/include/asm/irq.h', therefore they are renamed
'irqd' for consistency with ARM code. Other variables of the same type
in the file are also renamed 'irqd' for consistency.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Renamed the local variables instead of the global.
- Edited subject from x86/irq to x86/vmsi
Renaming everything to 'desc' would have been the most obvious choice,
but given that there's also 'msi_desc' used in the same functions, 'irqd'
is less error-prone in my opinion.
---
xen/arch/x86/hvm/vmsi.c | 46 ++++++++++++++++++++---------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 3cd4923060..55d5e26a04 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
unsigned int nr_entry, index;
int r = X86EMUL_UNHANDLEABLE;
unsigned long flags;
- struct irq_desc *desc;
+ struct irq_desc *irqd;
if ( (len != 4 && len != 8) || (address & (len - 1)) )
return r;
@@ -330,21 +330,21 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
if ( !msi_desc || msi_desc->irq < 0 )
goto out;
- desc = irq_to_desc(msi_desc->irq);
- if ( !desc )
+ irqd = irq_to_desc(msi_desc->irq);
+ if ( !irqd )
goto out;
- spin_lock_irqsave(&desc->lock, flags);
+ spin_lock_irqsave(&irqd->lock, flags);
- if ( !desc->msi_desc )
+ if ( !irqd->msi_desc )
goto unlock;
- ASSERT(msi_desc == desc->msi_desc);
+ ASSERT(msi_desc == irqd->msi_desc);
- guest_mask_msi_irq(desc, !!(val & PCI_MSIX_VECTOR_BITMASK));
+ guest_mask_msi_irq(irqd, !!(val & PCI_MSIX_VECTOR_BITMASK));
unlock:
- spin_unlock_irqrestore(&desc->lock, flags);
+ spin_unlock_irqrestore(&irqd->lock, flags);
if ( len == 4 )
r = X86EMUL_OKAY;
@@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry *entry)
int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
{
- struct irq_desc *irq_desc;
+ struct irq_desc *irqd;
struct msi_desc *msi_desc;
struct pci_dev *pdev;
struct msixtbl_entry *entry, *new_entry;
@@ -482,14 +482,14 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
if ( !new_entry )
return -ENOMEM;
- irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
- if ( !irq_desc )
+ irqd = pirq_spin_lock_irq_desc(pirq, NULL);
+ if ( !irqd )
{
xfree(new_entry);
return r;
}
- msi_desc = irq_desc->msi_desc;
+ msi_desc = irqd->msi_desc;
if ( !msi_desc )
goto out;
@@ -508,7 +508,7 @@ found:
r = 0;
out:
- spin_unlock_irq(&irq_desc->lock);
+ spin_unlock_irq(&irqd->lock);
xfree(new_entry);
if ( !r )
@@ -533,7 +533,7 @@ out:
void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
{
- struct irq_desc *irq_desc;
+ struct irq_desc *irqd;
struct msi_desc *msi_desc;
struct pci_dev *pdev;
struct msixtbl_entry *entry;
@@ -544,11 +544,11 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
if ( !msixtbl_initialised(d) )
return;
- irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
- if ( !irq_desc )
+ irqd = pirq_spin_lock_irq_desc(pirq, NULL);
+ if ( !irqd )
return;
- msi_desc = irq_desc->msi_desc;
+ msi_desc = irqd->msi_desc;
if ( !msi_desc )
goto out;
@@ -559,14 +559,14 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
goto found;
out:
- spin_unlock_irq(&irq_desc->lock);
+ spin_unlock_irq(&irqd->lock);
return;
found:
if ( !atomic_dec_and_test(&entry->refcnt) )
del_msixtbl_entry(entry);
- spin_unlock_irq(&irq_desc->lock);
+ spin_unlock_irq(&irqd->lock);
}
void msixtbl_init(struct domain *d)
@@ -664,12 +664,12 @@ static unsigned int msi_gflags(uint16_t data, uint64_t addr, bool masked)
static void vpci_mask_pirq(struct domain *d, int pirq, bool mask)
{
unsigned long flags;
- struct irq_desc *desc = domain_spin_lock_irq_desc(d, pirq, &flags);
+ struct irq_desc *irqd = domain_spin_lock_irq_desc(d, pirq, &flags);
- if ( !desc )
+ if ( !irqd )
return;
- guest_mask_msi_irq(desc, mask);
- spin_unlock_irqrestore(&desc->lock, flags);
+ guest_mask_msi_irq(irqd, mask);
+ spin_unlock_irqrestore(&irqd->lock, flags);
}
void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev,
--
2.34.1On 08.08.2023 14:22, Nicola Vetrini wrote:
> The local variables 'irq_desc' shadow the homonymous global variable,
> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are renamed
> 'irqd' for consistency with ARM code. Other variables of the same type
> in the file are also renamed 'irqd' for consistency.
I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
So "consistency with ARM code" doesn't ...
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
> unsigned int nr_entry, index;
> int r = X86EMUL_UNHANDLEABLE;
> unsigned long flags;
> - struct irq_desc *desc;
> + struct irq_desc *irqd;
... require e.g. this rename. As mentioned before: Let's limit code
churn where possible, and where going beyond what's strictly necessary
isn't otherwise useful; there's already enough of it with all these not
really helpful Misra changes.
> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry *entry)
>
> int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
> {
> - struct irq_desc *irq_desc;
> + struct irq_desc *irqd;
This one indeed wants renaming, but then - consistent within the file -
to "desc". At least that's my view.
Jan
On 08/08/2023 15:22, Jan Beulich wrote:
> On 08.08.2023 14:22, Nicola Vetrini wrote:
>> The local variables 'irq_desc' shadow the homonymous global variable,
>> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are
>> renamed
>> 'irqd' for consistency with ARM code. Other variables of the same type
>> in the file are also renamed 'irqd' for consistency.
>
> I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
> So "consistency with ARM code" doesn't ...
>
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned
>> long address,
>> unsigned int nr_entry, index;
>> int r = X86EMUL_UNHANDLEABLE;
>> unsigned long flags;
>> - struct irq_desc *desc;
>> + struct irq_desc *irqd;
>
> ... require e.g. this rename. As mentioned before: Let's limit code
> churn where possible, and where going beyond what's strictly necessary
> isn't otherwise useful; there's already enough of it with all these not
> really helpful Misra changes.
>
>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry
>> *entry)
>>
>> int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t
>> gtable)
>> {
>> - struct irq_desc *irq_desc;
>> + struct irq_desc *irqd;
>
> This one indeed wants renaming, but then - consistent within the file -
> to "desc". At least that's my view.
>
> Jan
Well, but having
struct irq_desc *desc;
struct msi_desc *msi_desc;
and then using them both within the function doesn't seem that readable,
but if you
prefer "desc" I have no objection (just two local variables that need to
be changed).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
On 08.08.2023 15:38, Nicola Vetrini wrote:
> On 08/08/2023 15:22, Jan Beulich wrote:
>> On 08.08.2023 14:22, Nicola Vetrini wrote:
>>> The local variables 'irq_desc' shadow the homonymous global variable,
>>> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are
>>> renamed
>>> 'irqd' for consistency with ARM code. Other variables of the same type
>>> in the file are also renamed 'irqd' for consistency.
>>
>> I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
>> So "consistency with ARM code" doesn't ...
>>
>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned
>>> long address,
>>> unsigned int nr_entry, index;
>>> int r = X86EMUL_UNHANDLEABLE;
>>> unsigned long flags;
>>> - struct irq_desc *desc;
>>> + struct irq_desc *irqd;
>>
>> ... require e.g. this rename. As mentioned before: Let's limit code
>> churn where possible, and where going beyond what's strictly necessary
>> isn't otherwise useful; there's already enough of it with all these not
>> really helpful Misra changes.
>>
>>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry
>>> *entry)
>>>
>>> int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t
>>> gtable)
>>> {
>>> - struct irq_desc *irq_desc;
>>> + struct irq_desc *irqd;
>>
>> This one indeed wants renaming, but then - consistent within the file -
>> to "desc". At least that's my view.
>
> Well, but having
>
> struct irq_desc *desc;
> struct msi_desc *msi_desc;
>
> and then using them both within the function doesn't seem that readable,
You have a point there, yes. Still I'd then probably follow up with a
change to rename msi_desc -> msi (and I say this despite seeing that
farther down in the file "msi" is also used for another pointer type
variables/parameters). But with what you say in mind I'd also be okay
with you renaming to irqd where renaming is needed, but leaving "desc"
alone.
Jan
>>>
>>>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct
>>>> msixtbl_entry
>>>> *entry)
>>>>
>>>> int msixtbl_pt_register(struct domain *d, struct pirq *pirq,
>>>> uint64_t
>>>> gtable)
>>>> {
>>>> - struct irq_desc *irq_desc;
>>>> + struct irq_desc *irqd;
>>>
>>> This one indeed wants renaming, but then - consistent within the file
>>> -
>>> to "desc". At least that's my view.
>>
>> Well, but having
>>
>> struct irq_desc *desc;
>> struct msi_desc *msi_desc;
>>
>> and then using them both within the function doesn't seem that
>> readable,
>
> You have a point there, yes. Still I'd then probably follow up with a
> change to rename msi_desc -> msi (and I say this despite seeing that
> farther down in the file "msi" is also used for another pointer type
> variables/parameters). But with what you say in mind I'd also be okay
> with you renaming to irqd where renaming is needed, but leaving "desc"
> alone.
>
> Jan
I'll go for the latter (it's quicker) as a separate patch, since
hopefully the other patch
in the series can go in unmodified.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
© 2016 - 2026 Red Hat, Inc.