linux-kernel - RE: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash

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: <000901d7b1fa$ff9b57f0$fed207d0$@mentor.com>
Date:   Sat, 25 Sep 2021 13:49:13 +0300
From:   Andrew Gabbasov <andrew_gabbasov@...tor.com>
To:     'Wolfram Sang' <wsa@...nel.org>
CC:     <linux-renesas-soc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Sergei Shtylyov <sergei.shtylyov@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>
Subject: RE: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash

Hello Wolfram,

Thank you for your comments!

> -----Original Message-----
> From: Wolfram Sang <wsa@...nel.org>
> Sent: Saturday, September 25, 2021 12:55 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@...tor.com>
> Cc: linux-renesas-soc@...r.kernel.org; linux-kernel@...r.kernel.org; Krzysztof Kozlowski <krzk@...nel.org>;
> Sergei Shtylyov <sergei.shtylyov@...il.com>; Geert Uytterhoeven <geert+renesas@...der.be>
> Subject: Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
> 
> Hi Andrew,
> 
> thanks for this patch!
> 
> > +	const int maxw = (IS_ENABLED(CONFIG_64BIT)) ? 8 : 4;
> > +	u8 buf[2];
> 
> I could imagine the code becomes more readable if we make use of
> something like:
> 
> 	unsigned long from_ul = from;
> 
> and then use it throughout the function?

It could make sense if "from" would not change along the function.
But in case of this function "from" is changed between usages, so
it would be necessary to synchronize "from_ul" with "from" again
(make "from_ul++" together with "from++", or re-assign it)
before we could use it again correctly.

> > +#ifdef CONFIG_64BIT
> > +		*(u64 *)to = __raw_readq(from);
> > +#else
> > +		*(u32 *)to = __raw_readl(from);
> > +#endif
> 
> To keep the ifdeffery minimal:
> 
> 	if (maxw == 8)
> 		*(u64 *)to = __raw_readq(from);
> 	else
>  		*(u32 *)to = __raw_readl(from);
> 
> and let the compiler do its job.

I don't like #ifdef's inside the function body too, but the problem is that
"__raw_readq" is defined in arch/arm64/include/asm/io.h unconditionally,
but in include/asm-generic/io.h under "#ifdef CONFIG_64BIT" only.
This file drivers/memory/renesas-rpc-if.c can be compiled not only
for renesas/arm64 architecture, but for CONFIG_COMPILE_TEST case too.
It means, that the file could be compiled for some other architecture,
that does not have CONFIG_64BIT, and in this case "__raw_readq" function
will be undefined. So, we need to hide it under "#ifdef CONFIG_64BIT" here.

> I wondered if this could be a helper function somewhere instead of open
> coded in this driver. However, I did not find any similar code in the
> kernel yet, so it might be too early to make this a helper. Have you
> looked for similar code? I might have just missed it.

I looked through the whole kernel code too, and unfortunately didn't find
any similar code that could be re-used or had some parts, extractable as
a common helper. That's why I ended up with a local custom function,
at least so far, until it could be found useful by somebody else ;)

Thanks!

Best regards,
Andrew