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

[Andrew Gabbasov <andrew_gabbasov@mentor.com>]

Hello Wolfram,

Thank you for your comments!

> -----Original Message-----
> From: Wolfram Sang <wsa@kernel.org>
> Sent: Saturday, September 25, 2021 12:55 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-kernel@vger.kernel.org; Krzysztof Kozlowski <krzk@kernel.org>;
> Sergei Shtylyov <sergei.shtylyov@gmail.com>; Geert Uytterhoeven <geert+renesas@glider.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