On 09/21/22 at 04:40pm, Christophe Leroy wrote:

Le 13/09/2022 à 17:11, Baoquan He a écrit :

quoted

On 09/12/22 at 07:10am, Christophe Leroy wrote:

quoted

Hi Baoquan,

Le 12/09/2022 à 04:55, Baoquan He a écrit :

quoted

Hi Christophe,

On 08/28/22 at 07:10pm, Baoquan He wrote:

quoted

On 08/23/22 at 07:03pm, Christophe Leroy wrote:

......

quoted

quoted

quoted

quoted

quoted

quoted

Is it really the best approach ? Wouldn't it be better to have helpers
to do that, those helpers being called by the ioremap_prot(), instead of
doing it inside the arch_ioremap() function ?

This is suggested too by Alexander during his v1 reviewing. I tried, but
feel the current way taken in this patchset is better. Because not all
architecutres need the address fix up, only parisc, and only few need
adjust the 'prot'. Introducing other helpers seems too much, that only
increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
method for people to understand and take.

I can't understand. Why is it difficult to do something like:

#ifndef ioremap_adjust_prot
static inline unsigned long ioremap_adjust_prot(unsigned long flags)
{
	return flags;
}
#endif

Then for arc you do

static inline unsigned long ioremap_adjust_prot(unsigned long flags)
{
	return pgprot_val(pgprot_noncached(__pgprot(flags)));
}
#define ioremap_adjust_prot ioremap_adjust_prot

My thinking is we have four things to do in the added hookers.
1) check if we should do ioremap on ARCHes. If not, return NULL from
ioremap_prot();
2) handling the mapping io address specifically on ARCHes, e.g arc,
ia64, s390;
3) the original physical address passed into ioremap_prot() need be
fixed up, e.g arc;
4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
and xtensa.

With Kefeng's patches, the case 1) is handled with introduced
ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
case is not difficult from my side. I worry that as time goes by, those
several hooks my cause issue, e.g if a new adjustment need be done,
should we introduce a new helper or make do with the existed hook; how

When I investigated this, one arch_ioremap() looks not complicated
since not all ARCHes need cover all above 4 cases. That's why I finally
choose one hook. I am open to new idea, please let me know if we should
change it to introduce several different helpers.

A new idea that would have my preference would be to do just like we did
with arch_get_unmapped_area(). Look at
https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131

Instead of having the generic that calls the arch specific, make it the
other way round, have the arch specific call the generic after doing its
specialties.

This sounds good. I made a draft patch to change code in generic code
part, just showing what it looks like.

Both arch_ioremap() way and the arch sepcific call the generic way look
good to me. Just it will take less effort for me to continue the
arch_ioremap() way. I would like to hear Christoph's opinion since he
introduced the GENERIC_IOREMAP method and suggested the earlier
arch_ioremap() way and change in this patchset.

I will make another round change and post. Since Christoph doesn't
reply, I would like to continue with the existing
arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
on the new idea to reintroduce ioremap().

I still dislike you approach with the architectures modifying local vars
by reference, and as you said earlier I'm not the only one : "This is
suggested too by Alexander during his v1 reviewing".

Alexander suggested several helpers, as I have explained earlier, that
will cause at least four helpers currently. And could be more later if
new change is introduced. And the address fixup and prot modifcation
are related in few architecutures. Adding all of them is is not so
necessary.

quoted

So I'd really prefer you to reconsider your approach and avoid passign
pointers to local vars to architecture helpers.

If only passing pointers to local vars is disliked, I can explain why I
did so. Let me take arch_ioremap() of a64 as example. I can derefence
pointer in arch_ioremap() to avoid assigning pointers to local vars.
Please see below two version for comparing, and please tell which one is
better.

Ok, yes I overlooked and didn't remember it right.

quoted

To me, assigning pointers to local vars make code simple and clean,
honestly.

Well, for me it looks ood, not intellectually natural.

If I understand correctly, you do

ioremap()
   --> Call arch_ioremap()
     --> If the arch doesn't want to handle ioremap itself, it returns NULL
     --> Then you fallback on generic handling.

The arch may say "I don't want to handle it", but at the same time it 
blindly modifies the parameters so that the generic handling is not 
exacly the generic handling.

No, it's not like that.

Arch_ioremap() provides a place where arch specific handling can be
done. While it doesn't mean arch has to do all of the follow four
things. From the current change I made listed as below, arch_ioremap()
in each architecture is quite simple to do one or two things.

People only need to provide arch_ioremap() if needed, or do not provide
arch_ioremap() at all if not needed. No such thing could hapen like you
said that arch don't want to handle, but it blindly modifes the parameter.

arm64:
  - address checking;
hexagon: removes the ioremap() directly since it has the same code as standard;
  - N/A
arc:
  - address checking
  - modify the prot flag;
ia64:
  - specific io address mapping
openrisc: removes ioremap() directly after its early ioremap is cleared away;
  - N/A
parisc:
  - physical address fixup
  - address checking;
s390:
  - specific io address mapping
sh:
  - specific io address mapping 
xtensa:
  - address checking;

Four optional things could be done in arch_ioremap():
1) check if we should do ioremap on ARCHes. If not, return NULL from
ioremap_prot();
2) handling the mapping io address specifically on ARCHes, e.g arc,
ia64, s390;
3) the original physical address passed into ioremap_prot() need be
fixed up, e.g arc;
4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
and xtensa.

Not easy to follow for the reader. Do you have any exemple in the kernel 
that works more or less with the same approach ?


What I propose is

Arch specific ioremap()
   --> do proper preparation
   --> call generic_ioremap()

And the generic fallback implementation when the arch doesn't have a 
specific ioremap()

__weak ioremap()
   --> call generic_ioremap().

Hmm, __weak is not suggested any more in kernel. Please see below
thread.

[PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add]
https://lore.kernel.org/all/20220518181828.645877-1-naveen.n.rao@linux.vnet.ibm.com/T/#u (local)


So with arch_ioremap() hook, now in each architecture it is:

  arch_ioremap() is provided and called
  or 
  no hookd is needed at all in arch 

It's simpler, isn't it?

The above looks a lot more natural and easier to follow, it is clear for 
the reader which function does what.

Before, there's only arm64 taking GENERIC_IOREMAP way and having
arch_ioremap() as an exmaple. Now after this patchset, there are so many
architectures taking the way, it's very easy to refer to and study,
right?

Thanks
Baoquan


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