| Message ID | 605687eff99db7181ef6c05bce671e20f194b46b.1691488505.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: address MISRA C:2012 Rule 5.3 and 8.3 | expand |
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
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).
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.
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,
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(-)