diff mbox series

mdeia: ipu3: ipu33-mmu: Replace macro IPU3_ADDR2PTE() with a function

Message ID 20230124135554.13787-1-bpappas@pappasbrent.com
State New
Headers
Series mdeia: ipu3: ipu33-mmu: Replace macro IPU3_ADDR2PTE() with a function |

Commit Message

Brent Pappas Jan. 24, 2023, 1:55 p.m. UTC 
  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(-)

Comments

Dan Carpenter Jan. 24, 2023, 2:42 p.m. UTC | #1 
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
Brent Pappas Jan. 25, 2023, 2:36 p.m. UTC | #2 
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
Dan Carpenter Jan. 25, 2023, 3:27 p.m. UTC | #3 
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 mbox series

Patch

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);