Message-ID: <a5463522-1fa8-4ede-aec9-73f8a0aee196@kernel.org>
Date: Fri, 16 May 2025 08:31:44 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Nicolas Pitre <nico@...xnic.net>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] vt: add VT_GETCONSIZECSRPOS to retrieve console
 size and cursor position

On 15. 05. 25, 18:02, Nicolas Pitre wrote:
> On Thu, 15 May 2025, Jiri Slaby wrote:
> 
>> On 14. 05. 25, 21:42, Nicolas Pitre wrote:
>>> From: Nicolas Pitre <npitre@...libre.com>
>>>
>>> The console dimension and cursor position are available through the
>>> /dev/vcsa interface already. However the /dev/vcsa header format uses
>>> single-byte fields therefore those values are clamped to 255.
>>>
>>> As surprizing as this may seem, some people do use 240-column 67-row
>>> screens (a 1920x1080 monitor with 8x16 pixel fonts) which is getting
>>> close to the limit. Monitors with higher resolution are not uncommon
>>> these days (3840x2160 producing a 480x135 character display) and it is
>>> just a matter of time before someone with, say, a braille display using
>>> the Linux VT console and BRLTTY on such a screen reports a bug about
>>> missing and oddly misaligned screen content.
>>>
>>> Let's add VT_GETCONSIZECSRPOS for the retrieval of console size and cursor
>>> position without byte-sized limitations. The actual console size limit as
>>> encoded in vt.c is 32767x32767 so using a short here is appropriate. Then
>>> this can be used to get the cursor position when /dev/vcsa reports 255.
>>>
>>> The screen dimension may already be obtained using TIOCGWINSZ and adding
>>> the same information to VT_GETCONSIZECSRPOS might be redundant. However
>>> applications that care about cursor position also care about display
>>> size and having 2 separate system calls to obtain them separately is
>>> wasteful. Also, the cursor position can be queried by writing "\e[6n" to
>>> a tty and reading back the result but that may be done only by the actual
>>> application using that tty and not a sideline observer.
>>>
>>> Signed-off-by: Nicolas Pitre <npitre@...libre.com>
>>> ---
>>>    drivers/tty/vt/vt_ioctl.c | 16 ++++++++++++++++
>>>    include/uapi/linux/vt.h   |  9 +++++++++
>>>    2 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
>>> index 4b91072f3a4e..83a3d49535e5 100644
>>> --- a/drivers/tty/vt/vt_ioctl.c
>>> +++ b/drivers/tty/vt/vt_ioctl.c
>>> @@ -951,6 +951,22 @@ int vt_ioctl(struct tty_struct *tty,
>>>     				(unsigned short __user *)arg);
>>>     case VT_WAITEVENT:
>>>    		return vt_event_wait_ioctl((struct vt_event __user *)arg);
>>> +
>>> +	case VT_GETCONSIZECSRPOS:
>>> +	{
>>> +		struct vt_consizecsrpos concsr;
>>> +
>>> +		console_lock();
>>> +		concsr.con_cols = vc->vc_cols;
>>> +		concsr.con_rows = vc->vc_rows;
>>> +		concsr.csr_col = vc->state.x;
>>> +		concsr.csr_row = vc->state.y;
>>> +		console_unlock();
>>
>> Makes a lot of sense!
>>
>>> +		if (copy_to_user(up, &concsr, sizeof(concsr)))
>>> +			return -EFAULT;
>>> +		return 0;
>>> +	}
>>> +
>>>     default:
>>>     	return -ENOIOCTLCMD;
>>>    	}
>>> diff --git a/include/uapi/linux/vt.h b/include/uapi/linux/vt.h
>>> index e9d39c48520a..e93c8910133b 100644
>>> --- a/include/uapi/linux/vt.h
>>> +++ b/include/uapi/linux/vt.h
>>> @@ -84,4 +84,13 @@ struct vt_setactivate {
>>>    
>>>    #define VT_SETACTIVATE	0x560F	/* Activate and set the mode of a
>>>    console */
>>>    
>>> +struct vt_consizecsrpos {
>>> +	unsigned short con_rows;	/* number of console rows */
>>> +	unsigned short con_cols;	/* number of console columns */
>>> +	unsigned short csr_row;		/* current cursor's row */
>>> +	unsigned short csr_col;		/* current cursor's column */
>>
>> Use __u16 pls.
> 
> I beg to differ. Not because __u16 is fundamentally wrong.

Fundamentaly wrong -- for what reason? These types are exactly what 
should be used in userspace APIs instead of bare C types.

> But
> everything else in this file uses only basic C types already and adding
> one struct with __u16 would look odd.

The whole file needs to be fixed, eventually. It's no reason to add 
another bad entry.

> And adding some include to define
> that type would be needed since there are currently no such includes in
> that file currently, and that could potentially cause issues with
> existing consumers of that header file that didn't expect extra
> definitions, etc.

On one side yes, on the other side, sticking with ancient coding style 
while being afraid to include basic headers would be moot.

> So I think that such a change, if it is to happen,
> should be done for the whole file at once and in a separate patch.

Let me bite the bullet and send something. (Likely on Mon -- now queued 
up in my queue for build tests).

>>> +};
>>> +
>>> +#define VT_GETCONSIZECSRPOS 0x5610  /* get console size and cursor position
>>> */
>>
>> Can we define that properly as
>>    _IOR(0x56, 0x10, struct vt_consizecsrpos)
>> ? Note this would still differ from "conflicting":
>> #define VIDIOC_G_FBUF            _IOR('V', 10, struct v4l2_framebuffer)
> 
> Similarly as the reason above: given that no other definitions in that
> file use the _IO*() scheme for historical reasons, it is preferable to
> follow what's already there to avoid unsuspected confusion. The VT layer
> is pretty much unlykely to grow many additional ioctls in the
> foreseeable future so I'd lean towards keeping things simple and in line
> with the existing code.

I tend to disagree. We should not follow bad practices only because they 
are already there.

-- 
js
suse labs

Powered by blists - more mailing lists