diff mbox series

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

Message ID ed056326540f04b72c97a276fbcc316e1b2f6371.1712053848.git.legion@kernel.org (mailing list archive)
State Superseded
Headers show
Series VT: Add ability to get font requirements | expand

Commit Message

Alexey Gladkov April 2, 2024, 10:32 a.m. UTC 
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.

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(-)

Comments

Jiri Slaby April 2, 2024, 11:02 a.m. UTC | #1 
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

> 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?

> +		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.

> +#define KD_FONT_INFO_FLAG_LOW_SIZE	(1U << 0) /* 256 */
> +#define KD_FONT_INFO_FLAG_HIGH_SIZE	(1U << 1) /* 512 */

_BITUL()

> +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.

> +};

thanks,
Alexey Gladkov April 2, 2024, 1:19 p.m. UTC | #2 
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?
Jiri Slaby April 3, 2024, 5:27 a.m. UTC | #3 
On 02. 04. 24, 15:19, Alexey Gladkov wrote:
>>> --- a/include/uapi/linux/kd.h
>>> +++ b/include/uapi/linux/kd.h
...
>>> +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.

It would not. UAPI structs are set in stone once released.

And in this case, it's likely you would want to know more info about 
fonts in the future.

> Can you suggest how best to do this?

Given you have flags in there already (to state that the structure 
contains more), just add an array of u32 reserved[] space. 3 or 5, I 
would say (to align the struct to 64bit).

thanks,
Alexey Gladkov April 10, 2024, 4:29 p.m. UTC | #4 
On Wed, Apr 03, 2024 at 07:27:55AM +0200, Jiri Slaby wrote:
> On 02. 04. 24, 15:19, Alexey Gladkov wrote:
> >>> --- a/include/uapi/linux/kd.h
> >>> +++ b/include/uapi/linux/kd.h
> ...
> >>> +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.
> 
> It would not. UAPI structs are set in stone once released.
> 
> And in this case, it's likely you would want to know more info about 
> fonts in the future.
> 
> > Can you suggest how best to do this?
> 
> Given you have flags in there already (to state that the structure 
> contains more), just add an array of u32 reserved[] space. 3 or 5, I 
> would say (to align the struct to 64bit).

struct console_font_info {
       __u32 min_width, min_height;    /* minimal font size */
       __u32 max_width, max_height;    /* maximum font size */
       __u32  flags;                   /* KD_FONT_INFO_FLAG_* */
       __u32 reserved[5];              /* This field is reserved forfuture use. Must be 0. */
};

So, struct should be like this ?

I wouldn't add the version to the flags. Maybe it would be better to add a
separate field with the version?
Greg KH April 10, 2024, 5:11 p.m. UTC | #5 
On Wed, Apr 10, 2024 at 06:29:19PM +0200, Alexey Gladkov wrote:
> On Wed, Apr 03, 2024 at 07:27:55AM +0200, Jiri Slaby wrote:
> > On 02. 04. 24, 15:19, Alexey Gladkov wrote:
> > >>> --- a/include/uapi/linux/kd.h
> > >>> +++ b/include/uapi/linux/kd.h
> > ...
> > >>> +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.
> > 
> > It would not. UAPI structs are set in stone once released.
> > 
> > And in this case, it's likely you would want to know more info about 
> > fonts in the future.
> > 
> > > Can you suggest how best to do this?
> > 
> > Given you have flags in there already (to state that the structure 
> > contains more), just add an array of u32 reserved[] space. 3 or 5, I 
> > would say (to align the struct to 64bit).
> 
> struct console_font_info {
>        __u32 min_width, min_height;    /* minimal font size */
>        __u32 max_width, max_height;    /* maximum font size */
>        __u32  flags;                   /* KD_FONT_INFO_FLAG_* */
>        __u32 reserved[5];              /* This field is reserved forfuture use. Must be 0. */
> };
> 
> So, struct should be like this ?
> 
> I wouldn't add the version to the flags. Maybe it would be better to add a
> separate field with the version?

Versions do not work for ioctls, just use flags for stuff.  And you
might want to put flags first?

thanks,

greg k-h
diff mbox series

Patch

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;
+
+	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;
+		ret = con_font_info(vc, &fnt_info);
+		if (ret)
+			return ret;
+		if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
+			return -EFAULT;
+		break;
+	}
+
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/linux/console.h b/include/linux/console.h
index 779d388af8a0..5bea6f6c2042 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -20,6 +20,7 @@ 
 #include <linux/types.h>
 
 struct vc_data;
+struct console_font_info;
 struct console_font_op;
 struct console_font;
 struct module;
@@ -59,6 +60,7 @@  struct consw {
 			unsigned int lines);
 	int	(*con_switch)(struct vc_data *vc);
 	int	(*con_blank)(struct vc_data *vc, int blank, int mode_switch);
+	int	(*con_font_info)(struct vc_data *vc, struct console_font_info *info);
 	int	(*con_font_set)(struct vc_data *vc, struct console_font *font,
 			unsigned int vpitch, unsigned int flags);
 	int	(*con_font_get)(struct vc_data *vc, struct console_font *font,
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index c1f5aebef170..6bda4cc1fe6f 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -32,6 +32,7 @@  void do_blank_screen(int entering_gfx);
 void do_unblank_screen(int leaving_gfx);
 void poke_blanked_console(void);
 int con_font_op(struct vc_data *vc, struct console_font_op *op);
+int con_font_info(struct vc_data *vc, struct console_font_info *info);
 int con_set_cmap(unsigned char __user *cmap);
 int con_get_cmap(unsigned char __user *cmap);
 void scrollback(struct vc_data *vc);
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 6b384065c013..781e086e55bf 100644
--- 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 */
+
+#define KD_FONT_INFO_FLAG_LOW_SIZE	(1U << 0) /* 256 */
+#define KD_FONT_INFO_FLAG_HIGH_SIZE	(1U << 1) /* 512 */
+
+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_* */
+};
+
 /* note: 0x4B00-0x4B4E all have had a value at some time;
    don't reuse for the time being */
-/* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */
+/* note: 0x4B60-0x4B6D, 0x4B70-0x4B73 used above */
 
 #endif /* _UAPI_LINUX_KD_H */