Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl

[Alexey Gladkov <legion@kernel.org>]

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@gmx.de>
> > Signed-off-by: Alexey Gladkov <legion@kernel.org>
> > ---
> >   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