diff mbox series

[XEN,3/6] AMD/IOMMU: address violations of MISRA C:2012 Rule 8.2

Message ID c529cb0d8eba253436c5bba22f8bceddeaaf5831.1701764980.git.federico.serafini@bugseng.com (mailing list archive)
State Superseded
Headers show
Series xen: address violations of MISRA C:2012 Rule 8.2 | expand

Commit Message

Federico Serafini Dec. 5, 2023, 11:55 a.m. UTC 
Add missing parameter names to address violations of MISRA C:2012
Rule 8.2. Furthermore, remove trailing spaces and use C standard types to
comply with XEN coding style.

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/drivers/passthrough/amd/iommu.h      | 17 ++++++++++-------
 xen/drivers/passthrough/amd/iommu_init.c | 24 ++++++++++++++----------
 2 files changed, 24 insertions(+), 17 deletions(-)

Comments

Stefano Stabellini Dec. 6, 2023, 3:15 a.m. UTC | #1 
On Tue, 5 Dec 2023, Federico Serafini wrote:
> Add missing parameter names to address violations of MISRA C:2012
> Rule 8.2. Furthermore, remove trailing spaces and use C standard types to
> comply with XEN coding style.
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/drivers/passthrough/amd/iommu.h      | 17 ++++++++++-------
>  xen/drivers/passthrough/amd/iommu_init.c | 24 ++++++++++++++----------
>  2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
> index d4416ebc43..1b62c083ba 100644
> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -138,10 +138,12 @@ struct ivrs_mappings {
>  extern unsigned int ivrs_bdf_entries;
>  extern u8 ivhd_type;
>  
> -struct ivrs_mappings *get_ivrs_mappings(u16 seg);
> -int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
> -int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
> -                                 struct ivrs_mappings *, uint16_t));
> +struct ivrs_mappings *get_ivrs_mappings(uint16_t seg);
> +int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
> +                                         struct ivrs_mappings *map));
> +int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
> +                                        struct ivrs_mappings *map,
> +                                        uint16_t bdf));
>  
>  /* iommu tables in guest space */
>  struct mmio_reg {
> @@ -226,7 +228,7 @@ struct acpi_ivrs_hardware;
>  /* amd-iommu-detect functions */
>  int amd_iommu_get_ivrs_dev_entries(void);
>  int amd_iommu_get_supported_ivhd_type(void);
> -int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *);
> +int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *ivhd_block);
>  int amd_iommu_detect_acpi(void);
>  void get_iommu_features(struct amd_iommu *iommu);
>  
> @@ -295,9 +297,10 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf);
>  bool cf_check iov_supports_xt(void);
>  int amd_iommu_setup_ioapic_remapping(void);
>  void *amd_iommu_alloc_intremap_table(
> -    const struct amd_iommu *, unsigned long **, unsigned int nr);
> +    const struct amd_iommu *iommu, unsigned long **inuse_map, unsigned int nr);
>  int cf_check amd_iommu_free_intremap_table(
> -    const struct amd_iommu *, struct ivrs_mappings *, uint16_t);
> +    const struct amd_iommu *iommu, struct ivrs_mappings *ivrs_mapping,
> +    uint16_t bdf);
>  unsigned int amd_iommu_intremap_table_order(
>      const void *irt, const struct amd_iommu *iommu);
>  void cf_check amd_iommu_ioapic_update_ire(
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index 5515cb70fd..62f9bfdfc8 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -300,12 +300,13 @@ static void cf_check set_iommu_ppr_log_control(
>  static int iommu_read_log(struct amd_iommu *iommu,
>                            struct ring_buffer *log,
>                            unsigned int entry_size,
> -                          void (*parse_func)(struct amd_iommu *, u32 *))
> +                          void (*parse_func)(struct amd_iommu *iommu,
> +                                             uint32_t *entry))
>  {
>      unsigned int tail, tail_offest, head_offset;
>  
>      BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
> -    
> +
>      spin_lock(&log->lock);
>  
>      /* make sure there's an entry in the log */
> @@ -361,14 +362,15 @@ static int iommu_read_log(struct amd_iommu *iommu,
>  
>   out:
>      spin_unlock(&log->lock);
> -   
> +
>      return 0;
>  }
>  
>  /* reset event log or ppr log when overflow */
>  static void iommu_reset_log(struct amd_iommu *iommu,
>                              struct ring_buffer *log,
> -                            void (*ctrl_func)(struct amd_iommu *iommu, bool))
> +                            void (*ctrl_func)(struct amd_iommu *iommu,
> +                                              bool iommu_control))

instead of iommu_control it should be iommu_enable ?


>  {
>      unsigned int entry, run_bit, loop_count = 1000;
>      bool log_run;
> @@ -1158,14 +1160,15 @@ static void __init amd_iommu_init_cleanup(void)
>      iommuv2_enabled = 0;
>  }
>  
> -struct ivrs_mappings *get_ivrs_mappings(u16 seg)
> +struct ivrs_mappings *get_ivrs_mappings(uint16_t seg)
>  {
>      return radix_tree_lookup(&ivrs_maps, seg);
>  }
>  
> -int iterate_ivrs_mappings(int (*handler)(u16 seg, struct ivrs_mappings *))
> +int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
> +                                         struct ivrs_mappings *map))

Instead of map it should be ivrs_mappings ? Actually it reads better as
map and I know it is not a MISRA requirement to have function pointer
args match. I'll leave this one to Jan.




>  {
> -    u16 seg = 0;
> +    uint16_t seg = 0;
>      int rc = 0;
>  
>      do {
> @@ -1180,10 +1183,11 @@ int iterate_ivrs_mappings(int (*handler)(u16 seg, struct ivrs_mappings *))
>      return rc;
>  }
>  
> -int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *,
> -                                        struct ivrs_mappings *, uint16_t bdf))
> +int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
> +                                        struct ivrs_mappings *map,

same here ivrs_mappings instead of map, but actually map reads better

I checked everything else and looks OK
Jan Beulich Dec. 6, 2023, 8:16 a.m. UTC | #2 
On 06.12.2023 04:15, Stefano Stabellini wrote:
> On Tue, 5 Dec 2023, Federico Serafini wrote:
>> --- a/xen/drivers/passthrough/amd/iommu.h
>> +++ b/xen/drivers/passthrough/amd/iommu.h
>> @@ -138,10 +138,12 @@ struct ivrs_mappings {
>>  extern unsigned int ivrs_bdf_entries;
>>  extern u8 ivhd_type;
>>  
>> -struct ivrs_mappings *get_ivrs_mappings(u16 seg);
>> -int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
>> -int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
>> -                                 struct ivrs_mappings *, uint16_t));
>> +struct ivrs_mappings *get_ivrs_mappings(uint16_t seg);
>> +int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
>> +                                         struct ivrs_mappings *map));
>> +int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
>> +                                        struct ivrs_mappings *map,
>> +                                        uint16_t bdf));

(Note this for the comment near the end.)

>> @@ -361,14 +362,15 @@ static int iommu_read_log(struct amd_iommu *iommu,
>>  
>>   out:
>>      spin_unlock(&log->lock);
>> -   
>> +
>>      return 0;
>>  }
>>  
>>  /* reset event log or ppr log when overflow */
>>  static void iommu_reset_log(struct amd_iommu *iommu,
>>                              struct ring_buffer *log,
>> -                            void (*ctrl_func)(struct amd_iommu *iommu, bool))
>> +                            void (*ctrl_func)(struct amd_iommu *iommu,
>> +                                              bool iommu_control))
> 
> instead of iommu_control it should be iommu_enable ?

What purpose would "iommu_" serve? It would be actively confusing, for
colliding with the same-name global we have. Both functions passed here
use simply "enable".

>> @@ -1158,14 +1160,15 @@ static void __init amd_iommu_init_cleanup(void)
>>      iommuv2_enabled = 0;
>>  }
>>  
>> -struct ivrs_mappings *get_ivrs_mappings(u16 seg)
>> +struct ivrs_mappings *get_ivrs_mappings(uint16_t seg)
>>  {
>>      return radix_tree_lookup(&ivrs_maps, seg);
>>  }
>>  
>> -int iterate_ivrs_mappings(int (*handler)(u16 seg, struct ivrs_mappings *))
>> +int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
>> +                                         struct ivrs_mappings *map))
> 
> Instead of map it should be ivrs_mappings ? Actually it reads better as
> map and I know it is not a MISRA requirement to have function pointer
> args match. I'll leave this one to Jan.

The name is entirely meaningless here (i.e. not helping with anything),
so imo "map" is not only fine but also (see above) consistent with other
code.

Jan
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index d4416ebc43..1b62c083ba 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -138,10 +138,12 @@  struct ivrs_mappings {
 extern unsigned int ivrs_bdf_entries;
 extern u8 ivhd_type;
 
-struct ivrs_mappings *get_ivrs_mappings(u16 seg);
-int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
-int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
-                                 struct ivrs_mappings *, uint16_t));
+struct ivrs_mappings *get_ivrs_mappings(uint16_t seg);
+int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
+                                         struct ivrs_mappings *map));
+int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
+                                        struct ivrs_mappings *map,
+                                        uint16_t bdf));
 
 /* iommu tables in guest space */
 struct mmio_reg {
@@ -226,7 +228,7 @@  struct acpi_ivrs_hardware;
 /* amd-iommu-detect functions */
 int amd_iommu_get_ivrs_dev_entries(void);
 int amd_iommu_get_supported_ivhd_type(void);
-int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *);
+int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *ivhd_block);
 int amd_iommu_detect_acpi(void);
 void get_iommu_features(struct amd_iommu *iommu);
 
@@ -295,9 +297,10 @@  struct amd_iommu *find_iommu_for_device(int seg, int bdf);
 bool cf_check iov_supports_xt(void);
 int amd_iommu_setup_ioapic_remapping(void);
 void *amd_iommu_alloc_intremap_table(
-    const struct amd_iommu *, unsigned long **, unsigned int nr);
+    const struct amd_iommu *iommu, unsigned long **inuse_map, unsigned int nr);
 int cf_check amd_iommu_free_intremap_table(
-    const struct amd_iommu *, struct ivrs_mappings *, uint16_t);
+    const struct amd_iommu *iommu, struct ivrs_mappings *ivrs_mapping,
+    uint16_t bdf);
 unsigned int amd_iommu_intremap_table_order(
     const void *irt, const struct amd_iommu *iommu);
 void cf_check amd_iommu_ioapic_update_ire(
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 5515cb70fd..62f9bfdfc8 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -300,12 +300,13 @@  static void cf_check set_iommu_ppr_log_control(
 static int iommu_read_log(struct amd_iommu *iommu,
                           struct ring_buffer *log,
                           unsigned int entry_size,
-                          void (*parse_func)(struct amd_iommu *, u32 *))
+                          void (*parse_func)(struct amd_iommu *iommu,
+                                             uint32_t *entry))
 {
     unsigned int tail, tail_offest, head_offset;
 
     BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
-    
+
     spin_lock(&log->lock);
 
     /* make sure there's an entry in the log */
@@ -361,14 +362,15 @@  static int iommu_read_log(struct amd_iommu *iommu,
 
  out:
     spin_unlock(&log->lock);
-   
+
     return 0;
 }
 
 /* reset event log or ppr log when overflow */
 static void iommu_reset_log(struct amd_iommu *iommu,
                             struct ring_buffer *log,
-                            void (*ctrl_func)(struct amd_iommu *iommu, bool))
+                            void (*ctrl_func)(struct amd_iommu *iommu,
+                                              bool iommu_control))
 {
     unsigned int entry, run_bit, loop_count = 1000;
     bool log_run;
@@ -1158,14 +1160,15 @@  static void __init amd_iommu_init_cleanup(void)
     iommuv2_enabled = 0;
 }
 
-struct ivrs_mappings *get_ivrs_mappings(u16 seg)
+struct ivrs_mappings *get_ivrs_mappings(uint16_t seg)
 {
     return radix_tree_lookup(&ivrs_maps, seg);
 }
 
-int iterate_ivrs_mappings(int (*handler)(u16 seg, struct ivrs_mappings *))
+int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
+                                         struct ivrs_mappings *map))
 {
-    u16 seg = 0;
+    uint16_t seg = 0;
     int rc = 0;
 
     do {
@@ -1180,10 +1183,11 @@  int iterate_ivrs_mappings(int (*handler)(u16 seg, struct ivrs_mappings *))
     return rc;
 }
 
-int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *,
-                                        struct ivrs_mappings *, uint16_t bdf))
+int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
+                                        struct ivrs_mappings *map,
+                                        uint16_t bdf))
 {
-    u16 seg = 0;
+    uint16_t seg = 0;
     int rc = 0;
 
     do {