xen/PCI: address violations of MISRA C:2012 Rules 8.2 and 8.3

[XEN PATCH] xen/PCI: address violations of MISRA C:2012 Rules 8.2 and 8.3

Posted by Federico Serafini 7 months, 4 weeks ago

Add missing parameter names and make function declarations and
definitions consistent. No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/pci.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 7d8a7cd213..3ed79d15cd 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -166,14 +166,14 @@ int scan_pci_devices(void);
 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn);
 int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
 
-void setup_hwdom_pci_devices(struct domain *,
-                            int (*)(u8 devfn, struct pci_dev *));
+void setup_hwdom_pci_devices(struct domain *d,
+                            int (*handler)(u8 devfn, struct pci_dev *pdev));
 int pci_release_devices(struct domain *d);
 void pci_segments_init(void);
 int pci_add_segment(u16 seg);
 const unsigned long *pci_get_ro_map(u16 seg);
 int pci_add_device(u16 seg, u8 bus, u8 devfn,
-                   const struct pci_dev_info *, nodeid_t node);
+                   const struct pci_dev_info *info, nodeid_t node);
 int pci_remove_device(u16 seg, u8 bus, u8 devfn);
 int pci_ro_device(int seg, int bus, int devfn);
 int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
@@ -198,10 +198,11 @@ int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
 int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
 int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
                                  int cap);
-const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
-                      unsigned int *dev, unsigned int *func);
-const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
-                          unsigned int *dev, unsigned int *func, bool *def_seg);
+const char *parse_pci(const char *s, unsigned int *seg_p, unsigned int *bus_p,
+                      unsigned int *dev_p, unsigned int *func_p);
+const char *parse_pci_seg(const char *s, unsigned int *seg_p,
+                          unsigned int *bus_p, unsigned int *dev_p,
+                          unsigned int *func_p, bool *def_seg);
 
 #define PCI_BAR_VF      (1u << 0)
 #define PCI_BAR_LAST    (1u << 1)
@@ -210,12 +211,12 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos,
                               uint64_t *paddr, uint64_t *psize,
                               unsigned int flags);
 
-void pci_intx(const struct pci_dev *, bool enable);
-bool pcie_aer_get_firmware_first(const struct pci_dev *);
+void pci_intx(const struct pci_dev *pdev, bool enable);
+bool pcie_aer_get_firmware_first(const struct pci_dev *pdev);
 
 struct pirq;
-int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
-void msixtbl_pt_unregister(struct domain *, struct pirq *);
+int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable);
+void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq);
 void msixtbl_pt_cleanup(struct domain *d);
 
 #ifdef CONFIG_HVM
-- 
2.34.1

Re: [XEN PATCH] xen/PCI: address violations of MISRA C:2012 Rules 8.2 and 8.3

Posted by Jan Beulich 7 months, 4 weeks ago

On 11.09.2023 10:15, Federico Serafini wrote:
> Add missing parameter names and make function declarations and
> definitions consistent. No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Since formally correct:
Acked-by: Jan Beulich <jbeulich@suse.com>

Nevertheless, it is probably about time to mention that ...

> @@ -198,10 +198,11 @@ int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
>  int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
>  int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
>                                   int cap);
> -const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
> -                      unsigned int *dev, unsigned int *func);
> -const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
> -                          unsigned int *dev, unsigned int *func, bool *def_seg);
> +const char *parse_pci(const char *s, unsigned int *seg_p, unsigned int *bus_p,
> +                      unsigned int *dev_p, unsigned int *func_p);
> +const char *parse_pci_seg(const char *s, unsigned int *seg_p,
> +                          unsigned int *bus_p, unsigned int *dev_p,
> +                          unsigned int *func_p, bool *def_seg);

... parameter renaming like this, while fulfilling the word of Misra, is
actually hampering readability. To someone wanting to use the function
and hence looking at the declaration it is entirely uninteresting
whether the _p suffixes are there; there were similar changes earlier on.
The longer names merely make reading harder and - as is the case here -
also may require a single declaration to wrap across more lines. I view
such as going against the spirit of Misra (aiming to improve code
clarity).

Jan

Re: [XEN PATCH] xen/PCI: address violations of MISRA C:2012 Rules 8.2 and 8.3

Posted by Federico Serafini 7 months, 3 weeks ago

On 11/09/23 10:42, Jan Beulich wrote:
> On 11.09.2023 10:15, Federico Serafini wrote:
>> Add missing parameter names and make function declarations and
>> definitions consistent. No functional change.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> Since formally correct:
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Nevertheless, it is probably about time to mention that ...
> 
>> @@ -198,10 +198,11 @@ int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
>>   int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
>>   int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
>>                                    int cap);
>> -const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
>> -                      unsigned int *dev, unsigned int *func);
>> -const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
>> -                          unsigned int *dev, unsigned int *func, bool *def_seg);
>> +const char *parse_pci(const char *s, unsigned int *seg_p, unsigned int *bus_p,
>> +                      unsigned int *dev_p, unsigned int *func_p);
>> +const char *parse_pci_seg(const char *s, unsigned int *seg_p,
>> +                          unsigned int *bus_p, unsigned int *dev_p,
>> +                          unsigned int *func_p, bool *def_seg);
> 
> ... parameter renaming like this, while fulfilling the word of Misra, is
> actually hampering readability. To someone wanting to use the function
> and hence looking at the declaration it is entirely uninteresting
> whether the _p suffixes are there; there were similar changes earlier on.
> The longer names merely make reading harder and - as is the case here -
> also may require a single declaration to wrap across more lines. I view
> such as going against the spirit of Misra (aiming to improve code
> clarity).
> 
> Jan
> 

I agree with you, but, the removal of _p would lead to
other (mechanical) changes to the function body.
In such cases, do you think that an improvement in readability
justifies the code churn?

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH] xen/PCI: address violations of MISRA C:2012 Rules 8.2 and 8.3

Posted by Jan Beulich 7 months, 3 weeks ago

On 11.09.2023 17:13, Federico Serafini wrote:
> On 11/09/23 10:42, Jan Beulich wrote:
>> On 11.09.2023 10:15, Federico Serafini wrote:
>>> Add missing parameter names and make function declarations and
>>> definitions consistent. No functional change.
>>>
>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>
>> Since formally correct:
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> Nevertheless, it is probably about time to mention that ...
>>
>>> @@ -198,10 +198,11 @@ int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
>>>   int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
>>>   int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
>>>                                    int cap);
>>> -const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
>>> -                      unsigned int *dev, unsigned int *func);
>>> -const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
>>> -                          unsigned int *dev, unsigned int *func, bool *def_seg);
>>> +const char *parse_pci(const char *s, unsigned int *seg_p, unsigned int *bus_p,
>>> +                      unsigned int *dev_p, unsigned int *func_p);
>>> +const char *parse_pci_seg(const char *s, unsigned int *seg_p,
>>> +                          unsigned int *bus_p, unsigned int *dev_p,
>>> +                          unsigned int *func_p, bool *def_seg);
>>
>> ... parameter renaming like this, while fulfilling the word of Misra, is
>> actually hampering readability. To someone wanting to use the function
>> and hence looking at the declaration it is entirely uninteresting
>> whether the _p suffixes are there; there were similar changes earlier on.
>> The longer names merely make reading harder and - as is the case here -
>> also may require a single declaration to wrap across more lines. I view
>> such as going against the spirit of Misra (aiming to improve code
>> clarity).
> 
> I agree with you, but, the removal of _p would lead to
> other (mechanical) changes to the function body.
> In such cases, do you think that an improvement in readability
> justifies the code churn?

I didn't suggest changing the definition. I suggested keeping declaration
and definition (slightly) out of sync.

Jan