linux-kernel - Re: [PATCH] mdeia: ipu3: ipu33-mmu: Replace macro IPU3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y8/uTYK7qmYD5MSA@kadam>
Date:   Tue, 24 Jan 2023 17:42:21 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Brent Pappas <bpappas@...pasbrent.com>
Cc:     sakari.ailus@...ux.intel.com, bingbu.cao@...el.com,
        tian.shu.qiu@...el.com, mchehab@...nel.org,
        gregkh@...uxfoundation.org, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mdeia: ipu3: ipu33-mmu: Replace macro IPU3_ADDR2PTE()
 with a function

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@...pasbrent.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