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

[Jan Beulich <jbeulich@xxxxxxxx>]

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