Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-183-gbf7d00f500-fm-20230220.001-gbf7d00f5
Mime-Version: 1.0
Message-Id: <5edd5304-ef11-4607-9189-a07613ecfee2@app.fastmail.com>
In-Reply-To: <20230301102208.148490-2-bhe@redhat.com>
References: <20230301102208.148490-1-bhe@redhat.com>
 <20230301102208.148490-2-bhe@redhat.com>
Date:   Wed, 01 Mar 2023 15:06:57 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Baoquan He" <bhe@redhat.com>, linux-kernel@vger.kernel.org
Cc:     Linux-Arch <linux-arch@vger.kernel.org>, linux-mm@kvack.org,
        "Geert Uytterhoeven" <geert@linux-m68k.org>,
        "Christoph Hellwig" <hch@infradead.org>,
        "Luis Chamberlain" <mcgrof@kernel.org>,
        "Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
        "Huacai Chen" <chenhuacai@kernel.org>,
        "Jiaxun Yang" <jiaxun.yang@flygoat.com>, linux-mips@vger.kernel.org
Subject: Re: [PATCH v2 1/2] mips: add <asm-generic/io.h> including
Content-Type: text/plain
Precedence: bulk

On Wed, Mar 1, 2023, at 11:22, Baoquan He wrote:
> With the adding, some default ioremap_xx methods defined in
> asm-generic/io.h can be used. E.g the default ioremap_uc() returning
> NULL.
>
> Here, remove the <asm/io.h> including in asm/mmiowb.h, otherwise nested
> including will cause compiling error.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Huacai Chen <chenhuacai@kernel.org>
> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Cc: linux-mips@vger.kernel.org

This looks good to me,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

but it obviously needs to be properly reviewed by the MIPS
maintainers as well. I think others have tried to do this
in the past but did not make it in.

> @@ -548,6 +552,46 @@ extern void (*_dma_cache_inv)(unsigned long start, 
> unsigned long size);
>  #define csr_out32(v, a) (*(volatile u32 *)((unsigned long)(a) + 
> __CSR_32_ADJUST) = (v))
>  #define csr_in32(a)    (*(volatile u32 *)((unsigned long)(a) + 
> __CSR_32_ADJUST))
> 
> +
> +#define inb_p inb_p
> +#define inw_p inw_p
> +#define inl_p inl_p
> +#define insb insb
> +#define insw insw
> +#define insl insl

I would prefer to put the #defines next to the function declarations,
even when they come from macros.

> 
> -#include <asm/io.h>
> -
>  #define mmiowb()	iobarrier_w()
> 

I think this only works as long as asm/spinlock.h also includes
asm/io.h, otherwise linux/spinlock.h will be missing the
iobarrier_w definition.

Most likely this is implicitly included from somewhere else
below linux/spinlock.h, but it would be better not to rely
on that, and instead define mmiowb() to wmb() directly.

     Arnd