| Message ID | 20230124135554.13787-1-bpappas@pappasbrent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mdeia: ipu3: ipu33-mmu: Replace macro IPU3_ADDR2PTE() with a function | expand |
On Tue, Jan 24, 2023 at 08:55:54AM -0500, Brent Pappas wrote: > Replace the macro IPU3_ADDR2PTE() with a static function to match > Linux coding style standards. When you say "Linux coding style standards" what exactly does that mean? I've just re-read the Documentation/process/coding-style.rst section on "Macros, Enums and RTL" and I don't see an issue with the macro. This code is in the middle of a big section full of macros. Why did you pick this particular macro? Now it doesn't mirror the IPU3_PTE2ADDR() so this patch hurts readability. > > Signed-off-by: Brent Pappas <bpappas@pappasbrent.com> > --- > drivers/staging/media/ipu3/ipu3-mmu.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/media/ipu3/ipu3-mmu.c b/drivers/staging/media/ipu3/ipu3-mmu.c > index cb9bf5fb29a5..d2d603c32773 100644 > --- a/drivers/staging/media/ipu3/ipu3-mmu.c > +++ b/drivers/staging/media/ipu3/ipu3-mmu.c > @@ -25,7 +25,11 @@ > #define IPU3_PT_SIZE (IPU3_PT_PTES << 2) > #define IPU3_PT_ORDER (IPU3_PT_SIZE >> PAGE_SHIFT) > > -#define IPU3_ADDR2PTE(addr) ((addr) >> IPU3_PAGE_SHIFT) > +static u32 ipu3_addr2pte(phys_addr_t addr) > +{ > + return addr >> IPU3_PAGE_SHIFT; > +} To me the original macro is fine. The inline would also be fine if it were done consistently. But I guess I just don't see a lot of value in changing the existing code. If you were taking ownership of this driver in a more meaningful way then I would defer to your taste... But I just don't see a lot of value in the patch. regards, dan carpenter
Hi Dan, > When you say "Linux coding style standards" what exactly does that mean? I am specifically referring to this line of Documentation/process/coding-style.rst, from the section "Macros, Enums, and RTL": > Generally, inline functions are preferable to macros resembling > functions. This is the first reason I chose this specific macro. IPU3_ADD2PTE() would behave the same as a function, so based on my reading of coding-style.rst, I thought it would be appropriate to proprose turning it into a function. Full disclosure, I am university student, and my current research project is on creating a static analysis framework for finding macros that can be easily turned into functions. I want this project to have an impact on widely-used code, and so I have been using this framework to find such macros in Linux. That is why I have recently been submitting patches to turn macros into functions. So the second reason I chose this macro was because my framework identifies it as transformable. > This code is in the middle of a big section full of macros. Why did you > pick this particular macro? Now it doesn't mirror the IPU3_PTE2ADDR() > so this patch hurts readability. The reason why I did not try to turn the macro IPU3_PTE2ADDR() into a function is that it is never invoked, and my framework does not identify uninvoked macros as transformable. There are more macros in drivers/staging that I think could be turned into functions, and I would like to continue submitting patches to do so. However, if you would rather I change the way I am doing this, or that I stop submitting these sorts of patches altogether, please let me know. Thank you, Brent
I'm sorry, but I don't think this is a worthwhile approach. If you created a tool to automatically re-write macros as functions, that's super impressive. But the choice between using a macro and a function is just a style debate. The note in Coding Style is more talking about complicated macros instead of these simple ones. And anyway, when it comes to gray areas in the style guidelines, we generally defer to the original author because that's who is doing all the work. There are some sorts of bugs associated with using macros like Macro Expansion Precedence Bugs where there isn't parentheses around a macro, or Double Evaluation Bugs where a parameter is evaluated twice. But these sorts of bugs are very rare in the Linux kernel. Generally kernel programmers have always been good about this sort of stuff. Also checkpatch insists on parentheses. And it's not like error paths where the bugs are difficult to find in testing. Probably we get a macro bug every three years (compared to uninitialized variable bugs where we get several per week). I have a Smatch check for both of these kinds of macro bugs. Another kind of bug would be type related bugs, because macros don't have type checking. But I think those are caught in testing so they're extremely rare. I don't think I have seen a real life example of one of those. regards, dan carpenter
diff --git a/drivers/staging/media/ipu3/ipu3-mmu.c b/drivers/staging/media/ipu3/ipu3-mmu.c index cb9bf5fb29a5..d2d603c32773 100644 --- a/drivers/staging/media/ipu3/ipu3-mmu.c +++ b/drivers/staging/media/ipu3/ipu3-mmu.c @@ -25,7 +25,11 @@ #define IPU3_PT_SIZE (IPU3_PT_PTES << 2) #define IPU3_PT_ORDER (IPU3_PT_SIZE >> PAGE_SHIFT) -#define IPU3_ADDR2PTE(addr) ((addr) >> IPU3_PAGE_SHIFT) +static u32 ipu3_addr2pte(phys_addr_t addr) +{ + return addr >> IPU3_PAGE_SHIFT; +} + #define IPU3_PTE2ADDR(pte) ((phys_addr_t)(pte) << IPU3_PAGE_SHIFT) #define IPU3_L2PT_SHIFT IPU3_PT_BITS @@ -200,7 +204,7 @@ static u32 *imgu_mmu_get_l2pt(struct imgu_mmu *mmu, u32 l1pt_idx) l2pt = new_l2pt; mmu->l2pts[l1pt_idx] = new_l2pt; - pteval = IPU3_ADDR2PTE(virt_to_phys(new_l2pt)); + pteval = ipu3_addr2pte(virt_to_phys(new_l2pt)); mmu->l1pt[l1pt_idx] = pteval; spin_unlock_irqrestore(&mmu->lock, flags); @@ -230,7 +234,7 @@ static int __imgu_mmu_map(struct imgu_mmu *mmu, unsigned long iova, return -EBUSY; } - l2pt[l2pt_idx] = IPU3_ADDR2PTE(paddr); + l2pt[l2pt_idx] = ipu3_addr2pte(paddr); spin_unlock_irqrestore(&mmu->lock, flags); @@ -447,7 +451,7 @@ struct imgu_mmu_info *imgu_mmu_init(struct device *parent, void __iomem *base) mmu->dummy_page = (void *)__get_free_page(GFP_KERNEL); if (!mmu->dummy_page) goto fail_group; - pteval = IPU3_ADDR2PTE(virt_to_phys(mmu->dummy_page)); + pteval = ipu3_addr2pte(virt_to_phys(mmu->dummy_page)); mmu->dummy_page_pteval = pteval; /* @@ -457,7 +461,7 @@ struct imgu_mmu_info *imgu_mmu_init(struct device *parent, void __iomem *base) mmu->dummy_l2pt = imgu_mmu_alloc_page_table(pteval); if (!mmu->dummy_l2pt) goto fail_dummy_page; - pteval = IPU3_ADDR2PTE(virt_to_phys(mmu->dummy_l2pt)); + pteval = ipu3_addr2pte(virt_to_phys(mmu->dummy_l2pt)); mmu->dummy_l2pt_pteval = pteval; /* @@ -473,7 +477,7 @@ struct imgu_mmu_info *imgu_mmu_init(struct device *parent, void __iomem *base) if (!mmu->l1pt) goto fail_l2pts; - pteval = IPU3_ADDR2PTE(virt_to_phys(mmu->l1pt)); + pteval = ipu3_addr2pte(virt_to_phys(mmu->l1pt)); writel(pteval, mmu->base + REG_L1_PHYS); imgu_mmu_tlb_invalidate(mmu); imgu_mmu_set_halt(mmu, false); @@ -529,7 +533,7 @@ void imgu_mmu_resume(struct imgu_mmu_info *info) imgu_mmu_set_halt(mmu, true); - pteval = IPU3_ADDR2PTE(virt_to_phys(mmu->l1pt)); + pteval = ipu3_addr2pte(virt_to_phys(mmu->l1pt)); writel(pteval, mmu->base + REG_L1_PHYS); imgu_mmu_tlb_invalidate(mmu);
Replace the macro IPU3_ADDR2PTE() with a static function to match Linux coding style standards. Signed-off-by: Brent Pappas <bpappas@pappasbrent.com> --- drivers/staging/media/ipu3/ipu3-mmu.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)