Hi David,

On 08/24/22 at 08:16am, David Laight wrote:
......

quoted

quoted

quoted

quoted

quoted

Le 20/08/2022 à 02:31, Baoquan He a écrit :

quoted

In some architectures, there are ARCH specifici io address mapping
handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
openrisc, s390, sh.

In oder to convert them to take GENERIC_IOREMAP method, we need change
the return value of hook ioremap_allowed() and iounmap_allowed().
Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
their current behaviour.

Thanks for reviewing.

quoted

Please don't just say you need to change the return value. Explain why.

The 1st paragraph and the sentence 'In oder to convert them to take
GENERIC_IOREMAP method' tell the reason, no?

What I would like to read is _why_ you need to change the return value
in order to convert to GENERIC_IOREMAP

I rephrase the log as below, it's OK to you? Or please help check and
tell what I need to improve to better explain the reason.

====
The current io[re|un]map_allowed() hooks are used to check if the
io[re|un]map() actions are qualified to proceed when taking
GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map()
will return NULL.

On some architectures like arc, ia64, openris, s390, sh, there are
ARCH specific io address mapping to translate the passed in physical
address to io address when calling ioremap(). In order to convert
these architectures to take GENERIC_IOREMAP way to ioremap(), we need
change the return value of hook ioremap_allowed() and iounmap_allowed().
With the change, we can move the architecture specific io address
mapping into ioremap_allowed() hook, and give the mapped io address
out to let ioremap_prot() return it. While at it, rename the hooks to
arch_ioremap() and arch_iounmap() to reflect their new behaviour.
====

That looks more in line with the type of explanation I foresee in the
commit message, thanks.

I think you also need to summarise the change itself.
If the success/fail return actually changes then you really
need to change something so the compiler errors unchanged code.
Otherwise it is a complete recipe for disaster.

Thanks for looking into this and sorry for late response.

I am not sure if I follow you. In this patch, I just rename the old
ioremap_allowed() to arch_ioremap(), and change its return value. Except
of arm64 which has taken GENERIC_IOREMAP way to provide
ioremap_allowed(), no other ARCHes are affected yet. This is what have
been changed. Could you be more specific what I should add?

Or are you suggesting words like below sentences need be added to patch
log?

If the success/fail return actually changes then you really
need to change something so the compiler errors unchanged code.
Otherwise it is a complete recipe for disaster.

Thanks
Baoquan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel