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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> > > > ---
> > > > ? 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.
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