On Tue, Apr 02, 2024 at 01:02:20PM +0200, Jiri Slaby wrote:
> Hi,
>
> On 02. 04. 24, 12:32, Alexey Gladkov wrote:
> > Each driver has its own restrictions on font size. There is currently no
> > way to understand what the requirements are. The new ioctl allows
> > userspace to get the minmum and maximum font size values.
>
> minimum
Typo. Sorry.
> > Acked-by: Helge Deller <deller@xxxxxx>
> > Signed-off-by: Alexey Gladkov <legion@xxxxxxxxxx>
> > ---
> > drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
> > drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
> > include/linux/console.h | 2 ++
> > include/linux/vt_kern.h | 1 +
> > include/uapi/linux/kd.h | 13 ++++++++++++-
> > 5 files changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 156efda7c80d..8c2a3d98b5ec 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -4680,6 +4680,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
> > return -ENOSYS;
> > }
> >
> > +int con_font_info(struct vc_data *vc, struct console_font_info *info)
> > +{
> > + int rc = -EINVAL;
>
> This initialization appears to be unneeded.
>
> > +
> > + info->min_height = 0;
> > + info->max_height = max_font_height;
> > +
> > + info->min_width = 0;
> > + info->max_width = max_font_width;
> > +
> > + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> > +
> > + console_lock();
> > + if (vc->vc_mode != KD_TEXT)
> > + rc = -EINVAL;
> > + else if (vc->vc_sw->con_font_info)
> > + rc = vc->vc_sw->con_font_info(vc, info);
> > + else
> > + rc = -ENOSYS;
> > + console_unlock();
> > +
> > + return rc;
> > +}
> > +
> > /*
> > * Interface exported to selection and vcs.
> > */
> > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > index 8c685b501404..b3b4e4b69366 100644
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> > break;
> > }
> >
> > + case KDFONTINFO: {
> > + struct console_font_info fnt_info;
> > +
> > + if (copy_from_user(&fnt_info, up, sizeof(fnt_info)))
> > + return -EFAULT;
>
> Who uses the copied values?
No one. I did it by analogy with KDFONTOP. Thanks!
> > + ret = con_font_info(vc, &fnt_info);
> > + if (ret)
> > + return ret;
> > + if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
>
> We should do the preferred sizeof(*up) here...
>
> > + return -EFAULT;
> > + break;
> > + }
> > +
> > default:
> > return -ENOIOCTLCMD;
> > }
> ...
> > --- a/include/uapi/linux/kd.h
> > +++ b/include/uapi/linux/kd.h
> > @@ -183,8 +183,19 @@ struct console_font {
> >
> > #define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
> >
> > +#define KDFONTINFO 0x4B73 /* font information */
>
> Why not properly define the number using IOC() et al.? K (that 0x4b) is
> even reserved for kd.h.
I just did the same as the numbers above. This entire header does not use
IOC().
Should I convert this header as a separate commit?
> > +#define KD_FONT_INFO_FLAG_LOW_SIZE (1U << 0) /* 256 */
> > +#define KD_FONT_INFO_FLAG_HIGH_SIZE (1U << 1) /* 512 */
>
> _BITUL()
Make sense. I will use it.
> > +struct console_font_info {
> > + unsigned int min_width, min_height; /* minimal font size */
> > + unsigned int max_width, max_height; /* maximum font size */
> > + unsigned int flags; /* KD_FONT_INFO_FLAG_* */
>
> This does not look like a well-defined™ and extendable uapi structure.
> While it won't change anything here, still use fixed-length __u32.
>
> And you should perhaps add some reserved fields. Do not repeat the same
> mistakes as your predecessors with the current kd uapi.
I thought about it, but I thought it would be overengineering.
Can you suggest how best to do this?
--
Rgrds, legion
