Re: [PATCH v11 05/10] genirq/msi-doorbell: msi_doorbell_pages
From: Thomas Gleixner <hidden>
Date: 2016-07-19 14:40:58
Also in:
kvmarm, linux-arm-kernel, linux-iommu, lkml
On Tue, 19 Jul 2016, Eric Auger wrote:
msi_doorbell_pages sum up the number of iommu pages of a given order
adding () to the function name would make it immediately clear that msi_doorbell_pages is a function.
+/** + * msi_doorbell_pages: compute the number of iommu pages of size 1 << order + * requested to map all the registered doorbells + * + * @order: iommu page order + */
Why are you adding the kernel doc to the header and not to the implementation?
quoted hunk ↗ jump to hunk
+int msi_doorbell_pages(unsigned int order); + #else static inline struct irq_chip_msi_doorbell_info *@@ -47,6 +55,12 @@ msi_doorbell_register_global(phys_addr_t base, size_t size, static inline void msi_doorbell_unregister_global(struct irq_chip_msi_doorbell_info *db) {} +static inline int +msi_doorbell_pages(unsigned int order)
What's the point of this line break?
quoted hunk ↗ jump to hunk
+{ + return 0; +} + #endif /* CONFIG_MSI_DOORBELL */ #endifdiff --git a/kernel/irq/msi-doorbell.c b/kernel/irq/msi-doorbell.c index 0ff541e..a5bde37 100644 --- a/kernel/irq/msi-doorbell.c +++ b/kernel/irq/msi-doorbell.c@@ -60,3 +60,55 @@ void msi_doorbell_unregister_global(struct irq_chip_msi_doorbell_info *dbinfo) mutex_unlock(&irqchip_doorbell_mutex); } EXPORT_SYMBOL_GPL(msi_doorbell_unregister_global); + +static int compute_db_mapping_requirements(phys_addr_t addr, size_t size, + unsigned int order) +{ + phys_addr_t offset, granule; + unsigned int nb_pages; + + granule = (uint64_t)(1 << order); + offset = addr & (granule - 1); + size = ALIGN(size + offset, granule); + nb_pages = size >> order; + + return nb_pages; +} + +static int +compute_dbinfo_mapping_requirements(struct irq_chip_msi_doorbell_info *dbinfo, + unsigned int order)
I'm sure you can find even longer function names which require more line breaks.
+{
+ int ret = 0;
+
+ if (!dbinfo->doorbell_is_percpu) {
+ ret = compute_db_mapping_requirements(dbinfo->global_doorbell,
+ dbinfo->size, order);
+ } else {
+ phys_addr_t __percpu *pbase;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ pbase = per_cpu_ptr(dbinfo->percpu_doorbells, cpu);
+ ret += compute_db_mapping_requirements(*pbase,
+ dbinfo->size,
+ order);
+ }
+ }
+ return ret;
+}
+
+int msi_doorbell_pages(unsigned int order)
+{
+ struct irqchip_doorbell *db;
+ int ret = 0;
+
+ mutex_lock(&irqchip_doorbell_mutex);
+ list_for_each_entry(db, &irqchip_doorbell_list, next) {Pointless braces
+ ret += compute_dbinfo_mapping_requirements(&db->info, order); + } + mutex_unlock(&irqchip_doorbell_mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(msi_doorbell_pages);
So here is a general rant about your naming choices.
struct irqchip_doorbell
struct irq_chip_msi_doorbell_info
struct irq_chip {
.... *(*msi_doorbell_info);
}
irqchip_doorbell_mutex
msi_doorbell_register_global
msi_doorbell_unregister_global
msi_doorbell_pages
This really sucks. Your public functions start sensibly with msi_doorbell.
Though what is the _global postfix for the register/unregister functions for?
Are there _private functions in the pipeline?
msi_doorbell_pages() is not telling me what it does. msi_calc_doorbell_pages()
would describe it right away.
You doorbell info structure can really do with:
struct msi_doorbell_info;
And the wrapper struct around it is fine with:
struct msi_doorbell;
Thanks,
tglx