[PATCH] iomap: Fix -Wmissing-prototypes on UM

[PATCH] iomap: Fix -Wmissing-prototypes on UM

[Thomas Weißschuh]

Building lib/iomap.o on UM triggers warnings about missing prototypes.
These prototypes should be defined by asm-generic/iomap.h, depending on
other symbols. For example "ioread64_lo_hi" is based on "readq".
However the generic variants of those tested symbols are defined in
asm-generic/io.h, only after asm-generic/iomap.h has already been
included, breaking the ifdef logic.

Move the inclusion of asm-generic/iomap.h in asm-generic/io.h after the
generic symbols have been defined, so the checks can work.

Triggered warnings:

$ make ARCH=um allyesconfig lib/iomap.o
make[1]: Entering directory '/tmp/um'
  GEN     Makefile
  GEN     Makefile
  CALL    scripts/checksyscalls.sh
  CC      lib/iomap.o
lib/iomap.c:156:5: error: no previous prototype for ‘ioread64_lo_hi’ [-Werror=missing-prototypes]
  156 | u64 ioread64_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
lib/iomap.c:163:5: error: no previous prototype for ‘ioread64_hi_lo’ [-Werror=missing-prototypes]
  163 | u64 ioread64_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
lib/iomap.c:170:5: error: no previous prototype for ‘ioread64be_lo_hi’ [-Werror=missing-prototypes]
  170 | u64 ioread64be_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
lib/iomap.c:178:5: error: no previous prototype for ‘ioread64be_hi_lo’ [-Werror=missing-prototypes]
  178 | u64 ioread64be_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
lib/iomap.c:264:6: error: no previous prototype for ‘iowrite64_lo_hi’ [-Werror=missing-prototypes]
  264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
lib/iomap.c:272:6: error: no previous prototype for ‘iowrite64_hi_lo’ [-Werror=missing-prototypes]
  272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
lib/iomap.c:280:6: error: no previous prototype for ‘iowrite64be_lo_hi’ [-Werror=missing-prototypes]
  280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
iomap.c:288:6: error: no previous prototype for ‘iowrite64be_hi_lo’ [-Werror=missing-prototypes]
  288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Fixes: 0fcb70851fbf ("Makefile.extrawarn: turn on missing-prototypes globally")
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 include/asm-generic/io.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index a5cbbf3e26ec7d06f7e67ee9731021031b39aa13..1bfdc4d5643054701c27073c146a6d8cc3903384 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -13,10 +13,6 @@
 #include <linux/types.h>
 #include <linux/instruction_pointer.h>
 
-#ifdef CONFIG_GENERIC_IOMAP
-#include <asm-generic/iomap.h>
-#endif
-
 #include <asm/mmiowb.h>
 #include <asm-generic/pci_iomap.h>
 
@@ -1250,4 +1246,8 @@ extern int devmem_is_allowed(unsigned long pfn);
 
 #endif /* __KERNEL__ */
 
+#ifdef CONFIG_GENERIC_IOMAP
+#include <asm-generic/iomap.h>
+#endif
+
 #endif /* __ASM_GENERIC_IO_H */

---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250129-um-io-6ffdf196774c

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Arnd Bergmann]

On Mon, Feb 3, 2025, at 08:26, Thomas Weißschuh wrote:
> Building lib/iomap.o on UM triggers warnings about missing prototypes.
> These prototypes should be defined by asm-generic/iomap.h, depending on
> other symbols. For example "ioread64_lo_hi" is based on "readq".
> However the generic variants of those tested symbols are defined in
> asm-generic/io.h, only after asm-generic/iomap.h has already been
> included, breaking the ifdef logic.

Sorry I never took the time to fix this so far.

> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -13,10 +13,6 @@
>  #include <linux/types.h>
>  #include <linux/instruction_pointer.h>
> 
> -#ifdef CONFIG_GENERIC_IOMAP
> -#include <asm-generic/iomap.h>
> -#endif
> -
>  #include <asm/mmiowb.h>
>  #include <asm-generic/pci_iomap.h>
> 
> @@ -1250,4 +1246,8 @@ extern int devmem_is_allowed(unsigned long pfn);
> 
>  #endif /* __KERNEL__ */
> 
> +#ifdef CONFIG_GENERIC_IOMAP
> +#include <asm-generic/iomap.h>
> +#endif
> +
>  #endif /* __ASM_GENERIC_IO_H */

I have not tried it yet, but I suspect this is not the correct
fix here. Unfortunately the indirect header inclusions in this
file are way too complicated with corner cases in various
architectures. How much testing have you given your patch
across other targets? I think the last time we tried to address
it, we broke mips or parisc.

    Arnd

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Thomas Weißschuh]

On Mon, Feb 03, 2025 at 09:18:33AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 3, 2025, at 08:26, Thomas Weißschuh wrote:
> > Building lib/iomap.o on UM triggers warnings about missing prototypes.
> > These prototypes should be defined by asm-generic/iomap.h, depending on
> > other symbols. For example "ioread64_lo_hi" is based on "readq".
> > However the generic variants of those tested symbols are defined in
> > asm-generic/io.h, only after asm-generic/iomap.h has already been
> > included, breaking the ifdef logic.
> 
> Sorry I never took the time to fix this so far.
> 
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -13,10 +13,6 @@
> >  #include <linux/types.h>
> >  #include <linux/instruction_pointer.h>
> > 
> > -#ifdef CONFIG_GENERIC_IOMAP
> > -#include <asm-generic/iomap.h>
> > -#endif
> > -
> >  #include <asm/mmiowb.h>
> >  #include <asm-generic/pci_iomap.h>
> > 
> > @@ -1250,4 +1246,8 @@ extern int devmem_is_allowed(unsigned long pfn);
> > 
> >  #endif /* __KERNEL__ */
> > 
> > +#ifdef CONFIG_GENERIC_IOMAP
> > +#include <asm-generic/iomap.h>
> > +#endif
> > +
> >  #endif /* __ASM_GENERIC_IO_H */
> 
> I have not tried it yet, but I suspect this is not the correct
> fix here. Unfortunately the indirect header inclusions in this
> file are way too complicated with corner cases in various
> architectures. How much testing have you given your patch
> across other targets? I think the last time we tried to address
> it, we broke mips or parisc.

It was build-tested on 0day.
I also gave it some light boot testing on kunit/qemu.
(Neither on mips or parisc)

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Arnd Bergmann]

On Mon, Feb 3, 2025, at 09:25, Thomas Weißschuh wrote:
> On Mon, Feb 03, 2025 at 09:18:33AM +0100, Arnd Bergmann wrote:
>> I have not tried it yet, but I suspect this is not the correct
>> fix here. Unfortunately the indirect header inclusions in this
>> file are way too complicated with corner cases in various
>> architectures. How much testing have you given your patch
>> across other targets? I think the last time we tried to address
>> it, we broke mips or parisc.
>
> It was build-tested on 0day.
> I also gave it some light boot testing on kunit/qemu.
> (Neither on mips or parisc)

Ok, I see. I checked the usage of these functions again and
now remembered the reason we didn't fix it last time, which is
that the semantics are inconsistent across architectures
and I think this should be addressed first, so we can untangle
the definitions:

There is one driver that is specific to ARM processors
(drivers/firmware/arm_scmi) using  ioread64_hi_lo/iowrite64_hi_lo
and this uses the documented semantics from
Documentation/driver-api/device-io.rst, which says that the
helpers always do separate 32-bit accesses (even on 64-bit
machines), but that it's possible to just call
ioread64()/iowrite64() after including linux/io-64-nonatomic-hi-lo.h
in order to always get 64-bit accesses on 64-bit architectures
but get 32-bit accesses on 32-bit architectures. There are
another dozen or so drivers that do this.

There are two other drivers that were written for x86 that
use other semantics (drivers/net/wwan/t7xx/ and
drivers/ptp/ptp_pch.c): Here, the definition from lib/iomap.c
means that even on 64-bit architectures calling
ioread64_hi_lo/iowrite64_hi_lo on an MMIO space always
results in a 64-bit access, and only the PIO version is split
up. On 32-bit architectures, this part is not provided, so
it should fall back to split access (I think this is where
we had bugs in the past).

Another complication is that alpha, parisc and sh (not mips
any more) explicitly include asm-generic/iomap.h but don't
select CONFIG_GENERIC_IOMAP. At this time, GENERIC_IOMAP
is selected at least in some configurations on m68k, mips,
powerpc, sh, um and x86, but most of these should not do that
(mips, m68k and sh have no PIO instructions, powerpc had
a hack that I think was just removed but needs more cleanup).

I'm testing with the patch below now, which separates the
CONFIG_GENERIC_IOMAP implementation (with the 32-bit PIO
access) from the normal version, and picks an appropriate
one in linux/io-64-nonatomic-*.h based on the architecture
to avoid some of the more confusing nested 
"#ifdef ioread64" etc checks.

I checked that none of the three drivers ever actually uses
PIO registers instead of MMIO, and since none of them use the
big-endian accessors, this all turns into readq/writeq
in practice anyway.

The ptp_pch.c driver still needs more work as I noticed two
issues there: the driver has a mix of lo_hi and hi_lo
variants, but it is unclear whether is is actually required
on 32-bit or if the hardware works the same either way.
In addition, these seem to be timer registers that may overrun
from the lo into the hi field between the two accesses, so
technically a 32-bit host needs to do an extra read to
check for overflow and possibly repeat the access.

      Arnd

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 60050da54bf2..1c75a4c9c371 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1997,17 +1997,7 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
 	else if (db->width == 4)
 		SCMI_PROTO_FC_RING_DB(32);
 	else /* db->width == 8 */
-#ifdef CONFIG_64BIT
 		SCMI_PROTO_FC_RING_DB(64);
-#else
-	{
-		u64 val = 0;
-
-		if (db->mask)
-			val = ioread64_hi_lo(db->addr) & db->mask;
-		iowrite64_hi_lo(db->set | val, db->addr);
-	}
-#endif
 }
 
 /**
diff --git a/drivers/net/wwan/t7xx/t7xx_cldma.c b/drivers/net/wwan/t7xx/t7xx_cldma.c
index f0a4783baf1f..9f43f256db1d 100644
--- a/drivers/net/wwan/t7xx/t7xx_cldma.c
+++ b/drivers/net/wwan/t7xx/t7xx_cldma.c
@@ -106,7 +106,7 @@ bool t7xx_cldma_tx_addr_is_set(struct t7xx_cldma_hw *hw_info, unsigned int qno)
 {
 	u32 offset = REG_CLDMA_UL_START_ADDRL_0 + qno * ADDR_SIZE;
 
-	return ioread64_lo_hi(hw_info->ap_pdn_base + offset);
+	return ioread64(hw_info->ap_pdn_base + offset);
 }
 
 void t7xx_cldma_hw_set_start_addr(struct t7xx_cldma_hw *hw_info, unsigned int qno, u64 address,
@@ -117,7 +117,7 @@ void t7xx_cldma_hw_set_start_addr(struct t7xx_cldma_hw *hw_info, unsigned int qn
 
 	reg = tx_rx == MTK_RX ? hw_info->ap_ao_base + REG_CLDMA_DL_START_ADDRL_0 :
 				hw_info->ap_pdn_base + REG_CLDMA_UL_START_ADDRL_0;
-	iowrite64_lo_hi(address, reg + offset);
+	iowrite64(address, reg + offset);
 }
 
 void t7xx_cldma_hw_resume_queue(struct t7xx_cldma_hw *hw_info, unsigned int qno,
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
index 97163e1e5783..13f5a2442d20 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
@@ -137,9 +137,9 @@ static int t7xx_cldma_gpd_rx_from_q(struct cldma_queue *queue, int budget, bool
 				return -ENODEV;
 			}
 
-			gpd_addr = ioread64_lo_hi(hw_info->ap_pdn_base +
-						  REG_CLDMA_DL_CURRENT_ADDRL_0 +
-						  queue->index * sizeof(u64));
+			gpd_addr = ioread64(hw_info->ap_pdn_base +
+					    REG_CLDMA_DL_CURRENT_ADDRL_0 +
+					    queue->index * sizeof(u64));
 			if (req->gpd_addr == gpd_addr || hwo_polling_count++ >= 100)
 				return 0;
 
@@ -317,8 +317,8 @@ static void t7xx_cldma_txq_empty_hndl(struct cldma_queue *queue)
 		struct t7xx_cldma_hw *hw_info = &md_ctrl->hw_info;
 
 		/* Check current processing TGPD, 64-bit address is in a table by Q index */
-		ul_curr_addr = ioread64_lo_hi(hw_info->ap_pdn_base + REG_CLDMA_UL_CURRENT_ADDRL_0 +
-					      queue->index * sizeof(u64));
+		ul_curr_addr = ioread64(hw_info->ap_pdn_base + REG_CLDMA_UL_CURRENT_ADDRL_0 +
+					queue->index * sizeof(u64));
 		if (req->gpd_addr != ul_curr_addr) {
 			spin_unlock_irqrestore(&md_ctrl->cldma_lock, flags);
 			dev_err(md_ctrl->dev, "CLDMA%d queue %d is not empty\n",
diff --git a/drivers/net/wwan/t7xx/t7xx_pcie_mac.c b/drivers/net/wwan/t7xx/t7xx_pcie_mac.c
index f071ec7ff23d..76da4c15e3de 100644
--- a/drivers/net/wwan/t7xx/t7xx_pcie_mac.c
+++ b/drivers/net/wwan/t7xx/t7xx_pcie_mac.c
@@ -75,7 +75,7 @@ static void t7xx_pcie_mac_atr_tables_dis(void __iomem *pbase, enum t7xx_atr_src_
 	for (i = 0; i < ATR_TABLE_NUM_PER_ATR; i++) {
 		offset = ATR_PORT_OFFSET * port + ATR_TABLE_OFFSET * i;
 		reg = pbase + ATR_PCIE_WIN0_T0_ATR_PARAM_SRC_ADDR + offset;
-		iowrite64_lo_hi(0, reg);
+		iowrite64(0, reg);
 	}
 }
 
@@ -112,17 +112,17 @@ static int t7xx_pcie_mac_atr_cfg(struct t7xx_pci_dev *t7xx_dev, struct t7xx_atr_
 
 	reg = pbase + ATR_PCIE_WIN0_T0_TRSL_ADDR + offset;
 	value = cfg->trsl_addr & ATR_PCIE_WIN0_ADDR_ALGMT;
-	iowrite64_lo_hi(value, reg);
+	iowrite64(value, reg);
 
 	reg = pbase + ATR_PCIE_WIN0_T0_TRSL_PARAM + offset;
 	iowrite32(cfg->trsl_id, reg);
 
 	reg = pbase + ATR_PCIE_WIN0_T0_ATR_PARAM_SRC_ADDR + offset;
 	value = (cfg->src_addr & ATR_PCIE_WIN0_ADDR_ALGMT) | (atr_size << 1) | BIT(0);
-	iowrite64_lo_hi(value, reg);
+	iowrite64(value, reg);
 
 	/* Ensure ATR is set */
-	ioread64_lo_hi(reg);
+	ioread64(reg);
 	return 0;
 }
 
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index b8a9a54a176c..1d3b1f83cda8 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -13,7 +13,6 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
-#include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -147,14 +146,14 @@ static u64 pch_systime_read(struct pch_ts_regs __iomem *regs)
 {
 	u64 ns;
 
-	ns = ioread64_lo_hi(&regs->systime_lo);
+	ns = ioread64(&regs->systime_lo);
 
 	return ns << TICKS_NS_SHIFT;
 }
 
 static void pch_systime_write(struct pch_ts_regs __iomem *regs, u64 ns)
 {
-	iowrite64_lo_hi(ns >> TICKS_NS_SHIFT, &regs->systime_lo);
+	iowrite64(ns >> TICKS_NS_SHIFT, &regs->systime_lo);
 }
 
 static inline void pch_block_reset(struct pch_dev *chip)
@@ -221,7 +220,7 @@ u64 pch_rx_snap_read(struct pci_dev *pdev)
 	struct pch_dev *chip = pci_get_drvdata(pdev);
 	u64 ns;
 
-	ns = ioread64_lo_hi(&chip->regs->rx_snap_lo);
+	ns = ioread64(&chip->regs->rx_snap_lo);
 
 	return ns << TICKS_NS_SHIFT;
 }
@@ -232,7 +231,7 @@ u64 pch_tx_snap_read(struct pci_dev *pdev)
 	struct pch_dev *chip = pci_get_drvdata(pdev);
 	u64 ns;
 
-	ns = ioread64_lo_hi(&chip->regs->tx_snap_lo);
+	ns = ioread64(&chip->regs->tx_snap_lo);
 
 	return ns << TICKS_NS_SHIFT;
 }
@@ -283,7 +282,7 @@ int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
 	}
 
 	dev_dbg(&pdev->dev, "invoking pch_station_set\n");
-	iowrite64_lo_hi(mac, &chip->regs->ts_st);
+	iowrite64(mac, &chip->regs->ts_st);
 	return 0;
 }
 EXPORT_SYMBOL(pch_set_station_address);
@@ -305,7 +304,7 @@ static irqreturn_t isr(int irq, void *priv)
 		if (pch_dev->exts0_enabled) {
 			event.type = PTP_CLOCK_EXTTS;
 			event.index = 0;
-			event.timestamp = ioread64_hi_lo(&regs->asms_hi);
+			event.timestamp = ioread64(&regs->asms_hi);
 			event.timestamp <<= TICKS_NS_SHIFT;
 			ptp_clock_event(pch_dev->ptp_clock, &event);
 		}
@@ -316,7 +315,7 @@ static irqreturn_t isr(int irq, void *priv)
 		if (pch_dev->exts1_enabled) {
 			event.type = PTP_CLOCK_EXTTS;
 			event.index = 1;
-			event.timestamp = ioread64_hi_lo(&regs->asms_hi);
+			event.timestamp = ioread64(&regs->asms_hi);
 			event.timestamp <<= TICKS_NS_SHIFT;
 			ptp_clock_event(pch_dev->ptp_clock, &event);
 		}
@@ -493,7 +492,7 @@ pch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	pch_reset(chip);
 
 	iowrite32(DEFAULT_ADDEND, &chip->regs->addend);
-	iowrite64_lo_hi(1, &chip->regs->trgt_lo);
+	iowrite64(1, &chip->regs->trgt_lo);
 	iowrite32(PCH_TSE_TTIPEND, &chip->regs->event);
 
 	pch_eth_enable_set(chip);
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 196087a8126e..9f3f25d7fc58 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -31,42 +31,22 @@ extern unsigned int ioread16(const void __iomem *);
 extern unsigned int ioread16be(const void __iomem *);
 extern unsigned int ioread32(const void __iomem *);
 extern unsigned int ioread32be(const void __iomem *);
-#ifdef CONFIG_64BIT
-extern u64 ioread64(const void __iomem *);
-extern u64 ioread64be(const void __iomem *);
-#endif
 
-#ifdef readq
-#define ioread64_lo_hi ioread64_lo_hi
-#define ioread64_hi_lo ioread64_hi_lo
-#define ioread64be_lo_hi ioread64be_lo_hi
-#define ioread64be_hi_lo ioread64be_hi_lo
-extern u64 ioread64_lo_hi(const void __iomem *addr);
-extern u64 ioread64_hi_lo(const void __iomem *addr);
-extern u64 ioread64be_lo_hi(const void __iomem *addr);
-extern u64 ioread64be_hi_lo(const void __iomem *addr);
-#endif
+extern u64 __ioread64_lo_hi(const void __iomem *addr);
+extern u64 __ioread64_hi_lo(const void __iomem *addr);
+extern u64 __ioread64be_lo_hi(const void __iomem *addr);
+extern u64 __ioread64be_hi_lo(const void __iomem *addr);
 
 extern void iowrite8(u8, void __iomem *);
 extern void iowrite16(u16, void __iomem *);
 extern void iowrite16be(u16, void __iomem *);
 extern void iowrite32(u32, void __iomem *);
 extern void iowrite32be(u32, void __iomem *);
-#ifdef CONFIG_64BIT
-extern void iowrite64(u64, void __iomem *);
-extern void iowrite64be(u64, void __iomem *);
-#endif
 
-#ifdef writeq
-#define iowrite64_lo_hi iowrite64_lo_hi
-#define iowrite64_hi_lo iowrite64_hi_lo
-#define iowrite64be_lo_hi iowrite64be_lo_hi
-#define iowrite64be_hi_lo iowrite64be_hi_lo
-extern void iowrite64_lo_hi(u64 val, void __iomem *addr);
-extern void iowrite64_hi_lo(u64 val, void __iomem *addr);
-extern void iowrite64be_lo_hi(u64 val, void __iomem *addr);
-extern void iowrite64be_hi_lo(u64 val, void __iomem *addr);
-#endif
+extern void __iowrite64_lo_hi(u64 val, void __iomem *addr);
+extern void __iowrite64_hi_lo(u64 val, void __iomem *addr);
+extern void __iowrite64be_lo_hi(u64 val, void __iomem *addr);
+extern void __iowrite64be_hi_lo(u64 val, void __iomem *addr);
 
 /*
  * "string" versions of the above. Note that they
diff --git a/include/linux/io-64-nonatomic-lo-hi.h b/include/linux/io-64-nonatomic-lo-hi.h
index 448a21435dba..70d091769baa 100644
--- a/include/linux/io-64-nonatomic-lo-hi.h
+++ b/include/linux/io-64-nonatomic-lo-hi.h
@@ -101,22 +101,38 @@ static inline void iowrite64be_lo_hi(u64 val, void __iomem *addr)
 
 #ifndef ioread64
 #define ioread64_is_nonatomic
+#ifdef CONFIG_GENERIC_IOREMAP
+#define ioread64 __ioread64_lo_hi
+#else
 #define ioread64 ioread64_lo_hi
 #endif
+#endif
 
 #ifndef iowrite64
 #define iowrite64_is_nonatomic
+#ifdef CONFIG_GENERIC_IOREMAP
+#define ioread64 __iowrite64_lo_hi
+#else
 #define iowrite64 iowrite64_lo_hi
 #endif
+#endif
 
 #ifndef ioread64be
 #define ioread64be_is_nonatomic
+#ifdef CONFIG_GENERIC_IOREMAP
+#define ioread64 __ioread64be_lo_hi
+#else
 #define ioread64be ioread64be_lo_hi
 #endif
+#endif
 
 #ifndef iowrite64be
 #define iowrite64be_is_nonatomic
+#ifdef CONFIG_GENERIC_IOREMAP
+#define ioread64 __iowrite64be_lo_hi
+#else
 #define iowrite64be iowrite64be_lo_hi
 #endif
+#endif
 
 #endif	/* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
diff --git a/lib/iomap.c b/lib/iomap.c
index 4f8b31baa575..a65717cd86f7 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -111,7 +111,7 @@ EXPORT_SYMBOL(ioread16be);
 EXPORT_SYMBOL(ioread32);
 EXPORT_SYMBOL(ioread32be);
 
-#ifdef readq
+#ifdef CONFIG_64BIT
 static u64 pio_read64_lo_hi(unsigned long port)
 {
 	u64 lo, hi;
@@ -153,21 +153,21 @@ static u64 pio_read64be_hi_lo(unsigned long port)
 }
 
 __no_kmsan_checks
-u64 ioread64_lo_hi(const void __iomem *addr)
+u64 __ioread64_lo_hi(const void __iomem *addr)
 {
 	IO_COND(addr, return pio_read64_lo_hi(port), return readq(addr));
 	return 0xffffffffffffffffULL;
 }
 
 __no_kmsan_checks
-u64 ioread64_hi_lo(const void __iomem *addr)
+u64 __ioread64_hi_lo(const void __iomem *addr)
 {
 	IO_COND(addr, return pio_read64_hi_lo(port), return readq(addr));
 	return 0xffffffffffffffffULL;
 }
 
 __no_kmsan_checks
-u64 ioread64be_lo_hi(const void __iomem *addr)
+u64 __ioread64be_lo_hi(const void __iomem *addr)
 {
 	IO_COND(addr, return pio_read64be_lo_hi(port),
 		return mmio_read64be(addr));
@@ -175,19 +175,19 @@ u64 ioread64be_lo_hi(const void __iomem *addr)
 }
 
 __no_kmsan_checks
-u64 ioread64be_hi_lo(const void __iomem *addr)
+u64 __ioread64be_hi_lo(const void __iomem *addr)
 {
 	IO_COND(addr, return pio_read64be_hi_lo(port),
 		return mmio_read64be(addr));
 	return 0xffffffffffffffffULL;
 }
 
-EXPORT_SYMBOL(ioread64_lo_hi);
-EXPORT_SYMBOL(ioread64_hi_lo);
-EXPORT_SYMBOL(ioread64be_lo_hi);
-EXPORT_SYMBOL(ioread64be_hi_lo);
+EXPORT_SYMBOL(__ioread64_lo_hi);
+EXPORT_SYMBOL(__ioread64_hi_lo);
+EXPORT_SYMBOL(__ioread64be_lo_hi);
+EXPORT_SYMBOL(__ioread64be_hi_lo);
 
-#endif /* readq */
+#endif /* CONFIG_64BIT */
 
 #ifndef pio_write16be
 #define pio_write16be(val,port) outw(swab16(val),port)
@@ -236,7 +236,7 @@ EXPORT_SYMBOL(iowrite16be);
 EXPORT_SYMBOL(iowrite32);
 EXPORT_SYMBOL(iowrite32be);
 
-#ifdef writeq
+#ifdef CONFIG_64BIT
 static void pio_write64_lo_hi(u64 val, unsigned long port)
 {
 	outl(val, port);
@@ -261,7 +261,7 @@ static void pio_write64be_hi_lo(u64 val, unsigned long port)
 	pio_write32be(val, port + sizeof(u32));
 }
 
-void iowrite64_lo_hi(u64 val, void __iomem *addr)
+void __iowrite64_lo_hi(u64 val, void __iomem *addr)
 {
 	/* Make sure uninitialized memory isn't copied to devices. */
 	kmsan_check_memory(&val, sizeof(val));
@@ -269,7 +269,7 @@ void iowrite64_lo_hi(u64 val, void __iomem *addr)
 		writeq(val, addr));
 }
 
-void iowrite64_hi_lo(u64 val, void __iomem *addr)
+void __iowrite64_hi_lo(u64 val, void __iomem *addr)
 {
 	/* Make sure uninitialized memory isn't copied to devices. */
 	kmsan_check_memory(&val, sizeof(val));
@@ -277,7 +277,7 @@ void iowrite64_hi_lo(u64 val, void __iomem *addr)
 		writeq(val, addr));
 }
 
-void iowrite64be_lo_hi(u64 val, void __iomem *addr)
+void __iowrite64be_lo_hi(u64 val, void __iomem *addr)
 {
 	/* Make sure uninitialized memory isn't copied to devices. */
 	kmsan_check_memory(&val, sizeof(val));
@@ -285,7 +285,7 @@ void iowrite64be_lo_hi(u64 val, void __iomem *addr)
 		mmio_write64be(val, addr));
 }
 
-void iowrite64be_hi_lo(u64 val, void __iomem *addr)
+void __iowrite64be_hi_lo(u64 val, void __iomem *addr)
 {
 	/* Make sure uninitialized memory isn't copied to devices. */
 	kmsan_check_memory(&val, sizeof(val));
@@ -293,12 +293,12 @@ void iowrite64be_hi_lo(u64 val, void __iomem *addr)
 		mmio_write64be(val, addr));
 }
 
-EXPORT_SYMBOL(iowrite64_lo_hi);
-EXPORT_SYMBOL(iowrite64_hi_lo);
-EXPORT_SYMBOL(iowrite64be_lo_hi);
-EXPORT_SYMBOL(iowrite64be_hi_lo);
+EXPORT_SYMBOL(__iowrite64_lo_hi);
+EXPORT_SYMBOL(__iowrite64_hi_lo);
+EXPORT_SYMBOL(__iowrite64be_lo_hi);
+EXPORT_SYMBOL(__iowrite64be_hi_lo);
 
-#endif /* readq */
+#endif /* CONFIG_64BIT */
 
 /*
  * These are the "repeat MMIO read/write" functions.

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Andy Shevchenko]

On Mon, Feb 03, 2025 at 12:43:01PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 3, 2025, at 09:25, Thomas Weißschuh wrote:
> > On Mon, Feb 03, 2025 at 09:18:33AM +0100, Arnd Bergmann wrote:
> >> I have not tried it yet, but I suspect this is not the correct
> >> fix here. Unfortunately the indirect header inclusions in this
> >> file are way too complicated with corner cases in various
> >> architectures. How much testing have you given your patch
> >> across other targets? I think the last time we tried to address
> >> it, we broke mips or parisc.
> >
> > It was build-tested on 0day.
> > I also gave it some light boot testing on kunit/qemu.
> > (Neither on mips or parisc)
> 
> Ok, I see. I checked the usage of these functions again and
> now remembered the reason we didn't fix it last time, which is
> that the semantics are inconsistent across architectures
> and I think this should be addressed first, so we can untangle
> the definitions:
> 
> There is one driver that is specific to ARM processors
> (drivers/firmware/arm_scmi) using  ioread64_hi_lo/iowrite64_hi_lo
> and this uses the documented semantics from
> Documentation/driver-api/device-io.rst, which says that the
> helpers always do separate 32-bit accesses (even on 64-bit
> machines), but that it's possible to just call
> ioread64()/iowrite64() after including linux/io-64-nonatomic-hi-lo.h
> in order to always get 64-bit accesses on 64-bit architectures
> but get 32-bit accesses on 32-bit architectures. There are
> another dozen or so drivers that do this.
> 
> There are two other drivers that were written for x86 that
> use other semantics (drivers/net/wwan/t7xx/ and
> drivers/ptp/ptp_pch.c): Here, the definition from lib/iomap.c
> means that even on 64-bit architectures calling
> ioread64_hi_lo/iowrite64_hi_lo on an MMIO space always
> results in a 64-bit access,

Interesting... I believe both cases mentioned in this paragraph
were written with only the include/linux/io-64*.h in mind.

> and only the PIO version is split
> up. On 32-bit architectures, this part is not provided, so
> it should fall back to split access (I think this is where
> we had bugs in the past).
> 
> Another complication is that alpha, parisc and sh (not mips
> any more) explicitly include asm-generic/iomap.h but don't
> select CONFIG_GENERIC_IOMAP. At this time, GENERIC_IOMAP
> is selected at least in some configurations on m68k, mips,
> powerpc, sh, um and x86, but most of these should not do that
> (mips, m68k and sh have no PIO instructions, powerpc had
> a hack that I think was just removed but needs more cleanup).
> 
> I'm testing with the patch below now, which separates the
> CONFIG_GENERIC_IOMAP implementation (with the 32-bit PIO
> access) from the normal version, and picks an appropriate
> one in linux/io-64-nonatomic-*.h based on the architecture
> to avoid some of the more confusing nested 
> "#ifdef ioread64" etc checks.
> 
> I checked that none of the three drivers ever actually uses
> PIO registers instead of MMIO, and since none of them use the
> big-endian accessors, this all turns into readq/writeq
> in practice anyway.
> 
> The ptp_pch.c driver still needs more work as I noticed two
> issues there: the driver has a mix of lo_hi and hi_lo
> variants, but it is unclear whether is is actually required
> on 32-bit or if the hardware works the same either way.

PTP is special, some registers are related to a read timestamp IIRC and
hence hi_lo is a must there.

> In addition, these seem to be timer registers that may overrun
> from the lo into the hi field between the two accesses, so
> technically a 32-bit host needs to do an extra read to
> check for overflow and possibly repeat the access.

Yes, precisely why hi_lo is used to minimize the error when it races like this.

But IIRC *_pch drivers are for 32-bit platform, the code, if so, was made
to be compiled on 64-bit but never used IRL, just for test coverage.

(I believe the PCH stands for EG20 PCH, I have [32-bit] boards with it.)

...

I like the lib/* and include/* cleanup but PTP probably should stay as is.
OTOH WWAN case most likely had been tested on 64-bit platforms only and
proves that readq()/writeq() works there, so it's okay to unify.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Arnd Bergmann]

On Mon, Feb 3, 2025, at 13:07, Andy Shevchenko wrote:
> On Mon, Feb 03, 2025 at 12:43:01PM +0100, Arnd Bergmann wrote:
>> In addition, these seem to be timer registers that may overrun
>> from the lo into the hi field between the two accesses, so
>> technically a 32-bit host needs to do an extra read to
>> check for overflow and possibly repeat the access.
>
> Yes, precisely why hi_lo is used to minimize the error when it races like this.
>
> But IIRC *_pch drivers are for 32-bit platform, the code, if so, was made
> to be compiled on 64-bit but never used IRL, just for test coverage.
>
> (I believe the PCH stands for EG20 PCH, I have [32-bit] boards with it.)

Ok, so we don't even know how that hardware block would read to
a 64-bit bus transaction, it might give a race-free result, might
have the same race as 32-bit or might just cause data corruption
(e.g. ignoring half the bits).

I think the usual way to access a timestamp in two registers works
like this

u64 read_double_reg(u32 __iomem *reg)
{
       u32 hi, lo;

       /* check for overflow race by re-reading upper bits */
       do {
               hi = readl(reg + 1);
               lo = readl(reg);
       } while (hi != readl(reg + 1);

       return (u64)hi << 32 | lo;
}

void write_double_reg(u32 __iomem *reg, u64 val)
{
       /* ensure the low bits don't overflow right now, assumes
          low word is ticking up */
       writel(reg, 0);

       writel(reg + 1, upper_32_bits(val));
       writel(reg,     lower_32_bits(val));
}

[If there might be concurrent read/write accesses, it gets
much more complicated than this.]

Do you know why the driver doesn't do it like that?

> I like the lib/* and include/* cleanup but PTP probably should stay as is.
> OTOH WWAN case most likely had been tested on 64-bit platforms only and
> proves that readq()/writeq() works there, so it's okay to unify.

Ok, I'll try to split it up into sensible patches then. For ptp
(both ixp46x and pch), these are the options I see:

- leave it unchanged since nobody cares any more
- add some comments about being racy and possibly broken on 64-bit
- revert your pch patch d09adf61002f/8664d49a815e3 to make it have 32-
  bit accesses again and fix the theoretical 64-bit issue but not the
  race
- use helper functions like the ones I showed above and test it
  properly

I also added Richard Cochran to cc, as he wrote the original
ixp46x driver and may know of other ptp drivers that have
this issue. One potential candidate I see is
https://elixir.bootlin.com/linux/v6.13.1/source/drivers/ptp/ptp_dfl_tod.c#L226
and other functions in that file.

       Arnd

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Andy Shevchenko]

On Mon, Feb 03, 2025 at 02:08:35PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 3, 2025, at 13:07, Andy Shevchenko wrote:
> > On Mon, Feb 03, 2025 at 12:43:01PM +0100, Arnd Bergmann wrote:
> >> In addition, these seem to be timer registers that may overrun
> >> from the lo into the hi field between the two accesses, so
> >> technically a 32-bit host needs to do an extra read to
> >> check for overflow and possibly repeat the access.
> >
> > Yes, precisely why hi_lo is used to minimize the error when it races like this.
> >
> > But IIRC *_pch drivers are for 32-bit platform, the code, if so, was made
> > to be compiled on 64-bit but never used IRL, just for test coverage.
> >
> > (I believe the PCH stands for EG20 PCH, I have [32-bit] boards with it.)
> 
> Ok, so we don't even know how that hardware block would read to
> a 64-bit bus transaction, it might give a race-free result, might
> have the same race as 32-bit or might just cause data corruption
> (e.g. ignoring half the bits).
> 
> I think the usual way to access a timestamp in two registers works
> like this
> 
> u64 read_double_reg(u32 __iomem *reg)
> {
>        u32 hi, lo;
> 
>        /* check for overflow race by re-reading upper bits */
>        do {
>                hi = readl(reg + 1);
>                lo = readl(reg);
>        } while (hi != readl(reg + 1);
> 
>        return (u64)hi << 32 | lo;
> }
> 
> void write_double_reg(u32 __iomem *reg, u64 val)
> {
>        /* ensure the low bits don't overflow right now, assumes
>           low word is ticking up */
>        writel(reg, 0);
> 
>        writel(reg + 1, upper_32_bits(val));
>        writel(reg,     lower_32_bits(val));
> }
> 
> [If there might be concurrent read/write accesses, it gets
> much more complicated than this.]
> 
> Do you know why the driver doesn't do it like that?

No idea.

> > I like the lib/* and include/* cleanup but PTP probably should stay as is.
> > OTOH WWAN case most likely had been tested on 64-bit platforms only and
> > proves that readq()/writeq() works there, so it's okay to unify.
> 
> Ok, I'll try to split it up into sensible patches then. For ptp
> (both ixp46x and pch), these are the options I see:

> - leave it unchanged since nobody cares any more
> - add some comments about being racy and possibly broken on 64-bit

Any combination of these two I would prefer.

> - revert your pch patch d09adf61002f/8664d49a815e3 to make it have 32-
>   bit accesses again and fix the theoretical 64-bit issue but not the
>   race

Definitely not this. I assume that _hi_lo and _lo_hi semantics of IO
APIs implies non-atomicity access and hence always splits the IO in
64-bit case. These helpers make code much less verbose and actually
(due to naming) clearer about the sequence of the reads or writes.
I prefer to have them stay (in the drivers).

> - use helper functions like the ones I showed above and test it
>   properly

If so, these helper functions should be available to wider audience.
But I think it will be a premature effort.

> I also added Richard Cochran to cc, as he wrote the original
> ixp46x driver and may know of other ptp drivers that have

I also suggest to ping Linus W. He seems to have an IXP4xx hardware,

> this issue. One potential candidate I see is
> https://elixir.bootlin.com/linux/v6.13.1/source/drivers/ptp/ptp_dfl_tod.c#L226
> and other functions in that file.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Arnd Bergmann]

On Mon, Feb 3, 2025, at 14:23, Andy Shevchenko wrote:
> On Mon, Feb 03, 2025 at 02:08:35PM +0100, Arnd Bergmann wrote:
>
>> > I like the lib/* and include/* cleanup but PTP probably should stay as is.
>> > OTOH WWAN case most likely had been tested on 64-bit platforms only and
>> > proves that readq()/writeq() works there, so it's okay to unify.
>> 
>> Ok, I'll try to split it up into sensible patches then. For ptp
>> (both ixp46x and pch), these are the options I see:

Update: While splitting out the wwan patch, I see that this would
revert 7d5a7dd5a358 ("net: wwan: t7xx: Split 64bit accesses to fix
alignment issues"), so that driver also needs to keep using the
split accessors, at least for some of the registers. From what
I can tell, the problem here is that REG_CLDMA_UL_START_ADDRL_0
is not a multiple of 8, and arm64 CPUs require MMIO registers
to be naturally aligned.

If that is the cause of the problem, this means that on x86-64
the unaligned readq() is still used but happens to work.
Once I change linux/io-64-nonatomic-lo-hi.h, it will split
the access on x86-64 and other using GENERIC_IOMAP as well.

The accesses to ATR_PCIE_WIN0_T0_TRSL_ADDR and
ATR_PCIE_WIN0_T0_ATR_PARAM_SRC_ADDR can probably use ioread64()
because they are on aligned addresses, but it should be safe
to just leave this driver alone and keep all the split
accesses.

>> - leave it unchanged since nobody cares any more
>> - add some comments about being racy and possibly broken on 64-bit
>
> Any combination of these two I would prefer.
>
>> - revert your pch patch d09adf61002f/8664d49a815e3 to make it have 32-
>>   bit accesses again and fix the theoretical 64-bit issue but not the
>>   race
>
> Definitely not this. I assume that _hi_lo and _lo_hi semantics of IO
> APIs implies non-atomicity access and hence always splits the IO in
> 64-bit case. These helpers make code much less verbose and actually
> (due to naming) clearer about the sequence of the reads or writes.
> I prefer to have them stay (in the drivers).

Right, after my change to the headers, it will at least behave
consistently according to the documentation, so there is no
additional problem if someone reuses that driver on (problem
nonexistent) 64-bit hardware.

       Arnd

Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Andy Shevchenko]

On Mon, Feb 03, 2025 at 02:34:11PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 3, 2025, at 14:23, Andy Shevchenko wrote:
> > On Mon, Feb 03, 2025 at 02:08:35PM +0100, Arnd Bergmann wrote:
> >
> >> > I like the lib/* and include/* cleanup but PTP probably should stay as is.
> >> > OTOH WWAN case most likely had been tested on 64-bit platforms only and
> >> > proves that readq()/writeq() works there, so it's okay to unify.
> >> 
> >> Ok, I'll try to split it up into sensible patches then. For ptp
> >> (both ixp46x and pch), these are the options I see:
> 
> Update: While splitting out the wwan patch, I see that this would
> revert 7d5a7dd5a358 ("net: wwan: t7xx: Split 64bit accesses to fix
> alignment issues"), so that driver also needs to keep using the
> split accessors, at least for some of the registers. From what
> I can tell, the problem here is that REG_CLDMA_UL_START_ADDRL_0
> is not a multiple of 8, and arm64 CPUs require MMIO registers
> to be naturally aligned.
> 
> If that is the cause of the problem, this means that on x86-64
> the unaligned readq() is still used but happens to work.
> Once I change linux/io-64-nonatomic-lo-hi.h, it will split
> the access on x86-64 and other using GENERIC_IOMAP as well.
> 
> The accesses to ATR_PCIE_WIN0_T0_TRSL_ADDR and
> ATR_PCIE_WIN0_T0_ATR_PARAM_SRC_ADDR can probably use ioread64()
> because they are on aligned addresses, but it should be safe
> to just leave this driver alone and keep all the split
> accesses.

Interesting. I haven't followed the development of that driver, so it seems it
gets some fixes over the time. In previous reply I was under impression that
the code in question was initially written by Intel and hence was assumed to
split (because readq()/writeq() usually works there and no explicit treatment
is needed in the most of the cases).

-- 
With Best Regards,
Andy Shevchenko