[PATCH 01/36] tty/vt: consolemap: use ARRAY_SIZE()

[PATCH 01/36] tty/vt: consolemap: use ARRAY_SIZE()

[Jiri Slaby]

The code uses constants as bounds in loops. Use ARRAY_SIZE() with
appropriate parameters instead. This makes the loop bounds obvious.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index d815ac98b39e..839d75d1a6c0 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -408,7 +408,7 @@ static void con_release_unimap(struct uni_pagedir *p)
 		}
 		p->uni_pgdir[i] = NULL;
 	}
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < ARRAY_SIZE(p->inverse_translations); i++) {
 		kfree(p->inverse_translations[i]);
 		p->inverse_translations[i] = NULL;
 	}
@@ -798,7 +798,7 @@ u32 conv_8bit_to_uni(unsigned char c)
 int conv_uni_to_8bit(u32 uni)
 {
 	int c;
-	for (c = 0; c < 0x100; c++)
+	for (c = 0; c < ARRAY_SIZE(translations[USER_MAP]); c++)
 		if (translations[USER_MAP][c] == uni ||
 		   (translations[USER_MAP][c] == (c | 0xf000) && uni == c))
 			return c;
-- 
2.36.1

[PATCH 02/36] tty/vt: consolemap: rename and document struct uni_pagedir

[Jiri Slaby]

struct uni_pagedir contains 32 unicode page directories, so the name of
the structure is a bit misleading. Rename the structure to uni_pagedict,
so it looks like this:
struct uni_pagedict
  -> 32 page dirs
     -> 32 rows
       -> 64 glyphs

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c    | 47 ++++++++++++++++++++--------------
 drivers/video/console/vgacon.c |  4 +--
 include/linux/console_struct.h |  6 ++---
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 839d75d1a6c0..5acafeea9afc 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -186,17 +186,26 @@ static unsigned short translations[][256] = {
 
 static int inv_translate[MAX_NR_CONSOLES];
 
-struct uni_pagedir {
-	u16 		**uni_pgdir[32];
+/**
+ * struct uni_pagedict -- unicode directory
+ *
+ * @uni_pgdir: 32*32*64 table with glyphs
+ * @refcount: reference count of this structure
+ * @sum: checksum
+ * @inverse_translations: best-effort inverse mapping
+ * @inverse_trans_unicode: best-effort inverse mapping to unicode
+ */
+struct uni_pagedict {
+	u16		**uni_pgdir[32];
 	unsigned long	refcount;
 	unsigned long	sum;
 	unsigned char	*inverse_translations[4];
 	u16		*inverse_trans_unicode;
 };
 
-static struct uni_pagedir *dflt;
+static struct uni_pagedict *dflt;
 
-static void set_inverse_transl(struct vc_data *conp, struct uni_pagedir *p, int i)
+static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p, int i)
 {
 	int j, glyph;
 	unsigned short *t = translations[i];
@@ -221,7 +230,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedir *p, int
 }
 
 static void set_inverse_trans_unicode(struct vc_data *conp,
-				      struct uni_pagedir *p)
+				      struct uni_pagedict *p)
 {
 	int i, j, k, glyph;
 	u16 **p1, *p2;
@@ -270,7 +279,7 @@ unsigned short *set_translate(int m, struct vc_data *vc)
  */
 u16 inverse_translate(const struct vc_data *conp, int glyph, int use_unicode)
 {
-	struct uni_pagedir *p;
+	struct uni_pagedict *p;
 	int m;
 	if (glyph < 0 || glyph >= MAX_GLYPH)
 		return 0;
@@ -297,7 +306,7 @@ EXPORT_SYMBOL_GPL(inverse_translate);
 static void update_user_maps(void)
 {
 	int i;
-	struct uni_pagedir *p, *q = NULL;
+	struct uni_pagedict *p, *q = NULL;
 	
 	for (i = 0; i < MAX_NR_CONSOLES; i++) {
 		if (!vc_cons_allocated(i))
@@ -393,7 +402,7 @@ int con_get_trans_new(ushort __user * arg)
 extern u8 dfont_unicount[];	/* Defined in console_defmap.c */
 extern u16 dfont_unitable[];
 
-static void con_release_unimap(struct uni_pagedir *p)
+static void con_release_unimap(struct uni_pagedict *p)
 {
 	u16 **p1;
 	int i, j;
@@ -419,7 +428,7 @@ static void con_release_unimap(struct uni_pagedir *p)
 /* Caller must hold the console lock */
 void con_free_unimap(struct vc_data *vc)
 {
-	struct uni_pagedir *p;
+	struct uni_pagedict *p;
 
 	p = *vc->vc_uni_pagedir_loc;
 	if (!p)
@@ -431,10 +440,10 @@ void con_free_unimap(struct vc_data *vc)
 	kfree(p);
 }
   
-static int con_unify_unimap(struct vc_data *conp, struct uni_pagedir *p)
+static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
 {
 	int i, j, k;
-	struct uni_pagedir *q;
+	struct uni_pagedict *q;
 	
 	for (i = 0; i < MAX_NR_CONSOLES; i++) {
 		if (!vc_cons_allocated(i))
@@ -472,7 +481,7 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedir *p)
 }
 
 static int
-con_insert_unipair(struct uni_pagedir *p, u_short unicode, u_short fontpos)
+con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 {
 	int i, n;
 	u16 **p1, *p2;
@@ -503,7 +512,7 @@ con_insert_unipair(struct uni_pagedir *p, u_short unicode, u_short fontpos)
 /* Caller must hold the lock */
 static int con_do_clear_unimap(struct vc_data *vc)
 {
-	struct uni_pagedir *p, *q;
+	struct uni_pagedict *p, *q;
 
 	p = *vc->vc_uni_pagedir_loc;
 	if (!p || --p->refcount) {
@@ -536,7 +545,7 @@ int con_clear_unimap(struct vc_data *vc)
 int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 {
 	int err = 0, err1, i;
-	struct uni_pagedir *p, *q;
+	struct uni_pagedict *p, *q;
 	struct unipair *unilist, *plist;
 
 	if (!ct)
@@ -569,7 +578,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 		
 		/*
 		 * Since refcount was > 1, con_clear_unimap() allocated a
-		 * a new uni_pagedir for this vc.  Re: p != q
+		 * a new uni_pagedict for this vc.  Re: p != q
 		 */
 		q = *vc->vc_uni_pagedir_loc;
 
@@ -660,7 +669,7 @@ int con_set_default_unimap(struct vc_data *vc)
 {
 	int i, j, err = 0, err1;
 	u16 *q;
-	struct uni_pagedir *p;
+	struct uni_pagedict *p;
 
 	if (dflt) {
 		p = *vc->vc_uni_pagedir_loc;
@@ -714,7 +723,7 @@ EXPORT_SYMBOL(con_set_default_unimap);
  */
 int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 {
-	struct uni_pagedir *q;
+	struct uni_pagedict *q;
 
 	if (!*src_vc->vc_uni_pagedir_loc)
 		return -EINVAL;
@@ -739,7 +748,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 	int i, j, k, ret = 0;
 	ushort ect;
 	u16 **p1, *p2;
-	struct uni_pagedir *p;
+	struct uni_pagedict *p;
 	struct unipair *unilist;
 
 	unilist = kvmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
@@ -810,7 +819,7 @@ conv_uni_to_pc(struct vc_data *conp, long ucs)
 {
 	int h;
 	u16 **p1, *p2;
-	struct uni_pagedir *p;
+	struct uni_pagedict *p;
   
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 576612f18d59..058a78b8dbcf 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -75,7 +75,7 @@ static void vgacon_scrolldelta(struct vc_data *c, int lines);
 static int vgacon_set_origin(struct vc_data *c);
 static void vgacon_save_screen(struct vc_data *c);
 static void vgacon_invert_region(struct vc_data *c, u16 * p, int count);
-static struct uni_pagedir *vgacon_uni_pagedir;
+static struct uni_pagedict *vgacon_uni_pagedir;
 static int vgacon_refcount;
 
 /* Description of the hardware situation */
@@ -342,7 +342,7 @@ static const char *vgacon_startup(void)
 
 static void vgacon_init(struct vc_data *c, int init)
 {
-	struct uni_pagedir *p;
+	struct uni_pagedict *p;
 
 	/*
 	 * We cannot be loaded as a module, therefore init will be 1
diff --git a/include/linux/console_struct.h b/include/linux/console_struct.h
index d5b9c8d40c18..f75033f0277f 100644
--- a/include/linux/console_struct.h
+++ b/include/linux/console_struct.h
@@ -17,7 +17,7 @@
 #include <linux/vt.h>
 #include <linux/workqueue.h>
 
-struct uni_pagedir;
+struct uni_pagedict;
 struct uni_screen;
 
 #define NPAR 16
@@ -157,8 +157,8 @@ struct vc_data {
 	unsigned int	vc_bell_duration;	/* Console bell duration */
 	unsigned short	vc_cur_blink_ms;	/* Cursor blink duration */
 	struct vc_data **vc_display_fg;		/* [!] Ptr to var holding fg console for this display */
-	struct uni_pagedir *vc_uni_pagedir;
-	struct uni_pagedir **vc_uni_pagedir_loc; /* [!] Location of uni_pagedir variable for this console */
+	struct uni_pagedict *vc_uni_pagedir;
+	struct uni_pagedict **vc_uni_pagedir_loc; /* [!] Location of uni_pagedict variable for this console */
 	struct uni_screen *vc_uni_screen;	/* unicode screen content */
 	/* additional information is in vt_kern.h */
 };
-- 
2.36.1

Re: [PATCH 02/36] tty/vt: consolemap: rename and document struct uni_pagedir

[Ilpo Järvinen]

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> struct uni_pagedir contains 32 unicode page directories, so the name of
> the structure is a bit misleading. Rename the structure to uni_pagedict,
> so it looks like this:
> struct uni_pagedict
>   -> 32 page dirs
>      -> 32 rows
>        -> 64 glyphs
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---

The rename looks incomplete:

>  drivers/tty/vt/consolemap.c    | 47 ++++++++++++++++++++--------------
>  drivers/video/console/vgacon.c |  4 +--
>  include/linux/console_struct.h |  6 ++---
>  3 files changed, 33 insertions(+), 24 deletions(-)

vs

$ git grep -l vc_uni_pagedir
drivers/tty/vt/consolemap.c
drivers/tty/vt/vt.c
drivers/usb/misc/sisusbvga/sisusb_con.c
drivers/video/console/vgacon.c
drivers/video/fbdev/core/fbcon.c
include/linux/console_struct.h
$

???


-- 
 i.

Re: [PATCH 02/36] tty/vt: consolemap: rename and document struct uni_pagedir

[Jiri Slaby]

On 07. 06. 22, 14:36, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
>> struct uni_pagedir contains 32 unicode page directories, so the name of
>> the structure is a bit misleading. Rename the structure to uni_pagedict,
>> so it looks like this:
>> struct uni_pagedict
>>    -> 32 page dirs
>>       -> 32 rows
>>         -> 64 glyphs
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> ---
> 
> The rename looks incomplete:
> 
>>   drivers/tty/vt/consolemap.c    | 47 ++++++++++++++++++++--------------
>>   drivers/video/console/vgacon.c |  4 +--
>>   include/linux/console_struct.h |  6 ++---
>>   3 files changed, 33 insertions(+), 24 deletions(-)
> 
> vs
> 
> $ git grep -l vc_uni_pagedir
> drivers/tty/vt/consolemap.c
> drivers/tty/vt/vt.c
> drivers/usb/misc/sisusbvga/sisusb_con.c
> drivers/video/console/vgacon.c
> drivers/video/fbdev/core/fbcon.c
> include/linux/console_struct.h

I renamed only the type, not the variables/members. Maybe the latter 
makes sense too. I will do that as a follow-up patch.

thanks,
-- 
js
suse labs

[PATCH 03/36] tty/vt: consolemap: define UNI_* macros for constants

[Jiri Slaby]

The code uses constants for sizes of dictionary substructures on many
places. Define 3 macros and use them in the code, so that loop bounds,
local variables and the dictionary always match. (And the loop bounds
are obvious now too.)

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 54 ++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 5acafeea9afc..15aa10ff87ad 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -186,6 +186,10 @@ static unsigned short translations[][256] = {
 
 static int inv_translate[MAX_NR_CONSOLES];
 
+#define UNI_DIRS	32U
+#define UNI_DIR_ROWS	32U
+#define UNI_ROW_GLYPHS	64U
+
 /**
  * struct uni_pagedict -- unicode directory
  *
@@ -196,7 +200,7 @@ static int inv_translate[MAX_NR_CONSOLES];
  * @inverse_trans_unicode: best-effort inverse mapping to unicode
  */
 struct uni_pagedict {
-	u16		**uni_pgdir[32];
+	u16		**uni_pgdir[UNI_DIRS];
 	unsigned long	refcount;
 	unsigned long	sum;
 	unsigned char	*inverse_translations[4];
@@ -246,15 +250,15 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 	}
 	memset(q, 0, MAX_GLYPH * sizeof(u16));
 
-	for (i = 0; i < 32; i++) {
+	for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
 		if (!p1)
 			continue;
-		for (j = 0; j < 32; j++) {
+		for (j = 0; j < UNI_DIR_ROWS; j++) {
 			p2 = p1[j];
 			if (!p2)
 				continue;
-			for (k = 0; k < 64; k++) {
+			for (k = 0; k < UNI_ROW_GLYPHS; k++) {
 				glyph = p2[k];
 				if (glyph >= 0 && glyph < MAX_GLYPH
 					       && q[glyph] < 32)
@@ -408,10 +412,10 @@ static void con_release_unimap(struct uni_pagedict *p)
 	int i, j;
 
 	if (p == dflt) dflt = NULL;  
-	for (i = 0; i < 32; i++) {
+	for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
 		if (p1 != NULL) {
-			for (j = 0; j < 32; j++)
+			for (j = 0; j < UNI_DIR_ROWS; j++)
 				kfree(p1[j]);
 			kfree(p1);
 		}
@@ -451,25 +455,26 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
 		q = *vc_cons[i].d->vc_uni_pagedir_loc;
 		if (!q || q == p || q->sum != p->sum)
 			continue;
-		for (j = 0; j < 32; j++) {
+		for (j = 0; j < UNI_DIRS; j++) {
 			u16 **p1, **q1;
 			p1 = p->uni_pgdir[j]; q1 = q->uni_pgdir[j];
 			if (!p1 && !q1)
 				continue;
 			if (!p1 || !q1)
 				break;
-			for (k = 0; k < 32; k++) {
+			for (k = 0; k < UNI_DIR_ROWS; k++) {
 				if (!p1[k] && !q1[k])
 					continue;
 				if (!p1[k] || !q1[k])
 					break;
-				if (memcmp(p1[k], q1[k], 64*sizeof(u16)))
+				if (memcmp(p1[k], q1[k],
+						UNI_ROW_GLYPHS * sizeof(u16)))
 					break;
 			}
-			if (k < 32)
+			if (k < UNI_DIR_ROWS)
 				break;
 		}
-		if (j == 32) {
+		if (j == UNI_DIRS) {
 			q->refcount++;
 			*conp->vc_uni_pagedir_loc = q;
 			con_release_unimap(p);
@@ -488,18 +493,19 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 
 	p1 = p->uni_pgdir[n = unicode >> 11];
 	if (!p1) {
-		p1 = p->uni_pgdir[n] = kmalloc_array(32, sizeof(u16 *),
-						     GFP_KERNEL);
+		p1 = p->uni_pgdir[n] = kmalloc_array(UNI_DIR_ROWS,
+						     sizeof(u16 *), GFP_KERNEL);
 		if (!p1) return -ENOMEM;
-		for (i = 0; i < 32; i++)
+		for (i = 0; i < UNI_DIR_ROWS; i++)
 			p1[i] = NULL;
 	}
 
 	p2 = p1[n = (unicode >> 6) & 0x1f];
 	if (!p2) {
-		p2 = p1[n] = kmalloc_array(64, sizeof(u16), GFP_KERNEL);
+		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
 		if (!p2) return -ENOMEM;
-		memset(p2, 0xff, 64*sizeof(u16)); /* No glyphs for the characters (yet) */
+		/* No glyphs for the characters (yet) */
+		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
 	}
 
 	p2[unicode & 0x3f] = fontpos;
@@ -589,13 +595,13 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 		 * entries from "p" (old) to "q" (new).
 		 */
 		l = 0;		/* unicode value */
-		for (i = 0; i < 32; i++) {
+		for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
 		if (p1)
-			for (j = 0; j < 32; j++) {
+			for (j = 0; j < UNI_DIR_ROWS; j++) {
 			p2 = p1[j];
 			if (p2) {
-				for (k = 0; k < 64; k++, l++)
+				for (k = 0; k < UNI_ROW_GLYPHS; k++, l++)
 				if (p2[k] != 0xffff) {
 					/*
 					 * Found one, copy entry for unicode
@@ -613,12 +619,12 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 				}
 			} else {
 				/* Account for row of 64 empty entries */
-				l += 64;
+				l += UNI_ROW_GLYPHS;
 			}
 		}
 		else
 			/* Account for empty table */
-			l += 32 * 64;
+			l += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
 		}
 
 		/*
@@ -760,13 +766,13 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 	ect = 0;
 	if (*vc->vc_uni_pagedir_loc) {
 		p = *vc->vc_uni_pagedir_loc;
-		for (i = 0; i < 32; i++) {
+		for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
 		if (p1)
-			for (j = 0; j < 32; j++) {
+			for (j = 0; j < UNI_DIR_ROWS; j++) {
 			p2 = *(p1++);
 			if (p2)
-				for (k = 0; k < 64; k++, p2++) {
+				for (k = 0; k < UNI_ROW_GLYPHS; k++, p2++) {
 					if (*p2 >= MAX_GLYPH)
 						continue;
 					if (ect < ct) {
-- 
2.36.1

Re: [PATCH 03/36] tty/vt: consolemap: define UNI_* macros for constants

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The code uses constants for sizes of dictionary substructures on many
> places. Define 3 macros and use them in the code, so that loop bounds,
> local variables and the dictionary always match. (And the loop bounds
> are obvious now too.)
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

> -			for (k = 0; k < 64; k++) {
> +			for (k = 0; k < UNI_ROW_GLYPHS; k++) {
>  				glyph = p2[k];
>  				if (glyph >= 0 && glyph < MAX_GLYPH
>  					       && q[glyph] < 32)

Probably unrelated to this change but what's that < 32? It seems to appear 
twice related to the inverse mapping (and you didn't end up naming it).


-- 
 i.

Re: [PATCH 03/36] tty/vt: consolemap: define UNI_* macros for constants

[Jiri Slaby]

On 07. 06. 22, 15:21, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
>> The code uses constants for sizes of dictionary substructures on many
>> places. Define 3 macros and use them in the code, so that loop bounds,
>> local variables and the dictionary always match. (And the loop bounds
>> are obvious now too.)
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
>> -			for (k = 0; k < 64; k++) {
>> +			for (k = 0; k < UNI_ROW_GLYPHS; k++) {
>>   				glyph = p2[k];
>>   				if (glyph >= 0 && glyph < MAX_GLYPH
>>   					       && q[glyph] < 32)
> 
> Probably unrelated to this change but what's that < 32? It seems to appear
> twice related to the inverse mapping (and you didn't end up naming it).

That's ascii C0 test _IMO_.

thanks,
-- 
js
suse labs

Re: [PATCH 03/36] tty/vt: consolemap: define UNI_* macros for constants

[Ilpo Järvinen]

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

On Wed, 8 Jun 2022, Jiri Slaby wrote:

> On 07. 06. 22, 15:21, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> > 
> > > The code uses constants for sizes of dictionary substructures on many
> > > places. Define 3 macros and use them in the code, so that loop bounds,
> > > local variables and the dictionary always match. (And the loop bounds
> > > are obvious now too.)
> > > 
> > > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > > -			for (k = 0; k < 64; k++) {
> > > +			for (k = 0; k < UNI_ROW_GLYPHS; k++) {
> > >   				glyph = p2[k];
> > >   				if (glyph >= 0 && glyph < MAX_GLYPH
> > >   					       && q[glyph] < 32)
> > 
> > Probably unrelated to this change but what's that < 32? It seems to appear
> > twice related to the inverse mapping (and you didn't end up naming it).
> 
> That's ascii C0 test _IMO_.

Ok, that's what I though it must be. But then this code seems even more 
odd to me, why it sets the inverse again but only if it's a control 
character...

Maybe this gives some hint why but it's unintelligible to me:
			/* prefer '-' above SHY etc. */
Both that quoted - and soft hyphen (SHY) are >= 32. So it kind of matches 
to what the if does, but it completely fails to explain why preference is 
the different way around with the control chars.

Btw, for set_inverse_transl(), q -> inv rename would still be useful on 
top of the other variable renames.

-- 
 i.

[PATCH 04/36] tty/vt: consolemap: decrypt inverse_translate()

[Jiri Slaby]

Fix invalid indentation and demystify the code by removing superfluous
"else"s. The "else"s are unneeded as they always follow an "if"-true
branch containing a "return". The code is now way more readable.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 15aa10ff87ad..fb61158f4dc6 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -285,25 +285,26 @@ u16 inverse_translate(const struct vc_data *conp, int glyph, int use_unicode)
 {
 	struct uni_pagedict *p;
 	int m;
+
 	if (glyph < 0 || glyph >= MAX_GLYPH)
 		return 0;
-	else {
-		p = *conp->vc_uni_pagedir_loc;
-		if (!p)
+
+	p = *conp->vc_uni_pagedir_loc;
+	if (!p)
+		return glyph;
+
+	if (use_unicode) {
+		if (!p->inverse_trans_unicode)
 			return glyph;
-		else if (use_unicode) {
-			if (!p->inverse_trans_unicode)
-				return glyph;
-			else
-				return p->inverse_trans_unicode[glyph];
-			} else {
-			m = inv_translate[conp->vc_num];
-			if (!p->inverse_translations[m])
-				return glyph;
-			else
-				return p->inverse_translations[m][glyph];
-			}
+
+		return p->inverse_trans_unicode[glyph];
 	}
+
+	m = inv_translate[conp->vc_num];
+	if (!p->inverse_translations[m])
+		return glyph;
+
+	return p->inverse_translations[m][glyph];
 }
 EXPORT_SYMBOL_GPL(inverse_translate);
 
-- 
2.36.1

Re: [PATCH 04/36] tty/vt: consolemap: decrypt inverse_translate()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> Fix invalid indentation and demystify the code by removing superfluous
> "else"s. The "else"s are unneeded as they always follow an "if"-true
> branch containing a "return". The code is now way more readable.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

-- 
 i.

[PATCH 05/36] tty/vt: consolemap: remove extern from function decls

[Jiri Slaby]

The extern keyword is not needed for function declarations. Remove it,
so that the consolemap header conforms to other tty headers.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 include/linux/consolemap.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/linux/consolemap.h b/include/linux/consolemap.h
index bcfce748c9d8..abc28e4450bf 100644
--- a/include/linux/consolemap.h
+++ b/include/linux/consolemap.h
@@ -17,12 +17,11 @@
 #ifdef CONFIG_CONSOLE_TRANSLATIONS
 struct vc_data;
 
-extern u16 inverse_translate(const struct vc_data *conp, int glyph,
-		int use_unicode);
-extern unsigned short *set_translate(int m, struct vc_data *vc);
-extern int conv_uni_to_pc(struct vc_data *conp, long ucs);
-extern u32 conv_8bit_to_uni(unsigned char c);
-extern int conv_uni_to_8bit(u32 uni);
+u16 inverse_translate(const struct vc_data *conp, int glyph, int use_unicode);
+unsigned short *set_translate(int m, struct vc_data *vc);
+int conv_uni_to_pc(struct vc_data *conp, long ucs);
+u32 conv_8bit_to_uni(unsigned char c);
+int conv_uni_to_8bit(u32 uni);
 void console_map_init(void);
 #else
 #define inverse_translate(conp, glyph, uni) ((uint16_t)glyph)
-- 
2.36.1

Re: [PATCH 05/36] tty/vt: consolemap: remove extern from function decls

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The extern keyword is not needed for function declarations. Remove it,
> so that the consolemap header conforms to other tty headers.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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


-- 
 i.

[PATCH 06/36] tty/vt: consolemap: convert macros to static inlines

[Jiri Slaby]

This commit changes !CONFIG_CONSOLE_TRANSLATIONS definitions to real
(inline) functions. So the commit:
* makes type checking much stronger,
* removes the need of many parentheses and casts, and
* makes the code more readable.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 include/linux/consolemap.h | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/include/linux/consolemap.h b/include/linux/consolemap.h
index abc28e4450bf..98171dbed51f 100644
--- a/include/linux/consolemap.h
+++ b/include/linux/consolemap.h
@@ -14,9 +14,9 @@
 
 #include <linux/types.h>
 
-#ifdef CONFIG_CONSOLE_TRANSLATIONS
 struct vc_data;
 
+#ifdef CONFIG_CONSOLE_TRANSLATIONS
 u16 inverse_translate(const struct vc_data *conp, int glyph, int use_unicode);
 unsigned short *set_translate(int m, struct vc_data *vc);
 int conv_uni_to_pc(struct vc_data *conp, long ucs);
@@ -24,12 +24,33 @@ u32 conv_8bit_to_uni(unsigned char c);
 int conv_uni_to_8bit(u32 uni);
 void console_map_init(void);
 #else
-#define inverse_translate(conp, glyph, uni) ((uint16_t)glyph)
-#define set_translate(m, vc) ((unsigned short *)NULL)
-#define conv_uni_to_pc(conp, ucs) ((int) (ucs > 0xff ? -1: ucs))
-#define conv_8bit_to_uni(c) ((uint32_t)(c))
-#define conv_uni_to_8bit(c) ((int) ((c) & 0xff))
-#define console_map_init(c) do { ; } while (0)
+static inline u16 inverse_translate(const struct vc_data *conp, int glyph,
+		int use_unicode)
+{
+	return glyph;
+}
+
+static inline unsigned short *set_translate(int m, struct vc_data *vc)
+{
+	return NULL;
+}
+
+static inline int conv_uni_to_pc(struct vc_data *conp, long ucs)
+{
+	return ucs > 0xff ? -1 : ucs;
+}
+
+static inline u32 conv_8bit_to_uni(unsigned char c)
+{
+	return c;
+}
+
+static inline int conv_uni_to_8bit(u32 uni)
+{
+	return uni & 0xff;
+}
+
+static inline void console_map_init(void) { }
 #endif /* CONFIG_CONSOLE_TRANSLATIONS */
 
 #endif /* __LINUX_CONSOLEMAP_H__ */
-- 
2.36.1

Re: [PATCH 06/36] tty/vt: consolemap: convert macros to static inlines

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> This commit changes !CONFIG_CONSOLE_TRANSLATIONS definitions to real
> (inline) functions. So the commit:
> * makes type checking much stronger,
> * removes the need of many parentheses and casts, and
> * makes the code more readable.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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


-- 
 i.

[PATCH 07/36] tty/vt: consolemap: make parameters of inverse_translate() saner

[Jiri Slaby]

- int use_unicode -> bool: it's used as bool at some places already, so
  make it explicit.
- int glyph -> u16: every caller passes a u16 in. So make it explicit
  too. And remove a negative check from inverse_translate() as it never
  could be negative.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/accessibility/braille/braille_console.c | 2 +-
 drivers/accessibility/speakup/main.c            | 2 +-
 drivers/tty/vt/consolemap.c                     | 4 ++--
 drivers/tty/vt/selection.c                      | 3 ++-
 drivers/tty/vt/vt.c                             | 2 +-
 include/linux/consolemap.h                      | 6 +++---
 6 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
index fdc6b593f500..c4d54a5326b1 100644
--- a/drivers/accessibility/braille/braille_console.c
+++ b/drivers/accessibility/braille/braille_console.c
@@ -131,7 +131,7 @@ static void vc_refresh(struct vc_data *vc)
 	for (i = 0; i < WIDTH; i++) {
 		u16 glyph = screen_glyph(vc,
 				2 * (vc_x + i) + vc_y * vc->vc_size_row);
-		buf[i] = inverse_translate(vc, glyph, 1);
+		buf[i] = inverse_translate(vc, glyph, true);
 	}
 	braille_write(buf);
 }
diff --git a/drivers/accessibility/speakup/main.c b/drivers/accessibility/speakup/main.c
index d726537fa16c..f52265293482 100644
--- a/drivers/accessibility/speakup/main.c
+++ b/drivers/accessibility/speakup/main.c
@@ -470,7 +470,7 @@ static u16 get_char(struct vc_data *vc, u16 *pos, u_char *attribs)
 			c |= 0x100;
 		}
 
-		ch = inverse_translate(vc, c, 1);
+		ch = inverse_translate(vc, c, true);
 		*attribs = (w & 0xff00) >> 8;
 	}
 	return ch;
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index fb61158f4dc6..157c7f936294 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -281,12 +281,12 @@ unsigned short *set_translate(int m, struct vc_data *vc)
  *    was active.
  * Still, it is now possible to a certain extent to cut and paste non-ASCII.
  */
-u16 inverse_translate(const struct vc_data *conp, int glyph, int use_unicode)
+u16 inverse_translate(const struct vc_data *conp, u16 glyph, bool use_unicode)
 {
 	struct uni_pagedict *p;
 	int m;
 
-	if (glyph < 0 || glyph >= MAX_GLYPH)
+	if (glyph >= MAX_GLYPH)
 		return 0;
 
 	p = *conp->vc_uni_pagedir_loc;
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index f7755e73696e..6ef22f01cc51 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -68,7 +68,8 @@ sel_pos(int n, bool unicode)
 {
 	if (unicode)
 		return screen_glyph_unicode(vc_sel.cons, n / 2);
-	return inverse_translate(vc_sel.cons, screen_glyph(vc_sel.cons, n), 0);
+	return inverse_translate(vc_sel.cons, screen_glyph(vc_sel.cons, n),
+			false);
 }
 
 /**
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index f8c87c4d7399..1ea1c11c42fd 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4741,7 +4741,7 @@ u32 screen_glyph_unicode(const struct vc_data *vc, int n)
 
 	if (uniscr)
 		return uniscr->lines[n / vc->vc_cols][n % vc->vc_cols];
-	return inverse_translate(vc, screen_glyph(vc, n * 2), 1);
+	return inverse_translate(vc, screen_glyph(vc, n * 2), true);
 }
 EXPORT_SYMBOL_GPL(screen_glyph_unicode);
 
diff --git a/include/linux/consolemap.h b/include/linux/consolemap.h
index 98171dbed51f..1ff2bf55eb85 100644
--- a/include/linux/consolemap.h
+++ b/include/linux/consolemap.h
@@ -17,15 +17,15 @@
 struct vc_data;
 
 #ifdef CONFIG_CONSOLE_TRANSLATIONS
-u16 inverse_translate(const struct vc_data *conp, int glyph, int use_unicode);
+u16 inverse_translate(const struct vc_data *conp, u16 glyph, bool use_unicode);
 unsigned short *set_translate(int m, struct vc_data *vc);
 int conv_uni_to_pc(struct vc_data *conp, long ucs);
 u32 conv_8bit_to_uni(unsigned char c);
 int conv_uni_to_8bit(u32 uni);
 void console_map_init(void);
 #else
-static inline u16 inverse_translate(const struct vc_data *conp, int glyph,
-		int use_unicode)
+static inline u16 inverse_translate(const struct vc_data *conp, u16 glyph,
+		bool use_unicode)
 {
 	return glyph;
 }
-- 
2.36.1

Re: [PATCH 07/36] tty/vt: consolemap: make parameters of inverse_translate() saner

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> - int use_unicode -> bool: it's used as bool at some places already, so
>   make it explicit.
> - int glyph -> u16: every caller passes a u16 in. So make it explicit
>   too. And remove a negative check from inverse_translate() as it never
>   could be negative.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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


-- 
 i.

[PATCH 08/36] tty/vt: consolemap: one line = one statement

[Jiri Slaby]

Some lines combine more statements on one line. This makes the code hard
to follow. Do it properly in the "one line = one statement" fashion.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 157c7f936294..f97081e01b71 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -215,12 +215,14 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p, int
 	unsigned short *t = translations[i];
 	unsigned char *q;
 	
-	if (!p) return;
+	if (!p)
+		return;
 	q = p->inverse_translations[i];
 
 	if (!q) {
 		q = p->inverse_translations[i] = kmalloc(MAX_GLYPH, GFP_KERNEL);
-		if (!q) return;
+		if (!q)
+			return;
 	}
 	memset(q, 0, MAX_GLYPH);
 
@@ -240,7 +242,8 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 	u16 **p1, *p2;
 	u16 *q;
 
-	if (!p) return;
+	if (!p)
+		return;
 	q = p->inverse_trans_unicode;
 	if (!q) {
 		q = p->inverse_trans_unicode =
@@ -412,7 +415,8 @@ static void con_release_unimap(struct uni_pagedict *p)
 	u16 **p1;
 	int i, j;
 
-	if (p == dflt) dflt = NULL;  
+	if (p == dflt)
+		dflt = NULL;
 	for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
 		if (p1 != NULL) {
@@ -458,7 +462,8 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
 			continue;
 		for (j = 0; j < UNI_DIRS; j++) {
 			u16 **p1, **q1;
-			p1 = p->uni_pgdir[j]; q1 = q->uni_pgdir[j];
+			p1 = p->uni_pgdir[j];
+			q1 = q->uni_pgdir[j];
 			if (!p1 && !q1)
 				continue;
 			if (!p1 || !q1)
@@ -492,19 +497,23 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 	int i, n;
 	u16 **p1, *p2;
 
-	p1 = p->uni_pgdir[n = unicode >> 11];
+	n = unicode >> 11;
+	p1 = p->uni_pgdir[n];
 	if (!p1) {
 		p1 = p->uni_pgdir[n] = kmalloc_array(UNI_DIR_ROWS,
 						     sizeof(u16 *), GFP_KERNEL);
-		if (!p1) return -ENOMEM;
+		if (!p1)
+			return -ENOMEM;
 		for (i = 0; i < UNI_DIR_ROWS; i++)
 			p1[i] = NULL;
 	}
 
-	p2 = p1[n = (unicode >> 6) & 0x1f];
+	n = (unicode >> 6) & 0x1f;
+	p2 = p1[n];
 	if (!p2) {
 		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
-		if (!p2) return -ENOMEM;
+		if (!p2)
+			return -ENOMEM;
 		/* No glyphs for the characters (yet) */
 		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
 	}
@@ -532,7 +541,8 @@ static int con_do_clear_unimap(struct vc_data *vc)
 		q->refcount=1;
 		*vc->vc_uni_pagedir_loc = q;
 	} else {
-		if (p == dflt) dflt = NULL;
+		if (p == dflt)
+			dflt = NULL;
 		p->refcount++;
 		p->sum = 0;
 		con_release_unimap(p);
-- 
2.36.1

Re: [PATCH 08/36] tty/vt: consolemap: one line = one statement

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> Some lines combine more statements on one line. This makes the code hard
> to follow. Do it properly in the "one line = one statement" fashion.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---

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


-- 
 i.

[PATCH 09/36] tty/vt: consolemap: use | for binary addition

[Jiri Slaby]

Unicode letters are composed as a bit shifts and sums of three values.
Use "|" and not "+" for these bit operations. The former is indeed more
appropriate.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index f97081e01b71..016c1a0b4290 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -265,7 +265,7 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 				glyph = p2[k];
 				if (glyph >= 0 && glyph < MAX_GLYPH
 					       && q[glyph] < 32)
-		  			q[glyph] = (i << 11) + (j << 6) + k;
+					q[glyph] = (i << 11) | (j << 6) | k;
 			}
 		}
 	}
@@ -788,7 +788,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 						continue;
 					if (ect < ct) {
 						unilist[ect].unicode =
-							(i<<11)+(j<<6)+k;
+							(i<<11) | (j<<6) | k;
 						unilist[ect].fontpos = *p2;
 					}
 					ect++;
-- 
2.36.1

Re: [PATCH 09/36] tty/vt: consolemap: use | for binary addition

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> Unicode letters are composed as a bit shifts and sums of three values.
> Use "|" and not "+" for these bit operations. The former is indeed more
> appropriate.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

> ---
>  drivers/tty/vt/consolemap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index f97081e01b71..016c1a0b4290 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -265,7 +265,7 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
>  				glyph = p2[k];
>  				if (glyph >= 0 && glyph < MAX_GLYPH
>  					       && q[glyph] < 32)
> -		  			q[glyph] = (i << 11) + (j << 6) + k;
> +					q[glyph] = (i << 11) | (j << 6) | k;
>  			}
>  		}
>  	}
> @@ -788,7 +788,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
>  						continue;
>  					if (ect < ct) {
>  						unilist[ect].unicode =
> -							(i<<11)+(j<<6)+k;
> +							(i<<11) | (j<<6) | k;

I'd have added also the spaces around <<.

-- 
 i.

Re: [PATCH 09/36] tty/vt: consolemap: use | for binary addition

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Ilpo Järvinen wrote:

> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
> > @@ -788,7 +788,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
> >  						continue;
> >  					if (ect < ct) {
> >  						unilist[ect].unicode =
> > -							(i<<11)+(j<<6)+k;
> > +							(i<<11) | (j<<6) | k;
> 
> I'd have added also the spaces around <<.

Ah, nevermind. I see the line gets changed later on to something entirely 
different.

-- 
 i.

[PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Jiri Slaby]

The code currently does shift, OR, and AND logic directly in the code.
It is not much obvious what happens there. Therefore define four macros
for that purpose and use them in the code. We use GENMASK() so that it
is clear which bits serve what purpose:
- UNI_GLYPH: bits  0.. 5
- UNI_ROW:   bits  6..10
- UNI_DIR:   bits 11..31

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 016c1a0b4290..e5fd225e87bd 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -190,6 +190,11 @@ static int inv_translate[MAX_NR_CONSOLES];
 #define UNI_DIR_ROWS	32U
 #define UNI_ROW_GLYPHS	64U
 
+#define UNI_DIR(uni)		( (uni)                   >> 11)
+#define UNI_ROW(uni)		(((uni) & GENMASK(10, 6)) >>  6)
+#define UNI_GLYPH(uni)		( (uni) & GENMASK( 5, 0))
+#define UNI(dir, row, glyph)	(((dir) << 11) | ((row) << 6) | (glyph))
+
 /**
  * struct uni_pagedict -- unicode directory
  *
@@ -265,7 +270,7 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 				glyph = p2[k];
 				if (glyph >= 0 && glyph < MAX_GLYPH
 					       && q[glyph] < 32)
-					q[glyph] = (i << 11) | (j << 6) | k;
+					q[glyph] = UNI(i, j, k);
 			}
 		}
 	}
@@ -497,7 +502,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 	int i, n;
 	u16 **p1, *p2;
 
-	n = unicode >> 11;
+	n = UNI_DIR(unicode);
 	p1 = p->uni_pgdir[n];
 	if (!p1) {
 		p1 = p->uni_pgdir[n] = kmalloc_array(UNI_DIR_ROWS,
@@ -508,7 +513,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 			p1[i] = NULL;
 	}
 
-	n = (unicode >> 6) & 0x1f;
+	n = UNI_ROW(unicode);
 	p2 = p1[n];
 	if (!p2) {
 		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
@@ -518,7 +523,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
 	}
 
-	p2[unicode & 0x3f] = fontpos;
+	p2[UNI_GLYPH(unicode)] = fontpos;
 	
 	p->sum += (fontpos << 20U) + unicode;
 
@@ -788,7 +793,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 						continue;
 					if (ect < ct) {
 						unilist[ect].unicode =
-							(i<<11) | (j<<6) | k;
+							UNI(i, j, k);
 						unilist[ect].fontpos = *p2;
 					}
 					ect++;
@@ -857,9 +862,9 @@ conv_uni_to_pc(struct vc_data *conp, long ucs)
 		return -3;
 
 	p = *conp->vc_uni_pagedir_loc;
-	if ((p1 = p->uni_pgdir[ucs >> 11]) &&
-	    (p2 = p1[(ucs >> 6) & 0x1f]) &&
-	    (h = p2[ucs & 0x3f]) < MAX_GLYPH)
+	if ((p1 = p->uni_pgdir[UNI_DIR(ucs)]) &&
+	    (p2 = p1[UNI_ROW(ucs)]) &&
+	    (h = p2[UNI_GLYPH(ucs)]) < MAX_GLYPH)
 		return h;
 
 	return -4;		/* not found */
-- 
2.36.1

Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Ilpo Järvinen]

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The code currently does shift, OR, and AND logic directly in the code.
> It is not much obvious what happens there. Therefore define four macros
> for that purpose and use them in the code. We use GENMASK() so that it
> is clear which bits serve what purpose:
> - UNI_GLYPH: bits  0.. 5
> - UNI_ROW:   bits  6..10
> - UNI_DIR:   bits 11..31
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  drivers/tty/vt/consolemap.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 016c1a0b4290..e5fd225e87bd 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -190,6 +190,11 @@ static int inv_translate[MAX_NR_CONSOLES];
>  #define UNI_DIR_ROWS	32U
>  #define UNI_ROW_GLYPHS	64U
>  
> +#define UNI_DIR(uni)		( (uni)                   >> 11)
> +#define UNI_ROW(uni)		(((uni) & GENMASK(10, 6)) >>  6)

This is opencoding what FIELD_GET() does. Maybe just define these as 
masks and use FIELD_GET in the code below.

> +#define UNI_GLYPH(uni)		( (uni) & GENMASK( 5, 0))
> +#define UNI(dir, row, glyph)	(((dir) << 11) | ((row) << 6) | (glyph))
>
>  /**
>   * struct uni_pagedict -- unicode directory
>   *
> @@ -265,7 +270,7 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
>  				glyph = p2[k];
>  				if (glyph >= 0 && glyph < MAX_GLYPH
>  					       && q[glyph] < 32)
> -					q[glyph] = (i << 11) | (j << 6) | k;
> +					q[glyph] = UNI(i, j, k);
>  			}
>  		}
>  	}
> @@ -497,7 +502,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
>  	int i, n;
>  	u16 **p1, *p2;
>  
> -	n = unicode >> 11;
> +	n = UNI_DIR(unicode);
>  	p1 = p->uni_pgdir[n];
>  	if (!p1) {
>  		p1 = p->uni_pgdir[n] = kmalloc_array(UNI_DIR_ROWS,
> @@ -508,7 +513,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
>  			p1[i] = NULL;
>  	}
>  
> -	n = (unicode >> 6) & 0x1f;
> +	n = UNI_ROW(unicode);
>  	p2 = p1[n];
>  	if (!p2) {
>  		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
> @@ -518,7 +523,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
>  		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
>  	}
>  
> -	p2[unicode & 0x3f] = fontpos;
> +	p2[UNI_GLYPH(unicode)] = fontpos;
>  	
>  	p->sum += (fontpos << 20U) + unicode;
>  
> @@ -788,7 +793,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
>  						continue;
>  					if (ect < ct) {
>  						unilist[ect].unicode =
> -							(i<<11) | (j<<6) | k;
> +							UNI(i, j, k);
>  						unilist[ect].fontpos = *p2;
>  					}
>  					ect++;
> @@ -857,9 +862,9 @@ conv_uni_to_pc(struct vc_data *conp, long ucs)
>  		return -3;
>  
>  	p = *conp->vc_uni_pagedir_loc;
> -	if ((p1 = p->uni_pgdir[ucs >> 11]) &&
> -	    (p2 = p1[(ucs >> 6) & 0x1f]) &&
> -	    (h = p2[ucs & 0x3f]) < MAX_GLYPH)
> +	if ((p1 = p->uni_pgdir[UNI_DIR(ucs)]) &&
> +	    (p2 = p1[UNI_ROW(ucs)]) &&
> +	    (h = p2[UNI_GLYPH(ucs)]) < MAX_GLYPH)
>  		return h;
>  
>  	return -4;		/* not found */
> 

-- 
 i.

Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Jiri Slaby]

On 07. 06. 22, 15:47, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
>> The code currently does shift, OR, and AND logic directly in the code.
>> It is not much obvious what happens there. Therefore define four macros
>> for that purpose and use them in the code. We use GENMASK() so that it
>> is clear which bits serve what purpose:
>> - UNI_GLYPH: bits  0.. 5
>> - UNI_ROW:   bits  6..10
>> - UNI_DIR:   bits 11..31
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> ---
>>   drivers/tty/vt/consolemap.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 016c1a0b4290..e5fd225e87bd 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -190,6 +190,11 @@ static int inv_translate[MAX_NR_CONSOLES];
>>   #define UNI_DIR_ROWS	32U
>>   #define UNI_ROW_GLYPHS	64U
>>   
>> +#define UNI_DIR(uni)		( (uni)                   >> 11)
>> +#define UNI_ROW(uni)		(((uni) & GENMASK(10, 6)) >>  6)
> 
> This is opencoding what FIELD_GET() does. Maybe just define these as
> masks and use FIELD_GET in the code below.

Ah, great -- I was thinking there should be something for that purpose 
already, but didn't find this. But let's define these UNI_* macros using 
appropriate FIELD_GET(). (And not using FIELD_GET() in the code.)

>> +#define UNI_GLYPH(uni)		( (uni) & GENMASK( 5, 0))
thanks,
-- 
js
suse labs

Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Jiri Slaby]

On 08. 06. 22, 8:59, Jiri Slaby wrote:
> On 07. 06. 22, 15:47, Ilpo Järvinen wrote:
>> On Tue, 7 Jun 2022, Jiri Slaby wrote:
>>
>>> The code currently does shift, OR, and AND logic directly in the code.
>>> It is not much obvious what happens there. Therefore define four macros
>>> for that purpose and use them in the code. We use GENMASK() so that it
>>> is clear which bits serve what purpose:
>>> - UNI_GLYPH: bits  0.. 5
>>> - UNI_ROW:   bits  6..10
>>> - UNI_DIR:   bits 11..31
>>>
>>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>>> ---
>>>   drivers/tty/vt/consolemap.c | 21 +++++++++++++--------
>>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>>> index 016c1a0b4290..e5fd225e87bd 100644
>>> --- a/drivers/tty/vt/consolemap.c
>>> +++ b/drivers/tty/vt/consolemap.c
>>> @@ -190,6 +190,11 @@ static int inv_translate[MAX_NR_CONSOLES];
>>>   #define UNI_DIR_ROWS    32U
>>>   #define UNI_ROW_GLYPHS    64U
>>> +#define UNI_DIR(uni)        ( (uni)                   >> 11)
>>> +#define UNI_ROW(uni)        (((uni) & GENMASK(10, 6)) >>  6)
>>
>> This is opencoding what FIELD_GET() does. Maybe just define these as
>> masks and use FIELD_GET in the code below.
> 
> Ah, great -- I was thinking there should be something for that purpose 
> already, but didn't find this. But let's define these UNI_* macros using 
> appropriate FIELD_GET(). (And not using FIELD_GET() in the code.)
> 
>>> +#define UNI_GLYPH(uni)        ( (uni) & GENMASK( 5, 0))
> thanks,

JFYI, I ended up with this diff to the original approach:
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -23,6 +23,8 @@
   * stack overflow.
   */

+#include <linux/bitfield.h>
+#include <linux/bits.h>
  #include <linux/module.h>
  #include <linux/kd.h>
  #include <linux/errno.h>
@@ -190,10 +192,17 @@ static int inv_translate[MAX_NR_CONSOLES];
  #define UNI_DIR_ROWS   32U
  #define UNI_ROW_GLYPHS 64U

-#define UNI_DIR(uni)           ( (uni)                   >> 11)
-#define UNI_ROW(uni)           (((uni) & GENMASK(10, 6)) >>  6)
-#define UNI_GLYPH(uni)         ( (uni) & GENMASK( 5, 0))
-#define UNI(dir, row, glyph)   (((dir) << 11) | ((row) << 6) | (glyph))
+#define UNI_DIR_BITS(max)      GENMASK((max), 11)
+#define UNI_ROW_BITS           GENMASK(10,  6)
+#define UNI_GLYPH_BITS         GENMASK( 5,  0)
+
+#define UNI_DIR(uni)   FIELD_GET(UNI_DIR_BITS(sizeof(uni) * 8 - 1), (uni))
+#define UNI_ROW(uni)   FIELD_GET(UNI_ROW_BITS, (uni))
+#define UNI_GLYPH(uni) FIELD_GET(UNI_GLYPH_BITS, (uni))
+
+#define UNI(dir, row, glyph)   (FIELD_PREP(UNI_DIR_BITS(31), (dir)) | \
+                                FIELD_PREP(UNI_ROW_BITS, (row)) | \
+                                FIELD_PREP(UNI_GLYPH_BITS, (glyph)))

  /**
   * struct uni_pagedict -- unicode directory

=======================================================

More text, but easier to follow, I think. except the UNI_DIR_BITS() has 
to have a parameter, otherwise compilation raises a too-big value 
warning with use of UNI_DIR() in con_insert_unipair() where uni is only 
of ushort type. Alternatively, we can cast uni to u32, but that produces 
worse assembly (extensions to u32 here and there).

thanks,
-- 
js
suse labs

Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Ilpo Järvinen]

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

On Wed, 8 Jun 2022, Jiri Slaby wrote:

> On 08. 06. 22, 8:59, Jiri Slaby wrote:
> > On 07. 06. 22, 15:47, Ilpo Järvinen wrote:
> > > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> > > 
> > > > The code currently does shift, OR, and AND logic directly in the code.
> > > > It is not much obvious what happens there. Therefore define four macros
> > > > for that purpose and use them in the code. We use GENMASK() so that it
> > > > is clear which bits serve what purpose:
> > > > - UNI_GLYPH: bits  0.. 5
> > > > - UNI_ROW:   bits  6..10
> > > > - UNI_DIR:   bits 11..31
> > > > 
> > > > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > > > ---
> > > >   drivers/tty/vt/consolemap.c | 21 +++++++++++++--------
> > > >   1 file changed, 13 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> > > > index 016c1a0b4290..e5fd225e87bd 100644
> > > > --- a/drivers/tty/vt/consolemap.c
> > > > +++ b/drivers/tty/vt/consolemap.c
> > > > @@ -190,6 +190,11 @@ static int inv_translate[MAX_NR_CONSOLES];
> > > >   #define UNI_DIR_ROWS    32U
> > > >   #define UNI_ROW_GLYPHS    64U
> > > > +#define UNI_DIR(uni)        ( (uni)                   >> 11)
> > > > +#define UNI_ROW(uni)        (((uni) & GENMASK(10, 6)) >>  6)
> > > 
> > > This is opencoding what FIELD_GET() does. Maybe just define these as
> > > masks and use FIELD_GET in the code below.
> > 
> > Ah, great -- I was thinking there should be something for that purpose
> > already, but didn't find this. But let's define these UNI_* macros using
> > appropriate FIELD_GET(). (And not using FIELD_GET() in the code.)
> > 
> > > > +#define UNI_GLYPH(uni)        ( (uni) & GENMASK( 5, 0))
> > thanks,
> 
> JFYI, I ended up with this diff to the original approach:
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -23,6 +23,8 @@
>   * stack overflow.
>   */
> 
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/module.h>
>  #include <linux/kd.h>
>  #include <linux/errno.h>
> @@ -190,10 +192,17 @@ static int inv_translate[MAX_NR_CONSOLES];
>  #define UNI_DIR_ROWS   32U
>  #define UNI_ROW_GLYPHS 64U
> 
> -#define UNI_DIR(uni)           ( (uni)                   >> 11)
> -#define UNI_ROW(uni)           (((uni) & GENMASK(10, 6)) >>  6)
> -#define UNI_GLYPH(uni)         ( (uni) & GENMASK( 5, 0))
> -#define UNI(dir, row, glyph)   (((dir) << 11) | ((row) << 6) | (glyph))
> +#define UNI_DIR_BITS(max)      GENMASK((max), 11)
> +#define UNI_ROW_BITS           GENMASK(10,  6)
> +#define UNI_GLYPH_BITS         GENMASK( 5,  0)
> +
> +#define UNI_DIR(uni)   FIELD_GET(UNI_DIR_BITS(sizeof(uni) * 8 - 1), (uni))

That would be * BITS_PER_BYTE. But see below.

> +#define UNI_ROW(uni)   FIELD_GET(UNI_ROW_BITS, (uni))
> +#define UNI_GLYPH(uni) FIELD_GET(UNI_GLYPH_BITS, (uni))
> +
> +#define UNI(dir, row, glyph)   (FIELD_PREP(UNI_DIR_BITS(31), (dir)) | \
> +                                FIELD_PREP(UNI_ROW_BITS, (row)) | \
> +                                FIELD_PREP(UNI_GLYPH_BITS, (glyph)))
> 
>  /**
>   * struct uni_pagedict -- unicode directory
> 
> =======================================================
> 
> More text, but easier to follow, I think. except the UNI_DIR_BITS() has to
> have a parameter, otherwise compilation raises a too-big value warning with
> use of UNI_DIR() in con_insert_unipair() where uni is only of ushort type.

It doesn't raise any warnings if I do:

#define UNI_DIR_BITS           GENMASK(15, 11)

As UNI_DIRS is 32 it cannot ever be larger than that?

> Alternatively, we can cast uni to u32, but that produces worse assembly
> (extensions to u32 here and there).

-- 
 i.

Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Jiri Slaby]

On 08. 06. 22, 10:02, Ilpo Järvinen wrote:
> It doesn't raise any warnings if I do:
> 
> #define UNI_DIR_BITS           GENMASK(15, 11)
> 
> As UNI_DIRS is 32 it cannot ever be larger than that?

Right, conv_uni_to_pc() properly checks:
   if (ucs > 0xffff)
       return -4;
before
   dir = dict->uni_pgdir[UNI_DIR(ucs)];


Even better!

I also noted to my TODO to check why ucs is "long" there. It makes no 
sense at all. Be it negative, or long-sized. IMO, it should simply be u32.

There is so much crap in the code :/...

thanks,
-- 
js
suse labs

[PATCH 11/36] tty/vt: consolemap: zero uni_pgdir using kcalloc()

[Jiri Slaby]

The newly allocated p->uni_pgdir[n] is initialized to NULLs right after
a kmalloc_array() allocation. Combine these two using kcalloc().

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index e5fd225e87bd..097ab7d01f8b 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -499,18 +499,16 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
 static int
 con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 {
-	int i, n;
+	int n;
 	u16 **p1, *p2;
 
 	n = UNI_DIR(unicode);
 	p1 = p->uni_pgdir[n];
 	if (!p1) {
-		p1 = p->uni_pgdir[n] = kmalloc_array(UNI_DIR_ROWS,
-						     sizeof(u16 *), GFP_KERNEL);
+		p1 = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(u16 *),
+				GFP_KERNEL);
 		if (!p1)
 			return -ENOMEM;
-		for (i = 0; i < UNI_DIR_ROWS; i++)
-			p1[i] = NULL;
 	}
 
 	n = UNI_ROW(unicode);
-- 
2.36.1

Re: [PATCH 11/36] tty/vt: consolemap: zero uni_pgdir using kcalloc()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The newly allocated p->uni_pgdir[n] is initialized to NULLs right after
> a kmalloc_array() allocation. Combine these two using kcalloc().
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

-- 
 i.

[PATCH 12/36] tty/vt: consolemap: use sizeof(*pointer) instead of sizeof(type)

[Jiri Slaby]

It is preferred to use sizeof(*pointer) instead of sizeof(type). First,
the type of the variable can change and one needs not change the former
(unlike the latter). Second, the latter is error-prone due to (u16),
(u16 *), and (u16 **) mixture here.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 097ab7d01f8b..79a62dcca046 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -251,12 +251,12 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 		return;
 	q = p->inverse_trans_unicode;
 	if (!q) {
-		q = p->inverse_trans_unicode =
-			kmalloc_array(MAX_GLYPH, sizeof(u16), GFP_KERNEL);
+		q = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
+				sizeof(*q), GFP_KERNEL);
 		if (!q)
 			return;
 	}
-	memset(q, 0, MAX_GLYPH * sizeof(u16));
+	memset(q, 0, MAX_GLYPH * sizeof(*q));
 
 	for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
@@ -478,8 +478,8 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
 					continue;
 				if (!p1[k] || !q1[k])
 					break;
-				if (memcmp(p1[k], q1[k],
-						UNI_ROW_GLYPHS * sizeof(u16)))
+				if (memcmp(p1[k], q1[k], UNI_ROW_GLYPHS *
+							sizeof(*p1[k])))
 					break;
 			}
 			if (k < UNI_DIR_ROWS)
@@ -505,7 +505,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 	n = UNI_DIR(unicode);
 	p1 = p->uni_pgdir[n];
 	if (!p1) {
-		p1 = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(u16 *),
+		p1 = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(*p1),
 				GFP_KERNEL);
 		if (!p1)
 			return -ENOMEM;
@@ -514,11 +514,12 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 	n = UNI_ROW(unicode);
 	p2 = p1[n];
 	if (!p2) {
-		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
+		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(*p2),
+				GFP_KERNEL);
 		if (!p2)
 			return -ENOMEM;
 		/* No glyphs for the characters (yet) */
-		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
+		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(*p2));
 	}
 
 	p2[UNI_GLYPH(unicode)] = fontpos;
@@ -571,7 +572,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	if (!ct)
 		return 0;
 
-	unilist = vmemdup_user(list, array_size(sizeof(struct unipair), ct));
+	unilist = vmemdup_user(list, array_size(sizeof(*unilist), ct));
 	if (IS_ERR(unilist))
 		return PTR_ERR(unilist);
 
@@ -771,7 +772,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 	struct uni_pagedict *p;
 	struct unipair *unilist;
 
-	unilist = kvmalloc_array(ct, sizeof(struct unipair), GFP_KERNEL);
+	unilist = kvmalloc_array(ct, sizeof(*unilist), GFP_KERNEL);
 	if (!unilist)
 		return -ENOMEM;
 
@@ -800,7 +801,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 		}
 	}
 	console_unlock();
-	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(struct unipair)))
+	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
 		ret = -EFAULT;
 	put_user(ect, uct);
 	kvfree(unilist);
-- 
2.36.1

Re: [PATCH 12/36] tty/vt: consolemap: use sizeof(*pointer) instead of sizeof(type)

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> It is preferred to use sizeof(*pointer) instead of sizeof(type). First,
> the type of the variable can change and one needs not change the former
> (unlike the latter). Second, the latter is error-prone due to (u16),
> (u16 *), and (u16 **) mixture here.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

This seems fine but see the comments below which are not directly related 
to the change itself.

> ---
>  drivers/tty/vt/consolemap.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 097ab7d01f8b..79a62dcca046 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -251,12 +251,12 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
>  		return;
>  	q = p->inverse_trans_unicode;
>  	if (!q) {
> -		q = p->inverse_trans_unicode =
> -			kmalloc_array(MAX_GLYPH, sizeof(u16), GFP_KERNEL);
> +		q = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
> +				sizeof(*q), GFP_KERNEL);
>  		if (!q)
>  			return;
>  	}
> -	memset(q, 0, MAX_GLYPH * sizeof(u16));
> +	memset(q, 0, MAX_GLYPH * sizeof(*q));

Convert kmalloc_array into kcalloc and place memset() into else branch?

>  	for (i = 0; i < UNI_DIRS; i++) {
>  		p1 = p->uni_pgdir[i];
> @@ -478,8 +478,8 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
>  					continue;
>  				if (!p1[k] || !q1[k])
>  					break;
> -				if (memcmp(p1[k], q1[k],
> -						UNI_ROW_GLYPHS * sizeof(u16)))
> +				if (memcmp(p1[k], q1[k], UNI_ROW_GLYPHS *
> +							sizeof(*p1[k])))
>  					break;
>  			}
>  			if (k < UNI_DIR_ROWS)
> @@ -505,7 +505,7 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
>  	n = UNI_DIR(unicode);
>  	p1 = p->uni_pgdir[n];
>  	if (!p1) {
> -		p1 = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(u16 *),
> +		p1 = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(*p1),
>  				GFP_KERNEL);
>  		if (!p1)
>  			return -ENOMEM;
> @@ -514,11 +514,12 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
>  	n = UNI_ROW(unicode);
>  	p2 = p1[n];
>  	if (!p2) {
> -		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
> +		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(*p2),
> +				GFP_KERNEL);
>  		if (!p2)
>  			return -ENOMEM;
>  		/* No glyphs for the characters (yet) */
> -		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
> +		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(*p2));

This could have been kcalloc'ed.


-- 
 i.

Re: [PATCH 12/36] tty/vt: consolemap: use sizeof(*pointer) instead of sizeof(type)

[Jiri Slaby]

On 07. 06. 22, 16:00, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
>> It is preferred to use sizeof(*pointer) instead of sizeof(type). First,
>> the type of the variable can change and one needs not change the former
>> (unlike the latter). Second, the latter is error-prone due to (u16),
>> (u16 *), and (u16 **) mixture here.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> This seems fine but see the comments below which are not directly related
> to the change itself.
> 
>> ---
>>   drivers/tty/vt/consolemap.c | 23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 097ab7d01f8b..79a62dcca046 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -251,12 +251,12 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
>>   		return;
>>   	q = p->inverse_trans_unicode;
>>   	if (!q) {
>> -		q = p->inverse_trans_unicode =
>> -			kmalloc_array(MAX_GLYPH, sizeof(u16), GFP_KERNEL);
>> +		q = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
>> +				sizeof(*q), GFP_KERNEL);
>>   		if (!q)
>>   			return;
>>   	}
>> -	memset(q, 0, MAX_GLYPH * sizeof(u16));
>> +	memset(q, 0, MAX_GLYPH * sizeof(*q));
> 
> Convert kmalloc_array into kcalloc and place memset() into else branch?

IMO, the way it is now is more obvious.

>> @@ -514,11 +514,12 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
>>   	n = UNI_ROW(unicode);
>>   	p2 = p1[n];
>>   	if (!p2) {
>> -		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16), GFP_KERNEL);
>> +		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(*p2),
>> +				GFP_KERNEL);
>>   		if (!p2)
>>   			return -ENOMEM;
>>   		/* No glyphs for the characters (yet) */
>> -		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
>> +		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(*p2));
> 
> This could have been kcalloc'ed.

Why would you zero it before setting it to 0xff?

thanks,
-- 
js
suse labs

Re: [PATCH 12/36] tty/vt: consolemap: use sizeof(*pointer) instead of sizeof(type)

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> On 07. 06. 22, 16:00, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> > 
> > > It is preferred to use sizeof(*pointer) instead of sizeof(type). First,
> > > the type of the variable can change and one needs not change the former
> > > (unlike the latter). Second, the latter is error-prone due to (u16),
> > > (u16 *), and (u16 **) mixture here.
> > > 
> > > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > This seems fine but see the comments below which are not directly related
> > to the change itself.
> > 
> > > ---
> > >   drivers/tty/vt/consolemap.c | 23 ++++++++++++-----------
> > >   1 file changed, 12 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> > > index 097ab7d01f8b..79a62dcca046 100644
> > > --- a/drivers/tty/vt/consolemap.c
> > > +++ b/drivers/tty/vt/consolemap.c
> > > @@ -251,12 +251,12 @@ static void set_inverse_trans_unicode(struct vc_data
> > > *conp,
> > >   		return;
> > >   	q = p->inverse_trans_unicode;
> > >   	if (!q) {
> > > -		q = p->inverse_trans_unicode =
> > > -			kmalloc_array(MAX_GLYPH, sizeof(u16), GFP_KERNEL);
> > > +		q = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
> > > +				sizeof(*q), GFP_KERNEL);
> > >   		if (!q)
> > >   			return;
> > >   	}
> > > -	memset(q, 0, MAX_GLYPH * sizeof(u16));
> > > +	memset(q, 0, MAX_GLYPH * sizeof(*q));
> > 
> > Convert kmalloc_array into kcalloc and place memset() into else branch?
> 
> IMO, the way it is now is more obvious.

Fair enough.

> > > @@ -514,11 +514,12 @@ con_insert_unipair(struct uni_pagedict *p, u_short
> > > unicode, u_short fontpos)
> > >   	n = UNI_ROW(unicode);
> > >   	p2 = p1[n];
> > >   	if (!p2) {
> > > -		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(u16),
> > > GFP_KERNEL);
> > > +		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(*p2),
> > > +				GFP_KERNEL);
> > >   		if (!p2)
> > >   			return -ENOMEM;
> > >   		/* No glyphs for the characters (yet) */
> > > -		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(u16));
> > > +		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(*p2));
> > 
> > This could have been kcalloc'ed.
> 
> Why would you zero it before setting it to 0xff?

Yes, nevermind that. I obviously completely missed the fact it wasn't 
memsetting to zero.

-- 
 i.

[PATCH 13/36] tty/vt: consolemap: make con_set_unimap() more readable

[Jiri Slaby]

The indentation was completely broken in con_set_unimap(). Reorder the
code using 'if (!cond) continue;'s so that the code makes sense. Not
that it is perfect now, but it can be followed at least. More cleanup to
come. And remove all those useless whitespaces at the EOLs too.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 42 ++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 79a62dcca046..3730a1c0f223 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -580,23 +580,21 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 
 	/* Save original vc_unipagdir_loc in case we allocate a new one */
 	p = *vc->vc_uni_pagedir_loc;
-
 	if (!p) {
 		err = -EINVAL;
-
 		goto out_unlock;
 	}
-	
+
 	if (p->refcount > 1) {
 		int j, k;
 		u16 **p1, *p2, l;
-		
+
 		err1 = con_do_clear_unimap(vc);
 		if (err1) {
 			err = err1;
 			goto out_unlock;
 		}
-		
+
 		/*
 		 * Since refcount was > 1, con_clear_unimap() allocated a
 		 * a new uni_pagedict for this vc.  Re: p != q
@@ -611,13 +609,26 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 		 */
 		l = 0;		/* unicode value */
 		for (i = 0; i < UNI_DIRS; i++) {
-		p1 = p->uni_pgdir[i];
-		if (p1)
+			p1 = p->uni_pgdir[i];
+			if (!p1) {
+				/* Account for empty table */
+				l += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
+				continue;
+			}
+
 			for (j = 0; j < UNI_DIR_ROWS; j++) {
-			p2 = p1[j];
-			if (p2) {
-				for (k = 0; k < UNI_ROW_GLYPHS; k++, l++)
-				if (p2[k] != 0xffff) {
+				p2 = p1[j];
+				if (!p2) {
+					/*
+					 * Account for row of 64 empty entries
+					 */
+					l += UNI_ROW_GLYPHS;
+					continue;
+				}
+
+				for (k = 0; k < UNI_ROW_GLYPHS; k++, l++) {
+					if (p2[k] == 0xffff)
+						continue;
 					/*
 					 * Found one, copy entry for unicode
 					 * l with fontpos value p2[k].
@@ -632,15 +643,8 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 						goto out_unlock;
 					}
 				}
-			} else {
-				/* Account for row of 64 empty entries */
-				l += UNI_ROW_GLYPHS;
 			}
 		}
-		else
-			/* Account for empty table */
-			l += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
-		}
 
 		/*
 		 * Finished copying font table, set vc_uni_pagedir to new table
@@ -658,7 +662,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 		if (err1)
 			err = err1;
 	}
-	
+
 	/*
 	 * Merge with fontmaps of any other virtual consoles.
 	 */
-- 
2.36.1

Re: [PATCH 13/36] tty/vt: consolemap: make con_set_unimap() more readable

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The indentation was completely broken in con_set_unimap(). Reorder the
> code using 'if (!cond) continue;'s so that the code makes sense. Not
> that it is perfect now, but it can be followed at least. More cleanup to
> come. And remove all those useless whitespaces at the EOLs too.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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


-- 
 i.

[PATCH 14/36] tty/vt: consolemap: make con_get_unimap() more readable

[Jiri Slaby]

The indentation is completely broken in con_get_unimap(). Reorder the
code using "if (!cond) continue;"s so that the code makes sense. Switch
also the "p" assignment and add a short path using goto. This makes the
code readable again.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 38 +++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 3730a1c0f223..84c8043a36d0 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -768,7 +768,8 @@ EXPORT_SYMBOL(con_copy_unimap);
  *	Read the console unicode data for this console. Called from the ioctl
  *	handlers.
  */
-int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct unipair __user *list)
+int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
+		struct unipair __user *list)
 {
 	int i, j, k, ret = 0;
 	ushort ect;
@@ -783,27 +784,32 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct, struct uni
 	console_lock();
 
 	ect = 0;
-	if (*vc->vc_uni_pagedir_loc) {
-		p = *vc->vc_uni_pagedir_loc;
-		for (i = 0; i < UNI_DIRS; i++) {
+	p = *vc->vc_uni_pagedir_loc;
+	if (!p)
+		goto unlock;
+
+	for (i = 0; i < UNI_DIRS; i++) {
 		p1 = p->uni_pgdir[i];
-		if (p1)
-			for (j = 0; j < UNI_DIR_ROWS; j++) {
+		if (!p1)
+			continue;
+
+		for (j = 0; j < UNI_DIR_ROWS; j++) {
 			p2 = *(p1++);
-			if (p2)
-				for (k = 0; k < UNI_ROW_GLYPHS; k++, p2++) {
-					if (*p2 >= MAX_GLYPH)
-						continue;
-					if (ect < ct) {
-						unilist[ect].unicode =
-							UNI(i, j, k);
-						unilist[ect].fontpos = *p2;
-					}
-					ect++;
+			if (!p2)
+				continue;
+
+			for (k = 0; k < UNI_ROW_GLYPHS; k++, p2++) {
+				if (*p2 >= MAX_GLYPH)
+					continue;
+				if (ect < ct) {
+					unilist[ect].unicode = UNI(i, j, k);
+					unilist[ect].fontpos = *p2;
 				}
+				ect++;
 			}
 		}
 	}
+unlock:
 	console_unlock();
 	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
 		ret = -EFAULT;
-- 
2.36.1

Re: [PATCH 14/36] tty/vt: consolemap: make con_get_unimap() more readable

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The indentation is completely broken in con_get_unimap(). Reorder the
> code using "if (!cond) continue;"s so that the code makes sense. Switch
> also the "p" assignment and add a short path using goto. This makes the
> code readable again.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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


-- 
 i.

[PATCH 15/36] tty/vt: consolemap: make p1 increment less confusing in con_get_unimap()

[Jiri Slaby]

p2 is already incremented like this few lines below, so do the same for
p1. This makes the code easier to follow.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 84c8043a36d0..831450f2bfd1 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -793,8 +793,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
 		if (!p1)
 			continue;
 
-		for (j = 0; j < UNI_DIR_ROWS; j++) {
-			p2 = *(p1++);
+		for (j = 0; j < UNI_DIR_ROWS; j++, p1++) {
+			p2 = *p1;
 			if (!p2)
 				continue;
 
-- 
2.36.1

[PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Jiri Slaby]

Only the return value of copy_to_user() is checked in con_get_unimap().
Do the same for put_user() of the count too.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 831450f2bfd1..92b5dddb00d9 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
 	console_unlock();
 	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
 		ret = -EFAULT;
-	put_user(ect, uct);
+	if (put_user(ect, uct))
+		ret = -EFAULT;
 	kvfree(unilist);
 	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
 }
-- 
2.36.1

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Ilpo Järvinen]

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> Only the return value of copy_to_user() is checked in con_get_unimap().
> Do the same for put_user() of the count too.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  drivers/tty/vt/consolemap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 831450f2bfd1..92b5dddb00d9 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
>  	console_unlock();
>  	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>  		ret = -EFAULT;
> -	put_user(ect, uct);
> +	if (put_user(ect, uct))
> +		ret = -EFAULT;
>  	kvfree(unilist);
>  	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>  }
> 

Doesn't this fix something?

-- 
 i.

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Jiri Slaby]

On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
>> Only the return value of copy_to_user() is checked in con_get_unimap().
>> Do the same for put_user() of the count too.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> ---
>>   drivers/tty/vt/consolemap.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 831450f2bfd1..92b5dddb00d9 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
>>   	console_unlock();
>>   	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>>   		ret = -EFAULT;
>> -	put_user(ect, uct);
>> +	if (put_user(ect, uct))
>> +		ret = -EFAULT;
>>   	kvfree(unilist);
>>   	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>>   }
>>
> 
> Doesn't this fix something?

If you mean a Fixes tag, this is pre-git.

If you mean a bug, well, likely yes, users now get informed. But I don't 
think anyone cares ;). But who knows, maybe we will start seeing 
userspace failures now (as they might not provide writable count field 
-- unlikely). That's one of the reasons why I did this as a separate 
commit. Let's see if are going to revert this or not...

thanks,
-- 
js
suse labs

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Ilpo Järvinen]

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

On Wed, 8 Jun 2022, Jiri Slaby wrote:

> On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> > 
> > > Only the return value of copy_to_user() is checked in con_get_unimap().
> > > Do the same for put_user() of the count too.
> > > 
> > > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > > ---
> > >   drivers/tty/vt/consolemap.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> > > index 831450f2bfd1..92b5dddb00d9 100644
> > > --- a/drivers/tty/vt/consolemap.c
> > > +++ b/drivers/tty/vt/consolemap.c
> > > @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct,
> > > ushort __user *uct,
> > >   	console_unlock();
> > >   	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
> > >   		ret = -EFAULT;
> > > -	put_user(ect, uct);
> > > +	if (put_user(ect, uct))
> > > +		ret = -EFAULT;
> > >   	kvfree(unilist);
> > >   	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
> > >   }
> > > 
> > 
> > Doesn't this fix something?
> 
> If you mean a Fixes tag, this is pre-git.
> 
> If you mean a bug, well, likely yes, users now get informed. But I don't think
> anyone cares ;).

Yes, I meant Fixes tag but I guess it's not important.

> But who knows, maybe we will start seeing userspace failures
> now (as they might not provide writable count field -- unlikely). That's one
> of the reasons why I did this as a separate commit. Let's see if are going to
> revert this or not...

Ok.

-- 
 i.

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Andy Shevchenko]

On Wed, Jun 8, 2022 at 10:56 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> > On Tue, 7 Jun 2022, Jiri Slaby wrote:

...

> > Doesn't this fix something?
>
> If you mean a Fixes tag, this is pre-git.

You may use history.git [1] for pregit SHAs.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Greg Kroah-Hartman]

On Wed, Jun 08, 2022 at 12:38:49PM +0200, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 10:56 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> > On 07. 06. 22, 16:19, Ilpo Järvinen wrote:
> > > On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
> ...
> 
> > > Doesn't this fix something?
> >
> > If you mean a Fixes tag, this is pre-git.
> 
> You may use history.git [1] for pregit SHAs.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/

Please don't, it just does not matter.

thanks,

greg k-h

RE: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[David Laight]

From: Jiri Slaby
> Sent: 07 June 2022 11:49
> 
> Only the return value of copy_to_user() is checked in con_get_unimap().
> Do the same for put_user() of the count too.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  drivers/tty/vt/consolemap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 831450f2bfd1..92b5dddb00d9 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
>  	console_unlock();
>  	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>  		ret = -EFAULT;
> -	put_user(ect, uct);
> +	if (put_user(ect, uct))
> +		ret = -EFAULT;
>  	kvfree(unilist);
>  	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>  }

How is the user expected to check the result of this code?

AFAICT -ENOMEM is returned if either kmalloc() fails or
the user buffer is too short?
Looks pretty hard to detect which.

I've not looked at the effect of all the patches, but setting
'ret = -ENOMEM' and breaking the loop when the array is too
small would simplify things.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Jiri Slaby]

On 08. 06. 22, 10:02, David Laight wrote:
> From: Jiri Slaby
>> Sent: 07 June 2022 11:49
>>
>> Only the return value of copy_to_user() is checked in con_get_unimap().
>> Do the same for put_user() of the count too.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> ---
>>   drivers/tty/vt/consolemap.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 831450f2bfd1..92b5dddb00d9 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
>>   	console_unlock();
>>   	if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>>   		ret = -EFAULT;
>> -	put_user(ect, uct);
>> +	if (put_user(ect, uct))
>> +		ret = -EFAULT;
>>   	kvfree(unilist);
>>   	return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>>   }
> 
> How is the user expected to check the result of this code?
> 
> AFAICT -ENOMEM is returned if either kmalloc() fails or
> the user buffer is too short?
> Looks pretty hard to detect which.

Agreed. The code is far from perfect. We might try to return ENOSPC and 
watch what breaks. (And decouple the double "?:" operator as it makes 
things only worse.)

> I've not looked at the effect of all the patches, but setting
> 'ret = -ENOMEM' and breaking the loop when the array is too
> small would simplify things.

Note that the patches try NOT to change the behavior in any way. If they 
do, it's likely a bug. They are first front cleanup. Definitely more to 
come. Either from me, or others -- patches welcome ;).

thanks,
-- 
js
suse labs

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Jiri Slaby]

On 08. 06. 22, 10:11, Jiri Slaby wrote:
> Note that the patches try NOT to change the behavior in any way. If they 
> do, it's likely a bug. They are first front cleanup. Definitely more to 

Ah, in English, it's "first line".

-- 
js
suse labs

Re: [PATCH 16/36] tty/vt: consolemap: check put_user() in con_get_unimap()

[Jiri Slaby]

On 08. 06. 22, 10:11, Jiri Slaby wrote:
> On 08. 06. 22, 10:02, David Laight wrote:
>> From: Jiri Slaby
>>> Sent: 07 June 2022 11:49
>>>
>>> Only the return value of copy_to_user() is checked in con_get_unimap().
>>> Do the same for put_user() of the count too.
>>>
>>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>>> ---
>>>   drivers/tty/vt/consolemap.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>>> index 831450f2bfd1..92b5dddb00d9 100644
>>> --- a/drivers/tty/vt/consolemap.c
>>> +++ b/drivers/tty/vt/consolemap.c
>>> @@ -813,7 +813,8 @@ int con_get_unimap(struct vc_data *vc, ushort ct, 
>>> ushort __user *uct,
>>>       console_unlock();
>>>       if (copy_to_user(list, unilist, min(ect, ct) * sizeof(*unilist)))
>>>           ret = -EFAULT;
>>> -    put_user(ect, uct);
>>> +    if (put_user(ect, uct))
>>> +        ret = -EFAULT;
>>>       kvfree(unilist);
>>>       return ret ? ret : (ect <= ct) ? 0 : -ENOMEM;
>>>   }
>>
>> How is the user expected to check the result of this code?
>>
>> AFAICT -ENOMEM is returned if either kmalloc() fails or
>> the user buffer is too short?
>> Looks pretty hard to detect which.
> 
> Agreed. The code is far from perfect. We might try to return ENOSPC and 
> watch what breaks.

brltty and kbd (see below) would break at least:
https://sources.debian.org/src/brltty/6.4-6/Drivers/Screen/Linux/screen.c/#L875

brltty apparently relies exactly on ENOMEM, increases buffer if that 
error is returned, and retries.

So I don't think we can change that ENOMEM to anything else.

>> I've not looked at the effect of all the patches, but setting
>> 'ret = -ENOMEM' and breaking the loop when the array is too
>> small would simplify things.

That would break kbd for example:
https://sources.debian.org/src/kbd/2.3.0-3/src/libkfont/kdmapop.c/?hl=154#L159

GIO_UNIMAP is called with zero count to actually find out the count...

So apart from the original patch which checks the return value of 
put_user, we cannot do anything else. (Except decoupling the "?:" to 
make it more readable.)

thanks,
-- 
js
suse labs

[PATCH 17/36] tty/vt: consolemap: introduce enum translation_map and use it

[Jiri Slaby]

Again, instead of magic constants in the code, declare an enum and be a
little bit more explicit. Both in the translations definition and in the
loops etc.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 39 +++++++++++++++++++------------------
 include/linux/consolemap.h  | 18 +++++++++++------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 92b5dddb00d9..80536687acef 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -38,7 +38,7 @@
 
 static unsigned short translations[][256] = {
   /* 8-bit Latin-1 mapped to Unicode -- trivial mapping */
-  {
+  [LAT1_MAP] = {
     0x0000, 0x0001, 0x0002, 0x0003, 0x0004, 0x0005, 0x0006, 0x0007,
     0x0008, 0x0009, 0x000a, 0x000b, 0x000c, 0x000d, 0x000e, 0x000f,
     0x0010, 0x0011, 0x0012, 0x0013, 0x0014, 0x0015, 0x0016, 0x0017,
@@ -71,9 +71,9 @@ static unsigned short translations[][256] = {
     0x00e8, 0x00e9, 0x00ea, 0x00eb, 0x00ec, 0x00ed, 0x00ee, 0x00ef,
     0x00f0, 0x00f1, 0x00f2, 0x00f3, 0x00f4, 0x00f5, 0x00f6, 0x00f7,
     0x00f8, 0x00f9, 0x00fa, 0x00fb, 0x00fc, 0x00fd, 0x00fe, 0x00ff
-  }, 
+  },
   /* VT100 graphics mapped to Unicode */
-  {
+  [GRAF_MAP] = {
     0x0000, 0x0001, 0x0002, 0x0003, 0x0004, 0x0005, 0x0006, 0x0007,
     0x0008, 0x0009, 0x000a, 0x000b, 0x000c, 0x000d, 0x000e, 0x000f,
     0x0010, 0x0011, 0x0012, 0x0013, 0x0014, 0x0015, 0x0016, 0x0017,
@@ -108,8 +108,8 @@ static unsigned short translations[][256] = {
     0x00f8, 0x00f9, 0x00fa, 0x00fb, 0x00fc, 0x00fd, 0x00fe, 0x00ff
   },
   /* IBM Codepage 437 mapped to Unicode */
-  {
-    0x0000, 0x263a, 0x263b, 0x2665, 0x2666, 0x2663, 0x2660, 0x2022, 
+  [IBMPC_MAP] = {
+    0x0000, 0x263a, 0x263b, 0x2665, 0x2666, 0x2663, 0x2660, 0x2022,
     0x25d8, 0x25cb, 0x25d9, 0x2642, 0x2640, 0x266a, 0x266b, 0x263c,
     0x25b6, 0x25c0, 0x2195, 0x203c, 0x00b6, 0x00a7, 0x25ac, 0x21a8,
     0x2191, 0x2193, 0x2192, 0x2190, 0x221f, 0x2194, 0x25b2, 0x25bc,
@@ -141,9 +141,9 @@ static unsigned short translations[][256] = {
     0x03a6, 0x0398, 0x03a9, 0x03b4, 0x221e, 0x03c6, 0x03b5, 0x2229,
     0x2261, 0x00b1, 0x2265, 0x2264, 0x2320, 0x2321, 0x00f7, 0x2248,
     0x00b0, 0x2219, 0x00b7, 0x221a, 0x207f, 0x00b2, 0x25a0, 0x00a0
-  }, 
+  },
   /* User mapping -- default to codes for direct font mapping */
-  {
+  [USER_MAP] = {
     0xf000, 0xf001, 0xf002, 0xf003, 0xf004, 0xf005, 0xf006, 0xf007,
     0xf008, 0xf009, 0xf00a, 0xf00b, 0xf00c, 0xf00d, 0xf00e, 0xf00f,
     0xf010, 0xf011, 0xf012, 0xf013, 0xf014, 0xf015, 0xf016, 0xf017,
@@ -184,7 +184,7 @@ static unsigned short translations[][256] = {
 
 #define MAX_GLYPH 512		/* Max possible glyph value */
 
-static int inv_translate[MAX_NR_CONSOLES];
+static enum translation_map inv_translate[MAX_NR_CONSOLES];
 
 #define UNI_DIRS	32U
 #define UNI_DIR_ROWS	32U
@@ -208,24 +208,25 @@ struct uni_pagedict {
 	u16		**uni_pgdir[UNI_DIRS];
 	unsigned long	refcount;
 	unsigned long	sum;
-	unsigned char	*inverse_translations[4];
+	unsigned char	*inverse_translations[LAST_MAP + 1];
 	u16		*inverse_trans_unicode;
 };
 
 static struct uni_pagedict *dflt;
 
-static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p, int i)
+static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
+	       enum translation_map m)
 {
 	int j, glyph;
-	unsigned short *t = translations[i];
+	unsigned short *t = translations[m];
 	unsigned char *q;
 	
 	if (!p)
 		return;
-	q = p->inverse_translations[i];
+	q = p->inverse_translations[m];
 
 	if (!q) {
-		q = p->inverse_translations[i] = kmalloc(MAX_GLYPH, GFP_KERNEL);
+		q = p->inverse_translations[m] = kmalloc(MAX_GLYPH, GFP_KERNEL);
 		if (!q)
 			return;
 	}
@@ -276,7 +277,7 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 	}
 }
 
-unsigned short *set_translate(int m, struct vc_data *vc)
+unsigned short *set_translate(enum translation_map m, struct vc_data *vc)
 {
 	inv_translate[vc->vc_num] = m;
 	return translations[m];
@@ -292,7 +293,7 @@ unsigned short *set_translate(int m, struct vc_data *vc)
 u16 inverse_translate(const struct vc_data *conp, u16 glyph, bool use_unicode)
 {
 	struct uni_pagedict *p;
-	int m;
+	enum translation_map m;
 
 	if (glyph >= MAX_GLYPH)
 		return 0;
@@ -669,8 +670,8 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	if (con_unify_unimap(vc, p))
 		goto out_unlock;
 
-	for (i = 0; i <= 3; i++)
-		set_inverse_transl(vc, p, i); /* Update inverse translations */
+	for (enum translation_map m = FIRST_MAP; m <= LAST_MAP; m++)
+		set_inverse_transl(vc, p, m); /* Update inverse translations */
 	set_inverse_trans_unicode(vc, p);
 
 out_unlock:
@@ -731,8 +732,8 @@ int con_set_default_unimap(struct vc_data *vc)
 		return err;
 	}
 
-	for (i = 0; i <= 3; i++)
-		set_inverse_transl(vc, p, i);	/* Update all inverse translations */
+	for (enum translation_map m = FIRST_MAP; m <= LAST_MAP; m++)
+		set_inverse_transl(vc, p, m);	/* Update all inverse translations */
 	set_inverse_trans_unicode(vc, p);
 	dflt = p;
 	return err;
diff --git a/include/linux/consolemap.h b/include/linux/consolemap.h
index 1ff2bf55eb85..c35db4896c37 100644
--- a/include/linux/consolemap.h
+++ b/include/linux/consolemap.h
@@ -7,10 +7,15 @@
 #ifndef __LINUX_CONSOLEMAP_H__
 #define __LINUX_CONSOLEMAP_H__
 
-#define LAT1_MAP 0
-#define GRAF_MAP 1
-#define IBMPC_MAP 2
-#define USER_MAP 3
+enum translation_map {
+	LAT1_MAP,
+	GRAF_MAP,
+	IBMPC_MAP,
+	USER_MAP,
+
+	FIRST_MAP = LAT1_MAP,
+	LAST_MAP = USER_MAP,
+};
 
 #include <linux/types.h>
 
@@ -18,7 +23,7 @@ struct vc_data;
 
 #ifdef CONFIG_CONSOLE_TRANSLATIONS
 u16 inverse_translate(const struct vc_data *conp, u16 glyph, bool use_unicode);
-unsigned short *set_translate(int m, struct vc_data *vc);
+unsigned short *set_translate(enum translation_map m, struct vc_data *vc);
 int conv_uni_to_pc(struct vc_data *conp, long ucs);
 u32 conv_8bit_to_uni(unsigned char c);
 int conv_uni_to_8bit(u32 uni);
@@ -30,7 +35,8 @@ static inline u16 inverse_translate(const struct vc_data *conp, u16 glyph,
 	return glyph;
 }
 
-static inline unsigned short *set_translate(int m, struct vc_data *vc)
+static inline unsigned short *set_translate(enum translation_map m,
+		struct vc_data *vc)
 {
 	return NULL;
 }
-- 
2.36.1

[PATCH 18/36] tty/vt: consolemap: remove glyph < 0 check from set_inverse_trans_unicode()

[Jiri Slaby]

glyph is now an int casted from u16. It can never be negative. So remove
the check and type glyph as u16 properly in set_inverse_trans_unicode().

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 80536687acef..733795a3dc68 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -244,7 +244,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
 static void set_inverse_trans_unicode(struct vc_data *conp,
 				      struct uni_pagedict *p)
 {
-	int i, j, k, glyph;
+	int i, j, k;
 	u16 **p1, *p2;
 	u16 *q;
 
@@ -268,9 +268,8 @@ static void set_inverse_trans_unicode(struct vc_data *conp,
 			if (!p2)
 				continue;
 			for (k = 0; k < UNI_ROW_GLYPHS; k++) {
-				glyph = p2[k];
-				if (glyph >= 0 && glyph < MAX_GLYPH
-					       && q[glyph] < 32)
+				u16 glyph = p2[k];
+				if (glyph < MAX_GLYPH && q[glyph] < 32)
 					q[glyph] = UNI(i, j, k);
 			}
 		}
-- 
2.36.1

[PATCH 19/36] tty/vt: consolemap: extract dict unsharing to con_unshare_unimap()

[Jiri Slaby]

The code in con_set_unimap() is too nested. Extract its obvious part
into a separate function and name it after what the code does:
con_unshare_unimap().

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 133 ++++++++++++++++++------------------
 1 file changed, 68 insertions(+), 65 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 733795a3dc68..eb5b4b519baf 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -562,11 +562,73 @@ int con_clear_unimap(struct vc_data *vc)
 	console_unlock();
 	return ret;
 }
-	
+
+static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
+		struct uni_pagedict *p)
+{
+	struct uni_pagedict *q;
+	u16 **p1, *p2, l;
+	int ret;
+	int i, j, k;
+
+	ret = con_do_clear_unimap(vc);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/*
+	 * Since refcount was > 1, con_clear_unimap() allocated a new
+	 * uni_pagedict for this vc.  Re: p != q
+	 */
+	q = *vc->vc_uni_pagedir_loc;
+
+	/*
+	 * uni_pgdir is a 32*32*64 table with rows allocated when its first
+	 * entry is added. The unicode value must still be incremented for
+	 * empty rows. We are copying entries from "p" (old) to "q" (new).
+	 */
+	l = 0;		/* unicode value */
+	for (i = 0; i < UNI_DIRS; i++) {
+		p1 = p->uni_pgdir[i];
+		if (!p1) {
+			/* Account for empty table */
+			l += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
+			continue;
+		}
+
+		for (j = 0; j < UNI_DIR_ROWS; j++) {
+			p2 = p1[j];
+			if (!p2) {
+				/* Account for row of 64 empty entries */
+				l += UNI_ROW_GLYPHS;
+				continue;
+			}
+
+			for (k = 0; k < UNI_ROW_GLYPHS; k++, l++) {
+				if (p2[k] == 0xffff)
+					continue;
+				/*
+				 * Found one, copy entry for unicode l with
+				 * fontpos value p2[k].
+				 */
+				ret = con_insert_unipair(q, l, p2[k]);
+				if (ret) {
+					p->refcount++;
+					*vc->vc_uni_pagedir_loc = p;
+					con_release_unimap(q);
+					kfree(q);
+					return ERR_PTR(ret);
+				}
+			}
+		}
+	}
+
+	return q;
+}
+
 int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 {
-	int err = 0, err1, i;
-	struct uni_pagedict *p, *q;
+	int err = 0, err1;
+	struct uni_pagedict *p;
 	struct unipair *unilist, *plist;
 
 	if (!ct)
@@ -586,70 +648,11 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	}
 
 	if (p->refcount > 1) {
-		int j, k;
-		u16 **p1, *p2, l;
-
-		err1 = con_do_clear_unimap(vc);
-		if (err1) {
-			err = err1;
+		p = con_unshare_unimap(vc, p);
+		if (IS_ERR(p)) {
+			err = PTR_ERR(p);
 			goto out_unlock;
 		}
-
-		/*
-		 * Since refcount was > 1, con_clear_unimap() allocated a
-		 * a new uni_pagedict for this vc.  Re: p != q
-		 */
-		q = *vc->vc_uni_pagedir_loc;
-
-		/*
-		 * uni_pgdir is a 32*32*64 table with rows allocated
-		 * when its first entry is added.  The unicode value must
-		 * still be incremented for empty rows.  We are copying
-		 * entries from "p" (old) to "q" (new).
-		 */
-		l = 0;		/* unicode value */
-		for (i = 0; i < UNI_DIRS; i++) {
-			p1 = p->uni_pgdir[i];
-			if (!p1) {
-				/* Account for empty table */
-				l += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
-				continue;
-			}
-
-			for (j = 0; j < UNI_DIR_ROWS; j++) {
-				p2 = p1[j];
-				if (!p2) {
-					/*
-					 * Account for row of 64 empty entries
-					 */
-					l += UNI_ROW_GLYPHS;
-					continue;
-				}
-
-				for (k = 0; k < UNI_ROW_GLYPHS; k++, l++) {
-					if (p2[k] == 0xffff)
-						continue;
-					/*
-					 * Found one, copy entry for unicode
-					 * l with fontpos value p2[k].
-					 */
-					err1 = con_insert_unipair(q, l, p2[k]);
-					if (err1) {
-						p->refcount++;
-						*vc->vc_uni_pagedir_loc = p;
-						con_release_unimap(q);
-						kfree(q);
-						err = err1;
-						goto out_unlock;
-					}
-				}
-			}
-		}
-
-		/*
-		 * Finished copying font table, set vc_uni_pagedir to new table
-		 */
-		p = q;
 	} else if (p == dflt) {
 		dflt = NULL;
 	}
-- 
2.36.1

Re: [PATCH 19/36] tty/vt: consolemap: extract dict unsharing to con_unshare_unimap()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The code in con_set_unimap() is too nested. Extract its obvious part
> into a separate function and name it after what the code does:
> con_unshare_unimap().
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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


-- 
 i.

[PATCH 20/36] tty/vt: consolemap: saner variable names in set_inverse_trans_unicode()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 38 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index eb5b4b519baf..3763a73706b2 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -244,33 +244,33 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
 static void set_inverse_trans_unicode(struct vc_data *conp,
 				      struct uni_pagedict *p)
 {
-	int i, j, k;
-	u16 **p1, *p2;
-	u16 *q;
+	unsigned int d, r, g;
+	u16 *inv;
 
 	if (!p)
 		return;
-	q = p->inverse_trans_unicode;
-	if (!q) {
-		q = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
-				sizeof(*q), GFP_KERNEL);
-		if (!q)
+
+	inv = p->inverse_trans_unicode;
+	if (!inv) {
+		inv = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
+				sizeof(*inv), GFP_KERNEL);
+		if (!inv)
 			return;
 	}
-	memset(q, 0, MAX_GLYPH * sizeof(*q));
+	memset(inv, 0, MAX_GLYPH * sizeof(*inv));
 
-	for (i = 0; i < UNI_DIRS; i++) {
-		p1 = p->uni_pgdir[i];
-		if (!p1)
+	for (d = 0; d < UNI_DIRS; d++) {
+		u16 **dir = p->uni_pgdir[d];
+		if (!dir)
 			continue;
-		for (j = 0; j < UNI_DIR_ROWS; j++) {
-			p2 = p1[j];
-			if (!p2)
+		for (r = 0; r < UNI_DIR_ROWS; r++) {
+			u16 *row = dir[r];
+			if (!row)
 				continue;
-			for (k = 0; k < UNI_ROW_GLYPHS; k++) {
-				u16 glyph = p2[k];
-				if (glyph < MAX_GLYPH && q[glyph] < 32)
-					q[glyph] = UNI(i, j, k);
+			for (g = 0; g < UNI_ROW_GLYPHS; g++) {
+				u16 glyph = row[g];
+				if (glyph < MAX_GLYPH && inv[glyph] < 32)
+					inv[glyph] = UNI(d, r, g);
 			}
 		}
 	}
-- 
2.36.1

Re: [PATCH 20/36] tty/vt: consolemap: saner variable names in set_inverse_trans_unicode()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The function uses too vague variable names like i, j, k for iterators, p,
> q, p1, p2 for pointers etc.
> 
> Rename all these, so that it is clear what is going on:
> - dict: for dictionaries.
> - d, r, g: for dir, row, glyph iterators -- these are unsigned now.
> - dir, row: for directory and row pointers.
> - glyph: for the glyph.
> - and so on...
> 
> This is a lot of shuffling, but the result pays off, IMO.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

-- 
 i.

[PATCH 21/36] tty/vt: consolemap: saner variable names in conv_uni_to_pc()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 3763a73706b2..374b1ba20635 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -852,10 +852,9 @@ int conv_uni_to_8bit(u32 uni)
 int
 conv_uni_to_pc(struct vc_data *conp, long ucs) 
 {
-	int h;
-	u16 **p1, *p2;
-	struct uni_pagedict *p;
-  
+	struct uni_pagedict *dict;
+	u16 **dir, *row, glyph;
+
 	/* Only 16-bit codes supported at this time */
 	if (ucs > 0xffff)
 		return -4;		/* Not found */
@@ -874,11 +873,11 @@ conv_uni_to_pc(struct vc_data *conp, long ucs)
 	if (!*conp->vc_uni_pagedir_loc)
 		return -3;
 
-	p = *conp->vc_uni_pagedir_loc;
-	if ((p1 = p->uni_pgdir[UNI_DIR(ucs)]) &&
-	    (p2 = p1[UNI_ROW(ucs)]) &&
-	    (h = p2[UNI_GLYPH(ucs)]) < MAX_GLYPH)
-		return h;
+	dict = *conp->vc_uni_pagedir_loc;
+	if ((dir = dict->uni_pgdir[UNI_DIR(ucs)]) &&
+	    (row = dir[UNI_ROW(ucs)]) &&
+	    (glyph = row[UNI_GLYPH(ucs)]) < MAX_GLYPH)
+		return glyph;
 
 	return -4;		/* not found */
 }
-- 
2.36.1

[PATCH 22/36] tty/vt: consolemap: saner variable names in con_insert_unipair()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 374b1ba20635..2454a4395722 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -499,31 +499,31 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
 static int
 con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 {
-	int n;
-	u16 **p1, *p2;
+	u16 **dir, *row;
+	unsigned int n;
 
 	n = UNI_DIR(unicode);
-	p1 = p->uni_pgdir[n];
-	if (!p1) {
-		p1 = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(*p1),
+	dir = p->uni_pgdir[n];
+	if (!dir) {
+		dir = p->uni_pgdir[n] = kcalloc(UNI_DIR_ROWS, sizeof(*dir),
 				GFP_KERNEL);
-		if (!p1)
+		if (!dir)
 			return -ENOMEM;
 	}
 
 	n = UNI_ROW(unicode);
-	p2 = p1[n];
-	if (!p2) {
-		p2 = p1[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(*p2),
+	row = dir[n];
+	if (!row) {
+		row = dir[n] = kmalloc_array(UNI_ROW_GLYPHS, sizeof(*row),
 				GFP_KERNEL);
-		if (!p2)
+		if (!row)
 			return -ENOMEM;
 		/* No glyphs for the characters (yet) */
-		memset(p2, 0xff, UNI_ROW_GLYPHS * sizeof(*p2));
+		memset(row, 0xff, UNI_ROW_GLYPHS * sizeof(*row));
 	}
 
-	p2[UNI_GLYPH(unicode)] = fontpos;
-	
+	row[UNI_GLYPH(unicode)] = fontpos;
+
 	p->sum += (fontpos << 20U) + unicode;
 
 	return 0;
-- 
2.36.1

[PATCH 23/36] tty/vt: consolemap: saner variable names in con_unify_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 49 ++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 2454a4395722..cbdc73605148 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -454,42 +454,41 @@ void con_free_unimap(struct vc_data *vc)
 	kfree(p);
 }
   
-static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *p)
+static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *dict1)
 {
-	int i, j, k;
-	struct uni_pagedict *q;
-	
-	for (i = 0; i < MAX_NR_CONSOLES; i++) {
-		if (!vc_cons_allocated(i))
+	struct uni_pagedict *dict2;
+	unsigned int cons, d, r;
+
+	for (cons = 0; cons < MAX_NR_CONSOLES; cons++) {
+		if (!vc_cons_allocated(cons))
 			continue;
-		q = *vc_cons[i].d->vc_uni_pagedir_loc;
-		if (!q || q == p || q->sum != p->sum)
+		dict2 = *vc_cons[cons].d->vc_uni_pagedir_loc;
+		if (!dict2 || dict2 == dict1 || dict2->sum != dict1->sum)
 			continue;
-		for (j = 0; j < UNI_DIRS; j++) {
-			u16 **p1, **q1;
-			p1 = p->uni_pgdir[j];
-			q1 = q->uni_pgdir[j];
-			if (!p1 && !q1)
+		for (d = 0; d < UNI_DIRS; d++) {
+			u16 **dir1 = dict1->uni_pgdir[d];
+			u16 **dir2 = dict2->uni_pgdir[d];
+			if (!dir1 && !dir2)
 				continue;
-			if (!p1 || !q1)
+			if (!dir1 || !dir2)
 				break;
-			for (k = 0; k < UNI_DIR_ROWS; k++) {
-				if (!p1[k] && !q1[k])
+			for (r = 0; r < UNI_DIR_ROWS; r++) {
+				if (!dir1[r] && !dir2[r])
 					continue;
-				if (!p1[k] || !q1[k])
+				if (!dir1[r] || !dir2[r])
 					break;
-				if (memcmp(p1[k], q1[k], UNI_ROW_GLYPHS *
-							sizeof(*p1[k])))
+				if (memcmp(dir1[r], dir2[r], UNI_ROW_GLYPHS *
+							sizeof(*dir1[r])))
 					break;
 			}
-			if (k < UNI_DIR_ROWS)
+			if (r < UNI_DIR_ROWS)
 				break;
 		}
-		if (j == UNI_DIRS) {
-			q->refcount++;
-			*conp->vc_uni_pagedir_loc = q;
-			con_release_unimap(p);
-			kfree(p);
+		if (d == UNI_DIRS) {
+			dict2->refcount++;
+			*conp->vc_uni_pagedir_loc = dict2;
+			con_release_unimap(dict1);
+			kfree(dict1);
 			return 1;
 		}
 	}
-- 
2.36.1

[PATCH 24/36] tty/vt: consolemap: saner variable names in con_do_clear_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index cbdc73605148..456aed3f717c 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -531,24 +531,23 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 /* Caller must hold the lock */
 static int con_do_clear_unimap(struct vc_data *vc)
 {
-	struct uni_pagedict *p, *q;
+	struct uni_pagedict *old = *vc->vc_uni_pagedir_loc;
 
-	p = *vc->vc_uni_pagedir_loc;
-	if (!p || --p->refcount) {
-		q = kzalloc(sizeof(*p), GFP_KERNEL);
-		if (!q) {
-			if (p)
-				p->refcount++;
+	if (!old || --old->refcount) {
+		struct uni_pagedict *new = kzalloc(sizeof(*new), GFP_KERNEL);
+		if (!new) {
+			if (old)
+				old->refcount++;
 			return -ENOMEM;
 		}
-		q->refcount=1;
-		*vc->vc_uni_pagedir_loc = q;
+		new->refcount = 1;
+		*vc->vc_uni_pagedir_loc = new;
 	} else {
-		if (p == dflt)
+		if (old == dflt)
 			dflt = NULL;
-		p->refcount++;
-		p->sum = 0;
-		con_release_unimap(p);
+		old->refcount++;
+		old->sum = 0;
+		con_release_unimap(old);
 	}
 	return 0;
 }
-- 
2.36.1

[PATCH 25/36] tty/vt: consolemap: saner variable names in con_unshare_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 51 ++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 456aed3f717c..7e353455945d 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -562,12 +562,12 @@ int con_clear_unimap(struct vc_data *vc)
 }
 
 static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
-		struct uni_pagedict *p)
+		struct uni_pagedict *old)
 {
-	struct uni_pagedict *q;
-	u16 **p1, *p2, l;
+	struct uni_pagedict *new;
+	unsigned int d, r, g;
 	int ret;
-	int i, j, k;
+	u16 uni = 0;
 
 	ret = con_do_clear_unimap(vc);
 	if (ret)
@@ -575,52 +575,51 @@ static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
 
 	/*
 	 * Since refcount was > 1, con_clear_unimap() allocated a new
-	 * uni_pagedict for this vc.  Re: p != q
+	 * uni_pagedict for this vc.  Re: old != new
 	 */
-	q = *vc->vc_uni_pagedir_loc;
+	new = *vc->vc_uni_pagedir_loc;
 
 	/*
 	 * uni_pgdir is a 32*32*64 table with rows allocated when its first
 	 * entry is added. The unicode value must still be incremented for
-	 * empty rows. We are copying entries from "p" (old) to "q" (new).
+	 * empty rows. We are copying entries from "old" to "new".
 	 */
-	l = 0;		/* unicode value */
-	for (i = 0; i < UNI_DIRS; i++) {
-		p1 = p->uni_pgdir[i];
-		if (!p1) {
+	for (d = 0; d < UNI_DIRS; d++) {
+		u16 **dir = old->uni_pgdir[d];
+		if (!dir) {
 			/* Account for empty table */
-			l += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
+			uni += UNI_DIR_ROWS * UNI_ROW_GLYPHS;
 			continue;
 		}
 
-		for (j = 0; j < UNI_DIR_ROWS; j++) {
-			p2 = p1[j];
-			if (!p2) {
+		for (r = 0; r < UNI_DIR_ROWS; r++) {
+			u16 *row = dir[r];
+			if (!row) {
 				/* Account for row of 64 empty entries */
-				l += UNI_ROW_GLYPHS;
+				uni += UNI_ROW_GLYPHS;
 				continue;
 			}
 
-			for (k = 0; k < UNI_ROW_GLYPHS; k++, l++) {
-				if (p2[k] == 0xffff)
+			for (g = 0; g < UNI_ROW_GLYPHS; g++, uni++) {
+				if (row[g] == 0xffff)
 					continue;
 				/*
-				 * Found one, copy entry for unicode l with
-				 * fontpos value p2[k].
+				 * Found one, copy entry for unicode uni with
+				 * fontpos value row[g].
 				 */
-				ret = con_insert_unipair(q, l, p2[k]);
+				ret = con_insert_unipair(new, uni, row[g]);
 				if (ret) {
-					p->refcount++;
-					*vc->vc_uni_pagedir_loc = p;
-					con_release_unimap(q);
-					kfree(q);
+					old->refcount++;
+					*vc->vc_uni_pagedir_loc = old;
+					con_release_unimap(new);
+					kfree(new);
 					return ERR_PTR(ret);
 				}
 			}
 		}
 	}
 
-	return q;
+	return new;
 }
 
 int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
-- 
2.36.1

[PATCH 26/36] tty/vt: consolemap: saner variable names in con_release_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 7e353455945d..255d4e92a9d0 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -415,28 +415,30 @@ int con_get_trans_new(ushort __user * arg)
 extern u8 dfont_unicount[];	/* Defined in console_defmap.c */
 extern u16 dfont_unitable[];
 
-static void con_release_unimap(struct uni_pagedict *p)
+static void con_release_unimap(struct uni_pagedict *dict)
 {
-	u16 **p1;
-	int i, j;
+	unsigned int d, r;
 
-	if (p == dflt)
+	if (dict == dflt)
 		dflt = NULL;
-	for (i = 0; i < UNI_DIRS; i++) {
-		p1 = p->uni_pgdir[i];
-		if (p1 != NULL) {
-			for (j = 0; j < UNI_DIR_ROWS; j++)
-				kfree(p1[j]);
-			kfree(p1);
+
+	for (d = 0; d < UNI_DIRS; d++) {
+		u16 **dir = dict->uni_pgdir[d];
+		if (dir != NULL) {
+			for (r = 0; r < UNI_DIR_ROWS; r++)
+				kfree(dir[r]);
+			kfree(dir);
 		}
-		p->uni_pgdir[i] = NULL;
+		dict->uni_pgdir[d] = NULL;
 	}
-	for (i = 0; i < ARRAY_SIZE(p->inverse_translations); i++) {
-		kfree(p->inverse_translations[i]);
-		p->inverse_translations[i] = NULL;
+
+	for (r = 0; r < ARRAY_SIZE(dict->inverse_translations); r++) {
+		kfree(dict->inverse_translations[r]);
+		dict->inverse_translations[r] = NULL;
 	}
-	kfree(p->inverse_trans_unicode);
-	p->inverse_trans_unicode = NULL;
+
+	kfree(dict->inverse_trans_unicode);
+	dict->inverse_trans_unicode = NULL;
 }
 
 /* Caller must hold the console lock */
-- 
2.36.1

[PATCH 27/36] tty/vt: consolemap: saner variable names in con_copy_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 255d4e92a9d0..dcbeb38a3a0a 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -750,16 +750,16 @@ EXPORT_SYMBOL(con_set_default_unimap);
  */
 int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
 {
-	struct uni_pagedict *q;
+	struct uni_pagedict *src;
 
 	if (!*src_vc->vc_uni_pagedir_loc)
 		return -EINVAL;
 	if (*dst_vc->vc_uni_pagedir_loc == *src_vc->vc_uni_pagedir_loc)
 		return 0;
 	con_free_unimap(dst_vc);
-	q = *src_vc->vc_uni_pagedir_loc;
-	q->refcount++;
-	*dst_vc->vc_uni_pagedir_loc = q;
+	src = *src_vc->vc_uni_pagedir_loc;
+	src->refcount++;
+	*dst_vc->vc_uni_pagedir_loc = src;
 	return 0;
 }
 EXPORT_SYMBOL(con_copy_unimap);
-- 
2.36.1

[PATCH 28/36] tty/vt: consolemap: saner variable names in con_get_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index dcbeb38a3a0a..b8f2acb6e388 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -773,11 +773,11 @@ EXPORT_SYMBOL(con_copy_unimap);
 int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
 		struct unipair __user *list)
 {
-	int i, j, k, ret = 0;
 	ushort ect;
-	u16 **p1, *p2;
-	struct uni_pagedict *p;
+	struct uni_pagedict *dict;
 	struct unipair *unilist;
+	unsigned int d, r, g;
+	int ret = 0;
 
 	unilist = kvmalloc_array(ct, sizeof(*unilist), GFP_KERNEL);
 	if (!unilist)
@@ -786,26 +786,26 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
 	console_lock();
 
 	ect = 0;
-	p = *vc->vc_uni_pagedir_loc;
-	if (!p)
+	dict = *vc->vc_uni_pagedir_loc;
+	if (!dict)
 		goto unlock;
 
-	for (i = 0; i < UNI_DIRS; i++) {
-		p1 = p->uni_pgdir[i];
-		if (!p1)
+	for (d = 0; d < UNI_DIRS; d++) {
+		u16 **dir = dict->uni_pgdir[d];
+		if (!dir)
 			continue;
 
-		for (j = 0; j < UNI_DIR_ROWS; j++, p1++) {
-			p2 = *p1;
-			if (!p2)
+		for (r = 0; r < UNI_DIR_ROWS; r++) {
+			u16 *row = dir[r];
+			if (!row)
 				continue;
 
-			for (k = 0; k < UNI_ROW_GLYPHS; k++, p2++) {
-				if (*p2 >= MAX_GLYPH)
+			for (g = 0; g < UNI_ROW_GLYPHS; g++, row++) {
+				if (*row >= MAX_GLYPH)
 					continue;
 				if (ect < ct) {
-					unilist[ect].unicode = UNI(i, j, k);
-					unilist[ect].fontpos = *p2;
+					unilist[ect].unicode = UNI(d, r, g);
+					unilist[ect].fontpos = *row;
 				}
 				ect++;
 			}
-- 
2.36.1

[PATCH 29/36] tty/vt: consolemap: saner variable names in con_set_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index b8f2acb6e388..65c83f9228e9 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -627,7 +627,7 @@ static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
 int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 {
 	int err = 0, err1;
-	struct uni_pagedict *p;
+	struct uni_pagedict *dict;
 	struct unipair *unilist, *plist;
 
 	if (!ct)
@@ -640,19 +640,19 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	console_lock();
 
 	/* Save original vc_unipagdir_loc in case we allocate a new one */
-	p = *vc->vc_uni_pagedir_loc;
-	if (!p) {
+	dict = *vc->vc_uni_pagedir_loc;
+	if (!dict) {
 		err = -EINVAL;
 		goto out_unlock;
 	}
 
-	if (p->refcount > 1) {
-		p = con_unshare_unimap(vc, p);
-		if (IS_ERR(p)) {
-			err = PTR_ERR(p);
+	if (dict->refcount > 1) {
+		dict = con_unshare_unimap(vc, dict);
+		if (IS_ERR(dict)) {
+			err = PTR_ERR(dict);
 			goto out_unlock;
 		}
-	} else if (p == dflt) {
+	} else if (dict == dflt) {
 		dflt = NULL;
 	}
 
@@ -660,7 +660,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	 * Insert user specified unicode pairs into new table.
 	 */
 	for (plist = unilist; ct; ct--, plist++) {
-		err1 = con_insert_unipair(p, plist->unicode, plist->fontpos);
+		err1 = con_insert_unipair(dict, plist->unicode, plist->fontpos);
 		if (err1)
 			err = err1;
 	}
@@ -668,12 +668,12 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	/*
 	 * Merge with fontmaps of any other virtual consoles.
 	 */
-	if (con_unify_unimap(vc, p))
+	if (con_unify_unimap(vc, dict))
 		goto out_unlock;
 
 	for (enum translation_map m = FIRST_MAP; m <= LAST_MAP; m++)
-		set_inverse_transl(vc, p, m); /* Update inverse translations */
-	set_inverse_trans_unicode(vc, p);
+		set_inverse_transl(vc, dict, m);
+	set_inverse_trans_unicode(vc, dict);
 
 out_unlock:
 	console_unlock();
-- 
2.36.1

[PATCH 30/36] tty/vt: consolemap: saner variable names in con_set_default_unimap()

[Jiri Slaby]

The function uses too vague variable names like i, j, k for iterators, p,
q, p1, p2 for pointers etc.

Rename all these, so that it is clear what is going on:
- dict: for dictionaries.
- d, r, g: for dir, row, glyph iterators -- these are unsigned now.
- dir, row: for directory and row pointers.
- glyph: for the glyph.
- and so on...

This is a lot of shuffling, but the result pays off, IMO.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 43 +++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 65c83f9228e9..8abf114b6c68 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -694,49 +694,50 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
  */
 int con_set_default_unimap(struct vc_data *vc)
 {
-	int i, j, err = 0, err1;
-	u16 *q;
-	struct uni_pagedict *p;
+	struct uni_pagedict *dict;
+	unsigned int fontpos, count;
+	int err = 0, err1;
+	u16 *dfont;
 
 	if (dflt) {
-		p = *vc->vc_uni_pagedir_loc;
-		if (p == dflt)
+		dict = *vc->vc_uni_pagedir_loc;
+		if (dict == dflt)
 			return 0;
 
 		dflt->refcount++;
 		*vc->vc_uni_pagedir_loc = dflt;
-		if (p && !--p->refcount) {
-			con_release_unimap(p);
-			kfree(p);
+		if (dict && !--dict->refcount) {
+			con_release_unimap(dict);
+			kfree(dict);
 		}
 		return 0;
 	}
-	
+
 	/* The default font is always 256 characters */
 
 	err = con_do_clear_unimap(vc);
 	if (err)
 		return err;
-    
-	p = *vc->vc_uni_pagedir_loc;
-	q = dfont_unitable;
-	
-	for (i = 0; i < 256; i++)
-		for (j = dfont_unicount[i]; j; j--) {
-			err1 = con_insert_unipair(p, *(q++), i);
+
+	dict = *vc->vc_uni_pagedir_loc;
+	dfont = dfont_unitable;
+
+	for (fontpos = 0; fontpos < 256U; fontpos++)
+		for (count = dfont_unicount[fontpos]; count; count--) {
+			err1 = con_insert_unipair(dict, *(dfont++), fontpos);
 			if (err1)
 				err = err1;
 		}
-			
-	if (con_unify_unimap(vc, p)) {
+
+	if (con_unify_unimap(vc, dict)) {
 		dflt = *vc->vc_uni_pagedir_loc;
 		return err;
 	}
 
 	for (enum translation_map m = FIRST_MAP; m <= LAST_MAP; m++)
-		set_inverse_transl(vc, p, m);	/* Update all inverse translations */
-	set_inverse_trans_unicode(vc, p);
-	dflt = p;
+		set_inverse_transl(vc, dict, m);
+	set_inverse_trans_unicode(vc, dict);
+	dflt = dict;
 	return err;
 }
 EXPORT_SYMBOL(con_set_default_unimap);
-- 
2.36.1

[PATCH 31/36] tty/vt: consolemap: make conv_uni_to_pc() more readable

[Jiri Slaby]

1) Fetch *conp->vc_uni_pagedir_loc first and do the NULL check on the local
   variable.
2) Decouple the large "if" into few smaller "if"s.
3) Remove a \n from the definition line.

This makes the code more readable.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 8abf114b6c68..a9b497ffb346 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -849,8 +849,7 @@ int conv_uni_to_8bit(u32 uni)
 	return -1;
 }
 
-int
-conv_uni_to_pc(struct vc_data *conp, long ucs) 
+int conv_uni_to_pc(struct vc_data *conp, long ucs)
 {
 	struct uni_pagedict *dict;
 	u16 **dir, *row, glyph;
@@ -869,17 +868,24 @@ conv_uni_to_pc(struct vc_data *conp, long ucs)
 	 */
 	else if ((ucs & ~UNI_DIRECT_MASK) == UNI_DIRECT_BASE)
 		return ucs & UNI_DIRECT_MASK;
-  
-	if (!*conp->vc_uni_pagedir_loc)
-		return -3;
 
 	dict = *conp->vc_uni_pagedir_loc;
-	if ((dir = dict->uni_pgdir[UNI_DIR(ucs)]) &&
-	    (row = dir[UNI_ROW(ucs)]) &&
-	    (glyph = row[UNI_GLYPH(ucs)]) < MAX_GLYPH)
-		return glyph;
+	if (!dict)
+		return -3;
+
+	dir = dict->uni_pgdir[UNI_DIR(ucs)];
+	if (!dir)
+		return -4;
+
+	row = dir[UNI_ROW(ucs)];
+	if (!row)
+		return -4;
+
+	glyph = row[UNI_GLYPH(ucs)];
+	if (glyph >= MAX_GLYPH)
+		return -4;
 
-	return -4;		/* not found */
+	return glyph;
 }
 
 /*
-- 
2.36.1

[PATCH 32/36] tty/vt: consolemap: remove superfluous whitespace

[Jiri Slaby]

There are still some remaining tabs/spaces at EOLs or spaces before
tabs. Remove them all now.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index a9b497ffb346..01b7e49f1f91 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -220,7 +220,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
 	int j, glyph;
 	unsigned short *t = translations[m];
 	unsigned char *q;
-	
+
 	if (!p)
 		return;
 	q = p->inverse_translations[m];
@@ -236,7 +236,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
 		glyph = conv_uni_to_pc(conp, t[j]);
 		if (glyph >= 0 && glyph < MAX_GLYPH && q[glyph] < 32) {
 			/* prefer '-' above SHY etc. */
-		  	q[glyph] = j;
+			q[glyph] = j;
 		}
 	}
 }
@@ -320,7 +320,7 @@ static void update_user_maps(void)
 {
 	int i;
 	struct uni_pagedict *p, *q = NULL;
-	
+
 	for (i = 0; i < MAX_NR_CONSOLES; i++) {
 		if (!vc_cons_allocated(i))
 			continue;
@@ -403,7 +403,7 @@ int con_get_trans_new(ushort __user * arg)
 }
 
 /*
- * Unicode -> current font conversion 
+ * Unicode -> current font conversion
  *
  * A font has at most 512 chars, usually 256.
  * But one font position may represent several Unicode chars.
@@ -455,7 +455,7 @@ void con_free_unimap(struct vc_data *vc)
 	con_release_unimap(p);
 	kfree(p);
 }
-  
+
 static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *dict1)
 {
 	struct uni_pagedict *dict2;
@@ -688,7 +688,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
  *	Loads the unimap for the hardware font, as defined in uni_hash.tbl.
  *	The representation used was the most compact I could come up
  *	with.  This routine is executed at video setup, and when the
- *	PIO_FONTRESET ioctl is called. 
+ *	PIO_FONTRESET ioctl is called.
  *
  *	The caller must hold the console lock
  */
@@ -893,11 +893,11 @@ int conv_uni_to_pc(struct vc_data *conp, long ucs)
  * initialized.  It must be possible to call kmalloc(..., GFP_KERNEL)
  * from this function, hence the call from sys_setup.
  */
-void __init 
+void __init
 console_map_init(void)
 {
 	int i;
-	
+
 	for (i = 0; i < MAX_NR_CONSOLES; i++)
 		if (vc_cons_allocated(i) && !*vc_cons[i].d->vc_uni_pagedir_loc)
 			con_set_default_unimap(vc_cons[i].d);
-- 
2.36.1

[PATCH 33/36] tty/vt: consolemap: change refcount only if needed in con_do_clear_unimap()

[Jiri Slaby]

con_do_clear_unimap() currently decreases and increases refcount of old
dictionary in a back and forth fashion. This makes the code really hard
to follow. Decrease the refcount only if everything went well and we
really allocated a new one and decoupled from the old dictionary.

I sincerelly hope I did not make a mistake in this (ill) logic.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 01b7e49f1f91..4d8efe74315c 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -535,22 +535,23 @@ static int con_do_clear_unimap(struct vc_data *vc)
 {
 	struct uni_pagedict *old = *vc->vc_uni_pagedir_loc;
 
-	if (!old || --old->refcount) {
+	if (!old || old->refcount > 1) {
 		struct uni_pagedict *new = kzalloc(sizeof(*new), GFP_KERNEL);
-		if (!new) {
-			if (old)
-				old->refcount++;
+		if (!new)
 			return -ENOMEM;
-		}
+
 		new->refcount = 1;
 		*vc->vc_uni_pagedir_loc = new;
+
+		if (old)
+			old->refcount--;
 	} else {
 		if (old == dflt)
 			dflt = NULL;
-		old->refcount++;
 		old->sum = 0;
 		con_release_unimap(old);
 	}
+
 	return 0;
 }
 
-- 
2.36.1

Re: [PATCH 33/36] tty/vt: consolemap: change refcount only if needed in con_do_clear_unimap()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> con_do_clear_unimap() currently decreases and increases refcount of old
> dictionary in a back and forth fashion. This makes the code really hard
> to follow. Decrease the refcount only if everything went well and we
> really allocated a new one and decoupled from the old dictionary.
> 
> I sincerelly hope I did not make a mistake in this (ill) logic.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

It seems fine:

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

One unrelated comment below for additional cleanup opportunity.

> ---
>  drivers/tty/vt/consolemap.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> index 01b7e49f1f91..4d8efe74315c 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -535,22 +535,23 @@ static int con_do_clear_unimap(struct vc_data *vc)
>  {
>  	struct uni_pagedict *old = *vc->vc_uni_pagedir_loc;
>  
> -	if (!old || --old->refcount) {
> +	if (!old || old->refcount > 1) {
>  		struct uni_pagedict *new = kzalloc(sizeof(*new), GFP_KERNEL);
> -		if (!new) {
> -			if (old)
> -				old->refcount++;
> +		if (!new)
>  			return -ENOMEM;
> -		}
> +
>  		new->refcount = 1;
>  		*vc->vc_uni_pagedir_loc = new;
> +
> +		if (old)
> +			old->refcount--;
>  	} else {
>  		if (old == dflt)
>  			dflt = NULL;

This seems unnecessary as con_release_unimap() already does it.

> -		old->refcount++;
>  		old->sum = 0;
>  		con_release_unimap(old);
>  	}
> +
>  	return 0;
>  }
>  
> 

-- 
 i.

Re: [PATCH 33/36] tty/vt: consolemap: change refcount only if needed in con_do_clear_unimap()

[Jiri Slaby]

On 07. 06. 22, 17:31, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Jiri Slaby wrote:
> 
>> con_do_clear_unimap() currently decreases and increases refcount of old
>> dictionary in a back and forth fashion. This makes the code really hard
>> to follow. Decrease the refcount only if everything went well and we
>> really allocated a new one and decoupled from the old dictionary.
>>
>> I sincerelly hope I did not make a mistake in this (ill) logic.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> It seems fine:
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> One unrelated comment below for additional cleanup opportunity.
> 
>> ---
>>   drivers/tty/vt/consolemap.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
>> index 01b7e49f1f91..4d8efe74315c 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -535,22 +535,23 @@ static int con_do_clear_unimap(struct vc_data *vc)
>>   {
>>   	struct uni_pagedict *old = *vc->vc_uni_pagedir_loc;
>>   
>> -	if (!old || --old->refcount) {
>> +	if (!old || old->refcount > 1) {
>>   		struct uni_pagedict *new = kzalloc(sizeof(*new), GFP_KERNEL);
>> -		if (!new) {
>> -			if (old)
>> -				old->refcount++;
>> +		if (!new)
>>   			return -ENOMEM;
>> -		}
>> +
>>   		new->refcount = 1;
>>   		*vc->vc_uni_pagedir_loc = new;
>> +
>> +		if (old)
>> +			old->refcount--;
>>   	} else {
>>   		if (old == dflt)
>>   			dflt = NULL;
> 
> This seems unnecessary as con_release_unimap() already does it.

Good point -- the code is real mess... Now, the mess is more apparent, 
at least ;).

I will create a separate patch for this.

thanks,
-- 
js
suse labs

[PATCH 34/36] tty/vt: consolemap: extract con_allocate_new() from con_do_clear_unimap()

[Jiri Slaby]

The first part of con_do_clear_unimap() is needed on another place, so
extract it to a separate function called con_allocate_new(). It will be
used once more in the next patch.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 38 ++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 4d8efe74315c..14d3fbff015c 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -530,27 +530,35 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
 	return 0;
 }
 
+static int con_allocate_new(struct vc_data *vc)
+{
+	struct uni_pagedict *new, *old = *vc->vc_uni_pagedir_loc;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+
+	new->refcount = 1;
+	*vc->vc_uni_pagedir_loc = new;
+
+	if (old)
+		old->refcount--;
+
+	return 0;
+}
+
 /* Caller must hold the lock */
 static int con_do_clear_unimap(struct vc_data *vc)
 {
 	struct uni_pagedict *old = *vc->vc_uni_pagedir_loc;
 
-	if (!old || old->refcount > 1) {
-		struct uni_pagedict *new = kzalloc(sizeof(*new), GFP_KERNEL);
-		if (!new)
-			return -ENOMEM;
-
-		new->refcount = 1;
-		*vc->vc_uni_pagedir_loc = new;
+	if (!old || old->refcount > 1)
+		return con_allocate_new(vc);
 
-		if (old)
-			old->refcount--;
-	} else {
-		if (old == dflt)
-			dflt = NULL;
-		old->sum = 0;
-		con_release_unimap(old);
-	}
+	if (old == dflt)
+		dflt = NULL;
+	old->sum = 0;
+	con_release_unimap(old);
 
 	return 0;
 }
-- 
2.36.1

[PATCH 35/36] tty/vt: consolemap: use con_allocate_new() in con_unshare_unimap()

[Jiri Slaby]

The old->refcount is guaranteed to be > 1, so we can directly call
con_allocate_new() to make the code more obvious.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 14d3fbff015c..f97f7ee6268b 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -580,14 +580,10 @@ static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
 	int ret;
 	u16 uni = 0;
 
-	ret = con_do_clear_unimap(vc);
+	ret = con_allocate_new(vc);
 	if (ret)
 		return ERR_PTR(ret);
 
-	/*
-	 * Since refcount was > 1, con_clear_unimap() allocated a new
-	 * uni_pagedict for this vc.  Re: old != new
-	 */
 	new = *vc->vc_uni_pagedir_loc;
 
 	/*
-- 
2.36.1

[PATCH 36/36] tty/vt: consolemap: walk the buffer only once in con_set_trans_old()

[Jiri Slaby]

Fetch the user data one by one (by get_user()) and fill in the local
buffer simultaneously. I.e. we no longer require to walk two buffers and
save thus 256 B from stack (whole ubuf).

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/consolemap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index f97f7ee6268b..fff97ae87e00 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -343,15 +343,15 @@ static void update_user_maps(void)
  */
 int con_set_trans_old(unsigned char __user * arg)
 {
-	int i;
 	unsigned short inbuf[E_TABSZ];
-	unsigned char ubuf[E_TABSZ];
-
-	if (copy_from_user(ubuf, arg, E_TABSZ))
-		return -EFAULT;
+	unsigned int i;
+	unsigned char ch;
 
-	for (i = 0; i < E_TABSZ ; i++)
-		inbuf[i] = UNI_DIRECT_BASE | ubuf[i];
+	for (i = 0; i < ARRAY_SIZE(inbuf); i++) {
+		if (get_user(ch, &arg[i]))
+			return -EFAULT;
+		inbuf[i] = UNI_DIRECT_BASE | ch;
+	}
 
 	console_lock();
 	memcpy(translations[USER_MAP], inbuf, sizeof(inbuf));
-- 
2.36.1

Re: [PATCH 36/36] tty/vt: consolemap: walk the buffer only once in con_set_trans_old()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> Fetch the user data one by one (by get_user()) and fill in the local
> buffer simultaneously. I.e. we no longer require to walk two buffers and
> save thus 256 B from stack (whole ubuf).
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
...for this and all patches that were not commented individually.

-- 
 i.

Re: [PATCH 01/36] tty/vt: consolemap: use ARRAY_SIZE()

[Ilpo Järvinen]

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

On Tue, 7 Jun 2022, Jiri Slaby wrote:

> The code uses constants as bounds in loops. Use ARRAY_SIZE() with
> appropriate parameters instead. This makes the loop bounds obvious.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

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

-- 
 i.