The code still uses constants (macros) as bounds in loops after commit
17945d317a52 (tty/vt: consolemap: use ARRAY_SIZE()). The contants are at
least macros used also in the definition of the arrays. But use
ARRAY_SIZE() on two more places to ensure the loops never run out of
bounds even if the array definition change.
Signed-off-by: Jiri Slaby <[email protected]>
---
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 fff97ae87e00..8aa7a48b3647 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -232,7 +232,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
}
memset(q, 0, MAX_GLYPH);
- for (j = 0; j < E_TABSZ; j++) {
+ for (j = 0; j < ARRAY_SIZE(translations[i]); j++) {
glyph = conv_uni_to_pc(conp, t[j]);
if (glyph >= 0 && glyph < MAX_GLYPH && q[glyph] < 32) {
/* prefer '-' above SHY etc. */
@@ -367,7 +367,7 @@ int con_get_trans_old(unsigned char __user * arg)
unsigned char outbuf[E_TABSZ];
console_lock();
- for (i = 0; i < E_TABSZ ; i++)
+ for (i = 0; i < ARRAY_SIZE(outbuf); i++)
{
ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
outbuf[i] = (ch & ~0xff) ? 0 : ch;
--
2.36.1
Hi all,
Err, I take my Reviewed-by back. I just found an issue with this commit
while reviewing a later patch in the series that undos that error.
On Tue, 14 Jun 2022, Ilpo J?rvinen wrote:
> On Tue, 14 Jun 2022, Jiri Slaby wrote:
>
> > The code still uses constants (macros) as bounds in loops after commit
> > 17945d317a52 (tty/vt: consolemap: use ARRAY_SIZE()). The contants are at
> > least macros used also in the definition of the arrays. But use
> > ARRAY_SIZE() on two more places to ensure the loops never run out of
> > bounds even if the array definition change.
> >
> > Signed-off-by: Jiri Slaby <[email protected]>
> > ---
> > 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 fff97ae87e00..8aa7a48b3647 100644
> > --- a/drivers/tty/vt/consolemap.c
> > +++ b/drivers/tty/vt/consolemap.c
> > @@ -232,7 +232,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
> > }
> > memset(q, 0, MAX_GLYPH);
> >
> > - for (j = 0; j < E_TABSZ; j++) {
> > + for (j = 0; j < ARRAY_SIZE(translations[i]); j++) {
There's no i variable in this function.
> Any particular reason why you left its definition to have 256 instead of
> E_TABSZ (even after the patch series I mean):
>
> static unsigned short translations[][256] = {
>
>
> > glyph = conv_uni_to_pc(conp, t[j]);
> > if (glyph >= 0 && glyph < MAX_GLYPH && q[glyph] < 32) {
> > /* prefer '-' above SHY etc. */
> > @@ -367,7 +367,7 @@ int con_get_trans_old(unsigned char __user * arg)
> > unsigned char outbuf[E_TABSZ];
> >
> > console_lock();
> > - for (i = 0; i < E_TABSZ ; i++)
> > + for (i = 0; i < ARRAY_SIZE(outbuf); i++)
> > {
> > ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
> > outbuf[i] = (ch & ~0xff) ? 0 : ch;
> >
>
> Reviewed-by: Ilpo J?rvinen <[email protected]>
This patch is not fine so I take my rev-by back.
--
i.
The function still uses too vague parameter name after commit
50c92a1b2d50 (tty/vt: consolemap: saner variable names in
set_inverse_trans_unicode()).
So use "dict" instead of "p" for that parameter too.
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index a7507c5c3154..a69dfda8e3d0 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -241,17 +241,17 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
}
}
-static void set_inverse_trans_unicode(struct uni_pagedict *p)
+static void set_inverse_trans_unicode(struct uni_pagedict *dict)
{
unsigned int d, r, g;
u16 *inv;
- if (!p)
+ if (!dict)
return;
- inv = p->inverse_trans_unicode;
+ inv = dict->inverse_trans_unicode;
if (!inv) {
- inv = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
+ inv = dict->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
sizeof(*inv), GFP_KERNEL);
if (!inv)
return;
@@ -259,7 +259,7 @@ static void set_inverse_trans_unicode(struct uni_pagedict *p)
memset(inv, 0, MAX_GLYPH * sizeof(*inv));
for (d = 0; d < UNI_DIRS; d++) {
- u16 **dir = p->uni_pgdir[d];
+ u16 **dir = dict->uni_pgdir[d];
if (!dir)
continue;
for (r = 0; r < UNI_DIR_ROWS; r++) {
--
2.36.1
On Tue, 14 Jun 2022, Jiri Slaby wrote:
> The code still uses constants (macros) as bounds in loops after commit
> 17945d317a52 (tty/vt: consolemap: use ARRAY_SIZE()). The contants are at
> least macros used also in the definition of the arrays. But use
> ARRAY_SIZE() on two more places to ensure the loops never run out of
> bounds even if the array definition change.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> ---
> 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 fff97ae87e00..8aa7a48b3647 100644
> --- a/drivers/tty/vt/consolemap.c
> +++ b/drivers/tty/vt/consolemap.c
> @@ -232,7 +232,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
> }
> memset(q, 0, MAX_GLYPH);
>
> - for (j = 0; j < E_TABSZ; j++) {
> + for (j = 0; j < ARRAY_SIZE(translations[i]); j++) {
Any particular reason why you left its definition to have 256 instead of
E_TABSZ (even after the patch series I mean):
static unsigned short translations[][256] = {
> glyph = conv_uni_to_pc(conp, t[j]);
> if (glyph >= 0 && glyph < MAX_GLYPH && q[glyph] < 32) {
> /* prefer '-' above SHY etc. */
> @@ -367,7 +367,7 @@ int con_get_trans_old(unsigned char __user * arg)
> unsigned char outbuf[E_TABSZ];
>
> console_lock();
> - for (i = 0; i < E_TABSZ ; i++)
> + for (i = 0; i < ARRAY_SIZE(outbuf); i++)
> {
> ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
> outbuf[i] = (ch & ~0xff) ? 0 : ch;
>
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
The code still uses constants (macros) as bounds in loops after commit
17945d317a52 (tty/vt: consolemap: use ARRAY_SIZE()). The contants are at
least macros used also in the definition of the arrays. But use
ARRAY_SIZE() on two more places to ensure the loops never run out of
bounds even if the array definition change.
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
Notes:
[v2]
- fix build error (which was fixed by 4/7 in v1)
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 fff97ae87e00..2039237b5266 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -232,7 +232,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
}
memset(q, 0, MAX_GLYPH);
- for (j = 0; j < E_TABSZ; j++) {
+ for (j = 0; j < ARRAY_SIZE(translations[m]); j++) {
glyph = conv_uni_to_pc(conp, t[j]);
if (glyph >= 0 && glyph < MAX_GLYPH && q[glyph] < 32) {
/* prefer '-' above SHY etc. */
@@ -367,7 +367,7 @@ int con_get_trans_old(unsigned char __user * arg)
unsigned char outbuf[E_TABSZ];
console_lock();
- for (i = 0; i < E_TABSZ ; i++)
+ for (i = 0; i < ARRAY_SIZE(outbuf); i++)
{
ch = conv_uni_to_pc(vc_cons[fg_console].d, p[i]);
outbuf[i] = (ch & ~0xff) ? 0 : ch;
--
2.36.1
conp is unused in set_inverse_trans_unicode(), remove it.
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 2039237b5266..c5f5fa39d7b2 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -241,8 +241,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)
+static void set_inverse_trans_unicode(struct uni_pagedict *p)
{
unsigned int d, r, g;
u16 *inv;
@@ -327,7 +326,7 @@ static void update_user_maps(void)
p = *vc_cons[i].d->vc_uni_pagedir_loc;
if (p && p != q) {
set_inverse_transl(vc_cons[i].d, p, USER_MAP);
- set_inverse_trans_unicode(vc_cons[i].d, p);
+ set_inverse_trans_unicode(p);
q = p;
}
}
@@ -678,7 +677,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
for (enum translation_map m = FIRST_MAP; m <= LAST_MAP; m++)
set_inverse_transl(vc, dict, m);
- set_inverse_trans_unicode(vc, dict);
+ set_inverse_trans_unicode(dict);
out_unlock:
console_unlock();
@@ -741,7 +740,7 @@ int con_set_default_unimap(struct vc_data *vc)
for (enum translation_map m = FIRST_MAP; m <= LAST_MAP; m++)
set_inverse_transl(vc, dict, m);
- set_inverse_trans_unicode(vc, dict);
+ set_inverse_trans_unicode(dict);
dflt = dict;
return err;
}
--
2.36.1
Use FIELD_GET() and GENMASK() helpers instead of direct shifts and ANDs.
This makes the code even more obvious. I didn't know about the helpers
at the time of writing the macros.
Suggested-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 16d0d8f04f0e..9e94ec0e0f83 100644
--- 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 enum translation_map 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 GENMASK(15, 11)
+#define UNI_ROW_BITS GENMASK(10, 6)
+#define UNI_GLYPH_BITS GENMASK( 5, 0)
+
+#define UNI_DIR(uni) FIELD_GET(UNI_DIR_BITS, (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, (dir)) | \
+ FIELD_PREP(UNI_ROW_BITS, (row)) | \
+ FIELD_PREP(UNI_GLYPH_BITS, (glyph)))
/**
* struct uni_pagedict -- unicode directory
--
2.36.1
As a follow-up to the commit 4173f018aae1 (tty/vt: consolemap: rename
and document struct uni_pagedir), rename also the members of struct
vc_data. I.e. pagedir -> pagedict. And while touching all the places,
remove also the unnecessary vc_ prefix.
Suggested-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 46 ++++++++++++-------------
drivers/tty/vt/vt.c | 8 ++---
drivers/usb/misc/sisusbvga/sisusb_con.c | 2 +-
drivers/video/console/vgacon.c | 8 ++---
drivers/video/fbdev/core/fbcon.c | 8 ++---
include/linux/console_struct.h | 4 +--
6 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 3d0e10dac6d9..16d0d8f04f0e 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -296,7 +296,7 @@ u16 inverse_translate(const struct vc_data *conp, u16 glyph, bool use_unicode)
if (glyph >= MAX_GLYPH)
return 0;
- p = *conp->vc_uni_pagedir_loc;
+ p = *conp->uni_pagedict_loc;
if (!p)
return glyph;
@@ -323,7 +323,7 @@ static void update_user_maps(void)
for (i = 0; i < MAX_NR_CONSOLES; i++) {
if (!vc_cons_allocated(i))
continue;
- p = *vc_cons[i].d->vc_uni_pagedir_loc;
+ p = *vc_cons[i].d->uni_pagedict_loc;
if (p && p != q) {
set_inverse_transl(vc_cons[i].d, p, USER_MAP);
set_inverse_trans_unicode(p);
@@ -445,10 +445,10 @@ void con_free_unimap(struct vc_data *vc)
{
struct uni_pagedict *p;
- p = *vc->vc_uni_pagedir_loc;
+ p = *vc->uni_pagedict_loc;
if (!p)
return;
- *vc->vc_uni_pagedir_loc = NULL;
+ *vc->uni_pagedict_loc = NULL;
if (--p->refcount)
return;
con_release_unimap(p);
@@ -463,7 +463,7 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *dict1)
for (cons = 0; cons < MAX_NR_CONSOLES; cons++) {
if (!vc_cons_allocated(cons))
continue;
- dict2 = *vc_cons[cons].d->vc_uni_pagedir_loc;
+ dict2 = *vc_cons[cons].d->uni_pagedict_loc;
if (!dict2 || dict2 == dict1 || dict2->sum != dict1->sum)
continue;
for (d = 0; d < UNI_DIRS; d++) {
@@ -487,7 +487,7 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedict *dict1)
}
if (d == UNI_DIRS) {
dict2->refcount++;
- *conp->vc_uni_pagedir_loc = dict2;
+ *conp->uni_pagedict_loc = dict2;
con_release_unimap(dict1);
kfree(dict1);
return 1;
@@ -531,14 +531,14 @@ con_insert_unipair(struct uni_pagedict *p, u_short unicode, u_short fontpos)
static int con_allocate_new(struct vc_data *vc)
{
- struct uni_pagedict *new, *old = *vc->vc_uni_pagedir_loc;
+ struct uni_pagedict *new, *old = *vc->uni_pagedict_loc;
new = kzalloc(sizeof(*new), GFP_KERNEL);
if (!new)
return -ENOMEM;
new->refcount = 1;
- *vc->vc_uni_pagedir_loc = new;
+ *vc->uni_pagedict_loc = new;
if (old)
old->refcount--;
@@ -549,7 +549,7 @@ static int con_allocate_new(struct vc_data *vc)
/* Caller must hold the lock */
static int con_do_clear_unimap(struct vc_data *vc)
{
- struct uni_pagedict *old = *vc->vc_uni_pagedir_loc;
+ struct uni_pagedict *old = *vc->uni_pagedict_loc;
if (!old || old->refcount > 1)
return con_allocate_new(vc);
@@ -583,7 +583,7 @@ static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
if (ret)
return ERR_PTR(ret);
- new = *vc->vc_uni_pagedir_loc;
+ new = *vc->uni_pagedict_loc;
/*
* uni_pgdir is a 32*32*64 table with rows allocated when its first
@@ -616,7 +616,7 @@ static struct uni_pagedict *con_unshare_unimap(struct vc_data *vc,
ret = con_insert_unipair(new, uni, row[g]);
if (ret) {
old->refcount++;
- *vc->vc_uni_pagedir_loc = old;
+ *vc->uni_pagedict_loc = old;
con_release_unimap(new);
kfree(new);
return ERR_PTR(ret);
@@ -644,7 +644,7 @@ 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 */
- dict = *vc->vc_uni_pagedir_loc;
+ dict = *vc->uni_pagedict_loc;
if (!dict) {
err = -EINVAL;
goto out_unlock;
@@ -704,12 +704,12 @@ int con_set_default_unimap(struct vc_data *vc)
u16 *dfont;
if (dflt) {
- dict = *vc->vc_uni_pagedir_loc;
+ dict = *vc->uni_pagedict_loc;
if (dict == dflt)
return 0;
dflt->refcount++;
- *vc->vc_uni_pagedir_loc = dflt;
+ *vc->uni_pagedict_loc = dflt;
if (dict && !--dict->refcount) {
con_release_unimap(dict);
kfree(dict);
@@ -723,7 +723,7 @@ int con_set_default_unimap(struct vc_data *vc)
if (err)
return err;
- dict = *vc->vc_uni_pagedir_loc;
+ dict = *vc->uni_pagedict_loc;
dfont = dfont_unitable;
for (fontpos = 0; fontpos < 256U; fontpos++)
@@ -734,7 +734,7 @@ int con_set_default_unimap(struct vc_data *vc)
}
if (con_unify_unimap(vc, dict)) {
- dflt = *vc->vc_uni_pagedir_loc;
+ dflt = *vc->uni_pagedict_loc;
return err;
}
@@ -757,14 +757,14 @@ int con_copy_unimap(struct vc_data *dst_vc, struct vc_data *src_vc)
{
struct uni_pagedict *src;
- if (!*src_vc->vc_uni_pagedir_loc)
+ if (!*src_vc->uni_pagedict_loc)
return -EINVAL;
- if (*dst_vc->vc_uni_pagedir_loc == *src_vc->vc_uni_pagedir_loc)
+ if (*dst_vc->uni_pagedict_loc == *src_vc->uni_pagedict_loc)
return 0;
con_free_unimap(dst_vc);
- src = *src_vc->vc_uni_pagedir_loc;
+ src = *src_vc->uni_pagedict_loc;
src->refcount++;
- *dst_vc->vc_uni_pagedir_loc = src;
+ *dst_vc->uni_pagedict_loc = src;
return 0;
}
EXPORT_SYMBOL(con_copy_unimap);
@@ -791,7 +791,7 @@ int con_get_unimap(struct vc_data *vc, ushort ct, ushort __user *uct,
console_lock();
ect = 0;
- dict = *vc->vc_uni_pagedir_loc;
+ dict = *vc->uni_pagedict_loc;
if (!dict)
goto unlock;
@@ -873,7 +873,7 @@ int conv_uni_to_pc(struct vc_data *conp, long ucs)
else if ((ucs & ~UNI_DIRECT_MASK) == UNI_DIRECT_BASE)
return ucs & UNI_DIRECT_MASK;
- dict = *conp->vc_uni_pagedir_loc;
+ dict = *conp->uni_pagedict_loc;
if (!dict)
return -3;
@@ -903,7 +903,7 @@ 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)
+ if (vc_cons_allocated(i) && !*vc_cons[i].d->uni_pagedict_loc)
con_set_default_unimap(vc_cons[i].d);
}
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index c718b0d01e3d..1899b8a5d73e 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1063,10 +1063,10 @@ static void visual_init(struct vc_data *vc, int num, int init)
__module_get(vc->vc_sw->owner);
vc->vc_num = num;
vc->vc_display_fg = &master_display_fg;
- if (vc->vc_uni_pagedir_loc)
+ if (vc->uni_pagedict_loc)
con_free_unimap(vc);
- vc->vc_uni_pagedir_loc = &vc->vc_uni_pagedir;
- vc->vc_uni_pagedir = NULL;
+ vc->uni_pagedict_loc = &vc->uni_pagedict;
+ vc->uni_pagedict = NULL;
vc->vc_hi_font_mask = 0;
vc->vc_complement_mask = 0;
vc->vc_can_do_color = 0;
@@ -1136,7 +1136,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */
visual_init(vc, currcons, 1);
- if (!*vc->vc_uni_pagedir_loc)
+ if (!*vc->uni_pagedict_loc)
con_set_default_unimap(vc);
err = -EINVAL;
diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index dfa0d5ce6012..fcb95fb639e0 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -248,7 +248,7 @@ sisusbcon_init(struct vc_data *c, int init)
*/
kref_get(&sisusb->kref);
- if (!*c->vc_uni_pagedir_loc)
+ if (!*c->uni_pagedict_loc)
con_set_default_unimap(c);
mutex_unlock(&sisusb->lock);
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 058a78b8dbcf..fcdf017e2665 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -367,10 +367,10 @@ static void vgacon_init(struct vc_data *c, int init)
c->vc_complement_mask = 0x7700;
if (vga_512_chars)
c->vc_hi_font_mask = 0x0800;
- p = *c->vc_uni_pagedir_loc;
- if (c->vc_uni_pagedir_loc != &vgacon_uni_pagedir) {
+ p = *c->uni_pagedict_loc;
+ if (c->uni_pagedict_loc != &vgacon_uni_pagedir) {
con_free_unimap(c);
- c->vc_uni_pagedir_loc = &vgacon_uni_pagedir;
+ c->uni_pagedict_loc = &vgacon_uni_pagedir;
vgacon_refcount++;
}
if (!vgacon_uni_pagedir && p)
@@ -392,7 +392,7 @@ static void vgacon_deinit(struct vc_data *c)
if (!--vgacon_refcount)
con_free_unimap(c);
- c->vc_uni_pagedir_loc = &c->vc_uni_pagedir;
+ c->uni_pagedict_loc = &c->uni_pagedict;
con_set_default_unimap(c);
}
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 1be8aa9f8074..238a136c0e11 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -1060,9 +1060,9 @@ static void fbcon_init(struct vc_data *vc, int init)
vc->vc_complement_mask <<= 1;
}
- if (!*svc->vc_uni_pagedir_loc)
+ if (!*svc->uni_pagedict_loc)
con_set_default_unimap(svc);
- if (!*vc->vc_uni_pagedir_loc)
+ if (!*vc->uni_pagedict_loc)
con_copy_unimap(vc, svc);
ops = info->fbcon_par;
@@ -1384,9 +1384,9 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
vc->vc_complement_mask <<= 1;
}
- if (!*svc->vc_uni_pagedir_loc)
+ if (!*svc->uni_pagedict_loc)
con_set_default_unimap(svc);
- if (!*vc->vc_uni_pagedir_loc)
+ if (!*vc->uni_pagedict_loc)
con_copy_unimap(vc, svc);
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
diff --git a/include/linux/console_struct.h b/include/linux/console_struct.h
index f75033f0277f..1518568aaf0f 100644
--- a/include/linux/console_struct.h
+++ b/include/linux/console_struct.h
@@ -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_pagedict *vc_uni_pagedir;
- struct uni_pagedict **vc_uni_pagedir_loc; /* [!] Location of uni_pagedict variable for this console */
+ struct uni_pagedict *uni_pagedict;
+ struct uni_pagedict **uni_pagedict_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
The code expects "translations" to have 256 (E_TABSZ) values. Use the
macro instead of the constant to be explicit about this.
Suggested-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
Notes:
[v2]
- this is new in this series
drivers/tty/vt/consolemap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 5f3e58165b98..f02d21e2a96e 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -38,7 +38,7 @@
#include <linux/vt_kern.h>
#include <linux/string.h>
-static unsigned short translations[][256] = {
+static unsigned short translations[][E_TABSZ] = {
/* 8-bit Latin-1 mapped to Unicode -- trivial mapping */
[LAT1_MAP] = {
0x0000, 0x0001, 0x0002, 0x0003, 0x0004, 0x0005, 0x0006, 0x0007,
--
2.36.1
On 14. 06. 22, 10:17, Ilpo Järvinen wrote:
> On Tue, 14 Jun 2022, Jiri Slaby wrote:
>
>> The code still uses constants (macros) as bounds in loops after commit
>> 17945d317a52 (tty/vt: consolemap: use ARRAY_SIZE()). The contants are at
>> least macros used also in the definition of the arrays. But use
>> ARRAY_SIZE() on two more places to ensure the loops never run out of
>> bounds even if the array definition change.
>>
>> Signed-off-by: Jiri Slaby <[email protected]>
>> ---
>> 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 fff97ae87e00..8aa7a48b3647 100644
>> --- a/drivers/tty/vt/consolemap.c
>> +++ b/drivers/tty/vt/consolemap.c
>> @@ -232,7 +232,7 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
>> }
>> memset(q, 0, MAX_GLYPH);
>>
>> - for (j = 0; j < E_TABSZ; j++) {
>> + for (j = 0; j < ARRAY_SIZE(translations[i]); j++) {
>
> Any particular reason why you left its definition to have 256 instead of
> E_TABSZ (even after the patch series I mean):
>
> static unsigned short translations[][256] = {
I will. (Only if it wasn't so badly chosen name. And even exported to
userspace.)
thanks,
--
js
suse labs
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.
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
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 55fb466361c1..3d0e10dac6d9 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -214,29 +214,29 @@ struct uni_pagedict {
static struct uni_pagedict *dflt;
-static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
+static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *dict,
enum translation_map m)
{
- int j, glyph;
unsigned short *t = translations[m];
- unsigned char *q;
+ unsigned char *inv;
- if (!p)
+ if (!dict)
return;
- q = p->inverse_translations[m];
+ inv = dict->inverse_translations[m];
- if (!q) {
- q = p->inverse_translations[m] = kmalloc(MAX_GLYPH, GFP_KERNEL);
- if (!q)
+ if (!inv) {
+ inv = dict->inverse_translations[m] = kmalloc(MAX_GLYPH,
+ GFP_KERNEL);
+ if (!inv)
return;
}
- memset(q, 0, MAX_GLYPH);
+ memset(inv, 0, MAX_GLYPH);
- for (j = 0; j < ARRAY_SIZE(translations[m]); j++) {
- glyph = conv_uni_to_pc(conp, t[j]);
- if (glyph >= 0 && glyph < MAX_GLYPH && q[glyph] < 32) {
+ for (unsigned int ch = 0; ch < ARRAY_SIZE(translations[m]); ch++) {
+ int glyph = conv_uni_to_pc(conp, t[ch]);
+ if (glyph >= 0 && glyph < MAX_GLYPH && inv[glyph] < 32) {
/* prefer '-' above SHY etc. */
- q[glyph] = j;
+ inv[glyph] = ch;
}
}
}
--
2.36.1
con_do_clear_unimap() sets dflt to NULL and then calls
con_release_unimap() which does the very same as the first thing. So
remove the former as it is apparently superfluous.
Suggested-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 9e94ec0e0f83..5f3e58165b98 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -563,8 +563,6 @@ static int con_do_clear_unimap(struct vc_data *vc)
if (!old || old->refcount > 1)
return con_allocate_new(vc);
- if (old == dflt)
- dflt = NULL;
old->sum = 0;
con_release_unimap(old);
--
2.36.1
The function still uses too vague parameter name after commit
50c92a1b2d50 (tty/vt: consolemap: saner variable names in
set_inverse_trans_unicode()).
So use "dict" instead of "p" for that parameter too.
Reviewed-by: Ilpo Järvinen <[email protected]>
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/tty/vt/consolemap.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index c5f5fa39d7b2..55fb466361c1 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -241,17 +241,17 @@ static void set_inverse_transl(struct vc_data *conp, struct uni_pagedict *p,
}
}
-static void set_inverse_trans_unicode(struct uni_pagedict *p)
+static void set_inverse_trans_unicode(struct uni_pagedict *dict)
{
unsigned int d, r, g;
u16 *inv;
- if (!p)
+ if (!dict)
return;
- inv = p->inverse_trans_unicode;
+ inv = dict->inverse_trans_unicode;
if (!inv) {
- inv = p->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
+ inv = dict->inverse_trans_unicode = kmalloc_array(MAX_GLYPH,
sizeof(*inv), GFP_KERNEL);
if (!inv)
return;
@@ -259,7 +259,7 @@ static void set_inverse_trans_unicode(struct uni_pagedict *p)
memset(inv, 0, MAX_GLYPH * sizeof(*inv));
for (d = 0; d < UNI_DIRS; d++) {
- u16 **dir = p->uni_pgdir[d];
+ u16 **dir = dict->uni_pgdir[d];
if (!dir)
continue;
for (r = 0; r < UNI_DIR_ROWS; r++) {
--
2.36.1