[PATCH 02/33] powerpc/legacy_serial: cache serial port and info in add_legacy_port()

[PATCH 02/33] powerpc/legacy_serial: cache serial port and info in add_legacy_port()

[Jiri Slaby (SUSE)]

Caching the port and info in local variables makes the code more compact
and easier to understand.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/legacy_serial.c | 52 ++++++++++++++---------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index 1da2f6e7d2a1..d9080189c28c 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -77,6 +77,8 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
 				  phys_addr_t taddr, unsigned long irq,
 				  upf_t flags, int irq_check_parent)
 {
+	struct plat_serial8250_port *legacy_port;
+	struct legacy_serial_info *legacy_info;
 	const __be32 *clk, *spd, *rs;
 	u32 clock = BASE_BAUD * 16;
 	u32 shift = 0;
@@ -110,16 +112,17 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
 	if (index >= legacy_serial_count)
 		legacy_serial_count = index + 1;
 
+	legacy_port = &legacy_serial_ports[index];
+	legacy_info = &legacy_serial_infos[index];
+
 	/* Check if there is a port who already claimed our slot */
-	if (legacy_serial_infos[index].np != NULL) {
+	if (legacy_info->np != NULL) {
 		/* if we still have some room, move it, else override */
 		if (legacy_serial_count < MAX_LEGACY_SERIAL_PORTS) {
 			printk(KERN_DEBUG "Moved legacy port %d -> %d\n",
 			       index, legacy_serial_count);
-			legacy_serial_ports[legacy_serial_count] =
-				legacy_serial_ports[index];
-			legacy_serial_infos[legacy_serial_count] =
-				legacy_serial_infos[index];
+			legacy_serial_ports[legacy_serial_count] = *legacy_port;
+			legacy_serial_infos[legacy_serial_count] = *legacy_info;
 			legacy_serial_count++;
 		} else {
 			printk(KERN_DEBUG "Replacing legacy port %d\n", index);
@@ -127,36 +130,33 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
 	}
 
 	/* Now fill the entry */
-	memset(&legacy_serial_ports[index], 0,
-	       sizeof(struct plat_serial8250_port));
+	memset(legacy_port, 0, sizeof(*legacy_port));
 	if (iotype == UPIO_PORT)
-		legacy_serial_ports[index].iobase = base;
+		legacy_port->iobase = base;
 	else
-		legacy_serial_ports[index].mapbase = base;
-
-	legacy_serial_ports[index].iotype = iotype;
-	legacy_serial_ports[index].uartclk = clock;
-	legacy_serial_ports[index].irq = irq;
-	legacy_serial_ports[index].flags = flags;
-	legacy_serial_ports[index].regshift = shift;
-	legacy_serial_infos[index].taddr = taddr;
-	legacy_serial_infos[index].np = of_node_get(np);
-	legacy_serial_infos[index].clock = clock;
-	legacy_serial_infos[index].speed = spd ? be32_to_cpup(spd) : 0;
-	legacy_serial_infos[index].irq_check_parent = irq_check_parent;
+		legacy_port->mapbase = base;
+
+	legacy_port->iotype = iotype;
+	legacy_port->uartclk = clock;
+	legacy_port->irq = irq;
+	legacy_port->flags = flags;
+	legacy_port->regshift = shift;
+	legacy_info->taddr = taddr;
+	legacy_info->np = of_node_get(np);
+	legacy_info->clock = clock;
+	legacy_info->speed = spd ? be32_to_cpup(spd) : 0;
+	legacy_info->irq_check_parent = irq_check_parent;
 
 	if (iotype == UPIO_TSI) {
-		legacy_serial_ports[index].serial_in = tsi_serial_in;
-		legacy_serial_ports[index].serial_out = tsi_serial_out;
+		legacy_port->serial_in = tsi_serial_in;
+		legacy_port->serial_out = tsi_serial_out;
 	}
 
-	printk(KERN_DEBUG "Found legacy serial port %d for %pOF\n",
-	       index, np);
+	printk(KERN_DEBUG "Found legacy serial port %d for %pOF\n", index, np);
 	printk(KERN_DEBUG "  %s=%llx, taddr=%llx, irq=%lx, clk=%d, speed=%d\n",
 	       (iotype == UPIO_PORT) ? "port" : "mem",
 	       (unsigned long long)base, (unsigned long long)taddr, irq,
-	       legacy_serial_ports[index].uartclk,
-	       legacy_serial_infos[index].speed);
+	       legacy_port->uartclk, legacy_info->speed);
 
 	return index;
 }
-- 
2.49.0

Re: [PATCH 02/33] powerpc/legacy_serial: cache serial port and info in add_legacy_port()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4542 bytes --]

On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> Caching the port and info in local variables makes the code more compact
> and easier to understand.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: linuxppc-dev@lists.ozlabs.org

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

--
 i.

> ---
>  arch/powerpc/kernel/legacy_serial.c | 52 ++++++++++++++---------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
> index 1da2f6e7d2a1..d9080189c28c 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -77,6 +77,8 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
>  				  phys_addr_t taddr, unsigned long irq,
>  				  upf_t flags, int irq_check_parent)
>  {
> +	struct plat_serial8250_port *legacy_port;
> +	struct legacy_serial_info *legacy_info;
>  	const __be32 *clk, *spd, *rs;
>  	u32 clock = BASE_BAUD * 16;
>  	u32 shift = 0;
> @@ -110,16 +112,17 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
>  	if (index >= legacy_serial_count)
>  		legacy_serial_count = index + 1;
>  
> +	legacy_port = &legacy_serial_ports[index];
> +	legacy_info = &legacy_serial_infos[index];
> +
>  	/* Check if there is a port who already claimed our slot */
> -	if (legacy_serial_infos[index].np != NULL) {
> +	if (legacy_info->np != NULL) {
>  		/* if we still have some room, move it, else override */
>  		if (legacy_serial_count < MAX_LEGACY_SERIAL_PORTS) {
>  			printk(KERN_DEBUG "Moved legacy port %d -> %d\n",
>  			       index, legacy_serial_count);
> -			legacy_serial_ports[legacy_serial_count] =
> -				legacy_serial_ports[index];
> -			legacy_serial_infos[legacy_serial_count] =
> -				legacy_serial_infos[index];
> +			legacy_serial_ports[legacy_serial_count] = *legacy_port;
> +			legacy_serial_infos[legacy_serial_count] = *legacy_info;
>  			legacy_serial_count++;
>  		} else {
>  			printk(KERN_DEBUG "Replacing legacy port %d\n", index);
> @@ -127,36 +130,33 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
>  	}
>  
>  	/* Now fill the entry */
> -	memset(&legacy_serial_ports[index], 0,
> -	       sizeof(struct plat_serial8250_port));
> +	memset(legacy_port, 0, sizeof(*legacy_port));
>  	if (iotype == UPIO_PORT)
> -		legacy_serial_ports[index].iobase = base;
> +		legacy_port->iobase = base;
>  	else
> -		legacy_serial_ports[index].mapbase = base;
> -
> -	legacy_serial_ports[index].iotype = iotype;
> -	legacy_serial_ports[index].uartclk = clock;
> -	legacy_serial_ports[index].irq = irq;
> -	legacy_serial_ports[index].flags = flags;
> -	legacy_serial_ports[index].regshift = shift;
> -	legacy_serial_infos[index].taddr = taddr;
> -	legacy_serial_infos[index].np = of_node_get(np);
> -	legacy_serial_infos[index].clock = clock;
> -	legacy_serial_infos[index].speed = spd ? be32_to_cpup(spd) : 0;
> -	legacy_serial_infos[index].irq_check_parent = irq_check_parent;
> +		legacy_port->mapbase = base;
> +
> +	legacy_port->iotype = iotype;
> +	legacy_port->uartclk = clock;
> +	legacy_port->irq = irq;
> +	legacy_port->flags = flags;
> +	legacy_port->regshift = shift;
> +	legacy_info->taddr = taddr;
> +	legacy_info->np = of_node_get(np);
> +	legacy_info->clock = clock;
> +	legacy_info->speed = spd ? be32_to_cpup(spd) : 0;
> +	legacy_info->irq_check_parent = irq_check_parent;
>  
>  	if (iotype == UPIO_TSI) {
> -		legacy_serial_ports[index].serial_in = tsi_serial_in;
> -		legacy_serial_ports[index].serial_out = tsi_serial_out;
> +		legacy_port->serial_in = tsi_serial_in;
> +		legacy_port->serial_out = tsi_serial_out;
>  	}
>  
> -	printk(KERN_DEBUG "Found legacy serial port %d for %pOF\n",
> -	       index, np);
> +	printk(KERN_DEBUG "Found legacy serial port %d for %pOF\n", index, np);
>  	printk(KERN_DEBUG "  %s=%llx, taddr=%llx, irq=%lx, clk=%d, speed=%d\n",
>  	       (iotype == UPIO_PORT) ? "port" : "mem",
>  	       (unsigned long long)base, (unsigned long long)taddr, irq,
> -	       legacy_serial_ports[index].uartclk,
> -	       legacy_serial_infos[index].speed);
> +	       legacy_port->uartclk, legacy_info->speed);
>  
>  	return index;
>  }
> 

[PATCH 05/33] powerpc/powermac: remove unneeded tty includes

[Jiri Slaby (SUSE)]

All these includes must have been cut & pasted. The code does not use
any tty or vt functionality at all.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/platforms/powermac/setup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/platforms/powermac/setup.c b/arch/powerpc/platforms/powermac/setup.c
index e119ced05d10..eb092f293113 100644
--- a/arch/powerpc/platforms/powermac/setup.c
+++ b/arch/powerpc/platforms/powermac/setup.c
@@ -28,13 +28,11 @@
 #include <linux/ptrace.h>
 #include <linux/export.h>
 #include <linux/user.h>
-#include <linux/tty.h>
 #include <linux/string.h>
 #include <linux/delay.h>
 #include <linux/ioport.h>
 #include <linux/major.h>
 #include <linux/initrd.h>
-#include <linux/vt_kern.h>
 #include <linux/console.h>
 #include <linux/pci.h>
 #include <linux/adb.h>
-- 
2.49.0

Re: [PATCH 05/33] powerpc/powermac: remove unneeded tty includes

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> All these includes must have been cut & pasted. The code does not use
> any tty or vt functionality at all.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>  arch/powerpc/platforms/powermac/setup.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powermac/setup.c b/arch/powerpc/platforms/powermac/setup.c
> index e119ced05d10..eb092f293113 100644
> --- a/arch/powerpc/platforms/powermac/setup.c
> +++ b/arch/powerpc/platforms/powermac/setup.c
> @@ -28,13 +28,11 @@
>  #include <linux/ptrace.h>
>  #include <linux/export.h>
>  #include <linux/user.h>
> -#include <linux/tty.h>
>  #include <linux/string.h>
>  #include <linux/delay.h>
>  #include <linux/ioport.h>
>  #include <linux/major.h>
>  #include <linux/initrd.h>
> -#include <linux/vt_kern.h>
>  #include <linux/console.h>
>  #include <linux/pci.h>
>  #include <linux/adb.h>

Seems true. I had to actually scan through the file as it does have some 
console related setup.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH 08/33] serial: 8250: sanitize uart_port::serial_{in,out}() types

[Jiri Slaby (SUSE)]

uart_port::{serial_in,serial_out} (and plat_serial8250_port::* likewise)
historically use:
* 'unsigned int' for 32-bit register values in reads and writes, and
* 'int' for offsets.

Make them sane such that:
* 'u32' is used for register values, and
* 'unsigned int' is used for offsets.

While at it, name hooks' parameters, so it is clear what is what.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Paul Cercueil <paul@crapouillou.net>
Cc: Vladimir Zapolskiy <vz@mleia.com>
Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
---
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-mips@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
---
 arch/powerpc/kernel/legacy_serial.c     |  7 ++---
 drivers/tty/serial/8250/8250_dw.c       | 34 ++++++++++++-------------
 drivers/tty/serial/8250/8250_em.c       |  4 +--
 drivers/tty/serial/8250/8250_ingenic.c  |  8 +++---
 drivers/tty/serial/8250/8250_ioc3.c     |  4 +--
 drivers/tty/serial/8250/8250_lpc18xx.c  |  2 +-
 drivers/tty/serial/8250/8250_pci.c      |  6 ++---
 drivers/tty/serial/8250/8250_port.c     | 30 +++++++++++-----------
 drivers/tty/serial/8250/8250_rt288x.c   |  4 +--
 drivers/tty/serial/8250/8250_uniphier.c |  4 +--
 include/linux/serial_8250.h             |  4 +--
 include/linux/serial_core.h             |  4 +--
 12 files changed, 56 insertions(+), 55 deletions(-)

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index a874eb8e000b..ae1906bfe8a5 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -54,9 +54,10 @@ static int legacy_serial_console = -1;
 static const upf_t legacy_port_flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST |
 	UPF_SHARE_IRQ | UPF_FIXED_PORT;
 
-static unsigned int tsi_serial_in(struct uart_port *p, int offset)
+static u32 tsi_serial_in(struct uart_port *p, unsigned int offset)
 {
-	unsigned int tmp;
+	u32 tmp;
+
 	offset = offset << p->regshift;
 	if (offset == UART_IIR) {
 		tmp = readl(p->membase + (UART_IIR & ~3));
@@ -65,7 +66,7 @@ static unsigned int tsi_serial_in(struct uart_port *p, int offset)
 		return readb(p->membase + offset);
 }
 
-static void tsi_serial_out(struct uart_port *p, int offset, int value)
+static void tsi_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	if (!((offset == UART_IER) && (value & UART_IER_UUE)))
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1902f29444a1..0a22f0cb8896 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -67,8 +67,8 @@ struct dw8250_data {
 	struct dw8250_port_data	data;
 	const struct dw8250_platform_data *pdata;
 
-	int			msr_mask_on;
-	int			msr_mask_off;
+	u32			msr_mask_on;
+	u32			msr_mask_off;
 	struct clk		*clk;
 	struct clk		*pclk;
 	struct notifier_block	clk_notifier;
@@ -94,7 +94,7 @@ static inline struct dw8250_data *work_to_dw8250_data(struct work_struct *work)
 	return container_of(work, struct dw8250_data, clk_work);
 }
 
-static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
+static inline u32 dw8250_modify_msr(struct uart_port *p, unsigned int offset, u32 value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 
@@ -145,7 +145,7 @@ static void dw8250_force_idle(struct uart_port *p)
  * routine. Hence, it must not call serial_port_out() or serial_out()
  * against the modified registers here, i.e. LCR.
  */
-static void dw8250_check_lcr(struct uart_port *p, int offset, int value)
+static void dw8250_check_lcr(struct uart_port *p, unsigned int offset, u32 value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	void __iomem *addr = p->membase + (offset << p->regshift);
@@ -156,7 +156,7 @@ static void dw8250_check_lcr(struct uart_port *p, int offset, int value)
 
 	/* Make sure LCR write wasn't ignored */
 	while (tries--) {
-		unsigned int lcr = serial_port_in(p, offset);
+		u32 lcr = serial_port_in(p, offset);
 
 		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 			return;
@@ -205,13 +205,13 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
 	}
 }
 
-static void dw8250_serial_out(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	writeb(value, p->membase + (offset << p->regshift));
 	dw8250_check_lcr(p, offset, value);
 }
 
-static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out38x(struct uart_port *p, unsigned int offset, u32 value)
 {
 	/* Allow the TX to drain before we reconfigure */
 	if (offset == UART_LCR)
@@ -220,22 +220,22 @@ static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
 	dw8250_serial_out(p, offset, value);
 }
 
-static unsigned int dw8250_serial_in(struct uart_port *p, int offset)
+static u32 dw8250_serial_in(struct uart_port *p, unsigned int offset)
 {
-	unsigned int value = readb(p->membase + (offset << p->regshift));
+	u32 value = readb(p->membase + (offset << p->regshift));
 
 	return dw8250_modify_msr(p, offset, value);
 }
 
 #ifdef CONFIG_64BIT
-static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
+static u32 dw8250_serial_inq(struct uart_port *p, unsigned int offset)
 {
 	u8 value = __raw_readq(p->membase + (offset << p->regshift));
 
 	return dw8250_modify_msr(p, offset, value);
 }
 
-static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
+static void dw8250_serial_outq(struct uart_port *p, unsigned int offset, u32 value)
 {
 	value &= 0xff;
 	__raw_writeq(value, p->membase + (offset << p->regshift));
@@ -246,28 +246,28 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out32(struct uart_port *p, unsigned int offset, u32 value)
 {
 	writel(value, p->membase + (offset << p->regshift));
 	dw8250_check_lcr(p, offset, value);
 }
 
-static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
+static u32 dw8250_serial_in32(struct uart_port *p, unsigned int offset)
 {
-	unsigned int value = readl(p->membase + (offset << p->regshift));
+	u32 value = readl(p->membase + (offset << p->regshift));
 
 	return dw8250_modify_msr(p, offset, value);
 }
 
-static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out32be(struct uart_port *p, unsigned int offset, u32 value)
 {
 	iowrite32be(value, p->membase + (offset << p->regshift));
 	dw8250_check_lcr(p, offset, value);
 }
 
-static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+static u32 dw8250_serial_in32be(struct uart_port *p, unsigned int offset)
 {
-       unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+       u32 value = ioread32be(p->membase + (offset << p->regshift));
 
        return dw8250_modify_msr(p, offset, value);
 }
diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 35094f884492..e90c71494944 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -59,7 +59,7 @@ static void serial8250_em_serial_out_helper(struct uart_port *p, int offset,
 	}
 }
 
-static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
+static u32 serial8250_em_serial_in(struct uart_port *p, unsigned int offset)
 {
 	switch (offset) {
 	case UART_RX: /* RX @ 0x00 */
@@ -119,7 +119,7 @@ static void serial8250_em_reg_update(struct uart_port *p, int off, int value)
 	serial8250_em_serial_out_helper(p, UART_HCR0_EM, hcr0);
 }
 
-static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
+static void serial8250_em_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	switch (offset) {
 	case UART_TX:
diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
index a73dd3773640..94542fc143c2 100644
--- a/drivers/tty/serial/8250/8250_ingenic.c
+++ b/drivers/tty/serial/8250/8250_ingenic.c
@@ -168,9 +168,9 @@ OF_EARLYCON_DECLARE(jz4780_uart, "ingenic,jz4780-uart",
 OF_EARLYCON_DECLARE(x1000_uart, "ingenic,x1000-uart",
 		    ingenic_early_console_setup);
 
-static void ingenic_uart_serial_out(struct uart_port *p, int offset, int value)
+static void ingenic_uart_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
-	int ier;
+	u32 ier;
 
 	switch (offset) {
 	case UART_FCR:
@@ -206,9 +206,9 @@ static void ingenic_uart_serial_out(struct uart_port *p, int offset, int value)
 	writeb(value, p->membase + (offset << p->regshift));
 }
 
-static unsigned int ingenic_uart_serial_in(struct uart_port *p, int offset)
+static u32 ingenic_uart_serial_in(struct uart_port *p, unsigned int offset)
 {
-	unsigned int value;
+	u8 value;
 
 	value = readb(p->membase + (offset << p->regshift));
 
diff --git a/drivers/tty/serial/8250/8250_ioc3.c b/drivers/tty/serial/8250/8250_ioc3.c
index 499e80aa4cf9..3ebda9a5d07d 100644
--- a/drivers/tty/serial/8250/8250_ioc3.c
+++ b/drivers/tty/serial/8250/8250_ioc3.c
@@ -21,12 +21,12 @@ struct ioc3_8250_data {
 	int line;
 };
 
-static unsigned int ioc3_serial_in(struct uart_port *p, int offset)
+static u32 ioc3_serial_in(struct uart_port *p, unsigned int offset)
 {
 	return readb(p->membase + (offset ^ 3));
 }
 
-static void ioc3_serial_out(struct uart_port *p, int offset, int value)
+static void ioc3_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	writeb(value, p->membase + (offset ^ 3));
 }
diff --git a/drivers/tty/serial/8250/8250_lpc18xx.c b/drivers/tty/serial/8250/8250_lpc18xx.c
index d52445948da0..6c0489c9c253 100644
--- a/drivers/tty/serial/8250/8250_lpc18xx.c
+++ b/drivers/tty/serial/8250/8250_lpc18xx.c
@@ -67,7 +67,7 @@ static int lpc18xx_rs485_config(struct uart_port *port, struct ktermios *termios
 	return 0;
 }
 
-static void lpc18xx_uart_serial_out(struct uart_port *p, int offset, int value)
+static void lpc18xx_uart_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	/*
 	 * For DMA mode one must ensure that the UART_FCR_DMA_SELECT
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 73c200127b08..152f914c599d 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1751,7 +1751,7 @@ static int pci_fintek_init(struct pci_dev *dev)
 	return max_port;
 }
 
-static void f815xxa_mem_serial_out(struct uart_port *p, int offset, int value)
+static void f815xxa_mem_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	struct f815xxa_data *data = p->private_data;
 	unsigned long flags;
@@ -1846,10 +1846,10 @@ static void kt_handle_break(struct uart_port *p)
 	serial8250_clear_and_reinit_fifos(up);
 }
 
-static unsigned int kt_serial_in(struct uart_port *p, int offset)
+static u32 kt_serial_in(struct uart_port *p, unsigned int offset)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
-	unsigned int val;
+	u32 val;
 
 	/*
 	 * When the Intel ME (management engine) gets reset its serial
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6d7b8c4667c9..f5407832e8a7 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -339,14 +339,14 @@ static void default_serial_dl_write(struct uart_8250_port *up, u32 value)
 }
 
 #ifdef CONFIG_HAS_IOPORT
-static unsigned int hub6_serial_in(struct uart_port *p, int offset)
+static u32 hub6_serial_in(struct uart_port *p, unsigned int offset)
 {
 	offset = offset << p->regshift;
 	outb(p->hub6 - 1 + offset, p->iobase);
 	return inb(p->iobase + 1);
 }
 
-static void hub6_serial_out(struct uart_port *p, int offset, int value)
+static void hub6_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	outb(p->hub6 - 1 + offset, p->iobase);
@@ -354,73 +354,73 @@ static void hub6_serial_out(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_HAS_IOPORT */
 
-static unsigned int mem_serial_in(struct uart_port *p, int offset)
+static u32 mem_serial_in(struct uart_port *p, unsigned int offset)
 {
 	offset = offset << p->regshift;
 	return readb(p->membase + offset);
 }
 
-static void mem_serial_out(struct uart_port *p, int offset, int value)
+static void mem_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	writeb(value, p->membase + offset);
 }
 
-static void mem16_serial_out(struct uart_port *p, int offset, int value)
+static void mem16_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	writew(value, p->membase + offset);
 }
 
-static unsigned int mem16_serial_in(struct uart_port *p, int offset)
+static u32 mem16_serial_in(struct uart_port *p, unsigned int offset)
 {
 	offset = offset << p->regshift;
 	return readw(p->membase + offset);
 }
 
-static void mem32_serial_out(struct uart_port *p, int offset, int value)
+static void mem32_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	writel(value, p->membase + offset);
 }
 
-static unsigned int mem32_serial_in(struct uart_port *p, int offset)
+static u32 mem32_serial_in(struct uart_port *p, unsigned int offset)
 {
 	offset = offset << p->regshift;
 	return readl(p->membase + offset);
 }
 
-static void mem32be_serial_out(struct uart_port *p, int offset, int value)
+static void mem32be_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	iowrite32be(value, p->membase + offset);
 }
 
-static unsigned int mem32be_serial_in(struct uart_port *p, int offset)
+static u32 mem32be_serial_in(struct uart_port *p, unsigned int offset)
 {
 	offset = offset << p->regshift;
 	return ioread32be(p->membase + offset);
 }
 
 #ifdef CONFIG_HAS_IOPORT
-static unsigned int io_serial_in(struct uart_port *p, int offset)
+static u32 io_serial_in(struct uart_port *p, unsigned int offset)
 {
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }
 
-static void io_serial_out(struct uart_port *p, int offset, int value)
+static void io_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	offset = offset << p->regshift;
 	outb(value, p->iobase + offset);
 }
 #endif
-static unsigned int no_serial_in(struct uart_port *p, int offset)
+static u32 no_serial_in(struct uart_port *p, unsigned int offset)
 {
-	return (unsigned int)-1;
+	return ~0U;
 }
 
-static void no_serial_out(struct uart_port *p, int offset, int value)
+static void no_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 }
 
diff --git a/drivers/tty/serial/8250/8250_rt288x.c b/drivers/tty/serial/8250/8250_rt288x.c
index 6415ca8d3adf..bf28b8a9a710 100644
--- a/drivers/tty/serial/8250/8250_rt288x.c
+++ b/drivers/tty/serial/8250/8250_rt288x.c
@@ -33,7 +33,7 @@ static const u8 au_io_out_map[5] = {
 	[UART_MCR]	= 6,
 };
 
-static unsigned int au_serial_in(struct uart_port *p, int offset)
+static u32 au_serial_in(struct uart_port *p, unsigned int offset)
 {
 	if (offset >= ARRAY_SIZE(au_io_in_map))
 		return UINT_MAX;
@@ -42,7 +42,7 @@ static unsigned int au_serial_in(struct uart_port *p, int offset)
 	return __raw_readl(p->membase + (offset << p->regshift));
 }
 
-static void au_serial_out(struct uart_port *p, int offset, int value)
+static void au_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	if (offset >= ARRAY_SIZE(au_io_out_map))
 		return;
diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
index 4874a9632db3..e3db60bf50c9 100644
--- a/drivers/tty/serial/8250/8250_uniphier.c
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -63,7 +63,7 @@ OF_EARLYCON_DECLARE(uniphier, "socionext,uniphier-uart",
  * The register map is slightly different from that of 8250.
  * IO callbacks must be overridden for correct access to FCR, LCR, MCR and SCR.
  */
-static unsigned int uniphier_serial_in(struct uart_port *p, int offset)
+static u32 uniphier_serial_in(struct uart_port *p, unsigned int offset)
 {
 	unsigned int valshift = 0;
 
@@ -92,7 +92,7 @@ static unsigned int uniphier_serial_in(struct uart_port *p, int offset)
 	return (readl(p->membase + offset) >> valshift) & 0xff;
 }
 
-static void uniphier_serial_out(struct uart_port *p, int offset, int value)
+static void uniphier_serial_out(struct uart_port *p, unsigned int offset, u32 value)
 {
 	unsigned int valshift = 0;
 	bool normal = false;
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 144de7a7948d..01efdce0fda0 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -46,8 +46,8 @@ struct plat_serial8250_port {
 	unsigned int	type;		/* If UPF_FIXED_TYPE */
 	upf_t		flags;		/* UPF_* flags */
 	u16		bugs;		/* port bugs */
-	unsigned int	(*serial_in)(struct uart_port *, int);
-	void		(*serial_out)(struct uart_port *, int, int);
+	u32		(*serial_in)(struct uart_port *, unsigned int offset);
+	void		(*serial_out)(struct uart_port *, unsigned int offset, u32 val);
 	u32		(*dl_read)(struct uart_8250_port *up);
 	void		(*dl_write)(struct uart_8250_port *up, u32 value);
 	void		(*set_termios)(struct uart_port *,
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 914b5e97e056..d65b15449cfe 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -443,8 +443,8 @@ struct uart_port {
 	spinlock_t		lock;			/* port lock */
 	unsigned long		iobase;			/* in/out[bwl] */
 	unsigned char __iomem	*membase;		/* read/write[bwl] */
-	unsigned int		(*serial_in)(struct uart_port *, int);
-	void			(*serial_out)(struct uart_port *, int, int);
+	u32			(*serial_in)(struct uart_port *, unsigned int offset);
+	void			(*serial_out)(struct uart_port *, unsigned int offset, u32 val);
 	void			(*set_termios)(struct uart_port *,
 				               struct ktermios *new,
 				               const struct ktermios *old);
-- 
2.49.0

Re: [PATCH 08/33] serial: 8250: sanitize uart_port::serial_{in,out}() types

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 12:02:54PM +0200, Jiri Slaby (SUSE) wrote:
> uart_port::{serial_in,serial_out} (and plat_serial8250_port::* likewise)
> historically use:
> * 'unsigned int' for 32-bit register values in reads and writes, and
> * 'int' for offsets.
> 
> Make them sane such that:
> * 'u32' is used for register values, and
> * 'unsigned int' is used for offsets.
> 
> While at it, name hooks' parameters, so it is clear what is what.

At a glance this looks just mechanical change. Have you used coccinelle for
that?

As for the whole idea, I like it,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

...

> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Paul Cercueil <paul@crapouillou.net>
> Cc: Vladimir Zapolskiy <vz@mleia.com>
> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>

These also can be moved under '---' :-)

> ---
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-mips@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 08/33] serial: 8250: sanitize uart_port::serial_{in,out}() types

[Jiri Slaby]

On 11. 06. 25, 17:12, Andy Shevchenko wrote:
> On Wed, Jun 11, 2025 at 12:02:54PM +0200, Jiri Slaby (SUSE) wrote:
>> uart_port::{serial_in,serial_out} (and plat_serial8250_port::* likewise)
>> historically use:
>> * 'unsigned int' for 32-bit register values in reads and writes, and
>> * 'int' for offsets.
>>
>> Make them sane such that:
>> * 'u32' is used for register values, and
>> * 'unsigned int' is used for offsets.
>>
>> While at it, name hooks' parameters, so it is clear what is what.
> 
> At a glance this looks just mechanical change. Have you used coccinelle for
> that?

No, I haven't managed to learn that :P. But I used 
https://github.com/jirislaby/clang-struct.

thanks,
-- 
js
suse labs

Re: [PATCH 07/33] tty: vt: use _IO() to define ioctl numbers

[Christophe Leroy]

Hi Jiri,

Le 11/06/2025 à 12:02, Jiri Slaby (SUSE) a écrit :
> _IO*() is the proper way of defining ioctl numbers. All these vt numbers
> were synthetically built up the same way the _IO() macro does.
> 
> So instead of implicit hex numbers, use _IO() properly.
> 
> To not change the pre-existing numbers, use only _IO() (and not _IOR()
> or _IOW()). The latter would change the numbers indeed.

On powerpc your assumption is wrong, because _IOC_NONE is not 0:

$ git grep _IOC_NONE arch/powerpc/
arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE 1U

Therefore the value changes even with _IO(), leading to failure of Xorg 
as reported by Christian.

Christophe

> 
> Objdump of vt_ioctl.o reveals no difference with this patch.
> 
> Again, VT_GETCONSIZECSRPOS already uses _IOR(), so everything is paved
> for this patch.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> ---
>   include/uapi/linux/vt.h | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/include/uapi/linux/vt.h b/include/uapi/linux/vt.h
> index 714483d68c69..b60fcdfb2746 100644
> --- a/include/uapi/linux/vt.h
> +++ b/include/uapi/linux/vt.h
> @@ -14,9 +14,9 @@
>   		/* Note: the ioctl VT_GETSTATE does not work for
>   		   consoles 16 and higher (since it returns a short) */
>   
> -/* 0x56 is 'V', to avoid collision with termios and kd */
> +/* 'V' to avoid collision with termios and kd */
>   
> -#define VT_OPENQRY	0x5600	/* find available vt */
> +#define VT_OPENQRY		_IO('V', 0x00)	/* find available vt */
>   
>   struct vt_mode {
>   	__u8 mode;		/* vt mode */
> @@ -25,8 +25,8 @@ struct vt_mode {
>   	__s16 acqsig;		/* signal to raise on acquisition */
>   	__s16 frsig;		/* unused (set to 0) */
>   };
> -#define VT_GETMODE	0x5601	/* get mode of active vt */
> -#define VT_SETMODE	0x5602	/* set mode of active vt */
> +#define VT_GETMODE		_IO('V', 0x01)	/* get mode of active vt */
> +#define VT_SETMODE		_IO('V', 0x02)	/* set mode of active vt */
>   #define		VT_AUTO		0x00	/* auto vt switching */
>   #define		VT_PROCESS	0x01	/* process controls switching */
>   #define		VT_ACKACQ	0x02	/* acknowledge switch */
> @@ -36,21 +36,21 @@ struct vt_stat {
>   	__u16 v_signal;	/* signal to send */
>   	__u16 v_state;		/* vt bitmask */
>   };
> -#define VT_GETSTATE	0x5603	/* get global vt state info */
> -#define VT_SENDSIG	0x5604	/* signal to send to bitmask of vts */
> +#define VT_GETSTATE		_IO('V', 0x03)	/* get global vt state info */
> +#define VT_SENDSIG		_IO('V', 0x04)	/* signal to send to bitmask of vts */
>   
> -#define VT_RELDISP	0x5605	/* release display */
> +#define VT_RELDISP		_IO('V', 0x05)	/* release display */
>   
> -#define VT_ACTIVATE	0x5606	/* make vt active */
> -#define VT_WAITACTIVE	0x5607	/* wait for vt active */
> -#define VT_DISALLOCATE	0x5608  /* free memory associated to vt */
> +#define VT_ACTIVATE		_IO('V', 0x06)	/* make vt active */
> +#define VT_WAITACTIVE		_IO('V', 0x07)	/* wait for vt active */
> +#define VT_DISALLOCATE		_IO('V', 0x08)  /* free memory associated to vt */
>   
>   struct vt_sizes {
>   	__u16 v_rows;		/* number of rows */
>   	__u16 v_cols;		/* number of columns */
>   	__u16 v_scrollsize;	/* number of lines of scrollback */
>   };
> -#define VT_RESIZE	0x5609	/* set kernel's idea of screensize */
> +#define VT_RESIZE		_IO('V', 0x09)	/* set kernel's idea of screensize */
>   
>   struct vt_consize {
>   	__u16 v_rows;	/* number of rows */
> @@ -60,10 +60,10 @@ struct vt_consize {
>   	__u16 v_vcol;	/* number of pixel columns on screen */
>   	__u16 v_ccol;	/* number of pixel columns per character */
>   };
> -#define VT_RESIZEX      0x560A  /* set kernel's idea of screensize + more */
> -#define VT_LOCKSWITCH   0x560B  /* disallow vt switching */
> -#define VT_UNLOCKSWITCH 0x560C  /* allow vt switching */
> -#define VT_GETHIFONTMASK 0x560D  /* return hi font mask */
> +#define VT_RESIZEX		_IO('V', 0x0A)  /* set kernel's idea of screensize + more */
> +#define VT_LOCKSWITCH		_IO('V', 0x0B)  /* disallow vt switching */
> +#define VT_UNLOCKSWITCH		_IO('V', 0x0C)  /* allow vt switching */
> +#define VT_GETHIFONTMASK	_IO('V', 0x0D)  /* return hi font mask */
>   
>   struct vt_event {
>   	__u32 event;
> @@ -77,14 +77,14 @@ struct vt_event {
>   	__u32 pad[4];		/* Padding for expansion */
>   };
>   
> -#define VT_WAITEVENT	0x560E	/* Wait for an event */
> +#define VT_WAITEVENT		_IO('V', 0x0E)	/* Wait for an event */
>   
>   struct vt_setactivate {
>   	__u32 console;
>   	struct vt_mode mode;
>   };
>   
> -#define VT_SETACTIVATE	0x560F	/* Activate and set the mode of a console */
> +#define VT_SETACTIVATE		_IO('V', 0x0F)	/* Activate and set the mode of a console */
>   
>   /* get console size and cursor position */
>   struct vt_consizecsrpos {

Re: [PATCH 07/33] tty: vt: use _IO() to define ioctl numbers

[Christophe Leroy]

Le 31/07/2025 à 16:35, Christophe Leroy a écrit :
> Hi Jiri,
> 
> Le 11/06/2025 à 12:02, Jiri Slaby (SUSE) a écrit :
>> _IO*() is the proper way of defining ioctl numbers. All these vt numbers
>> were synthetically built up the same way the _IO() macro does.
>>
>> So instead of implicit hex numbers, use _IO() properly.
>>
>> To not change the pre-existing numbers, use only _IO() (and not _IOR()
>> or _IOW()). The latter would change the numbers indeed.
> 
> On powerpc your assumption is wrong, because _IOC_NONE is not 0:
> 
> $ git grep _IOC_NONE arch/powerpc/
> arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE 1U
> 
> Therefore the value changes even with _IO(), leading to failure of Xorg 
> as reported by Christian.
> 

And is likely an issue on the 4 following architectures:

$ git grep _IOC_NONE arch/ | grep 1U
arch/alpha/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
arch/mips/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
arch/sparc/include/uapi/asm/ioctl.h:#define _IOC_NONE        1U


Christophe

Re: [PATCH 07/33] tty: vt: use _IO() to define ioctl numbers

[Nicolas Pitre]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Thu, 31 Jul 2025, Christophe Leroy wrote:

> 
> 
> Le 31/07/2025 à 16:35, Christophe Leroy a écrit :
> > Hi Jiri,
> > 
> > Le 11/06/2025 à 12:02, Jiri Slaby (SUSE) a écrit :
> >> _IO*() is the proper way of defining ioctl numbers. All these vt numbers
> >> were synthetically built up the same way the _IO() macro does.
> >>
> >> So instead of implicit hex numbers, use _IO() properly.
> >>
> >> To not change the pre-existing numbers, use only _IO() (and not _IOR()
> >> or _IOW()). The latter would change the numbers indeed.
> > 
> > On powerpc your assumption is wrong, because _IOC_NONE is not 0:
> > 
> > $ git grep _IOC_NONE arch/powerpc/
> > arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE 1U
> > 
> > Therefore the value changes even with _IO(), leading to failure of Xorg as
> > reported by Christian.
> > 
> 
> And is likely an issue on the 4 following architectures:
> 
> $ git grep _IOC_NONE arch/ | grep 1U
> arch/alpha/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
> arch/mips/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
> arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
> arch/sparc/include/uapi/asm/ioctl.h:#define _IOC_NONE        1U

IMHO this one patch could simply be reverted and the "old" code let be.


Nicolas

Re: [PATCH 07/33] tty: vt: use _IO() to define ioctl numbers

[Jiri Slaby]

On 31. 07. 25, 22:58, Nicolas Pitre wrote:
> On Thu, 31 Jul 2025, Christophe Leroy wrote:
> 
>>
>>
>> Le 31/07/2025 à 16:35, Christophe Leroy a écrit :
>>> Hi Jiri,
>>>
>>> Le 11/06/2025 à 12:02, Jiri Slaby (SUSE) a écrit :
>>>> _IO*() is the proper way of defining ioctl numbers. All these vt numbers
>>>> were synthetically built up the same way the _IO() macro does.
>>>>
>>>> So instead of implicit hex numbers, use _IO() properly.
>>>>
>>>> To not change the pre-existing numbers, use only _IO() (and not _IOR()
>>>> or _IOW()). The latter would change the numbers indeed.
>>>
>>> On powerpc your assumption is wrong, because _IOC_NONE is not 0:
>>>
>>> $ git grep _IOC_NONE arch/powerpc/
>>> arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE 1U
>>>
>>> Therefore the value changes even with _IO(), leading to failure of Xorg as
>>> reported by Christian.
>>>
>>
>> And is likely an issue on the 4 following architectures:
>>
>> $ git grep _IOC_NONE arch/ | grep 1U
>> arch/alpha/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
>> arch/mips/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
>> arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
>> arch/sparc/include/uapi/asm/ioctl.h:#define _IOC_NONE        1U
> 
> IMHO this one patch could simply be reverted and the "old" code let be.

Oh, right -- it's easy to revert (no conflicts).

We could use _IOC(0, 'V', number, 0) directly, but I am not sure, that's 
worth it.

thanks,
-- 
js
suse labs

Re: [PATCH 07/33] tty: vt: use _IO() to define ioctl numbers

[Greg KH]

On Fri, Aug 01, 2025 at 06:47:46AM +0200, Jiri Slaby wrote:
> On 31. 07. 25, 22:58, Nicolas Pitre wrote:
> > On Thu, 31 Jul 2025, Christophe Leroy wrote:
> > 
> > > 
> > > 
> > > Le 31/07/2025 à 16:35, Christophe Leroy a écrit :
> > > > Hi Jiri,
> > > > 
> > > > Le 11/06/2025 à 12:02, Jiri Slaby (SUSE) a écrit :
> > > > > _IO*() is the proper way of defining ioctl numbers. All these vt numbers
> > > > > were synthetically built up the same way the _IO() macro does.
> > > > > 
> > > > > So instead of implicit hex numbers, use _IO() properly.
> > > > > 
> > > > > To not change the pre-existing numbers, use only _IO() (and not _IOR()
> > > > > or _IOW()). The latter would change the numbers indeed.
> > > > 
> > > > On powerpc your assumption is wrong, because _IOC_NONE is not 0:
> > > > 
> > > > $ git grep _IOC_NONE arch/powerpc/
> > > > arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE 1U
> > > > 
> > > > Therefore the value changes even with _IO(), leading to failure of Xorg as
> > > > reported by Christian.
> > > > 
> > > 
> > > And is likely an issue on the 4 following architectures:
> > > 
> > > $ git grep _IOC_NONE arch/ | grep 1U
> > > arch/alpha/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
> > > arch/mips/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
> > > arch/powerpc/include/uapi/asm/ioctl.h:#define _IOC_NONE	1U
> > > arch/sparc/include/uapi/asm/ioctl.h:#define _IOC_NONE        1U
> > 
> > IMHO this one patch could simply be reverted and the "old" code let be.
> 
> Oh, right -- it's easy to revert (no conflicts).
> 
> We could use _IOC(0, 'V', number, 0) directly, but I am not sure, that's
> worth it.

Great, can someone send me a revert?

thanks,

greg k-h