Le 23/08/2022 à 14:32, Baoquan He a écrit :

On 08/23/22 at 05:33am, Christophe Leroy wrote:

quoted

Le 23/08/2022 à 03:19, Baoquan He a écrit :

quoted

On 08/22/22 at 06:30am, Christophe Leroy wrote:

quoted

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

quoted

On some architectures, the physical address need be fixed up before
doing mapping, e.g, parisc. And on architectures, e.g arc, the
parameter 'prot' passed into ioremap_prot() need be adjusted too.

In oder to convert them to take GENERIC_IOREMAP method, we need wrap
the address fixing up code and page prot adjusting code into arch_ioremap()
and pass the new address and 'prot' out for ioremap_prot() handling.

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.

quoted

By the way, could be a good opportunity to change ioremap_prot() flags
type from unsigned long to pgprot_t

Tend to agree, I will give it a shot.

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