* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-04-05 8:56 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Oleg Nesterov, Andrii Nakryiko, Jiri Olsa, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <20240405102203.825c4a2e9d1c2be5b2bffe96@kernel.org>
On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> On Thu, 4 Apr 2024 18:11:09 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 04/05, Masami Hiramatsu wrote:
> > >
> > > Can we make this syscall and uprobe behavior clearer? As you said, if
> > > the application use sigreturn or longjump, it may skip returns and
> > > shadow stack entries are left in the kernel. In such cases, can uretprobe
> > > detect it properly, or just crash the process (or process runs wrongly)?
> >
> > Please see the comment in handle_trampoline(), it tries to detect this case.
> > This patch should not make any difference.
>
> I think you mean this loop will skip and discard the stacked return_instance
> to find the valid one.
>
> ----
> do {
> /*
> * We should throw out the frames invalidated by longjmp().
> * If this chain is valid, then the next one should be alive
> * or NULL; the latter case means that nobody but ri->func
> * could hit this trampoline on return. TODO: sigaltstack().
> */
> next = find_next_ret_chain(ri);
> valid = !next || arch_uretprobe_is_alive(next, RP_CHECK_RET, regs);
>
> instruction_pointer_set(regs, ri->orig_ret_vaddr);
> do {
> if (valid)
> handle_uretprobe_chain(ri, regs);
> ri = free_ret_instance(ri);
> utask->depth--;
> } while (ri != next);
> } while (!valid);
> ----
>
> I think this expects setjmp/longjmp as below
>
> foo() { <- retprobe1
> setjmp()
> bar() { <- retprobe2
> longjmp()
> }
> } <- return to trampoline
>
> In this case, we need to skip retprobe2's instance.
> My concern is, if we can not find appropriate return instance, what happen?
> e.g.
>
> foo() { <-- retprobe1
> bar() { # sp is decremented
> sys_uretprobe() <-- ??
> }
> }
>
> It seems sys_uretprobe() will handle retprobe1 at that point instead of
> SIGILL.
yes, and I think it's fine, you get the consumer called in wrong place,
but it's your fault and kernel won't crash
this can be fixed by checking the syscall is called from the trampoline
and prevent handle_trampoline call if it's not
>
> Can we avoid this with below strict check?
>
> if (ri->stack != regs->sp + expected_offset)
> goto sigill;
hm the current uprobe 'alive' check makes sure the return_instance is above
or at the same stack address, not sure we can match it exactly, need to think
about that more
>
> expected_offset should be 16 (push * 3 - ret) on x64 if we ri->stack is the
> regs->sp right after call.
the syscall trampoline already updates the regs->sp before calling
handle_trampoline
regs->sp += sizeof(r11_cx_ax);
jirka
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-05 1:22 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrii Nakryiko, Jiri Olsa, Steven Rostedt, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD), x86,
linux-api
In-Reply-To: <20240404161108.GG7153@redhat.com>
On Thu, 4 Apr 2024 18:11:09 +0200
Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/05, Masami Hiramatsu wrote:
> >
> > Can we make this syscall and uprobe behavior clearer? As you said, if
> > the application use sigreturn or longjump, it may skip returns and
> > shadow stack entries are left in the kernel. In such cases, can uretprobe
> > detect it properly, or just crash the process (or process runs wrongly)?
>
> Please see the comment in handle_trampoline(), it tries to detect this case.
> This patch should not make any difference.
I think you mean this loop will skip and discard the stacked return_instance
to find the valid one.
----
do {
/*
* We should throw out the frames invalidated by longjmp().
* If this chain is valid, then the next one should be alive
* or NULL; the latter case means that nobody but ri->func
* could hit this trampoline on return. TODO: sigaltstack().
*/
next = find_next_ret_chain(ri);
valid = !next || arch_uretprobe_is_alive(next, RP_CHECK_RET, regs);
instruction_pointer_set(regs, ri->orig_ret_vaddr);
do {
if (valid)
handle_uretprobe_chain(ri, regs);
ri = free_ret_instance(ri);
utask->depth--;
} while (ri != next);
} while (!valid);
----
I think this expects setjmp/longjmp as below
foo() { <- retprobe1
setjmp()
bar() { <- retprobe2
longjmp()
}
} <- return to trampoline
In this case, we need to skip retprobe2's instance.
My concern is, if we can not find appropriate return instance, what happen?
e.g.
foo() { <-- retprobe1
bar() { # sp is decremented
sys_uretprobe() <-- ??
}
}
It seems sys_uretprobe() will handle retprobe1 at that point instead of
SIGILL.
Can we avoid this with below strict check?
if (ri->stack != regs->sp + expected_offset)
goto sigill;
expected_offset should be 16 (push * 3 - ret) on x64 if we ri->stack is the
regs->sp right after call.
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Oleg Nesterov @ 2024-04-04 16:11 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Andrii Nakryiko, Jiri Olsa, Steven Rostedt, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD), x86,
linux-api
In-Reply-To: <20240405005405.9bcbe5072d2f32967501edb3@kernel.org>
On 04/05, Masami Hiramatsu wrote:
>
> Can we make this syscall and uprobe behavior clearer? As you said, if
> the application use sigreturn or longjump, it may skip returns and
> shadow stack entries are left in the kernel. In such cases, can uretprobe
> detect it properly, or just crash the process (or process runs wrongly)?
Please see the comment in handle_trampoline(), it tries to detect this case.
This patch should not make any difference.
Oleg.
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-04 16:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Andrii Nakryiko, Masami Hiramatsu, Steven Rostedt, linux-api,
Oleg Nesterov, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, linux-kernel, linux-trace-kernel, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86
In-Reply-To: <Zg6V8y2-OP_9at2l@krava>
On Thu, 4 Apr 2024 13:58:43 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Wed, Apr 03, 2024 at 07:00:07PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > Check rt_sigreturn syscall (manpage at [0], for example).
> >
> > sigreturn() exists only to allow the implementation of signal
> > handlers. It should never be called directly. (Indeed, a simple
> > sigreturn() wrapper in the GNU C library simply returns -1, with
> > errno set to ENOSYS.) Details of the arguments (if any) passed
> > to sigreturn() vary depending on the architecture. (On some
> > architectures, such as x86-64, sigreturn() takes no arguments,
> > since all of the information that it requires is available in the
> > stack frame that was previously created by the kernel on the
> > user-space stack.)
> >
> > This is a very similar use case. Also, check its source code in
> > arch/x86/kernel/signal_64.c. It sends SIGSEGV to the calling process
> > on any sign of something not being right. It's exactly the same with
> > sys_uretprobe.
> >
> > [0] https://man7.org/linux/man-pages/man2/sigreturn.2.html
> >
> > > And the number of syscalls are limited resource.
> >
> > We have almost 500 of them, it didn't seems like adding 1-2 for good
> > reasons would be a problem. Can you please point to where the limits
> > on syscalls as a resource are described? I'm curious to learn.
> >
> > >
> > > I'm actually not sure how much we need to care of it, but adding a new
> > > syscall is worth to be discussed carefully because all of them are
> > > user-space compatibility.
> >
> > Absolutely, it's a good discussion to have.
> >
> > >
> > > > > > > Also, we should run syzkaller on this syscall. And if uretprobe is
> > > > > >
> > > > > > right, I'll check on syzkaller
> > > > > >
> > > > > > > set in the user function, what happen if the user function directly
> > > > > > > calls this syscall? (maybe it consumes shadow stack?)
> > > > > >
> > > > > > the process should receive SIGILL if there's no pending uretprobe for
> > > > > > the current task, or it will trigger uretprobe if there's one pending
> > > > >
> > > > > No, that is too aggressive and not safe. Since the syscall is exposed to
> > > > > user program, it should return appropriate error code instead of SIGILL.
> > > > >
> > > >
> > > > This is the way it is today with uretprobes even through interrupt.
> > >
> > > I doubt that the interrupt (exception) and syscall should be handled
> > > differently. Especially, this exception is injected by uprobes but
> > > syscall will be caused by itself. But syscall can be called from user
> > > program (of couse this works as sys_kill(self, SIGILL)).
> >
> > Yep, I'd keep the behavior the same between uretprobes implemented
> > through int3 and sys_uretprobe.
>
> +1
>
> >
> > >
> > > > E.g., it could happen that user process is using fibers and is
> > > > replacing stack pointer without kernel realizing this, which will
> > > > trigger some defensive checks in uretprobe handling code and kernel
> > > > will send SIGILL because it can't support such cases. This is
> > > > happening today already, and it works fine in practice (except for
> > > > applications that manually change stack pointer, too bad, you can't
> > > > trace them with uretprobes, unfortunately).
> > >
> > > OK, we at least need to document it.
> >
> > +1, yep
> >
> > >
> > > >
> > > > So I think it's absolutely adequate to have this behavior if the user
> > > > process is *intentionally* abusing this API.
> > >
> > > Of course user expected that it is abusing. So at least we need to
> > > add a document that this syscall number is reserved to uprobes and
> > > user program must not use it.
> > >
> >
> > Totally agree about documenting this.
>
> ok there's map page on sigreturn.. do you think we should add man page
> for uretprobe or you can think of some other place to document it?
I think it is better to have a man-page. Anyway, to discuss and explain
this syscall, the man-page is a good format to describe it.
>
> >
> > > >
> > > > > >
> > > > > > but we could limit the syscall to be executed just from the trampoline,
> > > > > > that should prevent all the user space use cases, I'll do that in next
> > > > > > version and add more tests for that
> > > > >
> > > > > Why not limit? :) The uprobe_handle_trampoline() expects it is called
> > > > > only from the trampoline, so it is natural to check the caller address.
> > > > > (and uprobe should know where is the trampoline)
> > > > >
> > > > > Since the syscall is always exposed to the user program, it should
> > > > > - Do nothing and return an error unless it is properly called.
> > > > > - check the prerequisites for operation strictly.
> > > > > I concern that new system calls introduce vulnerabilities.
> > > > >
> > > >
> > > > As Oleg and Jiri mentioned, this syscall can't harm kernel or other
> > > > processes, only the process that is abusing the API. So any extra
> > > > checks that would slow down this approach is an unnecessary overhead
> > > > and complication that will never be useful in practice.
> > >
> > > I think at least it should check the caller address to ensure the
> > > address is in the trampoline.
> > > But anyway, uprobes itself can break the target process, so no one
> > > might care if this system call breaks the process now.
> >
> > If we already have an expected range of addresses, then I think it's
> > fine to do a quick unlikely() check. I'd be more concerned if we need
> > to do another lookup or search to just validate this. I'm sure Jiri
> > will figure it out.
>
> Oleg mentioned the trampoline address check could race with another
> thread's mremap call, however trap is using that check as well, it
> still seems like good idea to do it also in the uretprobe syscall
Yeah, and also, can we add a stack pointer check if the trampoline is
shared with other probe points?
Thank you,
>
> jirka
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-04 15:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Steven Rostedt, Oleg Nesterov, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD), x86,
linux-api
In-Reply-To: <CAEf4BzYH60TwvBipHWB_kUqZZ6D-iUVnnFsBv06imRikK3o-bg@mail.gmail.com>
On Wed, 3 Apr 2024 19:00:07 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Wed, Apr 3, 2024 at 5:58 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Wed, 3 Apr 2024 09:58:12 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > On Wed, Apr 3, 2024 at 7:09 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > >
> > > > On Wed, 3 Apr 2024 11:47:41 +0200
> > > > Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > > On Wed, Apr 03, 2024 at 10:07:08AM +0900, Masami Hiramatsu wrote:
> > > > > > Hi Jiri,
> > > > > >
> > > > > > On Tue, 2 Apr 2024 11:33:00 +0200
> > > > > > Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > >
> > > > > > > Adding uretprobe syscall instead of trap to speed up return probe.
> > > > > >
> > > > > > This is interesting approach. But I doubt we need to add additional
> > > > > > syscall just for this purpose. Can't we use another syscall or ioctl?
> > > > >
> > > > > so the plan is to optimize entry uprobe in a similar way and given
> > > > > the syscall is not a scarce resource I wanted to add another syscall
> > > > > for that one as well
> > > > >
> > > > > tbh I'm not sure sure which syscall or ioctl to reuse for this, it's
> > > > > possible to do that, the trampoline will just have to save one or
> > > > > more additional registers, but adding new syscall seems cleaner to me
> > > >
> > > > Hmm, I think a similar syscall is ptrace? prctl may also be a candidate.
> > >
> > > I think both ptrace and prctl are for completely different use cases
> > > and it would be an abuse of existing API to reuse them for uretprobe
> > > tracing. Also, keep in mind, that any extra argument that has to be
> > > passed into this syscall means that we need to complicate and slow
> > > generated assembly code that is injected into user process (to
> > > save/restore registers) and also kernel-side (again, to deal with all
> > > the extra registers that would be stored/restored on stack).
> > >
> > > Given syscalls are not some kind of scarce resources, what's the
> > > downside to have a dedicated and simple syscall?
> >
> > Syscalls are explicitly exposed to user space, thus, even if it is used
> > ONLY for a very specific situation, it is an official kernel interface,
> > and need to care about the compatibility. (If it causes SIGILL unless
> > a specific use case, I don't know there is a "compatibility".)
>
> Check rt_sigreturn syscall (manpage at [0], for example).
>
> sigreturn() exists only to allow the implementation of signal
> handlers. It should never be called directly. (Indeed, a simple
> sigreturn() wrapper in the GNU C library simply returns -1, with
> errno set to ENOSYS.) Details of the arguments (if any) passed
> to sigreturn() vary depending on the architecture. (On some
> architectures, such as x86-64, sigreturn() takes no arguments,
> since all of the information that it requires is available in the
> stack frame that was previously created by the kernel on the
> user-space stack.)
>
> This is a very similar use case. Also, check its source code in
> arch/x86/kernel/signal_64.c. It sends SIGSEGV to the calling process
> on any sign of something not being right. It's exactly the same with
> sys_uretprobe.
>
> [0] https://man7.org/linux/man-pages/man2/sigreturn.2.html
Thanks for a good example.
Hm, in the case of rt_sigreturn, it has no other way to do it so it
needs to use syscall. OTOH, sys_uretprobe is only for performance
optimization, and the performance may depend on the architecture.
> > And the number of syscalls are limited resource.
>
> We have almost 500 of them, it didn't seems like adding 1-2 for good
> reasons would be a problem. Can you please point to where the limits
> on syscalls as a resource are described? I'm curious to learn.
Syscall table is compiled as a fixed array, so if we increase
the number, we need more tables. Of course this just increase 1 entry
and at least for x86 we already allocated bigger table, so it is OK.
But I'm just afraid if we can add more syscalls without any clear
rules, we may fill the tables with more specific syscalls.
Ah, we also should follow this document.
https://docs.kernel.org/process/adding-syscalls.html
Let me Cc linux-api@vger.kernel.org.
> >
> > I'm actually not sure how much we need to care of it, but adding a new
> > syscall is worth to be discussed carefully because all of them are
> > user-space compatibility.
>
> Absolutely, it's a good discussion to have.
Thanks, if this is discussed enough and agreed from other maintainers,
I can safely pick this on my tree.
>
> >
> > > > > > Also, we should run syzkaller on this syscall. And if uretprobe is
> > > > >
> > > > > right, I'll check on syzkaller
> > > > >
> > > > > > set in the user function, what happen if the user function directly
> > > > > > calls this syscall? (maybe it consumes shadow stack?)
> > > > >
> > > > > the process should receive SIGILL if there's no pending uretprobe for
> > > > > the current task, or it will trigger uretprobe if there's one pending
> > > >
> > > > No, that is too aggressive and not safe. Since the syscall is exposed to
> > > > user program, it should return appropriate error code instead of SIGILL.
> > > >
> > >
> > > This is the way it is today with uretprobes even through interrupt.
> >
> > I doubt that the interrupt (exception) and syscall should be handled
> > differently. Especially, this exception is injected by uprobes but
> > syscall will be caused by itself. But syscall can be called from user
> > program (of couse this works as sys_kill(self, SIGILL)).
>
> Yep, I'd keep the behavior the same between uretprobes implemented
> through int3 and sys_uretprobe.
OK, so this syscall is something like coding int3 without debugger.
> >
> > > E.g., it could happen that user process is using fibers and is
> > > replacing stack pointer without kernel realizing this, which will
> > > trigger some defensive checks in uretprobe handling code and kernel
> > > will send SIGILL because it can't support such cases. This is
> > > happening today already, and it works fine in practice (except for
> > > applications that manually change stack pointer, too bad, you can't
> > > trace them with uretprobes, unfortunately).
> >
> > OK, we at least need to document it.
>
> +1, yep
Can we make this syscall and uprobe behavior clearer? As you said, if
the application use sigreturn or longjump, it may skip returns and
shadow stack entries are left in the kernel. In such cases, can uretprobe
detect it properly, or just crash the process (or process runs wrongly)?
>
> >
> > >
> > > So I think it's absolutely adequate to have this behavior if the user
> > > process is *intentionally* abusing this API.
> >
> > Of course user expected that it is abusing. So at least we need to
> > add a document that this syscall number is reserved to uprobes and
> > user program must not use it.
> >
>
> Totally agree about documenting this.
>
> > >
> > > > >
> > > > > but we could limit the syscall to be executed just from the trampoline,
> > > > > that should prevent all the user space use cases, I'll do that in next
> > > > > version and add more tests for that
> > > >
> > > > Why not limit? :) The uprobe_handle_trampoline() expects it is called
> > > > only from the trampoline, so it is natural to check the caller address.
> > > > (and uprobe should know where is the trampoline)
> > > >
> > > > Since the syscall is always exposed to the user program, it should
> > > > - Do nothing and return an error unless it is properly called.
> > > > - check the prerequisites for operation strictly.
> > > > I concern that new system calls introduce vulnerabilities.
> > > >
> > >
> > > As Oleg and Jiri mentioned, this syscall can't harm kernel or other
> > > processes, only the process that is abusing the API. So any extra
> > > checks that would slow down this approach is an unnecessary overhead
> > > and complication that will never be useful in practice.
> >
> > I think at least it should check the caller address to ensure the
> > address is in the trampoline.
> > But anyway, uprobes itself can break the target process, so no one
> > might care if this system call breaks the process now.
>
> If we already have an expected range of addresses, then I think it's
> fine to do a quick unlikely() check. I'd be more concerned if we need
> to do another lookup or search to just validate this. I'm sure Jiri
> will figure it out.
Good.
>
> >
> > >
> > > Also note that sys_uretprobe is a kind of internal and unstable API
> > > and it is explicitly called out that its contract can change at any
> > > time and user space shouldn't rely on it. It's purely for the kernel's
> > > own usage.
> >
> > Is that OK to use a syscall as "internal" and "unstable" API?
>
> See above about rt_sigreturn. It seems like yes, for some highly
> specialized syscalls it is the case already.
OK, but as I said it is just for performance optimization, that is
a bit different from rt_sigreturn case.
Thank you,
> >
> > >
> > > So let's please keep it fast and simple.
> > >
> > >
> > > > Thank you,
> > > >
> > > >
> > > > >
> > > > > thanks,
> > > > > jirka
> > > > >
> > > > >
> > > > > >
> > >
> > > [...]
> >
> >
> > ([OT] If we can add syscall so casually, I would like to add sys_traceevent
> > for recording user space events :-) .)
>
> Have you proposed this upstream? :) I have no clue and no opinion about it...
>
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-04-03 15:42 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <871q7md0ak.ffs@tglx>
On Wed, Apr 03 2024 at 15:48, Thomas Gleixner wrote:
> On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> It is just an implementation detail whether you use a flag or a
> clockid. You can carry the clockid for the clocks which actually can be
> read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
>
> struct ptp_sys_offset_extended {
> unsigned int n_samples; /* Desired number of measurements. */
> clockid_t clockid;
> unsigned int rsv[2]; /* Reserved for future use. */
> };
>
> and in the IOCTL:
>
> if (extoff->clockid != CLOCK_MONOTONIC_RAW)
> return -EINVAL;
That should obviously be:
switch (extoff->clockid) {
case CLOCK_REALTIME:
case CLOCK_MONOTONIC_RAW:
break;
default:
return -EINVAL;
}
...
> sts.clockid = extoff->clockid;
>
> and it all just works, no?
>
> I have no problem to decide that PTP_SYS_OFFSET will not get this
> treatment and the drivers have to be converted over to
> PTP_SYS_OFFSET_EXTENDED.
>
> But adding yet another callback just to carry a clockid as argument is a
> more than pointless exercise as I demonstrated.
>
> Thanks,
>
> tglx
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-04-03 13:48 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <CAF2d9jjg0PEgPorXdrBHVkvz-fmUV7UXUPqnpQGVEvgXTpHY0A@mail.gmail.com>
On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> The modification that you have proposed (in a couple of posts back)
> would work but it's still not ideal since the pre/post ts are not
> close enough as they are currently (properly implemented!)
> gettimex64() would have. The only way to do that would be to have
> another ioctl as I have proposed which is a superset of current
> gettimex64 and pre-post collection is the closest possible.
Errm. What I posted as sketch _is_ using gettimex64() with the extra
twist of the flag vs. a clockid (which is an implementation detail) and
the difference that I carry the information in ptp_system_timestamp
instead of needing a new argument clockid to all existing callbacks
because the modification to ptp_read_prets() and postts() will just be
sufficient, no?
For the case where the driver does not provide gettimex64() then the
extension of the original offset ioctl is still providing a better
mechanism than the proposed syscall.
I also clearly said that all drivers should be converted over to
gettimex64().
> Having said that, the 'flag' modification proposal is a good backup
> for the drivers that don't have good implementation (close enough but
> not ideal). Also, you don't need a new ioctl-op. So if we really want
> precision, I believe, we need a new ioctl op (with supporting
> implementation similar to the mlx4 code above). but we want to save
> the new ioctl-op and have less precision then proposed modification
> would work fine.
I disagree. The existing gettimex64() is good enough if the driver
implements it correctly today. If not then those drivers need to be
fixed independent of this.
So assumed that a driver does:
gettimex64()
ptp_prets(sts);
read_clock();
ptp_postts(sts);
today then having:
static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
if (sts) {
if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
ktime_get_raw_ts64(&sts->pre_ts);
else
ktime_get_real_ts64(&sts->pre_ts);
}
}
static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
if (sts) {
if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
ktime_get_raw_ts64(&sts->post_ts);
else
ktime_get_real_ts64(&sts->post_ts);
}
}
or
static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
if (sts) {
switch (sts->clockid) {
case CLOCK_MONOTONIC_RAW:
time_get_raw_ts64(&sts->pre_ts);
break;
case CLOCK_REALTIME:
ktime_get_real_ts64(&sts->pre_ts);
break;
}
}
}
static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
if (sts) {
switch (sts->clockid) {
case CLOCK_MONOTONIC_RAW:
time_get_raw_ts64(&sts->post_ts);
break;
case CLOCK_REALTIME:
ktime_get_real_ts64(&sts->post_ts);
break;
}
}
}
is doing the exact same thing as your proposal but without touching any
driver which implements gettimex64() correctly at all.
While your proposal requires to touch every single driver for no reason,
no?
It is just an implementation detail whether you use a flag or a
clockid. You can carry the clockid for the clocks which actually can be
read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
struct ptp_sys_offset_extended {
unsigned int n_samples; /* Desired number of measurements. */
clockid_t clockid;
unsigned int rsv[2]; /* Reserved for future use. */
};
and in the IOCTL:
if (extoff->clockid != CLOCK_MONOTONIC_RAW)
return -EINVAL;
sts.clockid = extoff->clockid;
and it all just works, no?
I have no problem to decide that PTP_SYS_OFFSET will not get this
treatment and the drivers have to be converted over to
PTP_SYS_OFFSET_EXTENDED.
But adding yet another callback just to carry a clockid as argument is a
more than pointless exercise as I demonstrated.
Thanks,
tglx
^ permalink raw reply
* Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Jiri Slaby @ 2024-04-03 5:27 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Greg Kroah-Hartman, LKML, kbd, linux-api, linux-fbdev,
linux-serial, Helge Deller
In-Reply-To: <ZgwF72yHH_0-A4FW@example.org>
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,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH v4 2/3] VT: Add KDFONTINFO ioctl
From: Jiri Slaby @ 2024-04-03 5:05 UTC (permalink / raw)
To: Alexey Gladkov, Greg Kroah-Hartman
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial, Helge Deller
In-Reply-To: <7cd32f988a147d7617742c9e074c753de0c6bc1f.1712080158.git.legion@kernel.org>
First, there was no need to send this v4 so quickly. Provided we have
not settled in v3... This makes the review process painful.
And then:
On 02. 04. 24, 19:50, 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 minimum and maximum font size values.
>
> Acked-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
...
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> break;
> }
>
> + case KDFONTINFO: {
> + struct console_font_info fnt_info;
> +
> + ret = con_font_info(vc, &fnt_info);
> + if (ret)
> + return ret;
> + if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
sizeof, I already commented.
Now you leak info to userspace unless everyone sets everything in
fnt_info. IOW, do memset() above.
> + return -EFAULT;
> + break;
> + }
> +
> default:
> return -ENOIOCTLCMD;
> }
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH v4 2/3] VT: Add KDFONTINFO ioctl
From: Greg Kroah-Hartman @ 2024-04-03 4:55 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Jiri Slaby, LKML, kbd, linux-api, linux-fbdev, linux-serial,
Helge Deller
In-Reply-To: <7cd32f988a147d7617742c9e074c753de0c6bc1f.1712080158.git.legion@kernel.org>
On Tue, Apr 02, 2024 at 07:50:45PM +0200, Alexey Gladkov wrote:
> +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_* */
> +};
As Jiri said, this will not work for an ioctl structure at all, sorry.
Please read the kernel documentation about how to write a new ioctl for
how to do this correctly (hint, you can not use 'unsigned int' in a
structure that crosses the kernel/user boundry for new ioctls.)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Mahesh Bandewar (महेश बंडेवार) @ 2024-04-02 23:37 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <877chfcrx3.ffs@tglx>
On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, Apr 02 2024 at 14:16, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Apr 2, 2024 at 2:25 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> Works as well. I'm not seing the point for CLOCK_MONOTONIC and the
> >> change logs are not really telling anything about the problem being
> >> solved....
> >>
> > https://lore.kernel.org/lkml/20240104212431.3275688-1-maheshb@google.com/T/#:~:text=*%20[PATCHv3%20net%2Dnext%200/3]%20add%20ptp_gettimex64any()%20API,21:24%20Mahesh%20Bandewar%200%20siblings%2C%200%20replies;
> >
> > This is the cover letter where I tried to explain the need for this.
>
> The justification for a patch needs to be in the change log and not in
> the cover letter because the cover letter is not part of the git
> history.
>
ack
> > Granted, my current use case is for CLOCK_MONOTONIC_RAW but just
> > because I don't have a use case doesn't mean someone else may not have
> > it and hence added it.
>
> Then why did you not five other clock IDs? Someone else might have a
> use case, no?
>
> While a syscall/ioctl should be flexible for future use, the kernel does
> not add features just because there might be some use case. It's
> documented how this works.
>
I see your point. I don't mind removing the CLOCK_MONOTONIC for now
and just have CLOCK_REALTIME and CLOCK_MONOTONIC_RAW support. Also as
I mentioned, it will be just a matter of adding new clock-ids and
support for the pre/post-ts for respective clock-ids if needed in the
future.
The modification that you have proposed (in a couple of posts back)
would work but it's still not ideal since the pre/post ts are not
close enough as they are currently (properly implemented!)
gettimex64() would have. The only way to do that would be to have
another ioctl as I have proposed which is a superset of current
gettimex64 and pre-post collection is the closest possible.
Here is my sample mlx4 (since I use that) of the new ioctl method
(just for the reference)
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -43,6 +43,7 @@
#include <linux/io-mapping.h>
#include <linux/delay.h>
#include <linux/etherdevice.h>
+#include <linux/ptp_clock_kernel.h>
#include <net/devlink.h>
#include <uapi/rdma/mlx4-abi.h>
@@ -1929,7 +1930,7 @@ static void unmap_bf_area(struct mlx4_dev *dev)
io_mapping_free(mlx4_priv(dev)->bf_mapping);
}
-u64 mlx4_read_clock(struct mlx4_dev *dev)
+u64 mlx4_read_clock(struct mlx4_dev *dev, struct ptp_system_timestamp
*sts, int clkid)
{
u32 clockhi, clocklo, clockhi1;
u64 cycles;
@@ -1937,7 +1938,13 @@ u64 mlx4_read_clock(struct mlx4_dev *dev)
struct mlx4_priv *priv = mlx4_priv(dev);
for (i = 0; i < 10; i++) {
- clockhi = swab32(readl(priv->clock_mapping));
+ if (sts) {
+ ptp_read_any_prets(sts, clkid);
+ clockhi = swab32(readl(priv->clock_mapping));
+ ptp_read_any_postts(sts, clkid);
+ } else {
+ clockhi = swab32(readl(priv->clock_mapping));
+ }
clocklo = swab32(readl(priv->clock_mapping + 4));
clockhi1 = swab32(readl(priv->clock_mapping));
if (clockhi == clockhi1)
Having said that, the 'flag' modification proposal is a good backup
for the drivers that don't have good implementation (close enough but
not ideal). Also, you don't need a new ioctl-op. So if we really want
precision, I believe, we need a new ioctl op (with supporting
implementation similar to the mlx4 code above). but we want to save
the new ioctl-op and have less precision then proposed modification
would work fine.
> Thanks,
>
> tglx
>
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-04-02 22:37 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <CAF2d9jj6km7aVSqgcOE-b-A-WDH2TJNGzGy-5MRyw5HrzbqhaA@mail.gmail.com>
On Tue, Apr 02 2024 at 14:16, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Tue, Apr 2, 2024 at 2:25 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> Works as well. I'm not seing the point for CLOCK_MONOTONIC and the
>> change logs are not really telling anything about the problem being
>> solved....
>>
> https://lore.kernel.org/lkml/20240104212431.3275688-1-maheshb@google.com/T/#:~:text=*%20[PATCHv3%20net%2Dnext%200/3]%20add%20ptp_gettimex64any()%20API,21:24%20Mahesh%20Bandewar%200%20siblings%2C%200%20replies;
>
> This is the cover letter where I tried to explain the need for this.
The justification for a patch needs to be in the change log and not in
the cover letter because the cover letter is not part of the git
history.
> Granted, my current use case is for CLOCK_MONOTONIC_RAW but just
> because I don't have a use case doesn't mean someone else may not have
> it and hence added it.
Then why did you not five other clock IDs? Someone else might have a
use case, no?
While a syscall/ioctl should be flexible for future use, the kernel does
not add features just because there might be some use case. It's
documented how this works.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Mahesh Bandewar (महेश बंडेवार) @ 2024-04-02 21:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <87il10ce1g.ffs@tglx>
On Tue, Apr 2, 2024 at 2:25 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, Apr 01 2024 at 22:42, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Mon, Apr 1, 2024 at 1:46 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> So if there is a backwards compability issue with PTP_SYS_OFFSET2, then
> >> you need to introduce PTP_SYS_OFFSET3. The PTP_SYS_*2 variants were
> >> introduced to avoid backwards compatibility issues as well, but
> >> unfortunately that did not address the reserved fields problem for
> >> PTP_SYS_OFFSET2. PTP_SYS_OFFSET_EXTENDED2 should just work, but maybe
> >> the PTP maintainers want a full extension to '3'. Either way is fine.
> >>
> > https://patchwork.kernel.org/project/netdevbpf/patch/20240104212436.3276057-1-maheshb@google.com/
> >
> > This was my attempt to solve a similar issue with the new ioctl op to
> > avoid backward compatibility issues. Instead of flags I used the
> > clockid_t in a similar fashion.
>
> Works as well. I'm not seing the point for CLOCK_MONOTONIC and the
> change logs are not really telling anything about the problem being
> solved....
>
https://lore.kernel.org/lkml/20240104212431.3275688-1-maheshb@google.com/T/#:~:text=*%20[PATCHv3%20net%2Dnext%200/3]%20add%20ptp_gettimex64any()%20API,21:24%20Mahesh%20Bandewar%200%20siblings%2C%200%20replies;
This is the cover letter where I tried to explain the need for this.
Granted, my current use case is for CLOCK_MONOTONIC_RAW but just
because I don't have a use case doesn't mean someone else may not have
it and hence added it. In either way it's just a matter of
adding/removing another flag/clock-id.
Thanks,
--mahesh..
> Thanks,
>
> tglx
^ permalink raw reply
* [PATCH v4 3/3] VT: Allow to get max font width and height
From: Alexey Gladkov @ 2024-04-02 17:50 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial, Helge Deller
In-Reply-To: <cover.1712080158.git.legion@kernel.org>
The Console drivers has more restrictive font size limits than vt_ioctl.
This leads to errors that are difficult to handle. If a font whose size
is not supported is used, an EINVAL error will be returned, which is
also returned in case of errors in the font itself. At the moment there
is no way to understand what font sizes the current console driver
supports.
To solve this problem, we need to transfer information about the
supported font to userspace from the console driver.
Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/video/console/newport_con.c | 21 +++++++++++++++++----
drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
drivers/video/fbdev/core/fbcon.c | 16 ++++++++++++++++
4 files changed, 76 insertions(+), 7 deletions(-)
diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
index a51cfc1d560e..6167f45326ac 100644
--- a/drivers/video/console/newport_con.c
+++ b/drivers/video/console/newport_con.c
@@ -33,6 +33,9 @@
#define NEWPORT_LEN 0x10000
+#define NEWPORT_MAX_FONT_WIDTH 8
+#define NEWPORT_MAX_FONT_HEIGHT 16
+
#define FONT_DATA ((unsigned char *)font_vga_8x16.data)
static unsigned char *font_data[MAX_NR_CONSOLES];
@@ -328,8 +331,8 @@ static void newport_init(struct vc_data *vc, bool init)
{
int cols, rows;
- cols = newport_xsize / 8;
- rows = newport_ysize / 16;
+ cols = newport_xsize / NEWPORT_MAX_FONT_WIDTH;
+ rows = newport_ysize / NEWPORT_MAX_FONT_HEIGHT;
vc->vc_can_do_color = 1;
if (init) {
vc->vc_cols = cols;
@@ -507,8 +510,8 @@ static int newport_set_font(int unit, const struct console_font *op,
/* ladis: when I grow up, there will be a day... and more sizes will
* be supported ;-) */
- if ((w != 8) || (h != 16) || (vpitch != 32)
- || (op->charcount != 256 && op->charcount != 512))
+ if ((w != NEWPORT_MAX_FONT_WIDTH) || (h != NEWPORT_MAX_FONT_HEIGHT) ||
+ (vpitch != 32) || (op->charcount != 256 && op->charcount != 512))
return -EINVAL;
if (!(new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size,
@@ -570,6 +573,15 @@ static int newport_font_default(struct vc_data *vc, struct console_font *op,
return newport_set_def_font(vc->vc_num, op);
}
+static int newport_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = info->max_width = NEWPORT_MAX_FONT_WIDTH;
+ info->min_height = info->max_height = NEWPORT_MAX_FONT_HEIGHT;
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static int newport_font_set(struct vc_data *vc, const struct console_font *font,
unsigned int vpitch, unsigned int flags)
{
@@ -689,6 +701,7 @@ const struct consw newport_con = {
.con_scroll = newport_scroll,
.con_switch = newport_switch,
.con_blank = newport_blank,
+ .con_font_info = newport_font_info,
.con_font_set = newport_font_set,
.con_font_default = newport_font_default,
.con_save_screen = newport_save_screen
diff --git a/drivers/video/console/sticon.c b/drivers/video/console/sticon.c
index 4c7b4959a1aa..490e6b266a31 100644
--- a/drivers/video/console/sticon.c
+++ b/drivers/video/console/sticon.c
@@ -56,6 +56,11 @@
#define BLANK 0
static int vga_is_gfx;
+#define STICON_MIN_FONT_WIDTH 6
+#define STICON_MIN_FONT_HEIGHT 6
+#define STICON_MAX_FONT_WIDTH 32
+#define STICON_MAX_FONT_HEIGHT 32
+
#define STI_DEF_FONT sticon_sti->font
/* borrowed from fbcon.c */
@@ -166,8 +171,10 @@ static int sticon_set_font(struct vc_data *vc, const struct console_font *op,
struct sti_cooked_font *cooked_font;
unsigned char *data = op->data, *p;
- if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
- || (op->charcount != 256 && op->charcount != 512))
+ if (!in_range(w, STICON_MIN_FONT_WIDTH, STICON_MAX_FONT_WIDTH) ||
+ !in_range(h, STICON_MIN_FONT_HEIGHT, STICON_MAX_FONT_HEIGHT) ||
+ (vpitch != 32) ||
+ (op->charcount != 256 && op->charcount != 512))
return -EINVAL;
pitch = ALIGN(w, 8) / 8;
bpc = pitch * h;
@@ -260,6 +267,19 @@ static int sticon_font_set(struct vc_data *vc, const struct console_font *font,
return sticon_set_font(vc, font, vpitch);
}
+static int sticon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = STICON_MIN_FONT_WIDTH;
+ info->min_height = STICON_MIN_FONT_HEIGHT;
+
+ info->max_width = STICON_MAX_FONT_WIDTH;
+ info->max_height = STICON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static void sticon_init(struct vc_data *c, bool init)
{
struct sti_struct *sti = sticon_sti;
@@ -356,6 +376,7 @@ static const struct consw sti_con = {
.con_scroll = sticon_scroll,
.con_switch = sticon_switch,
.con_blank = sticon_blank,
+ .con_font_info = sticon_font_info,
.con_font_set = sticon_font_set,
.con_font_default = sticon_font_default,
.con_build_attr = sticon_build_attr,
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 7597f04b0dc7..b5465e555fdc 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -61,6 +61,10 @@ static struct vgastate vgastate;
#define BLANK 0x0020
#define VGA_FONTWIDTH 8 /* VGA does not support fontwidths != 8 */
+
+#define VGACON_MAX_FONT_WIDTH VGA_FONTWIDTH
+#define VGACON_MAX_FONT_HEIGHT 32
+
/*
* Interface used by the world
*/
@@ -1039,6 +1043,19 @@ static int vgacon_adjust_height(struct vc_data *vc, unsigned fontheight)
return 0;
}
+static int vgacon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = VGACON_MAX_FONT_WIDTH;
+ info->min_height = 0;
+
+ info->max_width = VGACON_MAX_FONT_WIDTH;
+ info->max_height = VGACON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static int vgacon_font_set(struct vc_data *c, const struct console_font *font,
unsigned int vpitch, unsigned int flags)
{
@@ -1048,7 +1065,8 @@ static int vgacon_font_set(struct vc_data *c, const struct console_font *font,
if (vga_video_type < VIDEO_TYPE_EGAM)
return -EINVAL;
- if (font->width != VGA_FONTWIDTH || font->height > 32 || vpitch != 32 ||
+ if (font->width != VGACON_MAX_FONT_WIDTH ||
+ font->height > VGACON_MAX_FONT_HEIGHT || vpitch != 32 ||
(charcount != 256 && charcount != 512))
return -EINVAL;
@@ -1201,6 +1219,7 @@ const struct consw vga_con = {
.con_scroll = vgacon_scroll,
.con_switch = vgacon_switch,
.con_blank = vgacon_blank,
+ .con_font_info = vgacon_font_info,
.con_font_set = vgacon_font_set,
.con_font_get = vgacon_font_get,
.con_resize = vgacon_resize,
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index fcabc668e9fb..b54031da49fd 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2452,6 +2452,21 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
return ret;
}
+
+static int fbcon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = 0;
+ info->min_height = 0;
+
+ info->max_width = FB_MAX_BLIT_WIDTH;
+ info->max_height = FB_MAX_BLIT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
+
/*
* User asked to set font; we are guaranteed that charcount does not exceed 512
* but lets not assume that, since charcount of 512 is small for unicode support.
@@ -3127,6 +3142,7 @@ static const struct consw fb_con = {
.con_scroll = fbcon_scroll,
.con_switch = fbcon_switch,
.con_blank = fbcon_blank,
+ .con_font_info = fbcon_font_info,
.con_font_set = fbcon_set_font,
.con_font_get = fbcon_get_font,
.con_font_default = fbcon_set_def_font,
--
2.44.0
^ permalink raw reply related
* [PATCH v4 2/3] VT: Add KDFONTINFO ioctl
From: Alexey Gladkov @ 2024-04-02 17:50 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial, Helge Deller
In-Reply-To: <cover.1712080158.git.legion@kernel.org>
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 minimum 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 | 11 +++++++++++
include/linux/console.h | 3 +++
include/linux/vt_kern.h | 1 +
include/uapi/linux/kd.h | 13 ++++++++++++-
5 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9b5b98dfc8b4..e8db0e9ea674 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4851,6 +4851,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;
+
+ 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 4b91072f3a4e..40f9467f503d 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
break;
}
+ case KDFONTINFO: {
+ struct console_font_info fnt_info;
+
+ 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 31a8f5b85f5d..4b798322aa01 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -21,6 +21,7 @@
#include <linux/vesa.h>
struct vc_data;
+struct console_font_info;
struct console_font_op;
struct console_font;
struct module;
@@ -102,6 +103,8 @@ struct consw {
bool (*con_switch)(struct vc_data *vc);
bool (*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
bool mode_switch);
+ int (*con_font_info)(struct vc_data *vc,
+ struct console_font_info *info);
int (*con_font_set)(struct vc_data *vc,
const struct console_font *font,
unsigned int vpitch, unsigned int flags);
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index d008c3d0a9bb..383b3a4f6113 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -33,6 +33,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 8ddb2219a84b..abaf4dd6bb93 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -185,8 +185,19 @@ struct console_font {
#define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
+#define KDFONTINFO _IO(KD_IOCTL_BASE, 0x73) /* font information */
+
+#define KD_FONT_INFO_FLAG_LOW_SIZE _BITUL(0) /* 256 */
+#define KD_FONT_INFO_FLAG_HIGH_SIZE _BITUL(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 */
--
2.44.0
^ permalink raw reply related
* [PATCH v4 1/3] VT: Use macros to define ioctls
From: Alexey Gladkov @ 2024-04-02 17:50 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial
In-Reply-To: <cover.1712080158.git.legion@kernel.org>
All other headers use _IOC() macros to describe ioctls for a long time
now. This header is stuck in the last century.
Simply use the _IO() macro. No other changes.
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
include/uapi/linux/kd.h | 96 +++++++++++++++++++++--------------------
1 file changed, 49 insertions(+), 47 deletions(-)
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 6b384065c013..8ddb2219a84b 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -5,60 +5,61 @@
#include <linux/compiler.h>
/* 0x4B is 'K', to avoid collision with termios and vt */
+#define KD_IOCTL_BASE 'K'
-#define GIO_FONT 0x4B60 /* gets font in expanded form */
-#define PIO_FONT 0x4B61 /* use font in expanded form */
+#define GIO_FONT _IO(KD_IOCTL_BASE, 0x60) /* gets font in expanded form */
+#define PIO_FONT _IO(KD_IOCTL_BASE, 0x61) /* use font in expanded form */
-#define GIO_FONTX 0x4B6B /* get font using struct consolefontdesc */
-#define PIO_FONTX 0x4B6C /* set font using struct consolefontdesc */
+#define GIO_FONTX _IO(KD_IOCTL_BASE, 0x6B) /* get font using struct consolefontdesc */
+#define PIO_FONTX _IO(KD_IOCTL_BASE, 0x6C) /* set font using struct consolefontdesc */
struct consolefontdesc {
unsigned short charcount; /* characters in font (256 or 512) */
unsigned short charheight; /* scan lines per character (1-32) */
char __user *chardata; /* font data in expanded form */
};
-#define PIO_FONTRESET 0x4B6D /* reset to default font */
+#define PIO_FONTRESET _IO(KD_IOCTL_BASE, 0x6D) /* reset to default font */
-#define GIO_CMAP 0x4B70 /* gets colour palette on VGA+ */
-#define PIO_CMAP 0x4B71 /* sets colour palette on VGA+ */
+#define GIO_CMAP _IO(KD_IOCTL_BASE, 0x70) /* gets colour palette on VGA+ */
+#define PIO_CMAP _IO(KD_IOCTL_BASE, 0x71) /* sets colour palette on VGA+ */
-#define KIOCSOUND 0x4B2F /* start sound generation (0 for off) */
-#define KDMKTONE 0x4B30 /* generate tone */
+#define KIOCSOUND _IO(KD_IOCTL_BASE, 0x2F) /* start sound generation (0 for off) */
+#define KDMKTONE _IO(KD_IOCTL_BASE, 0x30) /* generate tone */
-#define KDGETLED 0x4B31 /* return current led state */
-#define KDSETLED 0x4B32 /* set led state [lights, not flags] */
+#define KDGETLED _IO(KD_IOCTL_BASE, 0x31) /* return current led state */
+#define KDSETLED _IO(KD_IOCTL_BASE, 0x32) /* set led state [lights, not flags] */
#define LED_SCR 0x01 /* scroll lock led */
#define LED_NUM 0x02 /* num lock led */
#define LED_CAP 0x04 /* caps lock led */
-#define KDGKBTYPE 0x4B33 /* get keyboard type */
+#define KDGKBTYPE _IO(KD_IOCTL_BASE, 0x33) /* get keyboard type */
#define KB_84 0x01
#define KB_101 0x02 /* this is what we always answer */
#define KB_OTHER 0x03
-#define KDADDIO 0x4B34 /* add i/o port as valid */
-#define KDDELIO 0x4B35 /* del i/o port as valid */
-#define KDENABIO 0x4B36 /* enable i/o to video board */
-#define KDDISABIO 0x4B37 /* disable i/o to video board */
+#define KDADDIO _IO(KD_IOCTL_BASE, 0x34) /* add i/o port as valid */
+#define KDDELIO _IO(KD_IOCTL_BASE, 0x35) /* del i/o port as valid */
+#define KDENABIO _IO(KD_IOCTL_BASE, 0x36) /* enable i/o to video board */
+#define KDDISABIO _IO(KD_IOCTL_BASE, 0x37) /* disable i/o to video board */
-#define KDSETMODE 0x4B3A /* set text/graphics mode */
+#define KDSETMODE _IO(KD_IOCTL_BASE, 0x3A) /* set text/graphics mode */
#define KD_TEXT 0x00
#define KD_GRAPHICS 0x01
#define KD_TEXT0 0x02 /* obsolete */
#define KD_TEXT1 0x03 /* obsolete */
-#define KDGETMODE 0x4B3B /* get current mode */
+#define KDGETMODE _IO(KD_IOCTL_BASE, 0x3B) /* get current mode */
-#define KDMAPDISP 0x4B3C /* map display into address space */
-#define KDUNMAPDISP 0x4B3D /* unmap display from address space */
+#define KDMAPDISP _IO(KD_IOCTL_BASE, 0x3C) /* map display into address space */
+#define KDUNMAPDISP _IO(KD_IOCTL_BASE, 0x3D) /* unmap display from address space */
typedef char scrnmap_t;
#define E_TABSZ 256
-#define GIO_SCRNMAP 0x4B40 /* get screen mapping from kernel */
-#define PIO_SCRNMAP 0x4B41 /* put screen mapping table in kernel */
-#define GIO_UNISCRNMAP 0x4B69 /* get full Unicode screen mapping */
-#define PIO_UNISCRNMAP 0x4B6A /* set full Unicode screen mapping */
+#define GIO_SCRNMAP _IO(KD_IOCTL_BASE, 0x40) /* get screen mapping from kernel */
+#define PIO_SCRNMAP _IO(KD_IOCTL_BASE, 0x41) /* put screen mapping table in kernel */
+#define GIO_UNISCRNMAP _IO(KD_IOCTL_BASE, 0x69) /* get full Unicode screen mapping */
+#define PIO_UNISCRNMAP _IO(KD_IOCTL_BASE, 0x6A) /* set full Unicode screen mapping */
-#define GIO_UNIMAP 0x4B66 /* get unicode-to-font mapping from kernel */
+#define GIO_UNIMAP _IO(KD_IOCTL_BASE, 0x66) /* get unicode-to-font mapping from kernel */
struct unipair {
unsigned short unicode;
unsigned short fontpos;
@@ -67,8 +68,8 @@ struct unimapdesc {
unsigned short entry_ct;
struct unipair __user *entries;
};
-#define PIO_UNIMAP 0x4B67 /* put unicode-to-font mapping in kernel */
-#define PIO_UNIMAPCLR 0x4B68 /* clear table, possibly advise hash algorithm */
+#define PIO_UNIMAP _IO(KD_IOCTL_BASE, 0x67) /* put unicode-to-font mapping in kernel */
+#define PIO_UNIMAPCLR _IO(KD_IOCTL_BASE, 0x68) /* clear table, possibly advise hash algorithm */
struct unimapinit {
unsigned short advised_hashsize; /* 0 if no opinion */
unsigned short advised_hashstep; /* 0 if no opinion */
@@ -83,19 +84,19 @@ struct unimapinit {
#define K_MEDIUMRAW 0x02
#define K_UNICODE 0x03
#define K_OFF 0x04
-#define KDGKBMODE 0x4B44 /* gets current keyboard mode */
-#define KDSKBMODE 0x4B45 /* sets current keyboard mode */
+#define KDGKBMODE _IO(KD_IOCTL_BASE, 0x44) /* gets current keyboard mode */
+#define KDSKBMODE _IO(KD_IOCTL_BASE, 0x45) /* sets current keyboard mode */
#define K_METABIT 0x03
#define K_ESCPREFIX 0x04
-#define KDGKBMETA 0x4B62 /* gets meta key handling mode */
-#define KDSKBMETA 0x4B63 /* sets meta key handling mode */
+#define KDGKBMETA _IO(KD_IOCTL_BASE, 0x62) /* gets meta key handling mode */
+#define KDSKBMETA _IO(KD_IOCTL_BASE, 0x63) /* sets meta key handling mode */
#define K_SCROLLLOCK 0x01
#define K_NUMLOCK 0x02
#define K_CAPSLOCK 0x04
-#define KDGKBLED 0x4B64 /* get led flags (not lights) */
-#define KDSKBLED 0x4B65 /* set led flags (not lights) */
+#define KDGKBLED _IO(KD_IOCTL_BASE, 0x64) /* get led flags (not lights) */
+#define KDSKBLED _IO(KD_IOCTL_BASE, 0x65) /* set led flags (not lights) */
struct kbentry {
unsigned char kb_table;
@@ -107,15 +108,15 @@ struct kbentry {
#define K_ALTTAB 0x02
#define K_ALTSHIFTTAB 0x03
-#define KDGKBENT 0x4B46 /* gets one entry in translation table */
-#define KDSKBENT 0x4B47 /* sets one entry in translation table */
+#define KDGKBENT _IO(KD_IOCTL_BASE, 0x46) /* gets one entry in translation table */
+#define KDSKBENT _IO(KD_IOCTL_BASE, 0x47) /* sets one entry in translation table */
struct kbsentry {
unsigned char kb_func;
unsigned char kb_string[512];
};
-#define KDGKBSENT 0x4B48 /* gets one function key string entry */
-#define KDSKBSENT 0x4B49 /* sets one function key string entry */
+#define KDGKBSENT _IO(KD_IOCTL_BASE, 0x48) /* gets one function key string entry */
+#define KDSKBSENT _IO(KD_IOCTL_BASE, 0x49) /* sets one function key string entry */
struct kbdiacr {
unsigned char diacr, base, result;
@@ -124,8 +125,8 @@ struct kbdiacrs {
unsigned int kb_cnt; /* number of entries in following array */
struct kbdiacr kbdiacr[256]; /* MAX_DIACR from keyboard.h */
};
-#define KDGKBDIACR 0x4B4A /* read kernel accent table */
-#define KDSKBDIACR 0x4B4B /* write kernel accent table */
+#define KDGKBDIACR _IO(KD_IOCTL_BASE, 0x4A) /* read kernel accent table */
+#define KDSKBDIACR _IO(KD_IOCTL_BASE, 0x4B) /* write kernel accent table */
struct kbdiacruc {
unsigned int diacr, base, result;
@@ -134,16 +135,16 @@ struct kbdiacrsuc {
unsigned int kb_cnt; /* number of entries in following array */
struct kbdiacruc kbdiacruc[256]; /* MAX_DIACR from keyboard.h */
};
-#define KDGKBDIACRUC 0x4BFA /* read kernel accent table - UCS */
-#define KDSKBDIACRUC 0x4BFB /* write kernel accent table - UCS */
+#define KDGKBDIACRUC _IO(KD_IOCTL_BASE, 0xFA) /* read kernel accent table - UCS */
+#define KDSKBDIACRUC _IO(KD_IOCTL_BASE, 0xFB) /* write kernel accent table - UCS */
struct kbkeycode {
unsigned int scancode, keycode;
};
-#define KDGETKEYCODE 0x4B4C /* read kernel keycode table entry */
-#define KDSETKEYCODE 0x4B4D /* write kernel keycode table entry */
+#define KDGETKEYCODE _IO(KD_IOCTL_BASE, 0x4C) /* read kernel keycode table entry */
+#define KDSETKEYCODE _IO(KD_IOCTL_BASE, 0x4D) /* write kernel keycode table entry */
-#define KDSIGACCEPT 0x4B4E /* accept kbd generated signals */
+#define KDSIGACCEPT _IO(KD_IOCTL_BASE, 0x4E) /* accept kbd generated signals */
struct kbd_repeat {
int delay; /* in msec; <= 0: don't change */
@@ -151,10 +152,11 @@ struct kbd_repeat {
/* earlier this field was misnamed "rate" */
};
-#define KDKBDREP 0x4B52 /* set keyboard delay/repeat rate;
- * actually used values are returned */
+#define KDKBDREP _IO(KD_IOCTL_BASE, 0x52) /* set keyboard delay/repeat rate;
+ * actually used values are returned
+ */
-#define KDFONTOP 0x4B72 /* font operations */
+#define KDFONTOP _IO(KD_IOCTL_BASE, 0x72) /* font operations */
struct console_font_op {
unsigned int op; /* operation code KD_FONT_OP_* */
--
2.44.0
^ permalink raw reply related
* [PATCH v4 0/3] VT: Add ability to get font requirements
From: Alexey Gladkov @ 2024-04-02 17:50 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial
In-Reply-To: <74ca50e0-61b1-4d4c-85dd-a5d920548c04@kernel.org>
We now have KD_FONT_OP_SET_TALL, but in fact such large fonts cannot be
loaded. No console driver supports tall fonts. Unfortunately, userspace
cannot distinguish the lack of support in the driver from errors in the
font itself. In all cases, EINVAL will be returned.
This patchset adds a separate ioctl to obtain the font parameters
supported by the console driver.
v4:
* Rebased on v6.9-rc1 and conflicts have been fixed.
* Do not copy KDFONTINFO data from the userspace.
* Header include/uapi/linux/kd.h uses _IOC macros to define ioctls.
v3:
* Added the use of the in_range macro.
* Squashed the commits that add ioctl to console divers.
v2:
* Instead of the KDFONTOP extension, a new ioctl has been added to
obtain font information.
Alexey Gladkov (3):
VT: Use macros to define ioctls
VT: Add KDFONTINFO ioctl
VT: Allow to get max font width and height
drivers/tty/vt/vt.c | 24 ++++++
drivers/tty/vt/vt_ioctl.c | 11 +++
drivers/video/console/newport_con.c | 21 +++++-
drivers/video/console/sticon.c | 25 ++++++-
drivers/video/console/vgacon.c | 21 +++++-
drivers/video/fbdev/core/fbcon.c | 16 ++++
include/linux/console.h | 3 +
include/linux/vt_kern.h | 1 +
include/uapi/linux/kd.h | 109 ++++++++++++++++------------
9 files changed, 176 insertions(+), 55 deletions(-)
--
2.44.0
^ permalink raw reply
* Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Alexey Gladkov @ 2024-04-02 13:19 UTC (permalink / raw)
To: Jiri Slaby
Cc: Greg Kroah-Hartman, LKML, kbd, linux-api, linux-fbdev,
linux-serial, Helge Deller
In-Reply-To: <74ca50e0-61b1-4d4c-85dd-a5d920548c04@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
^ permalink raw reply
* Re: [PATCH v3 2/2] VT: Allow to get max font width and height
From: Jiri Slaby @ 2024-04-02 11:09 UTC (permalink / raw)
To: Oleg Bulatov, legion
Cc: Greg Kroah-Hartman, LKML, kbd, linux-api, linux-fbdev,
linux-serial
In-Reply-To: <c3wrf2h7h45h2vee7gc42zmy43rsh7niueknvsrlsibnae4pdw@4u6b4qulfe6r>
On 13. 03. 24, 18:40, Oleg Bulatov wrote:
> On Tue, Mar 12, 2024 at 03:23:58PM +0100, legion@kernel.org wrote:
>> drivers/video/console/newport_con.c | 21 +++++++++++++++++----
>> drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
>> drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
>> drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
>> 4 files changed, 81 insertions(+), 8 deletions(-)
>
> newport_con.c is an interesting one, apparently it's for SGI Indy and
> Indigo2, both are discontinued in 1997. Do we still have a way to test
> this driver?
I doubt that.
Care to submit a removal patch? I am afraid, there is no other way to
find out anyway...
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Jiri Slaby @ 2024-04-02 11:02 UTC (permalink / raw)
To: Alexey Gladkov, Greg Kroah-Hartman
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial, Helge Deller
In-Reply-To: <ed056326540f04b72c97a276fbcc316e1b2f6371.1712053848.git.legion@kernel.org>
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,
--
js
suse labs
^ permalink raw reply
* [RESEND PATCH v3 2/2] VT: Allow to get max font width and height
From: Alexey Gladkov @ 2024-04-02 10:32 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial, Helge Deller
In-Reply-To: <cover.1712053848.git.legion@kernel.org>
The Console drivers has more restrictive font size limits than vt_ioctl.
This leads to errors that are difficult to handle. If a font whose size
is not supported is used, an EINVAL error will be returned, which is
also returned in case of errors in the font itself. At the moment there
is no way to understand what font sizes the current console driver
supports.
To solve this problem, we need to transfer information about the
supported font to userspace from the console driver.
Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/video/console/newport_con.c | 21 +++++++++++++++++----
drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
4 files changed, 81 insertions(+), 8 deletions(-)
diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
index e8e4f82cd4a1..87f174a95fa8 100644
--- a/drivers/video/console/newport_con.c
+++ b/drivers/video/console/newport_con.c
@@ -33,6 +33,9 @@
#define NEWPORT_LEN 0x10000
+#define NEWPORT_MAX_FONT_WIDTH 8
+#define NEWPORT_MAX_FONT_HEIGHT 16
+
#define FONT_DATA ((unsigned char *)font_vga_8x16.data)
static unsigned char *font_data[MAX_NR_CONSOLES];
@@ -328,8 +331,8 @@ static void newport_init(struct vc_data *vc, int init)
{
int cols, rows;
- cols = newport_xsize / 8;
- rows = newport_ysize / 16;
+ cols = newport_xsize / NEWPORT_MAX_FONT_WIDTH;
+ rows = newport_ysize / NEWPORT_MAX_FONT_HEIGHT;
vc->vc_can_do_color = 1;
if (init) {
vc->vc_cols = cols;
@@ -507,8 +510,8 @@ static int newport_set_font(int unit, struct console_font *op, unsigned int vpit
/* ladis: when I grow up, there will be a day... and more sizes will
* be supported ;-) */
- if ((w != 8) || (h != 16) || (vpitch != 32)
- || (op->charcount != 256 && op->charcount != 512))
+ if ((w != NEWPORT_MAX_FONT_WIDTH) || (h != NEWPORT_MAX_FONT_HEIGHT) ||
+ (vpitch != 32) || (op->charcount != 256 && op->charcount != 512))
return -EINVAL;
if (!(new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size,
@@ -569,6 +572,15 @@ static int newport_font_default(struct vc_data *vc, struct console_font *op, cha
return newport_set_def_font(vc->vc_num, op);
}
+static int newport_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = info->max_width = NEWPORT_MAX_FONT_WIDTH;
+ info->min_height = info->max_height = NEWPORT_MAX_FONT_HEIGHT;
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static int newport_font_set(struct vc_data *vc, struct console_font *font,
unsigned int vpitch, unsigned int flags)
{
@@ -688,6 +700,7 @@ const struct consw newport_con = {
.con_scroll = newport_scroll,
.con_switch = newport_switch,
.con_blank = newport_blank,
+ .con_font_info = newport_font_info,
.con_font_set = newport_font_set,
.con_font_default = newport_font_default,
.con_save_screen = newport_save_screen
diff --git a/drivers/video/console/sticon.c b/drivers/video/console/sticon.c
index 992a4fa431aa..d32ca458eb77 100644
--- a/drivers/video/console/sticon.c
+++ b/drivers/video/console/sticon.c
@@ -56,6 +56,11 @@
#define BLANK 0
static int vga_is_gfx;
+#define STICON_MIN_FONT_WIDTH 6
+#define STICON_MIN_FONT_HEIGHT 6
+#define STICON_MAX_FONT_WIDTH 32
+#define STICON_MAX_FONT_HEIGHT 32
+
#define STI_DEF_FONT sticon_sti->font
/* borrowed from fbcon.c */
@@ -180,8 +185,10 @@ static int sticon_set_font(struct vc_data *vc, struct console_font *op,
struct sti_cooked_font *cooked_font;
unsigned char *data = op->data, *p;
- if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
- || (op->charcount != 256 && op->charcount != 512))
+ if (!in_range(w, STICON_MIN_FONT_WIDTH, STICON_MAX_FONT_WIDTH) ||
+ !in_range(h, STICON_MIN_FONT_HEIGHT, STICON_MAX_FONT_HEIGHT) ||
+ (vpitch != 32) ||
+ (op->charcount != 256 && op->charcount != 512))
return -EINVAL;
pitch = ALIGN(w, 8) / 8;
bpc = pitch * h;
@@ -273,6 +280,19 @@ static int sticon_font_set(struct vc_data *vc, struct console_font *font,
return sticon_set_font(vc, font, vpitch);
}
+static int sticon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = STICON_MIN_FONT_WIDTH;
+ info->min_height = STICON_MIN_FONT_HEIGHT;
+
+ info->max_width = STICON_MAX_FONT_WIDTH;
+ info->max_height = STICON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static void sticon_init(struct vc_data *c, int init)
{
struct sti_struct *sti = sticon_sti;
@@ -371,6 +391,7 @@ static const struct consw sti_con = {
.con_scroll = sticon_scroll,
.con_switch = sticon_switch,
.con_blank = sticon_blank,
+ .con_font_info = sticon_font_info,
.con_font_set = sticon_font_set,
.con_font_default = sticon_font_default,
.con_build_attr = sticon_build_attr,
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 8ef1579fa57f..b75d31ef3353 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -61,6 +61,10 @@ static struct vgastate vgastate;
#define BLANK 0x0020
#define VGA_FONTWIDTH 8 /* VGA does not support fontwidths != 8 */
+
+#define VGACON_MAX_FONT_WIDTH VGA_FONTWIDTH
+#define VGACON_MAX_FONT_HEIGHT 32
+
/*
* Interface used by the world
*/
@@ -1013,6 +1017,19 @@ static int vgacon_adjust_height(struct vc_data *vc, unsigned fontheight)
return 0;
}
+static int vgacon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = VGACON_MAX_FONT_WIDTH;
+ info->min_height = 0;
+
+ info->max_width = VGACON_MAX_FONT_WIDTH;
+ info->max_height = VGACON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static int vgacon_font_set(struct vc_data *c, struct console_font *font,
unsigned int vpitch, unsigned int flags)
{
@@ -1022,7 +1039,8 @@ static int vgacon_font_set(struct vc_data *c, struct console_font *font,
if (vga_video_type < VIDEO_TYPE_EGAM)
return -EINVAL;
- if (font->width != VGA_FONTWIDTH || font->height > 32 || vpitch != 32 ||
+ if (font->width != VGACON_MAX_FONT_WIDTH ||
+ font->height > VGACON_MAX_FONT_HEIGHT || vpitch != 32 ||
(charcount != 256 && charcount != 512))
return -EINVAL;
@@ -1177,6 +1195,7 @@ const struct consw vga_con = {
.con_scroll = vgacon_scroll,
.con_switch = vgacon_switch,
.con_blank = vgacon_blank,
+ .con_font_info = vgacon_font_info,
.con_font_set = vgacon_font_set,
.con_font_get = vgacon_font_get,
.con_resize = vgacon_resize,
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 46823c2e2ba1..e10abe416159 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -101,6 +101,9 @@ enum {
FBCON_LOGO_DONTSHOW = -3 /* do not show the logo */
};
+#define FBCON_MAX_FONT_WIDTH 32
+#define FBCON_MAX_FONT_HEIGHT 32
+
static struct fbcon_display fb_display[MAX_NR_CONSOLES];
static struct fb_info *fbcon_registered_fb[FB_MAX];
@@ -2456,6 +2459,21 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
return ret;
}
+
+static int fbcon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = 0;
+ info->min_height = 0;
+
+ info->max_width = FBCON_MAX_FONT_WIDTH;
+ info->max_height = FBCON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
+
/*
* User asked to set font; we are guaranteed that charcount does not exceed 512
* but lets not assume that, since charcount of 512 is small for unicode support.
@@ -2483,7 +2501,8 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
return -EINVAL;
- if (font->width > 32 || font->height > 32)
+ if (font->width > FBCON_MAX_FONT_WIDTH ||
+ font->height > FBCON_MAX_FONT_HEIGHT)
return -EINVAL;
/* Make sure drawing engine can handle the font */
@@ -3158,6 +3177,7 @@ static const struct consw fb_con = {
.con_scroll = fbcon_scroll,
.con_switch = fbcon_switch,
.con_blank = fbcon_blank,
+ .con_font_info = fbcon_font_info,
.con_font_set = fbcon_set_font,
.con_font_get = fbcon_get_font,
.con_font_default = fbcon_set_def_font,
--
2.44.0
^ permalink raw reply related
* [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Alexey Gladkov @ 2024-04-02 10:32 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial, Helge Deller
In-Reply-To: <cover.1712053848.git.legion@kernel.org>
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(-)
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 */
--
2.44.0
^ permalink raw reply related
* [RESEND PATCH v3 0/2] VT: Add ability to get font requirements
From: Alexey Gladkov @ 2024-04-02 10:32 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial
In-Reply-To: <cover.1710252966.git.legion@kernel.org>
We now have KD_FONT_OP_SET_TALL, but in fact such large fonts cannot be
loaded. No console driver supports tall fonts. Unfortunately, userspace
cannot distinguish the lack of support in the driver from errors in the
font itself. In all cases, EINVAL will be returned.
This patchset adds a separate ioctl to obtain the font parameters
supported by the console driver.
v3:
* Added the use of the in_range macro.
* Squashed the commits that add ioctl to console divers.
v2:
* Instead of the KDFONTOP extension, a new ioctl has been added to
obtain font information.
Alexey Gladkov (2):
VT: Add KDFONTINFO ioctl
VT: Allow to get max font width and height
drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
drivers/video/console/newport_con.c | 21 +++++++++++++++++----
drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
include/linux/console.h | 2 ++
include/linux/vt_kern.h | 1 +
include/uapi/linux/kd.h | 13 ++++++++++++-
9 files changed, 133 insertions(+), 9 deletions(-)
--
2.44.0
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-04-02 9:24 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <CAF2d9jjA8iM1AoPUhQPK62tdd7gPnCnt51f_NMhOAs546rU3dA@mail.gmail.com>
On Mon, Apr 01 2024 at 22:42, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Mon, Apr 1, 2024 at 1:46 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> So if there is a backwards compability issue with PTP_SYS_OFFSET2, then
>> you need to introduce PTP_SYS_OFFSET3. The PTP_SYS_*2 variants were
>> introduced to avoid backwards compatibility issues as well, but
>> unfortunately that did not address the reserved fields problem for
>> PTP_SYS_OFFSET2. PTP_SYS_OFFSET_EXTENDED2 should just work, but maybe
>> the PTP maintainers want a full extension to '3'. Either way is fine.
>>
> https://patchwork.kernel.org/project/netdevbpf/patch/20240104212436.3276057-1-maheshb@google.com/
>
> This was my attempt to solve a similar issue with the new ioctl op to
> avoid backward compatibility issues. Instead of flags I used the
> clockid_t in a similar fashion.
Works as well. I'm not seing the point for CLOCK_MONOTONIC and the
change logs are not really telling anything about the problem being
solved....
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Mahesh Bandewar (महेश बंडेवार) @ 2024-04-02 5:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <87o7asdd65.ffs@tglx>
On Mon, Apr 1, 2024 at 1:46 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Sagi!
>
> On Thu, Mar 28 2024 at 17:40, Sagi Maimon wrote:
> > On Sat, Mar 23, 2024 at 2:38 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> On top this needs an analyis whether any of the gettimex64()
> >> implementations does something special instead of invoking the
> >> ptp_read_system_prets() and ptp_read_system_postts() helpers as close as
> >> possible to the PCH readout, but that's not rocket science either. It's
> >> just 21 callbacks to look at.
> >>
> > I like your suggestion, thanks!
> > it is what our user space needs from the kernel and with minimum kernel changes.
> > I will write it, test it and upload it with your permission (it is you
> > idea after all).
>
> You don't need permission. I made a suggestion and when you are doing the
> work I'm not in a position to veto posting it. We have an explicit tag
> for that 'Suggested-by:', which only says that someone suggested it to
> you, but then you went and implemented it, made sure it works etc.
>
> >> It might also require a new set of variant '3' IOTCLS to make that flag
> >> field work, but that's not going to make the change more complex and
> >> it's an exercise left to the experts of that IOCTL interface.
> >>
> > I think that I understand your meaning.
> > There is a backward compatibility problem here.
> >
> > Existing user space application using PTP_SYS_OFFSET_EXTENDED ioctl
> > won't have any problems because of the "extoff->rsv[0] ||
> > extoff->rsv[1] || extoff->rsv[2]" test, but what about all old user
> > space applications using: PTP_SYS_OFFSET ?
>
> So if there is a backwards compability issue with PTP_SYS_OFFSET2, then
> you need to introduce PTP_SYS_OFFSET3. The PTP_SYS_*2 variants were
> introduced to avoid backwards compatibility issues as well, but
> unfortunately that did not address the reserved fields problem for
> PTP_SYS_OFFSET2. PTP_SYS_OFFSET_EXTENDED2 should just work, but maybe
> the PTP maintainers want a full extension to '3'. Either way is fine.
>
https://patchwork.kernel.org/project/netdevbpf/patch/20240104212436.3276057-1-maheshb@google.com/
This was my attempt to solve a similar issue with the new ioctl op to
avoid backward compatibility issues. Instead of flags I used the
clockid_t in a similar fashion.
Thanks,
> Thanks,
>
> tglx
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox