Re: [PATCH printk v2 2/5] printk: remove safe buffers
On (21/04/01 16:17), Petr Mladek wrote: > > For the long term, we should introduce a printk-context API that allows > > callers to perfectly pack their multi-line output into a single > > entry. We discussed [0][1] this back in August 2020. > > We need a "short" term solution. There are currently 3 solutions: > > 1. Keep nmi_safe() and all the hacks around. > > 2. Serialize nmi_cpu_backtrace() by a spin lock and later by >the special lock used also by atomic consoles. > > 3. Tell complaining people how to sort the messed logs. Are we talking about nmi_cpu_backtrace()->dump_stack() or some other path? dump_stack() seems to be already serialized by `dump_lock`. Hmm, show_regs() is not serialized, seems like it should be under the same `dump_lock` as dump_stack().
Re: [PATCH printk v2 1/5] printk: track/limit recursion
On (21/04/01 12:00), Petr Mladek wrote: > On Tue 2021-03-30 17:35:08, John Ogness wrote: > > Currently the printk safe buffers provide a form of recursion > > protection by redirecting to the safe buffers whenever printk() is > > recursively called. > > > > In preparation for removal of the safe buffers, provide an alternate > > explicit recursion protection. Recursion is limited to 3 levels > > per-CPU and per-context. > > > > Signed-off-by: John Ogness > > Reviewed-by: Petr Mladek Reviewed-by: Sergey Senozhatsky
Re: [PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags
On (21/03/24 08:28), Ricardo Ribalda wrote: [..] > > > > > > Are you sure that you do not want to start with 1<<3, there might be > > > some hardware that support LE/SE > > > > How the hardware's going to support this? There is simply no way to > > pass these flags to the firmware, the values already overlap with > > auto-controls. So I guess these flags are for the driver (C code). > > uvcvideo driver is not doing any "lesser or equal rectangle" magic > > for ROI. No such thing is defined by UVC spec. > > The driver can implement se/le. Right. I wonder if we can actually fit ROI into selection API. v4l2 selection is focusing on rectangle, that's the only thing that matters, but in ROI rectangle and autocontrols are equally important.
Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/24 12:05), Sergey Senozhatsky wrote: > > > For ROI user-space also must provide valid auto-controls value, which > > > normally requires GET_MIN/GET_MAX discovery. > > > > > > v4l2 selection API mentions only rectangle adjustments and errnos like > > > -ERANGE also mention "It is not possible to adjust struct v4l2_rect r > > > rectangle to satisfy all constraints given in the flags argument". > > > > > > So in case when auto-controls is out of supported range (out of > > > GET_MIN, GET_MAX range) there is no way for us to tell user-space that > > > auto-controls is wrong. We probably need silently pick up the first > > > supported value, but not sure how well this will work out in the end. > > > > Shouldn't the autocontrol selection be done via a separate bitmask > > control rather than some custom flags in the selection API? > > That selection must be done before we send ROI to the firmware. > Firmware H that I have supports split controls - we can send > ROI::rectangle and ROI::autocontrols separately. But other > firmwares don't tolerate such a thing and by the time we issue > > uvc_query_ctrl(stream->dev, > UVC_SET_CUR > UVC_CT_REGION_OF_INTEREST_CONTROL > roi, > + sizeof(struct uvc_roi_rect)) > > roi rectangle should be of size 5 * u16 and contain values that firmware ^^^ roi structure > will accept, including autocontrols.
Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/24 12:00), Tomasz Figa wrote:
[..]
> > I guess in our case we need to talk about rectangle,auto-controls tuple
> > that we send to firmware
> >
> > rect {
> > (0, 0), (INT_MAX, INT_MAX)
> > }
> > auto-controls {
> > INT_MAX
> > }
> >
> > For ROI user-space also must provide valid auto-controls value, which
> > normally requires GET_MIN/GET_MAX discovery.
> >
> > v4l2 selection API mentions only rectangle adjustments and errnos like
> > -ERANGE also mention "It is not possible to adjust struct v4l2_rect r
> > rectangle to satisfy all constraints given in the flags argument".
> >
> > So in case when auto-controls is out of supported range (out of
> > GET_MIN, GET_MAX range) there is no way for us to tell user-space that
> > auto-controls is wrong. We probably need silently pick up the first
> > supported value, but not sure how well this will work out in the end.
>
> Shouldn't the autocontrol selection be done via a separate bitmask
> control rather than some custom flags in the selection API?
That selection must be done before we send ROI to the firmware.
Firmware H that I have supports split controls - we can send
ROI::rectangle and ROI::autocontrols separately. But other
firmwares don't tolerate such a thing and by the time we issue
uvc_query_ctrl(stream->dev,
UVC_SET_CUR
UVC_CT_REGION_OF_INTEREST_CONTROL
roi,
+ sizeof(struct uvc_roi_rect))
roi rectangle should be of size 5 * u16 and contain values that firmware
will accept, including autocontrols.
Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/24 11:34), Tomasz Figa wrote:
> On Wed, Mar 24, 2021 at 11:31 AM Sergey Senozhatsky
> wrote:
[..]
> > > Adjusting the rectangle to something supported by the hardware is
> > > mentioned explicitly in the V4L2 API documentation and is what drivers
> > > have to implement. Returning an error on invalid value is not a
> > > correct behavior here (and similarly for many other operations, e.g.
> > > S_FMT).
> >
> > Well, in this particular case we are talking about user-space that wants
> > to set ROI rectangle that is knowingly violates device's GET_MAX and
> > overflows UVC ROI rectangle u16 value range. That's a clear bug in
> > user-space.
> > Do we want to pretend that user-space does the correct thing and fixup
> > stuff behind the scenes?
> >
>
> That's how the API is defined. There is a valid use case for this -
> you don't need to run QUERY_CTRL if all you need is setting the
> biggest possible rectangle, just set it to (0, 0), (INT_MAX, INT_MAX).
I guess in our case we need to talk about rectangle,auto-controls tuple
that we send to firmware
rect {
(0, 0), (INT_MAX, INT_MAX)
}
auto-controls {
INT_MAX
}
For ROI user-space also must provide valid auto-controls value, which
normally requires GET_MIN/GET_MAX discovery.
v4l2 selection API mentions only rectangle adjustments and errnos like
-ERANGE also mention "It is not possible to adjust struct v4l2_rect r
rectangle to satisfy all constraints given in the flags argument".
So in case when auto-controls is out of supported range (out of
GET_MIN, GET_MAX range) there is no way for us to tell user-space that
auto-controls is wrong. We probably need silently pick up the first
supported value, but not sure how well this will work out in the end.
Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/24 11:14), Tomasz Figa wrote:
> > > > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > > > + struct v4l2_selection *sel)
> > > > +{
> > > > + struct uvc_fh *handle = fh;
> > > > + struct uvc_streaming *stream = handle->stream;
> > > > + struct uvc_roi_rect *roi;
> > > > + int ret;
> > > > +
> > > > + if (!validate_roi_bounds(stream, sel))
> > > > + return -E2BIG;
> > >
> > > Not sure if this is the correct approach or if we should convert the
> > > value to the closest valid...
> >
> > Well, at this point we know that ROI rectangle dimensions are out of
> > sane value range. I'd rather tell user-space about integer overflow.
>
> Adjusting the rectangle to something supported by the hardware is
> mentioned explicitly in the V4L2 API documentation and is what drivers
> have to implement. Returning an error on invalid value is not a
> correct behavior here (and similarly for many other operations, e.g.
> S_FMT).
Well, in this particular case we are talking about user-space that wants
to set ROI rectangle that is knowingly violates device's GET_MAX and
overflows UVC ROI rectangle u16 value range. That's a clear bug in user-space.
Do we want to pretend that user-space does the correct thing and fixup
stuff behind the scenes?
> > Looking for the closest ROI rectangle that suffice can be rather
> > tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
> > is what Firmware D returns for GET_MAX
> >
> > ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)
> >
> > 0, 0, 65535, 65535
>
> Perhaps the frame size would be the correct bounds?
I can check that.
Re: [PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags
On (21/03/23 17:04), Ricardo Ribalda wrote: > On Fri, Mar 19, 2021 at 6:53 AM Sergey Senozhatsky > wrote: > > > > UVC 1.5 defines the following Region Of Interest auto controls: > > > > D0: Auto Exposure > > D1: Auto Iris > > D2: Auto White Balance > > D3: Auto Focus > > D4: Auto Face Detect > > D5: Auto Detect and Track > > D6: Image Stabilization > > D7: Higher Quality > > D8 – D15: Reserved, set to zero > > > > Signed-off-by: Sergey Senozhatsky > > --- > > include/uapi/linux/v4l2-common.h | 10 ++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/include/uapi/linux/v4l2-common.h > > b/include/uapi/linux/v4l2-common.h > > index 3651ebb8cb23..34f1c262d6aa 100644 > > --- a/include/uapi/linux/v4l2-common.h > > +++ b/include/uapi/linux/v4l2-common.h > > @@ -92,6 +92,16 @@ > > #define V4L2_SEL_FLAG_LE (1 << 1) > > #define V4L2_SEL_FLAG_KEEP_CONFIG (1 << 2) > > > > Are you sure that you do not want to start with 1<<3, there might be > some hardware that support LE/SE How the hardware's going to support this? There is simply no way to pass these flags to the firmware, the values already overlap with auto-controls. So I guess these flags are for the driver (C code). uvcvideo driver is not doing any "lesser or equal rectangle" magic for ROI. No such thing is defined by UVC spec. I can move these flags to entirely different value range and do remapping to uvc auto-controls values in uvcvideo.
Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/23 17:16), Ricardo Ribalda wrote:
[..]
> > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > + struct v4l2_selection *sel)
> > +{
> > + if (sel->r.left > USHRT_MAX ||
> > + sel->r.top > USHRT_MAX ||
> > + (sel->r.width + sel->r.left) > USHRT_MAX ||
> > + (sel->r.height + sel->r.top) > USHRT_MAX ||
> > + !sel->r.width || !sel->r.height)
> > + return false;
> > +
> > + if (sel->flags > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY)
> > + return false;
>
> Is it not allowed V4L2_SEL_FLAG_ROI_AUTO_IRIS |
> V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY ?
Good question.
I don't know. Depends on what HIGHER_QUALITY can stand for (UVC doesn't
specify). But overall it seems like features there are mutually
exclusive. E.g. AUTO_FACE_DETECT and AUTO_DETECT_AND_TRACK.
I think it'll be better to replace this with
if (sel->flags > USHRT_MAX)
return false;
so that we don't let overflow happen and accidentally enable/disable
some of the features.
> > +
> > + return true;
> > +}
> > +
> > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > + struct v4l2_selection *sel)
> > +{
> > + struct uvc_fh *handle = fh;
> > + struct uvc_streaming *stream = handle->stream;
> > + struct uvc_roi_rect *roi;
> > + int ret;
> > +
> > + if (!validate_roi_bounds(stream, sel))
> > + return -E2BIG;
>
> Not sure if this is the correct approach or if we should convert the
> value to the closest valid...
Well, at this point we know that ROI rectangle dimensions are out of
sane value range. I'd rather tell user-space about integer overflow.
Looking for the closest ROI rectangle that suffice can be rather
tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
is what Firmware D returns for GET_MAX
ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)
0, 0, 65535, 65535
-ss
Re: [PATCH 3/5] cifsd: add file operations
On (21/03/22 17:09), Matthew Wilcox wrote: > On Mon, Mar 22, 2021 at 06:03:21PM +0900, Sergey Senozhatsky wrote: > > On (21/03/22 08:15), Matthew Wilcox wrote: > > > > > > What's the scenario for which your allocator performs better than slub > > > > > > > IIRC request and reply buffers can be up to 4M in size. So this stuff > > just allocates a number of fat buffers and keeps them around so that > > it doesn't have to vmalloc(4M) for every request and every response. > > Hang on a minute, this speaks to a deeper design problem. If we're doing > a 'request' or 'reply' that is that large, the I/O should be coming from > or going to the page cache. If it goes via a 4MB virtually contiguous > buffer first, that's a memcpy that could/should be avoided. But huge vmalloc buffers are still needed. For instance, `ls -la` in a directory with a huge number of entries. > But now i'm looking for how ksmbd_find_buffer() is used, and it isn't. > So it looks like someone came to the same conclusion I did, but forgot > to delete the wm code. Yes, I think it's disabled by default and requires some smb.conf configuration. So I guess that wm code can be removed. Especially given that > That said, there are plenty of opportunities to optimise the vmalloc code, > and that's worth pursuing. That would be really interesting to see! > And here's the receive path which contains > the memcpy that should be avoided (ok, it's actually the call to ->read; > we shouldn't be reading in the entire 4MB but rather the header): > + conn->request_buf = ksmbd_alloc_request(size); > + if (!conn->request_buf) > + continue; > + > + memcpy(conn->request_buf, hdr_buf, sizeof(hdr_buf)); > + if (!ksmbd_smb_request(conn)) > + break; > + > + /* > + * We already read 4 bytes to find out PDU size, now > + * read in PDU > + */ > + size = t->ops->read(t, conn->request_buf + 4, pdu_size); // A side note, it seems that the maximum read/write/trans buffer size that // windows supports is 8MB, not 4MB.
Re: [PATCH next v1 1/3] printk: track/limit recursion
On (21/03/22 11:53), John Ogness wrote: > On 2021-03-21, Sergey Senozhatsky wrote: > >> @@ -2055,6 +2122,9 @@ int vprintk_store(int facility, int level, > >> */ > >>ts_nsec = local_clock(); > >> > >> + if (!printk_enter_irqsave()) > >> + return 0; > > > > I guess it can be interesting to somehow signal us that we had > > printk() recursion overflow, and how many messages we lost. > > Honestly, if we hit 3 levels of recursion, we are probably dealing with > an infinite recursion issue. I tend to agree. > I do not see the value of counting the overflows in that case. The > logged messages at that recursion level would ben enough to point > us to the problem. > > > 3 levels of recursion seem like reasonable limit, but I maybe wouldn't > > mind one extra level. > > With 3 levels, we will see all the messages of: > > printk -> WARN_ON -> WARN_ON -> WARN_ON Well, not necessarily this simple. printk vsprintf handle_foo_specifier printk call_console_drivers timekeeping printk vsprintf We saw in the past that enabling CONFIG_DEBUG_OBJECTS (if I'm not mistaken) can add quite a bit of extra printk recursion paths. We also have other CONFIG_DEBUG_* config options that can pop up as recursive printk-s here and there. For instance, from vsprintf::foo_specifier() where we escape from printk() to various kernel subsystems: net, block, etc. Maybe sometimes on level 3+ we'll see something interesting, but I've no strong opinion on this. > Keep in mind that each additional level causes the reading of the logs > to be significantly more complex. Each level increases the output > exponentially: Yes, I realize that. That's why I suggested that maybe recursive printk-s can have some special extra prefix. Recursive printk-s will interleave with whatever is being printed on this_cpu, so prefix might be helpful. -ss
Re: [PATCH 2/5] cifsd: add server-side procedures for SMB3
On (21/03/22 08:34), Matthew Wilcox wrote:
> > +++ b/fs/cifsd/mgmt/ksmbd_ida.c
> > @@ -0,0 +1,69 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> > + */
> > +
> > +#include "ksmbd_ida.h"
> > +
> > +struct ksmbd_ida *ksmbd_ida_alloc(void)
> > +{
> > + struct ksmbd_ida *ida;
> > +
> > + ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL);
> > + if (!ida)
> > + return NULL;
> > +
> > + ida_init(>map);
> > + return ida;
> > +}
>
> ... why? Everywhere that you call ksmbd_ida_alloc(), you would
> be better off just embedding the struct ida into the struct that
> currently has a pointer to it. Or declaring it statically. Then
> you can even initialise it statically using DEFINE_IDA() and
> eliminate the initialiser functions.
IIRC this ida is per SMB session, so it probably cannot be static.
And Windows, IIRC, doesn't like "just any IDs". Some versions of Windows
would fail the session login if server would return the first id == 0,
instead of 1. Or vice versa. I don't remember all the details, the last
time I looked into this was in 2019.
[..]
> > +struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session
> > *sess,
> > + unsigned int id)
> > +{
> > + struct ksmbd_tree_connect *tree_conn;
> > + struct list_head *tmp;
> > +
> > + list_for_each(tmp, >tree_conn_list) {
> > + tree_conn = list_entry(tmp, struct ksmbd_tree_connect, list);
> > + if (tree_conn->id == id)
> > + return tree_conn;
> > + }
>
> ... walk the linked list looking for an ID match. You'd be much better
> off using an allocating XArray:
> https://www.kernel.org/doc/html/latest/core-api/xarray.html
I think cifsd code predates XArray ;)
> Then you could lookup tree connections in O(log(n)) time instead of
> O(n) time.
Agreed. Not sure I remember why the code does list traversal here.
Re: [PATCH 3/5] cifsd: add file operations
On (21/03/22 10:04), Dan Carpenter wrote:
>
> On Mon, Mar 22, 2021 at 02:13:42PM +0900, Namjae Jeon wrote:
> > +void *ksmbd_alloc(size_t size)
> > +{
> > + return kvmalloc(size, GFP_KERNEL | __GFP_ZERO);
>
>
> This patch adds a bunch of wrappers around kvmalloc(). Don't do that.
Agreed. This was not cleaned up properly. The initial implementation
of that function (IIRC... it was sometime in 2018) basically contained
kvmalloc() implementation, because back in the days Samsung used kernel
version that simply didn't have kvmalloc() ( < KERNEL_VERSION(5, 0, 0))
Re: [PATCH 3/5] cifsd: add file operations
On (21/03/22 07:02), Al Viro wrote:
> On Mon, Mar 22, 2021 at 02:13:42PM +0900, Namjae Jeon wrote:
> > +static struct ksmbd_file *__ksmbd_lookup_fd(struct ksmbd_file_table *ft,
> > + unsigned int id)
> > +{
> > + bool unclaimed = true;
> > + struct ksmbd_file *fp;
> > +
> > + read_lock(>lock);
> > + fp = idr_find(ft->idr, id);
> > + if (fp)
> > + fp = ksmbd_fp_get(fp);
> > +
> > + if (fp && fp->f_ci) {
> > + read_lock(>f_ci->m_lock);
> > + unclaimed = list_empty(>node);
> > + read_unlock(>f_ci->m_lock);
> > + }
> > + read_unlock(>lock);
> > +
> > + if (fp && unclaimed) {
> > + atomic_dec(>refcount);
> > + return NULL;
> > + }
>
> Can that atomic_dec() end up dropping the last remaining reference?
Yes, I think it should increment refcount only for "claimed" fp.
Re: [PATCH 3/5] cifsd: add file operations
On (21/03/22 08:15), Matthew Wilcox wrote: > > What's the scenario for which your allocator performs better than slub > IIRC request and reply buffers can be up to 4M in size. So this stuff just allocates a number of fat buffers and keeps them around so that it doesn't have to vmalloc(4M) for every request and every response.
Re: [PATCH next v1 1/3] printk: track/limit recursion
On (21/03/17 00:33), John Ogness wrote: [..] > static inline void printk_delay(void) > @@ -2040,11 +2105,13 @@ int vprintk_store(int facility, int level, > struct prb_reserved_entry e; > enum log_flags lflags = 0; > struct printk_record r; > + unsigned long irqflags; > u16 trunc_msg_len = 0; > char prefix_buf[8]; > u16 reserve_size; > va_list args2; > u16 text_len; > + int ret = 0; > u64 ts_nsec; > > /* > @@ -2055,6 +2122,9 @@ int vprintk_store(int facility, int level, >*/ > ts_nsec = local_clock(); > > + if (!printk_enter_irqsave()) > + return 0; I guess it can be interesting to somehow signal us that we had printk() recursion overflow, and how many messages we lost. 3 levels of recursion seem like reasonable limit, but I maybe wouldn't mind one extra level. And maybe we could add some sort of message prefix for high levels of recursion nesting (levels 3+), so that things should not be normal will be on the radars and, possibly, will be reported. -ss
Re: [PATCH next v1 2/3] printk: remove safe buffers
On (21/03/17 00:33), John Ogness wrote:
[..]
> void printk_nmi_direct_enter(void)
> {
> @@ -324,27 +44,8 @@ void printk_nmi_direct_exit(void)
> this_cpu_and(printk_context, ~PRINTK_NMI_DIRECT_CONTEXT_MASK);
> }
>
> -#else
> -
> -static __printf(1, 0) int vprintk_nmi(const char *fmt, va_list args)
> -{
> - return 0;
> -}
> -
> #endif /* CONFIG_PRINTK_NMI */
>
> -/*
> - * Lock-less printk(), to avoid deadlocks should the printk() recurse
> - * into itself. It uses a per-CPU buffer to store the message, just like
> - * NMI.
> - */
> -static __printf(1, 0) int vprintk_safe(const char *fmt, va_list args)
> -{
> - struct printk_safe_seq_buf *s = this_cpu_ptr(_print_seq);
> -
> - return printk_safe_log_store(s, fmt, args);
> -}
> -
> /* Can be preempted by NMI. */
> void __printk_safe_enter(void)
> {
> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list
> args)
>* Use the main logbuf even in NMI. But avoid calling console
>* drivers that might have their own locks.
>*/
> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
> + if (this_cpu_read(printk_context) &
> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
> + PRINTK_NMI_CONTEXT_MASK |
> + PRINTK_SAFE_CONTEXT_MASK)) {
Do we need printk_nmi_direct_enter/exit() and PRINTK_NMI_DIRECT_CONTEXT_MASK?
Seems like all printk_safe() paths are now DIRECT - we store messages to the
prb, but don't call console drivers.
-ss
Re: [PATCH v6 resend 0/3] mm, vsprintf: dump full information of page flags in pGp
On (21/03/19 18:12), Yafang Shao wrote: > > The existed pGp shows the names of page flags only, rather than the full > information including section, node, zone, last cpuipid and kasan tag. > While it is not easy to parse these information manually because there > are so many flavors. We'd better interpret them in printf. > > To be compitable with the existed format of pGp, the new introduced ones > also use '|' as the separator, then the user tools parsing pGp won't > need to make change, suggested by Matthew. The new added information is > tracked onto the end of the existed one, e.g. > [ 8838.835456] Slab 0x2828b78a objects=33 used=3 > fp=0xd04efc88 > flags=0x17c0010200(slab|head|node=0|zone=2|lastcpupid=0x1f) > > The documentation and test cases are also updated. The result of the > test cases as follows, > [68599.816764] test_printf: loaded. > [68599.819068] test_printf: all 388 tests passed > [68599.830367] test_printf: unloaded. > > This patchset also includes some code cleanup in mm/slub.c. The series looks good to me. FWIW, Reviewed-by: Sergey Senozhatsky
Re: [PATCH] MAINTAINERS: update Senozhatsky email address
On (21/03/19 11:43), Petr Mladek wrote: > > The patch is comitted in printk/linux.git, branch for-5.13. Thanks. > Now I have to remember using the new address, for example, when > calling git send-email from bash history ;-) :)
Re: [PATCHv3 6/6] MAINTAINERS: update Senozhatsky email address
On (21/03/19 14:53), Sergey Senozhatsky wrote: > > I don't check my @gmail.com addresses often enough these days. > Please ignore this one. It's a different story and does not belong to this series. -ss
[PATCHv3 6/6] MAINTAINERS: update Senozhatsky email address
I don't check my @gmail.com addresses often enough these days. Signed-off-by: Sergey Senozhatsky --- MAINTAINERS | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index b2baeb5e4a68..01b000cd5774 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14433,7 +14433,7 @@ F: kernel/sched/psi.c PRINTK M: Petr Mladek -M: Sergey Senozhatsky +M: Sergey Senozhatsky R: Steven Rostedt R: John Ogness S: Maintained @@ -19293,7 +19293,7 @@ F: drivers/net/vrf.c VSPRINTF M: Petr Mladek M: Steven Rostedt -M: Sergey Senozhatsky +M: Sergey Senozhatsky R: Andy Shevchenko R: Rasmus Villemoes S: Maintained @@ -19944,7 +19944,7 @@ F: drivers/staging/media/zoran/ ZRAM COMPRESSED RAM BLOCK DEVICE DRVIER M: Minchan Kim M: Nitin Gupta -R: Sergey Senozhatsky +R: Sergey Senozhatsky L: linux-kernel@vger.kernel.org S: Maintained F: Documentation/admin-guide/blockdev/zram.rst @@ -19958,7 +19958,7 @@ F: drivers/tty/serial/zs.* ZSMALLOC COMPRESSED SLAB MEMORY ALLOCATOR M: Minchan Kim M: Nitin Gupta -R: Sergey Senozhatsky +R: Sergey Senozhatsky L: linux...@kvack.org S: Maintained F: Documentation/vm/zsmalloc.rst -- 2.31.0.rc2.261.g7f71774620-goog
[PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control
This patch implements UVC 1.5 Region of Interest (ROI) control.
Note that, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL controls
and mentions that ROI rectangle coordinates "must be within
the current Digital Window as specified by the CT_WINDOW control."
(4.2.2.1.20 Digital Region of Interest (ROI) Control).
It's is not entirely clear if we need to implement WINDOW_CONTROL.
ROI is naturally limited by GET_MIN and GET_MAX rectangles.
Another thing to note is that ROI support is implemented as
V4L2 selection target: selection rectangle represents ROI
rectangle and selection flags represent ROI auto-controls.
User-space is required to set valid values for both rectangle
and auto-controls every time SET_CUR is issued.
Usage example:
struct v4l2_selection roi = {0, };
roi.target = V4L2_SEL_TGT_ROI;
roi.r.left = 0;
roi.r.top = 0;
roi.r.width= 42;
roi.r.height = 42;
roi.flags = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
ioctl(fd, VIDIOC_S_SELECTION, );
Signed-off-by: Sergey Senozhatsky
---
drivers/media/usb/uvc/uvc_v4l2.c | 147 ++-
include/uapi/linux/usb/video.h | 1 +
2 files changed, 145 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..d0fe6c33fab6 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1139,14 +1139,66 @@ static int uvc_ioctl_querymenu(struct file *file, void
*fh,
return uvc_query_v4l2_menu(chain, qm);
}
-static int uvc_ioctl_g_selection(struct file *file, void *fh,
-struct v4l2_selection *sel)
+/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
+struct uvc_roi_rect {
+ __u16 top;
+ __u16 left;
+ __u16 bottom;
+ __u16 right;
+ __u16 auto_controls;
+} __packed;
+
+static int uvc_ioctl_g_roi_target(struct file *file, void *fh,
+ struct v4l2_selection *sel)
{
struct uvc_fh *handle = fh;
struct uvc_streaming *stream = handle->stream;
+ struct uvc_roi_rect *roi;
+ u8 query;
+ int ret;
- if (sel->type != stream->type)
+ switch (sel->target) {
+ case V4L2_SEL_TGT_ROI:
+ query = UVC_GET_CUR;
+ break;
+ case V4L2_SEL_TGT_ROI_DEFAULT:
+ query = UVC_GET_DEF;
+ break;
+ case V4L2_SEL_TGT_ROI_BOUNDS_MIN:
+ query = UVC_GET_MAX;
+ break;
+ case V4L2_SEL_TGT_ROI_BOUNDS_MAX:
+ query = UVC_GET_MAX;
+ break;
+ default:
return -EINVAL;
+ }
+
+ roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
+ if (!roi)
+ return -ENOMEM;
+
+ ret = uvc_query_ctrl(stream->dev, query, 1, stream->dev->intfnum,
+UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
+sizeof(struct uvc_roi_rect));
+ if (!ret) {
+ /* ROI left, top, right, bottom are global coordinates. */
+ sel->r.left = roi->left;
+ sel->r.top = roi->top;
+ sel->r.width= roi->right - roi->left + 1;
+ sel->r.height = roi->bottom - roi->top + 1;
+ sel->flags = roi->auto_controls;
+ }
+
+ kfree(roi);
+ return ret;
+}
+
+static int uvc_ioctl_g_sel_target(struct file *file, void *fh,
+ struct v4l2_selection *sel)
+{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
switch (sel->target) {
case V4L2_SEL_TGT_CROP_DEFAULT:
@@ -1173,6 +1225,94 @@ static int uvc_ioctl_g_selection(struct file *file, void
*fh,
return 0;
}
+static int uvc_ioctl_g_selection(struct file *file, void *fh,
+struct v4l2_selection *sel)
+{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
+
+ if (sel->type != stream->type)
+ return -EINVAL;
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ return uvc_ioctl_g_sel_target(file, fh, sel);
+ case V4L2_SEL_TGT_ROI:
+ case V4L2_SEL_TGT_ROI_DEFAULT:
+ case V4L2_SEL_TGT_ROI_BOUNDS_MIN:
+ case V4L2_SEL_TGT_ROI_BOUNDS_MAX:
+ return uvc_ioctl_g_roi_target(file, fh, sel);
+ }
+
+ return -EINVAL;
+}
+
+static bool validate_roi_bounds(struct uvc_streaming *stream,
+ struct v4l2_selection *sel)
+{
+ if (sel->r.l
[PATCHv3 2/6] media: v4l UAPI: document ROI selection targets
Document V4L2 selection targets that will be used to ROI
implementation.
Signed-off-by: Sergey Senozhatsky
---
.../media/v4l/selection-api-configuration.rst | 22 +++
.../media/v4l/selection-api-examples.rst | 28 +++
.../media/v4l/v4l2-selection-targets.rst | 24
3 files changed, 74 insertions(+)
diff --git
a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
index fee49bf1a1c0..b5fdd765e2db 100644
--- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
+++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
@@ -135,3 +135,25 @@ and the height of rectangles obtained using
``V4L2_SEL_TGT_CROP`` and
``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the
scaling is applied. The application can compute the scaling ratios using
these values.
+
+Configuration of Region of Interest (ROI)
+=
+
+The range of auto-controls values and of coordinates of the top left
+corner, width and height of areas that can be ROI is given by the
+``V4L2_SEL_TGT_ROI_BOUNDS_MIN`` and ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``
+targets. It is recommended for the driver developers to put the top/left
+corner at position ``(0,0)``.
+
+The top left corner, width and height of the Region of Interest area
+and auto-controls currently being employed by the device are given by
+the ``V4L2_SEL_TGT_ROI`` target. It uses the same coordinate system
+as ``V4L2_SEL_TGT_ROI_BOUNDS_MIN`` and ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``.
+
+In order to change active ROI top left, width and height coordinates
+and ROI auto-controls use ``V4L2_SEL_TGT_ROI`` target.
+
+Each capture device has a default ROI rectangle and auto-controls
+value given by the ``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall
+set the ROI rectangle to the default when the driver is first loaded,
+but not later.
diff --git a/Documentation/userspace-api/media/v4l/selection-api-examples.rst
b/Documentation/userspace-api/media/v4l/selection-api-examples.rst
index 5f8e8a1f59d7..ad2664888700 100644
--- a/Documentation/userspace-api/media/v4l/selection-api-examples.rst
+++ b/Documentation/userspace-api/media/v4l/selection-api-examples.rst
@@ -82,3 +82,31 @@ Example: Querying for scaling factors
/* computing scaling factors */
hscale = (double)compose.r.width / crop.r.width;
vscale = (double)compose.r.height / crop.r.height;
+
+Setting Region Of Interest area to half of the default value
+
+Example: Simple ROI
+===
+
+.. code-block:: c
+
+ struct v4l2_selection roi = {
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+ .target = V4L2_SEL_TGT_ROI_DEFAULT,
+ };
+ struct v4l2_rect r;
+
+ ret = ioctl(fd, VIDIOC_G_SELECTION, );
+ if (ret)
+ exit(-1);
+ /* setting smaller ROI rectangle */
+ r.width = roi.r.width / 2;
+ r.height = roi.r.height / 2;
+ r.left = roi.r.width / 4;
+ r.top = roi.r.height / 4;
+ roi.r = r;
+ roi.target = V4L2_SEL_TGT_ROI;
+ roi.flags = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
+ ret = ioctl(fd, VIDIOC_S_SELECTION, );
+ if (ret)
+ exit(-1);
diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
index b46bae984f35..d1dc9c50eb05 100644
--- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
+++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
@@ -75,6 +75,30 @@ of the two interfaces they are used.
modified by hardware.
- Yes
- No
+* - ``V4L2_SEL_TGT_ROI``
+ - 0x0200
+ - Current Region of Interest rectangle and auto-controls value.
+ - Yes
+ - No
+* - ``V4L2_SEL_TGT_ROI_DEFAULT``
+ - 0x0201
+ - Suggested Region of Interest rectangle and auto-controls value.
+ - Yes
+ - No
+* - ``V4L2_SEL_TGT_ROI_BOUNDS_MIN``
+ - 0x0202
+ - Minimum bounds of the Region of Interest rectangle and minimum
+ auto-controls value. All valid ROI rectangles and auto-controls
+ should be within minimum-maximum range.
+ - Yes
+ - No
+* - ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``
+ - 0x0203
+ - Maximum bounds of the Region of Interest rectangle and maximum
+ auto-controls value. All valid ROI rectangles and auto-controls
+ should be within minimum-maximum range.
+ - Yes
+ - No
.. raw:: latex
--
2.31.0.rc2.261.g7f71774620-goog
[PATCHv3 4/6] media: v4l UAPI: document ROI auto-controls flags
Document ROI auto controls. Signed-off-by: Sergey Senozhatsky --- .../media/v4l/v4l2-selection-flags.rst| 40 +++ 1 file changed, 40 insertions(+) diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst index 1cb1531c1e52..536d29a6c4a5 100644 --- a/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst @@ -48,6 +48,46 @@ Selection flags inside the subdevice to all further processing steps. - No - Yes +* - ``V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE`` + - (1 << 0) + - Auto Exposure. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_IRIS`` + - (1 << 1) + - Auto Iris. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_WHITE_BALANCE`` + - (1 << 2) + - Auto White Balance. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_FOCUS`` + - (1 << 3) + - Auto Focus. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_FACE_DETECT`` + - (1 << 4) + - Auto Face Detect. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_DETECT_AND_TRACK`` + - (1 << 5) + - Auto Detect and Track. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_IMAGE_STABILIXATION`` + - (1 << 6) + - Image Stabilization. + - Yes + - No +* - ``V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY`` + - (1 << 7) + - Higher Quality. + - Yes + - No .. raw:: latex -- 2.31.0.rc2.261.g7f71774620-goog
[PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags
UVC 1.5 defines the following Region Of Interest auto controls:
D0: Auto Exposure
D1: Auto Iris
D2: Auto White Balance
D3: Auto Focus
D4: Auto Face Detect
D5: Auto Detect and Track
D6: Image Stabilization
D7: Higher Quality
D8 – D15: Reserved, set to zero
Signed-off-by: Sergey Senozhatsky
---
include/uapi/linux/v4l2-common.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 3651ebb8cb23..34f1c262d6aa 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -92,6 +92,16 @@
#define V4L2_SEL_FLAG_LE (1 << 1)
#define V4L2_SEL_FLAG_KEEP_CONFIG (1 << 2)
+/* ROI auto-controls flags */
+#define V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE(1 << 0)
+#define V4L2_SEL_FLAG_ROI_AUTO_IRIS(1 << 1)
+#define V4L2_SEL_FLAG_ROI_AUTO_WHITE_BALANCE (1 << 2)
+#define V4L2_SEL_FLAG_ROI_AUTO_FOCUS (1 << 3)
+#define V4L2_SEL_FLAG_ROI_AUTO_FACE_DETECT (1 << 4)
+#define V4L2_SEL_FLAG_ROI_AUTO_DETECT_AND_TRACK(1 << 5)
+#define V4L2_SEL_FLAG_ROI_AUTO_IMAGE_STABILIXATION (1 << 6)
+#define V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY (1 << 7)
+
struct v4l2_edid {
__u32 pad;
__u32 start_block;
--
2.31.0.rc2.261.g7f71774620-goog
[PATCHv3 0/6] media: uvcvideo: implement UVC 1.5 ROI
Hello, This patch set implements UVC 1.5 ROI using v4l2_selection API. V3: - reimplemented ROI. We dont' use split controls anymore. - Ricardo's feedback Sergey Senozhatsky (6): media: v4l UAPI: add ROI selection targets media: v4l UAPI: document ROI selection targets media: v4l UAPI: add ROI auto-controls flags media: v4l UAPI: document ROI auto-controls flags media: uvcvideo: add UVC 1.5 ROI control MAINTAINERS: update Senozhatsky email address .../media/v4l/selection-api-configuration.rst | 22 +++ .../media/v4l/selection-api-examples.rst | 28 .../media/v4l/v4l2-selection-flags.rst| 40 + .../media/v4l/v4l2-selection-targets.rst | 24 +++ MAINTAINERS | 8 +- drivers/media/usb/uvc/uvc_v4l2.c | 147 +- include/uapi/linux/usb/video.h| 1 + include/uapi/linux/v4l2-common.h | 18 +++ 8 files changed, 281 insertions(+), 7 deletions(-) -- 2.31.0.rc2.261.g7f71774620-goog
[PATCHv3 1/6] media: v4l UAPI: add ROI selection targets
UVC 1.5 requires Region Of Interest control to implement GET_CUR, GET_DEF, GET_MIN and GET_MAX requests. This patch adds new V4L2 selection API targets that will implement those ROI requests. Signed-off-by: Sergey Senozhatsky --- include/uapi/linux/v4l2-common.h | 8 1 file changed, 8 insertions(+) diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h index 7d21c1634b4d..3651ebb8cb23 100644 --- a/include/uapi/linux/v4l2-common.h +++ b/include/uapi/linux/v4l2-common.h @@ -78,6 +78,14 @@ #define V4L2_SEL_TGT_COMPOSE_BOUNDS0x0102 /* Current composing area plus all padding pixels */ #define V4L2_SEL_TGT_COMPOSE_PADDED0x0103 +/* Current Region of Interest area */ +#define V4L2_SEL_TGT_ROI 0x0200 +/* Default Region of Interest area */ +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 +/* Region of Interest minimum values */ +#define V4L2_SEL_TGT_ROI_BOUNDS_MIN0x0202 +/* Region of Interest maximum values */ +#define V4L2_SEL_TGT_ROI_BOUNDS_MAX0x0203 /* Selection flags */ #define V4L2_SEL_FLAG_GE (1 << 0) -- 2.31.0.rc2.261.g7f71774620-goog
[PATCH] MAINTAINERS: update Senozhatsky email address
I don't check my @gmail.com addresses often enough these days. Signed-off-by: Sergey Senozhatsky --- MAINTAINERS | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index b2baeb5e4a68..01b000cd5774 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14433,7 +14433,7 @@ F: kernel/sched/psi.c PRINTK M: Petr Mladek -M: Sergey Senozhatsky +M: Sergey Senozhatsky R: Steven Rostedt R: John Ogness S: Maintained @@ -19293,7 +19293,7 @@ F: drivers/net/vrf.c VSPRINTF M: Petr Mladek M: Steven Rostedt -M: Sergey Senozhatsky +M: Sergey Senozhatsky R: Andy Shevchenko R: Rasmus Villemoes S: Maintained @@ -19944,7 +19944,7 @@ F: drivers/staging/media/zoran/ ZRAM COMPRESSED RAM BLOCK DEVICE DRVIER M: Minchan Kim M: Nitin Gupta -R: Sergey Senozhatsky +R: Sergey Senozhatsky L: linux-kernel@vger.kernel.org S: Maintained F: Documentation/admin-guide/blockdev/zram.rst @@ -19958,7 +19958,7 @@ F: drivers/tty/serial/zs.* ZSMALLOC COMPRESSED SLAB MEMORY ALLOCATOR M: Minchan Kim M: Nitin Gupta -R: Sergey Senozhatsky +R: Sergey Senozhatsky L: linux...@kvack.org S: Maintained F: Documentation/vm/zsmalloc.rst -- 2.31.0.rc2.261.g7f71774620-goog
Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/18 22:19), Ricardo Ribalda wrote:
> >
> > May I please ask for more opinions on this?
>
> Could you try setting the roi in a loop in your device and verify that
> it accepts all the values with no modification. If so we can implement
> the set/get as a quirk for other devices.
Tested on two (very) different devices.
Firmware D:
CLAP all passed, we are cool
Firmware H:
CLAP all passed, we are cool
Code sample
---
in_selection.target = V4L2_SEL_TGT_ROI;
in_selection.flags = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
for (int i = 0; i < 1001; i++) {
in_selection.r.left = 0 + i;
in_selection.r.top = 0 + i;
in_selection.r.width= 42 + i;
in_selection.r.height = 42 + i;
ret = doioctl(fd, VIDIOC_S_SELECTION, _selection);
if (ret) {
fprintf(stderr, "BOOM %d\n", ret);
exit(1);
}
ret = doioctl(fd, VIDIOC_G_SELECTION, _selection);
if (ret) {
fprintf(stderr, "BANG %d\n", ret);
exit(2);
}
if (in_selection.r.left != i ||
in_selection.r.top != i ||
in_selection.r.width != i + 42 ||
in_selection.r.height != i + 42) {
fprintf(stderr, "SNAP %d %d %d %d != %d %d %d %d\n",
i, i, i + 42, i + 42,
in_selection.r.left,
in_selection.r.top,
in_selection.r.width,
in_selection.r.height);
exit(3);
}
}
fprintf(stderr, "CLAP all passed, we are cool\n");
---
Re: [PATCH] cifsd: fix a precedence bug in parse_dacl()
On (21/03/18 16:10), Dan Carpenter wrote:
>
> The shift has higher precedence than mask so this doesn't work as
> intended.
>
> Fixes: ef24dca82789 ("cifsd: add support for ACLs")
> Signed-off-by: Dan Carpenter
Thanks.
Reviewed-by: Sergey Senozhatsky
-ss
Re: [PATCH RESEND] cifsd: fix a IS_ERR() vs NULL bug
On (21/03/18 16:11), Dan Carpenter wrote:
>
> The smb_direct_alloc_sendmsg() function never returns NULL, it only
> returns error pointers so the check needs to be updated.
>
> Fixes: cabcebc31de4 ("cifsd: introduce SMB3 kernel server")
> Signed-off-by: Dan Carpenter
Thanks.
Reviewed-by: Sergey Senozhatsky
-ss
Re: [PATCH] cifsd: Fix a use after free on error path
On (21/03/18 16:12), Dan Carpenter wrote:
> The ksmbd_free_work_struct() frees "work" so we need to swap the order
> of these two function calls to avoid a use after free.
>
> Fixes: cabcebc31de4 ("cifsd: introduce SMB3 kernel server")
> Signed-off-by: Dan Carpenter
Thanks.
Reviewed-by: Sergey Senozhatsky
-ss
Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/17 08:58), Ricardo Ribalda Delgado wrote: [..] > > > > GET_CUR? > yep > > > > > > https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection > > > On success the struct v4l2_rect r field contains the adjusted > > > rectangle. > > > > What is the adjusted rectangle here? Does this mean that firmware can > > successfully apply SET_CUR and return 0, but in reality it was not happy > > with the rectangle dimensions so it modified it behind the scenes? > > I can imagine that some hw might have spooky requirements for the roi > rectangle (multiple of 4, not crossing the bayer filter, odd/even > line...) so they might be able to go the closest valid config. Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very hot path, depending on what user-space considers to be of interest and how frequently that object of interest changes its position/shape/etc. Doing GET_CUR after every SET_CUR doubles the number of firmware calls we issue, that's for sure; is it worth it - that's something that I'm not sure of. May I please ask for more opinions on this? -ss
Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
On (21/03/17 18:18), Sergey Senozhatsky wrote:
> On (21/02/08 14:17), Sergey Senozhatsky wrote:
> > This patch adds support for Region of Interest bmAutoControls.
> >
> > ROI control is a compound data type:
> > Control Selector CT_REGION_OF_INTEREST_CONTROL
> > Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> > wLength 10
> > Offset FieldSize
> > 0wROI_Top 2
> > 2wROI_Left2
> > 4wROI_Bottom 2
> > 6wROI_Right 2
> > 8bmAutoControls 2 (Bitmap)
> >
> > uvc_control_mapping, however, can handle only s32 data type at the
> > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> >
> > Hence ROI control handling is split into two patches:
> > a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> >separately, by the means of selection API.
>
> This approach is "no go".
>
> I just figured out (am still debugging tho) that this patch set works on
> some devices and doesn't work on other. The root cause seems to be the
> fact that some firmwares error out all ROI requests when sizeof() of the
> ROI data is not 5 * __u16.
>
> So those devices are not happy if we set/get ROI rectangle 4 * __u16
> and auto-controls __u16 separately; they want to set/get rect and
> rectanles in one shot.
>
> This fixes ROI on those devices.
>
> ---
>
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1190,6 +1190,7 @@ struct uvc_roi_rect {
> __u16 left;
> __u16 bottom;
> __u16 right;
> + __u16 auto_controls;
> };
Maybe I can drop the separate auto-control and just use v4l2_selection::flags
to set the roi auto-controls value.
Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
On (21/02/08 14:17), Sergey Senozhatsky wrote:
> This patch adds support for Region of Interest bmAutoControls.
>
> ROI control is a compound data type:
> Control Selector CT_REGION_OF_INTEREST_CONTROL
> Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> wLength 10
> Offset FieldSize
> 0wROI_Top 2
> 2wROI_Left2
> 4wROI_Bottom 2
> 6wROI_Right 2
> 8bmAutoControls 2 (Bitmap)
>
> uvc_control_mapping, however, can handle only s32 data type at the
> moment: ->get() returns s32 value, ->set() accepts s32 value; while
> v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
>
> Hence ROI control handling is split into two patches:
> a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
>separately, by the means of selection API.
This approach is "no go".
I just figured out (am still debugging tho) that this patch set works on
some devices and doesn't work on other. The root cause seems to be the
fact that some firmwares error out all ROI requests when sizeof() of the
ROI data is not 5 * __u16.
So those devices are not happy if we set/get ROI rectangle 4 * __u16
and auto-controls __u16 separately; they want to set/get rect and
rectanles in one shot.
This fixes ROI on those devices.
---
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1190,6 +1190,7 @@ struct uvc_roi_rect {
__u16 left;
__u16 bottom;
__u16 right;
+ __u16 auto_controls;
};
---
Back to base 1.
Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
Fixed Tomasz's email On (21/03/17 09:08), Ricardo Ribalda Delgado wrote: [..] > > > > + .id = V4L2_CID_REGION_OF_INTEREST_AUTO, > > > > + .name = "Region of Interest (auto)", > > > > + .entity = UVC_GUID_UVC_CAMERA, > > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > > + .size = 16, > > > > + .offset = 64, > > > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > > > Are > > > > Are? > > Aye! > You are not a good kernel reviewer if you do not talk pirate :P. Arr, Matey! -ss
Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
On (21/03/17 09:04), Ricardo Ribalda Delgado wrote: > On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky > wrote: > > > > On (21/03/16 19:19), Ricardo Ribalda Delgado wrote: > > > > +Configuration of Region of Interest (ROI) > > > > += > > > > + > > > > +The range of coordinates of the top left corner, width and height of > > > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` > > > > target. > > > > +It is recommended for the driver developers to put the top/left corner > > > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > > > > +coordinates. The units are in pixels and independent of the field of > > > > view. > > > > +They are not impacted by any cropping or scaling that is currently > > > > being > > > > +used. > > > > > > Can we also mention binning here? > > > > What's binning? Is it in the UVC spec? > > Binning is when you reduce an image by adding up surrounding pixels. > > So you have a 100x100 image that you convert to a 50x50 but showing > the same area of interest. I see. Hmm, not sure if I can comment on this. It's not in the spec, so it might be up to the firmware, maybe. What do you think? > > > > diff --git a/include/uapi/linux/v4l2-common.h > > > > b/include/uapi/linux/v4l2-common.h > > > > index 7d21c1634b4d..d0c108fba638 100644 > > > > --- a/include/uapi/linux/v4l2-common.h > > > > +++ b/include/uapi/linux/v4l2-common.h > > > > @@ -78,6 +78,14 @@ > > > > #define V4L2_SEL_TGT_COMPOSE_BOUNDS0x0102 > > > > /* Current composing area plus all padding pixels */ > > > > #define V4L2_SEL_TGT_COMPOSE_PADDED0x0103 > > > > +/* Current Region of Interest area */ > > > > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > > > > +/* Default Region of Interest area */ > > > > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > > > > +/* Region of Interest bounds */ > > > > +#define V4L2_SEL_TGT_ROI_BOUNDS0x0202 > > > > +/* Set Region of Interest area */ > > > > +#define V4L2_SEL_TGT_ROI 0x0203 > > > > > > Nit: Maybe it could be a good idea to split doc and code. This way the > > > backports/fixes are easier. > > > > I'm quite sure this is the first time I'm being asked to split code > > and documentation :) I'm usually asked to do the opposite - merge code > > and documentation. > > I got answered in both directions. I prefer to split it because the > doc can go to different audience than the code, and then it makes my > life easier when backporting. > > But if you or Laurent prefer otherwise I am of course happy with any option > ;) Either way works for me. Laurent, any preferences? -ss
Re: linux-next: build failure after merge of the cifsd tree
On (21/03/17 18:53), Stephen Rothwell wrote:
> Hi all,
>
> After merging the cifsd tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> ld: fs/cifsd/misc.o:(.opd+0xc0): multiple definition of `extract_sharename';
> fs/cifs/unc.o:(.opd+0x18): first defined here
> ld: fs/cifsd/misc.o: in function `.extract_sharename':
> misc.c:(.text.extract_sharename+0x0): multiple definition of
> `.extract_sharename'; fs/cifs/unc.o:unc.c:(.text.extract_sharename+0x0):
> first defined here
>
> Caused by commit
>
> cabcebc31de4 ("cifsd: introduce SMB3 kernel server")
>
> I applied the following patch for today:
>
> From: Stephen Rothwell
> Date: Wed, 17 Mar 2021 18:35:55 +1100
> Subject: [PATCH] cifsd: uniquify extract_sharename()
>
> Signed-off-by: Stephen Rothwell
Thanks!
Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control
On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > -static int uvc_ioctl_g_selection(struct file *file, void *fh,
> > -struct v4l2_selection *sel)
> > +/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
> > +struct uvc_roi_rect {
> > + __u16 top;
> > + __u16 left;
> > + __u16 bottom;
> > + __u16 right;
> > +};
>
> Perhaps __packed; ?
Perhaps.
> > +static int uvc_ioctl_g_selection(struct file *file, void *fh,
> > +struct v4l2_selection *sel)
> > +{
> > + struct uvc_fh *handle = fh;
> > + struct uvc_streaming *stream = handle->stream;
> > +
> > + if (sel->type != stream->type)
> > + return -EINVAL;
> > +
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > + return uvc_ioctl_g_sel_target(file, fh, sel);
> > + case V4L2_SEL_TGT_ROI_CURRENT:
> > + case V4L2_SEL_TGT_ROI_DEFAULT:
> > + case V4L2_SEL_TGT_ROI_BOUNDS:
> > + return uvc_ioctl_g_roi_target(file, fh, sel);
> > + }
> > +
> > + return -EINVAL;
> > +}
>
> Are you sure that there is no lock needed between the control and the
> selection API?
Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
Hmm. They write to different offsets and don't seem to be overlapping.
> > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > + struct v4l2_selection *sel)
> > +{
> > + bool ok = true;
> > +
> > + if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> > + sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> > + return false;
> > +
> > + /* perhaps also can test against ROI GET_MAX */
> > +
> > + mutex_lock(>mutex);
[...]
> > + if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > + ok = false;
> > + if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > + ok = false;
> Maybe you should not release this mutex until query_ctrl is done?
Maybe... These two tests are somewhat made up. Not sure if we need them.
On one hand it's reasonable to keep ROI within the frames. On the other
hand - such relation is not spelled out in the spec. If the stream change
its frame dimensions ROI is not getting invalidated or re-calculated by
the firmware. UVC spec says that ROI should be bounded only by the
CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
WINDOW_CONTROL.
So maybe I can remove stream->cuf_frame boundaries check instead.
> > + mutex_unlock(>mutex);
> > +
> > + return ok;
> > +}
> > +
> > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > + struct v4l2_selection *sel)
> > +{
> > + struct uvc_fh *handle = fh;
> > + struct uvc_streaming *stream = handle->stream;
> > + struct uvc_roi_rect *roi;
> > + int ret;
> > +
> > + if (!validate_roi_bounds(stream, sel))
> > + return -E2BIG;
> > +
> > + roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> > + if (!roi)
> > + return -ENOMEM;
> > +
> > + roi->left = sel->r.left;
> > + roi->top= sel->r.top;
> > + roi->right = sel->r.width;
> > + roi->bottom = sel->r.height;
> > +
> > + ret = uvc_query_ctrl(stream->dev, UVC_SET_CUR, 1,
> > stream->dev->intfnum,
> > +UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> > +sizeof(struct uvc_roi_rect));
>
> I think you need to read back from the device the actual value
GET_CUR?
> https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> On success the struct v4l2_rect r field contains the adjusted
> rectangle.
What is the adjusted rectangle here? Does this mean that firmware can
successfully apply SET_CUR and return 0, but in reality it was not happy
with the rectangle dimensions so it modified it behind the scenes?
I can add GET_CUR I guess, but do we want to double the traffic, given
that ROI SET_CUR can be executed relatively often?
Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
On (21/03/16 19:29), Ricardo Ribalda Delgado wrote:
> > ROI control is a compound data type:
> > Control Selector CT_REGION_OF_INTEREST_CONTROL
> > Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> > wLength 10
> > Offset FieldSize
> > 0wROI_Top 2
> > 2wROI_Left2
> > 4wROI_Bottom 2
> > 6wROI_Right 2
> > 8bmAutoControls 2 (Bitmap)
> >
> > uvc_control_mapping, however, can handle only s32 data type at the
> > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> >
> > Hence ROI control handling is split into two patches:
> > a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> >separately, by the means of selection API.
>
> Maybe a reference to the uvc doc would be a good thing to add here.
OK. What should be referenced there?
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> > + - Auto Exposure.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> > + - Auto Iris.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > + - Auto White Balance.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> > + - Auto Focus.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > + - Auto Face Detect.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > + - Auto Detect and Track.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> > + - Image Stabilization.
> > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > + - Higher Quality.
> > +
> >
> Nit: Same as before splitting doc and code.
OK, I guess I can split.
> > static const struct uvc_menu_info power_line_frequency_controls[] = {
> > @@ -753,6 +762,16 @@ static const struct uvc_control_mapping
> > uvc_ctrl_mappings[] = {
> > .v4l2_type = V4L2_CTRL_TYPE_BOOLEAN,
> > .data_type = UVC_CTRL_DATA_TYPE_BOOLEAN,
> > },
> > + {
> > + .id = V4L2_CID_REGION_OF_INTEREST_AUTO,
> > + .name = "Region of Interest (auto)",
> > + .entity = UVC_GUID_UVC_CAMERA,
> > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > + .size = 16,
> > + .offset = 64,
> > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK,
> Are
Are?
> > #define V4L2_CID_PAN_SPEED
> > (V4L2_CID_CAMERA_CLASS_BASE+32)
> > #define V4L2_CID_TILT_SPEED
> > (V4L2_CID_CAMERA_CLASS_BASE+33)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO
> > (V4L2_CID_CAMERA_CLASS_BASE+34)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS (1 << 1)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION (1 << 6)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY(1 << 7)
> >
> > #define V4L2_CID_CAMERA_ORIENTATION
> > (V4L2_CID_CAMERA_CLASS_BASE+34)
> > #define V4L2_CAMERA_ORIENTATION_FRONT 0
> > --
> > 2.30.0
> >
>
> I think we have to add the CID to v4l2_ctrl_get_name()
Sounds good. Only this one - V4L2_CID_REGION_OF_INTEREST_AUTO, right?
Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
On (21/03/16 19:19), Ricardo Ribalda Delgado wrote: > > +Configuration of Region of Interest (ROI) > > += > > + > > +The range of coordinates of the top left corner, width and height of > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > > +It is recommended for the driver developers to put the top/left corner > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > > +coordinates. The units are in pixels and independent of the field of view. > > +They are not impacted by any cropping or scaling that is currently being > > +used. > > Can we also mention binning here? What's binning? Is it in the UVC spec? > > +The top left corner, width and height of the Region of Interest area > > +currently being employed by the device is given by the > > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system > > +as ``V4L2_SEL_TGT_ROI_BOUNDS``. > > Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ? We don't. Will remove it. > > +* - ``V4L2_SEL_TGT_ROI_CURRENT`` > > + - 0x0200 > > + - Current Region of Interest rectangle. > > + - Yes > > + - No > > +* - ``V4L2_SEL_TGT_ROI_DEFAULT`` > > + - 0x0201 > > + - Suggested Region of Interest rectangle. > > + - Yes > > + - No > > +* - ``V4L2_SEL_TGT_ROI_BOUNDS`` > > + - 0x0202 > > + - Bounds of the Region of Interest rectangle. All valid ROI > > rectangles fit > > + inside the ROI bounds rectangle. > > + - Yes > > + - No > > +* - ``V4L2_SEL_TGT_ROI`` > > + - 0x0203 > > + - Sets the new Region of Interest rectangle. > > + - Yes > > + - No > As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI Agreed. > > diff --git a/include/uapi/linux/v4l2-common.h > > b/include/uapi/linux/v4l2-common.h > > index 7d21c1634b4d..d0c108fba638 100644 > > --- a/include/uapi/linux/v4l2-common.h > > +++ b/include/uapi/linux/v4l2-common.h > > @@ -78,6 +78,14 @@ > > #define V4L2_SEL_TGT_COMPOSE_BOUNDS0x0102 > > /* Current composing area plus all padding pixels */ > > #define V4L2_SEL_TGT_COMPOSE_PADDED0x0103 > > +/* Current Region of Interest area */ > > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > > +/* Default Region of Interest area */ > > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > > +/* Region of Interest bounds */ > > +#define V4L2_SEL_TGT_ROI_BOUNDS0x0202 > > +/* Set Region of Interest area */ > > +#define V4L2_SEL_TGT_ROI 0x0203 > > Nit: Maybe it could be a good idea to split doc and code. This way the > backports/fixes are easier. I'm quite sure this is the first time I'm being asked to split code and documentation :) I'm usually asked to do the opposite - merge code and documentation.
Re: [PATCHv2 0/3] Add UVC 1.5 Region Of Interest control to uvcvideo
On (21/02/08 14:17), Sergey Senozhatsky wrote: > Hello, > > RFC Hi Laurent, Gentle ping. -ss
Re: [PATCH] media: videobuf2: Fix integer overrun in allocation
On (21/03/10 00:43), Ricardo Ribalda wrote:
>
> The plane_length is an unsigned integer. So, if we have a size of
> 0x bytes we incorrectly allocate 0 bytes instead of 1 << 32.
>
> Cc: sta...@vger.kernel.org
> Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for
> mmap")
> Signed-off-by: Ricardo Ribalda
Reviewed-by: Sergey Senozhatsky
Re: [PATCH] media: videobuf2: Fix integer overrun in vb2_mmap
On (21/03/10 08:40), Ricardo Ribalda wrote:
>
> The plane_length is an unsigned integer. So, if we have a size of
> 0x bytes we incorrectly allocate 0 bytes instead of 1 << 32.
>
> Suggested-by: Sergey Senozhatsky
> Cc: sta...@vger.kernel.org
> Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for
> mmap")
> Signed-off-by: Ricardo Ribalda
FWIF,
Reviewed-by: Sergey Senozhatsky
Re: [PATCH] media: videobuf2: Fix integer overrun in allocation
On (21/03/10 00:43), Ricardo Ribalda wrote:
> The plane_length is an unsigned integer. So, if we have a size of
> 0x bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Hi Ricardo,
> @@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>* NOTE: mmapped areas should be page aligned
>*/
> for (plane = 0; plane < vb->num_planes; ++plane) {
> + unsigned long size = vb->planes[plane].length;
> +
> /* Memops alloc requires size to be page aligned. */
> - unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
> + size = PAGE_ALIGN(size);
>
> /* Did it wrap around? */
> if (size < vb->planes[plane].length)
Shouldn't the same be done in vb2_mmap()?
-ss
[PATCH] v4l-compliance: re-introduce NON_COHERENT and cache hints tests
This returns back non-coherent (previously known as NON_COHERENT)
memory flag and buffer cache management hints testing (for VB2_MEMORY_MMAP
buffers).
Signed-off-by: Sergey Senozhatsky
---
utils/common/cv4l-helpers.h | 8 +--
utils/common/v4l-helpers.h | 8 ++-
utils/v4l2-compliance/v4l2-test-buffers.cpp | 65 ++---
3 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/utils/common/cv4l-helpers.h b/utils/common/cv4l-helpers.h
index 712efde6..3cee372b 100644
--- a/utils/common/cv4l-helpers.h
+++ b/utils/common/cv4l-helpers.h
@@ -754,17 +754,17 @@ public:
int g_fd(unsigned index, unsigned plane) const { return
v4l_queue_g_fd(this, index, plane); }
void s_fd(unsigned index, unsigned plane, int fd) {
v4l_queue_s_fd(this, index, plane, fd); }
- int reqbufs(cv4l_fd *fd, unsigned count = 0)
+ int reqbufs(cv4l_fd *fd, unsigned count = 0, unsigned int flags = 0)
{
- return v4l_queue_reqbufs(fd->g_v4l_fd(), this, count);
+ return v4l_queue_reqbufs(fd->g_v4l_fd(), this, count, flags);
}
bool has_create_bufs(cv4l_fd *fd) const
{
return v4l_queue_has_create_bufs(fd->g_v4l_fd(), this);
}
- int create_bufs(cv4l_fd *fd, unsigned count, const v4l2_format *fmt =
NULL)
+ int create_bufs(cv4l_fd *fd, unsigned count, const v4l2_format *fmt =
NULL, unsigned int flags = 0)
{
- return v4l_queue_create_bufs(fd->g_v4l_fd(), this, count, fmt);
+ return v4l_queue_create_bufs(fd->g_v4l_fd(), this, count, fmt,
flags);
}
int mmap_bufs(cv4l_fd *fd, unsigned from = 0)
{
diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h
index f96b3c38..c09cd987 100644
--- a/utils/common/v4l-helpers.h
+++ b/utils/common/v4l-helpers.h
@@ -1515,7 +1515,7 @@ static inline int v4l_queue_querybufs(struct v4l_fd *f,
struct v4l_queue *q, uns
}
static inline int v4l_queue_reqbufs(struct v4l_fd *f,
- struct v4l_queue *q, unsigned count)
+ struct v4l_queue *q, unsigned count, unsigned int flags = 0)
{
struct v4l2_requestbuffers reqbufs;
int ret;
@@ -1523,6 +1523,7 @@ static inline int v4l_queue_reqbufs(struct v4l_fd *f,
reqbufs.type = q->type;
reqbufs.memory = q->memory;
reqbufs.count = count;
+ reqbufs.flags = flags;
/*
* Problem: if REQBUFS returns an error, did it free any old
* buffers or not?
@@ -1547,7 +1548,7 @@ static inline bool v4l_queue_has_create_bufs(struct
v4l_fd *f, const struct v4l_
static inline int v4l_queue_create_bufs(struct v4l_fd *f,
struct v4l_queue *q, unsigned count,
- const struct v4l2_format *fmt)
+ const struct v4l2_format *fmt, unsigned int flags = 0)
{
struct v4l2_create_buffers createbufs;
int ret;
@@ -1555,6 +1556,7 @@ static inline int v4l_queue_create_bufs(struct v4l_fd *f,
createbufs.format.type = q->type;
createbufs.memory = q->memory;
createbufs.count = count;
+ createbufs.flags = flags;
if (fmt) {
createbufs.format = *fmt;
} else {
@@ -1733,7 +1735,7 @@ static inline void v4l_queue_free(struct v4l_fd *f,
struct v4l_queue *q)
v4l_ioctl(f, VIDIOC_STREAMOFF, >type);
v4l_queue_release_bufs(f, q, 0);
v4l_queue_close_exported_fds(q);
- v4l_queue_reqbufs(f, q, 0);
+ v4l_queue_reqbufs(f, q, 0, 0);
}
static inline void v4l_queue_buffer_update(const struct v4l_queue *q,
diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp
b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index e40461bd..6555c0cb 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -663,6 +663,10 @@ int testReqBufs(struct node *node)
fail_on_test(q.reqbufs(node, 0));
for (m = V4L2_MEMORY_MMAP; m <= V4L2_MEMORY_DMABUF; m++) {
+ bool cache_hints_cap = false;
+ bool consistent;
+
+ cache_hints_cap = q.g_capabilities() &
V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
if (!(node->valid_memorytype & (1 << m)))
continue;
cv4l_queue q2(i, m);
@@ -678,8 +682,17 @@ int testReqBufs(struct node *node)
reqbufs.count = 1;
reqbufs.type = i;
reqbufs.memory = m;
+ reqbufs.flags = V4L2_FLAG_MEMORY_NON_COHERENT;
fail_on_test(doioctl(node, VIDIOC_REQBUFS, ));
- fail_on_test(check_0(reqbufs.reserved,
sizeof(reqbufs.reserved)));
+ consistent = reqbufs.flags &
V4L2_FLAG_MEMORY_NON_
Re: [PATCH] v4l-compliance: re-introduce NON_COHERENT and cache hints tests
On (21/03/02 09:49), Sergey Senozhatsky wrote: > This returns back non-coherent (previously known as NON_COHERENT) ^^^ NON_CONSISTENT... > memory flag and buffer cache management hints testing (for VB2_MEMORY_MMAP > buffers).
[PATCH 8/8] videobuf2: handle non-contiguous DMA allocations
This adds support for new noncontiguous DMA API, which
requires allocators to have two execution branches: one
for the current API, and one for the new one.
Signed-off-by: Sergey Senozhatsky
[hch: untested conversion to the ne API]
Signed-off-by: Christoph Hellwig
---
.../common/videobuf2/videobuf2-dma-contig.c | 141 +++---
1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index 1e218bc440c6..d6a9f7b682f3 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
@@ -42,8 +43,14 @@ struct vb2_dc_buf {
struct dma_buf_attachment *db_attach;
struct vb2_buffer *vb;
+ unsigned intnon_coherent_mem:1;
};
+static bool vb2_dc_is_coherent(struct vb2_dc_buf *buf)
+{
+ return !buf->non_coherent_mem;
+}
+
/*/
/*scatterlist table functions*/
/*/
@@ -78,12 +85,21 @@ static void *vb2_dc_cookie(struct vb2_buffer *vb, void
*buf_priv)
static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
{
struct vb2_dc_buf *buf = buf_priv;
- struct dma_buf_map map;
- int ret;
- if (!buf->vaddr && buf->db_attach) {
- ret = dma_buf_vmap(buf->db_attach->dmabuf, );
- buf->vaddr = ret ? NULL : map.vaddr;
+ if (buf->vaddr)
+ return buf->vaddr;
+
+ if (buf->db_attach) {
+ struct dma_buf_map map;
+
+ if (!dma_buf_vmap(buf->db_attach->dmabuf, ))
+ buf->vaddr = map.vaddr;
+ }
+
+ if (!vb2_dc_is_coherent(buf)) {
+ buf->vaddr = dma_vmap_noncontiguous(buf->dev,
+ buf->size,
+ buf->dma_sgt);
}
return buf->vaddr;
@@ -101,13 +117,26 @@ static void vb2_dc_prepare(void *buf_priv)
struct vb2_dc_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
+ /* This takes care of DMABUF and user-enforced cache sync hint */
if (buf->vb->skip_cache_sync_on_prepare)
return;
+ /*
+* Coherent MMAP buffers do not need to be synced, unlike coherent
+* USERPTR and non-coherent MMAP buffers.
+*/
+ if (buf->vb->memory == V4L2_MEMORY_MMAP && vb2_dc_is_coherent(buf))
+ return;
+
if (!sgt)
return;
+ /* For both USERPTR and non-coherent MMAP */
dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir);
+
+ /* Non-coherrent MMAP only */
+ if (!vb2_dc_is_coherent(buf) && buf->vaddr)
+ flush_kernel_vmap_range(buf->vaddr, buf->size);
}
static void vb2_dc_finish(void *buf_priv)
@@ -115,19 +144,46 @@ static void vb2_dc_finish(void *buf_priv)
struct vb2_dc_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
+ /* This takes care of DMABUF and user-enforced cache sync hint */
if (buf->vb->skip_cache_sync_on_finish)
return;
+ /*
+* Coherent MMAP buffers do not need to be synced, unlike coherent
+* USERPTR and non-coherent MMAP buffers.
+*/
+ if (buf->vb->memory == V4L2_MEMORY_MMAP && vb2_dc_is_coherent(buf))
+ return;
+
if (!sgt)
return;
+ /* For both USERPTR and non-coherent MMAP */
dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir);
+
+ /* Non-coherrent MMAP only */
+ if (!vb2_dc_is_coherent(buf) && buf->vaddr)
+ invalidate_kernel_vmap_range(buf->vaddr, buf->size);
}
/*/
/*callbacks for MMAP buffers */
/*/
+static void __vb2_dc_put(struct vb2_dc_buf *buf)
+{
+ if (vb2_dc_is_coherent(buf)) {
+ dma_free_attrs(buf->dev, buf->size, buf->cookie,
+ buf->dma_addr, buf->attrs);
+ return;
+ }
+
+ if (buf->vaddr)
+ dma_vunmap_noncontiguous(buf->dev, buf->vaddr);
+ dma_free_noncontiguous(buf->dev, buf->size,
+ buf->dma_sgt, buf->dma_addr);
+}
+
static void vb2_dc_put(void *buf_priv)
{
struct vb2_dc_buf *buf = buf_priv;
@@ -139,17 +195,47 @@ static void vb2_dc_put(void *buf_priv)
sg_free_table(buf->sgt_base);
kfree(buf->sgt_base);
[PATCH 7/8] videobuf2: handle V4L2_FLAG_MEMORY_NON_COHERENT flag
This patch lets user-space to request a non-coherent memory
allocation during CREATE_BUFS and REQBUFS ioctl calls.
= CREATE_BUFS
struct v4l2_create_buffers has seven 4-byte reserved areas,
so reserved[0] is renamed to ->flags. The struct, thus, now
has six reserved 4-byte regions.
= CREATE_BUFS32
struct v4l2_create_buffers32 has seven 4-byte reserved areas,
so reserved[0] is renamed to ->flags. The struct, thus, now
has six reserved 4-byte regions.
= REQBUFS
We use one bit of a ->reserved[1] member of struct v4l2_requestbuffers,
which is now renamed to ->flags. Unlike v4l2_create_buffers, struct
v4l2_requestbuffers does not have enough reserved room. Therefore for
backward compatibility ->reserved and ->flags were put into anonymous
union.
Signed-off-by: Sergey Senozhatsky
---
.../media/v4l/vidioc-create-bufs.rst | 7 ++-
.../media/v4l/vidioc-reqbufs.rst | 11 +--
.../media/common/videobuf2/videobuf2-core.c | 6 ++
.../media/common/videobuf2/videobuf2-v4l2.c | 19 ---
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 -
drivers/media/v4l2-core/v4l2-ioctl.c | 5 +
include/uapi/linux/videodev2.h| 11 +--
7 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
b/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
index b06e5b528e11..132c8b612a94 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
@@ -113,7 +113,12 @@ than the number requested.
``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type.
* - __u32
- - ``reserved``\ [7]
+ - ``flags``
+ - Specifies additional buffer management attributes.
+ See :ref:`memory-flags`.
+
+* - __u32
+ - ``reserved``\ [6]
- A place holder for future extensions. Drivers and applications
must set the array to zero.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index 950e7ec1aac5..80ea48acea84 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
@@ -104,10 +104,17 @@ aborting or finishing any DMA in progress, an implicit
``V4L2_MEMORY_MMAP`` and ``type`` set to the buffer type. This will
free any previously allocated buffers, so this is typically something
that will be done at the start of the application.
+* - union {
+ - (anonymous)
+* - __u32
+ - ``flags``
+ - Specifies additional buffer management attributes.
+ See :ref:`memory-flags`.
* - __u32
- ``reserved``\ [1]
- - A place holder for future extensions. Drivers and applications
- must set the array to zero.
+ - Kept for backwards compatibility. Use ``flags`` instead.
+* - }
+ -
.. tabularcolumns:: |p{6.1cm}|p{2.2cm}|p{8.7cm}|
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 7040b7f47133..5906a48e7757 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -768,6 +768,9 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
memory,
unsigned int i;
int ret;
+ if (flags & V4L2_FLAG_MEMORY_NON_COHERENT)
+ coherent_mem = false;
+
if (q->streaming) {
dprintk(q, 1, "streaming active\n");
return -EBUSY;
@@ -911,6 +914,9 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum
vb2_memory memory,
bool coherent_mem = true;
int ret;
+ if (flags & V4L2_FLAG_MEMORY_NON_COHERENT)
+ coherent_mem = false;
+
if (q->num_buffers == VB2_MAX_FRAME) {
dprintk(q, 1, "maximum number of buffers already allocated\n");
return -ENOBUFS;
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 1166d5a9291a..f6a8dcc1b5c6 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -692,12 +692,22 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
#endif
}
+static void validate_coherency_flags(struct vb2_queue *q,
+int memory,
+unsigned int *flags)
+{
+ if (!q->allow_cache_hints || memory != V4L2_MEMORY_MMAP)
+ *flags &= ~V4L2_FLAG_MEMORY_NON_COHERENT;
+}
+
int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
{
int ret = vb2_verify_memory_type(q, req->memory, req->type);
fill_buf_caps(q, >capabilities);
-
[PATCH 6/8] videobuf2: add queue memory coherency parameter
Preparations for future V4L2_FLAG_MEMORY_NON_COHERENT support.
Extend vb2_core_reqbufs() parameters list to accept requests'
->flags, which will be used for memory coherency configuration.
An attempt to allocate a buffer with coherency requirements
which don't match queue's consistency model will fail.
Signed-off-by: Sergey Senozhatsky
---
.../media/common/videobuf2/videobuf2-core.c | 40 ---
.../media/common/videobuf2/videobuf2-v4l2.c | 5 ++-
drivers/media/dvb-core/dvb_vb2.c | 2 +-
include/media/videobuf2-core.h| 8 +++-
4 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 55af63d54f23..7040b7f47133 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -738,11 +738,33 @@ int vb2_verify_memory_type(struct vb2_queue *q,
}
EXPORT_SYMBOL(vb2_verify_memory_type);
+static void set_queue_coherency(struct vb2_queue *q, bool coherent_mem)
+{
+ q->non_coherent_mem = 0;
+
+ if (!vb2_queue_allows_cache_hints(q))
+ return;
+ if (!coherent_mem)
+ q->non_coherent_mem = 1;
+}
+
+static bool verify_coherency_flags(struct vb2_queue *q, bool coherent_mem)
+{
+ bool queue_is_coherent = !q->non_coherent_mem;
+
+ if (coherent_mem != queue_is_coherent) {
+ dprintk(q, 1, "memory coherency model mismatch\n");
+ return false;
+ }
+ return true;
+}
+
int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
-unsigned int *count)
+unsigned int flags, unsigned int *count)
{
unsigned int num_buffers, allocated_buffers, num_planes = 0;
unsigned plane_sizes[VB2_MAX_PLANES] = { };
+ bool coherent_mem = true;
unsigned int i;
int ret;
@@ -757,7 +779,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
memory,
}
if (*count == 0 || q->num_buffers != 0 ||
- (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
+ (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) ||
+ !verify_coherency_flags(q, coherent_mem)) {
/*
* We already have buffers allocated, so first check if they
* are not in use and can be freed.
@@ -794,6 +817,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
memory,
num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
q->memory = memory;
+ set_queue_coherency(q, coherent_mem);
/*
* Ask the driver how many buffers and planes per buffer it requires.
@@ -878,12 +902,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
memory,
EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
-unsigned int *count,
+unsigned int flags, unsigned int *count,
unsigned int requested_planes,
const unsigned int requested_sizes[])
{
unsigned int num_planes = 0, num_buffers, allocated_buffers;
unsigned plane_sizes[VB2_MAX_PLANES] = { };
+ bool coherent_mem = true;
int ret;
if (q->num_buffers == VB2_MAX_FRAME) {
@@ -899,11 +924,14 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum
vb2_memory memory,
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
q->memory = memory;
q->waiting_for_buffers = !q->is_output;
+ set_queue_coherency(q, coherent_mem);
} else {
if (q->memory != memory) {
dprintk(q, 1, "memory model mismatch\n");
return -EINVAL;
}
+ if (!verify_coherency_flags(q, coherent_mem))
+ return -EINVAL;
}
num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
@@ -2576,7 +2604,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int
read)
fileio->memory = VB2_MEMORY_MMAP;
fileio->type = q->type;
q->fileio = fileio;
- ret = vb2_core_reqbufs(q, fileio->memory, >count);
+ ret = vb2_core_reqbufs(q, fileio->memory, 0, >count);
if (ret)
goto err_kfree;
@@ -2633,7 +2661,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int
read)
err_reqbufs:
fileio->count = 0;
- vb2_core_reqbufs(q, fileio->memory, >count);
+ vb2_core_reqbufs(q, fileio->memory, 0, >count);
err_kfree:
q->fileio = NULL;
@@ -2653,7 +2681,7 @@ static int __vb2_cleanup_filei
[PATCH 4/8] videobuf2: move cache_hints handling to allocators
This moves cache hints handling from videobuf2 core down to allocators level, because allocators do the sync/flush caches eventually and may take better decisions. Besides, allocators already decide whether cache sync/flush should be done or can be skipped. This patch moves the scattered buffer cache sync logic to one common place. Signed-off-by: Sergey Senozhatsky --- drivers/media/common/videobuf2/videobuf2-core.c | 6 -- drivers/media/common/videobuf2/videobuf2-dma-contig.c | 6 ++ drivers/media/common/videobuf2/videobuf2-dma-sg.c | 6 ++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 76210c006958..55af63d54f23 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -328,9 +328,6 @@ static void __vb2_buf_mem_prepare(struct vb2_buffer *vb) return; vb->synced = 1; - if (vb->skip_cache_sync_on_prepare) - return; - for (plane = 0; plane < vb->num_planes; ++plane) call_void_memop(vb, prepare, vb->planes[plane].mem_priv); } @@ -347,9 +344,6 @@ static void __vb2_buf_mem_finish(struct vb2_buffer *vb) return; vb->synced = 0; - if (vb->skip_cache_sync_on_finish) - return; - for (plane = 0; plane < vb->num_planes; ++plane) call_void_memop(vb, finish, vb->planes[plane].mem_priv); } diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index 019c3843dc6d..1e218bc440c6 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -101,6 +101,9 @@ static void vb2_dc_prepare(void *buf_priv) struct vb2_dc_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + if (buf->vb->skip_cache_sync_on_prepare) + return; + if (!sgt) return; @@ -112,6 +115,9 @@ static void vb2_dc_finish(void *buf_priv) struct vb2_dc_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + if (buf->vb->skip_cache_sync_on_finish) + return; + if (!sgt) return; diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 71094cb5c5d7..cb587c5a345b 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -204,6 +204,9 @@ static void vb2_dma_sg_prepare(void *buf_priv) struct vb2_dma_sg_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + if (buf->vb->skip_cache_sync_on_prepare) + return; + dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); } @@ -212,6 +215,9 @@ static void vb2_dma_sg_finish(void *buf_priv) struct vb2_dma_sg_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + if (buf->vb->skip_cache_sync_on_finish) + return; + dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); } -- 2.30.1.766.gb4fecdf3b7-goog
[PATCH 5/8] videobuf2: add V4L2_FLAG_MEMORY_NON_COHERENT flag
By setting or clearing V4L2_FLAG_MEMORY_NON_COHERENT flag
user-space should be able to hint vb2 that either a non-coherent
(if supported) or coherent memory should be used for the buffer
allocation.
The patch set also adds a corresponding capability flag:
fill_buf_caps() reports V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
when queue supports user-space cache management hints.
Signed-off-by: Sergey Senozhatsky
---
.../userspace-api/media/v4l/buffer.rst| 40 ++-
.../media/v4l/vidioc-reqbufs.rst | 5 ++-
include/uapi/linux/videodev2.h| 2 +
3 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/buffer.rst
b/Documentation/userspace-api/media/v4l/buffer.rst
index 1b0fdc160533..a39852d6174f 100644
--- a/Documentation/userspace-api/media/v4l/buffer.rst
+++ b/Documentation/userspace-api/media/v4l/buffer.rst
@@ -676,8 +676,6 @@ Buffer Flags
\normalsize
-.. _memory-flags:
-
enum v4l2_memory
@@ -701,6 +699,44 @@ enum v4l2_memory
- 4
- The buffer is used for :ref:`DMA shared buffer ` I/O.
+.. _memory-flags:
+
+Memory Consistency Flags
+
+
+.. raw:: latex
+
+\small
+
+.. tabularcolumns:: |p{7.0cm}|p{2.1cm}|p{8.4cm}|
+
+.. cssclass:: longtable
+
+.. flat-table::
+:header-rows: 0
+:stub-columns: 0
+:widths: 3 1 4
+
+* .. _`V4L2-FLAG-MEMORY-NON-COHERENT`:
+
+ - ``V4L2_FLAG_MEMORY_NON_COHERENT``
+ - 0x0001
+ - A buffer is allocated either in coherent (it will be automatically
+ coherent between the CPU and the bus) or non-coherent memory. The
+ latter can provide performance gains, for instance the CPU cache
+ sync/flush operations can be avoided if the buffer is accessed by the
+ corresponding device only and the CPU does not read/write to/from that
+ buffer. However, this requires extra care from the driver -- it must
+ guarantee memory consistency by issuing a cache flush/sync when
+ consistency is needed. If this flag is set V4L2 will attempt to
+ allocate the buffer in non-coherent memory. The flag takes effect
+ only if the buffer is used for :ref:`memory mapping ` I/O and the
+ queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
+ ` capability.
+
+.. raw:: latex
+
+\normalsize
Timecodes
=
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index c1c88e00b106..950e7ec1aac5 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
@@ -154,8 +154,9 @@ aborting or finishing any DMA in progress, an implicit
- This capability is set by the driver to indicate that the queue
supports
cache and memory management hints. However, it's only valid when the
queue is used for :ref:`memory mapping ` streaming I/O. See
-:ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
` and
-:ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN `.
+:ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
`,
+:ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN ` and
+:ref:`V4L2_FLAG_MEMORY_NON_COHERENT `.
Return Value
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 79dbde3bcf8d..b1d4171fe50b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -954,6 +954,8 @@ struct v4l2_requestbuffers {
__u32 reserved[1];
};
+#define V4L2_FLAG_MEMORY_NON_COHERENT (1 << 0)
+
/* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
#define V4L2_BUF_CAP_SUPPORTS_MMAP (1 << 0)
#define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
--
2.30.1.766.gb4fecdf3b7-goog
[PATCH 3/8] videobuf2: split buffer cache_hints initialisation
V4L2 is not the perfect place to manage vb2 buffer cache hints.
It works for V4L2 users, but there are backends that use vb2 core
and don't use V4L2. Factor buffer cache hints init and call it
when we allocate vb2 buffer.
Signed-off-by: Sergey Senozhatsky
---
.../media/common/videobuf2/videobuf2-core.c | 22 +++
.../media/common/videobuf2/videobuf2-v4l2.c | 18 ---
2 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 23e41fec9880..76210c006958 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -382,6 +382,27 @@ static void __setup_offsets(struct vb2_buffer *vb)
}
}
+static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+ /*
+* DMA exporter should take care of cache syncs, so we can avoid
+* explicit ->prepare()/->finish() syncs. For other ->memory types
+* we always need ->prepare() or/and ->finish() cache sync.
+*/
+ if (q->memory == VB2_MEMORY_DMABUF) {
+ vb->skip_cache_sync_on_finish = 1;
+ vb->skip_cache_sync_on_prepare = 1;
+ return;
+ }
+
+ /*
+* ->finish() cache sync can be avoided when queue direction is
+* TO_DEVICE.
+*/
+ if (q->dma_dir == DMA_TO_DEVICE)
+ vb->skip_cache_sync_on_finish = 1;
+}
+
/*
* __vb2_queue_alloc() - allocate videobuf buffer structures and (for MMAP
type)
* video buffer memory for all buffers/planes on the queue and initializes the
@@ -415,6 +436,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum
vb2_memory memory,
vb->index = q->num_buffers + buffer;
vb->type = q->type;
vb->memory = memory;
+ init_buffer_cache_hints(q, vb);
for (plane = 0; plane < num_planes; ++plane) {
vb->planes[plane].length = plane_sizes[plane];
vb->planes[plane].min_length = plane_sizes[plane];
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index db93678860bd..a02f365bbe60 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -345,17 +345,6 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
struct vb2_buffer *vb,
struct v4l2_buffer *b)
{
- /*
-* DMA exporter should take care of cache syncs, so we can avoid
-* explicit ->prepare()/->finish() syncs. For other ->memory types
-* we always need ->prepare() or/and ->finish() cache sync.
-*/
- if (q->memory == VB2_MEMORY_DMABUF) {
- vb->skip_cache_sync_on_finish = 1;
- vb->skip_cache_sync_on_prepare = 1;
- return;
- }
-
if (!vb2_queue_allows_cache_hints(q)) {
/*
* Clear buffer cache flags if queue does not support user
@@ -367,13 +356,6 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
return;
}
- /*
-* ->finish() cache sync can be avoided when queue direction is
-* TO_DEVICE.
-*/
- if (q->dma_dir == DMA_TO_DEVICE)
- vb->skip_cache_sync_on_finish = 1;
-
if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE)
vb->skip_cache_sync_on_finish = 1;
--
2.30.1.766.gb4fecdf3b7-goog
[PATCH 2/8] videobuf2: inverse buffer cache_hints flags
It would be less error prone if the default cache hints value
(we kzalloc() structs, so it's zeroed out by default) would be
to "always sync/flush" caches. Inverse and rename cache hints
flags.
Signed-off-by: Sergey Senozhatsky
---
.../media/common/videobuf2/videobuf2-core.c | 31 ++-
.../media/common/videobuf2/videobuf2-v4l2.c | 17 +++---
include/media/videobuf2-core.h| 12 +++
3 files changed, 21 insertions(+), 39 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 9a5cc3e63439..23e41fec9880 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -327,12 +327,12 @@ static void __vb2_buf_mem_prepare(struct vb2_buffer *vb)
if (vb->synced)
return;
- if (vb->need_cache_sync_on_prepare) {
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, prepare,
- vb->planes[plane].mem_priv);
- }
vb->synced = 1;
+ if (vb->skip_cache_sync_on_prepare)
+ return;
+
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
}
/*
@@ -346,12 +346,12 @@ static void __vb2_buf_mem_finish(struct vb2_buffer *vb)
if (!vb->synced)
return;
- if (vb->need_cache_sync_on_finish) {
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, finish,
- vb->planes[plane].mem_priv);
- }
vb->synced = 0;
+ if (vb->skip_cache_sync_on_finish)
+ return;
+
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ call_void_memop(vb, finish, vb->planes[plane].mem_priv);
}
/*
@@ -415,17 +415,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum
vb2_memory memory,
vb->index = q->num_buffers + buffer;
vb->type = q->type;
vb->memory = memory;
- /*
-* We need to set these flags here so that the videobuf2 core
-* will call ->prepare()/->finish() cache sync/flush on vb2
-* buffers when appropriate. However, we can avoid explicit
-* ->prepare() and ->finish() cache sync for DMABUF buffers,
-* because DMA exporter takes care of it.
-*/
- if (q->memory != VB2_MEMORY_DMABUF) {
- vb->need_cache_sync_on_prepare = 1;
- vb->need_cache_sync_on_finish = 1;
- }
for (plane = 0; plane < num_planes; ++plane) {
vb->planes[plane].length = plane_sizes[plane];
vb->planes[plane].min_length = plane_sizes[plane];
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 7e96f67c60ba..db93678860bd 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -351,18 +351,11 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
* we always need ->prepare() or/and ->finish() cache sync.
*/
if (q->memory == VB2_MEMORY_DMABUF) {
- vb->need_cache_sync_on_finish = 0;
- vb->need_cache_sync_on_prepare = 0;
+ vb->skip_cache_sync_on_finish = 1;
+ vb->skip_cache_sync_on_prepare = 1;
return;
}
- /*
-* Cache sync/invalidation flags are set by default in order to
-* preserve existing behaviour for old apps/drivers.
-*/
- vb->need_cache_sync_on_prepare = 1;
- vb->need_cache_sync_on_finish = 1;
-
if (!vb2_queue_allows_cache_hints(q)) {
/*
* Clear buffer cache flags if queue does not support user
@@ -379,13 +372,13 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
* TO_DEVICE.
*/
if (q->dma_dir == DMA_TO_DEVICE)
- vb->need_cache_sync_on_finish = 0;
+ vb->skip_cache_sync_on_finish = 1;
if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE)
- vb->need_cache_sync_on_finish = 0;
+ vb->skip_cache_sync_on_finish = 1;
if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN)
- vb->need_cache_sync_on_prepare = 0;
+ vb->skip_cache_sync_on_prepare = 1;
}
static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device
*mdev,
diff --git a/include/media/videobuf2-core.h b/include
[PATCH 1/8] videobuf2: rework vb2_mem_ops API
With new DMA API we need an extension of videobuf2 API. Previously,
videobuf2 core would set non-coherent DMA bit in vb2 queue dma_attr
(if user-space would pass a corresponding memory hint); vb2 core
then would pass the vb2 queue dma_attrs to the vb2 allocators.
vb2 allocator would use queue's dma_attr and DMA API would allocate
either coherent or non-coherent memory.
But we cannot do this anymore, since there is no corresponding DMA
attr flag and, hence, there is no way for the allocator to become
aware of what type of allocation user-space has requested. So we
need to pass more context from videobuf2 core to the allocators.
Fix this by changing call_ptr_memop() macro to pass vb2 pointer to
corresponding op callbacks.
Signed-off-by: Sergey Senozhatsky
---
.../media/common/videobuf2/videobuf2-core.c | 42 +++
.../common/videobuf2/videobuf2-dma-contig.c | 36 +---
.../media/common/videobuf2/videobuf2-dma-sg.c | 33 ---
.../common/videobuf2/videobuf2-vmalloc.c | 30 ++---
include/media/videobuf2-core.h| 37
5 files changed, 98 insertions(+), 80 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 02281d13505f..9a5cc3e63439 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -68,13 +68,13 @@ module_param(debug, int, 0644);
err;\
})
-#define call_ptr_memop(vb, op, args...)
\
+#define call_ptr_memop(op, vb, args...)
\
({ \
struct vb2_queue *_q = (vb)->vb2_queue; \
void *ptr; \
\
log_memop(vb, op); \
- ptr = _q->mem_ops->op ? _q->mem_ops->op(args) : NULL; \
+ ptr = _q->mem_ops->op ? _q->mem_ops->op(vb, args) : NULL; \
if (!IS_ERR_OR_NULL(ptr)) \
(vb)->cnt_mem_ ## op++; \
ptr;\
@@ -144,9 +144,9 @@ module_param(debug, int, 0644);
((vb)->vb2_queue->mem_ops->op ? \
(vb)->vb2_queue->mem_ops->op(args) : 0)
-#define call_ptr_memop(vb, op, args...)
\
+#define call_ptr_memop(op, vb, args...)
\
((vb)->vb2_queue->mem_ops->op ? \
- (vb)->vb2_queue->mem_ops->op(args) : NULL)
+ (vb)->vb2_queue->mem_ops->op(vb, args) : NULL)
#define call_void_memop(vb, op, args...) \
do {\
@@ -230,9 +230,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
if (size < vb->planes[plane].length)
goto free;
- mem_priv = call_ptr_memop(vb, alloc,
- q->alloc_devs[plane] ? : q->dev,
- q->dma_attrs, size, q->dma_dir, q->gfp_flags);
+ mem_priv = call_ptr_memop(alloc,
+ vb,
+ q->alloc_devs[plane] ? : q->dev,
+ size);
if (IS_ERR_OR_NULL(mem_priv)) {
if (mem_priv)
ret = PTR_ERR(mem_priv);
@@ -975,7 +976,7 @@ void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int
plane_no)
if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
- return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
+ return call_ptr_memop(vaddr, vb, vb->planes[plane_no].mem_priv);
}
EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
@@ -985,7 +986,7 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int
plane_no)
if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
- return call_ptr_memop(vb, cookie, vb->planes[plane_no].mem_priv);
+ return call_ptr_memop(cookie, vb, vb->planes[plane_no].mem_priv);
}
EXPORT_SYMBOL_GPL(vb2_plane_cookie);
@@ -1125,10 +1126,11 @@ static int __prepare_userptr(struct vb2_buffer *vb)
vb->planes[plane].data_offset = 0;
/* Acquire each plane's memory */
- mem_priv =
[PATCH 0/8] videobuf2: support new noncontiguous DMA API
Hello, RFC The series adds support for new noncontiguous DMA API [0] and adds V4L2_FLAG_MEMORY_NON_COHERENT UAPI. This is similar to previous V4L2_FLAG_MEMORY_NON_CONSISTENT (which was renamed), but the patch set goes a bit further this time and also does some videobuf2 API refactroings along the way. A corresponding v4l2-compliance patch will be posted shortly. [0] https://lore.kernel.org/lkml/20210301085236.947011-2-...@lst.de/ Sergey Senozhatsky (8): videobuf2: rework vb2_mem_ops API videobuf2: inverse buffer cache_hints flags videobuf2: split buffer cache_hints initialisation videobuf2: move cache_hints handling to allocators videobuf2: add V4L2_FLAG_MEMORY_NON_COHERENT flag videobuf2: add queue memory coherency parameter videobuf2: handle V4L2_FLAG_MEMORY_NON_COHERENT flag videobuf2: handle non-contiguous DMA allocations .../userspace-api/media/v4l/buffer.rst| 40 +++- .../media/v4l/vidioc-create-bufs.rst | 7 +- .../media/v4l/vidioc-reqbufs.rst | 16 +- .../media/common/videobuf2/videobuf2-core.c | 135 +- .../common/videobuf2/videobuf2-dma-contig.c | 175 ++ .../media/common/videobuf2/videobuf2-dma-sg.c | 39 ++-- .../media/common/videobuf2/videobuf2-v4l2.c | 47 ++--- .../common/videobuf2/videobuf2-vmalloc.c | 30 +-- drivers/media/dvb-core/dvb_vb2.c | 2 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 +- drivers/media/v4l2-core/v4l2-ioctl.c | 5 +- include/media/videobuf2-core.h| 57 +++--- include/uapi/linux/videodev2.h| 13 +- 13 files changed, 396 insertions(+), 179 deletions(-) -- 2.30.1.766.gb4fecdf3b7-goog
Re: [PATCH 6/7] dma-iommu: implement ->alloc_noncontiguous
On (21/03/01 08:21), Christoph Hellwig wrote: > On Mon, Mar 01, 2021 at 04:17:42PM +0900, Sergey Senozhatsky wrote: > > > > Do you think we could add the attrs parameter to the > > > > dma_alloc_noncontiguous() API? > > > > > > Yes, we could probably do that. > > > > I can cook a patch, unless somebody is already looking into it. > > I plan to resend the whole series with the comments very soon. Oh, OK. I thought the series was in linux-next already so a single patch would do. -ss
Re: [PATCH 6/7] dma-iommu: implement ->alloc_noncontiguous
On (21/02/16 09:49), Christoph Hellwig wrote: > > When working on the videobuf2 integration with Sergey I noticed that > > we always pass 0 as DMA attrs here, which removes the ability for > > drivers to use DMA_ATTR_ALLOC_SINGLE_PAGES. > > > > It's quite important from a system stability point of view, because by > > default the iommu_dma allocator would prefer big order allocations for > > TLB locality reasons. For many devices, though, it doesn't really > > affect the performance, because of random access patterns, so single > > pages are good enough and reduce the risk of allocation failures or > > latency due to fragmentation. > > > > Do you think we could add the attrs parameter to the > > dma_alloc_noncontiguous() API? > > Yes, we could probably do that. I can cook a patch, unless somebody is already looking into it. -ss
Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
On (21/02/17 13:15), Enrico Weigelt, metux IT consult wrote:
> Under rare circumstances it may happen that a device node's name is NULL
> (most likely kernel bug in some other place). In such situations anything
> but helpful, if the debug printout crashes, and nobody knows what actually
> happened here.
>
> Therefore protect it by an explicit NULL check and print out an extra
> warning.
>
> Signed-off-by: Enrico Weigelt, metux IT consult
> ---
> lib/vsprintf.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..050a60b88073 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2013,6 +2013,10 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
> break;
> case 'n': /* name */
> p = fwnode_get_name(of_fwnode_handle(dn));
> + if (!p) {
> + pr_warn("device_node without name. Kernel bug
> ?\n");
> + p = "";
> + }
> precision = str_spec.precision;
> str_spec.precision = strchrnul(p, '@') - p;
> buf = string(buf, end, p, str_spec);
What about other fwnode_get_name() calls in vsprintf?
-ss
Re: [PATCH v2] printk: avoid prb_first_valid_seq() where possible
On (21/02/12 10:47), Petr Mladek wrote:
> > Fixes: 896fbe20b4e2333fb55 ("printk: use the lockless ringbuffer")
> > Reported-by: kernel test robot
> > Reported-by: J. Avila
> > Signed-off-by: John Ogness
>
> Reviewed-by: Petr Mladek
>
> I am going to push the patch later today. I would prefer to do it
> later and give a chance others to react. But the merge window
> will likely start the following week and Sergey was fine
> with this approach.
Yup, ACK.
-ss
Re: [PATCH v3] printk: fix deadlock when kernel panic
On (21/02/10 11:48), Muchun Song wrote:
> printk_safe_flush_on_panic() caused the following deadlock on our
> server:
>
> CPU0: CPU1:
> panic rcu_dump_cpu_stacks
> kdump_nmi_shootdown_cpus nmi_trigger_cpumask_backtrace
> register_nmi_handler(crash_nmi_callback) printk_safe_flush
> __printk_safe_flush
>
> raw_spin_lock_irqsave(_lock)
> // send NMI to other processors
> apic_send_IPI_allbutself(NMI_VECTOR)
> // NMI interrupt,
> dead loop
> crash_nmi_callback
> printk_safe_flush_on_panic
> printk_safe_flush
> __printk_safe_flush
> // deadlock
> raw_spin_lock_irqsave(_lock)
>
> DEADLOCK: read_lock is taken on CPU1 and will never get released.
>
> It happens when panic() stops a CPU by NMI while it has been in
> the middle of printk_safe_flush().
>
> Handle the lock the same way as logbuf_lock. The printk_safe buffers
> are flushed only when both locks can be safely taken. It can avoid
> the deadlock _in this particular case_ at expense of losing contents
> of printk_safe buffers.
>
> Note: It would actually be safe to re-init the locks when all CPUs were
> stopped by NMI. But it would require passing this information
> from arch-specific code. It is not worth the complexity.
> Especially because logbuf_lock and printk_safe buffers have been
> obsoleted by the lockless ring buffer.
>
> Fixes: cf9b1106c81c ("printk/nmi: flush NMI messages on the system panic")
> Signed-off-by: Muchun Song
> Reviewed-by: Petr Mladek
> Cc:
Acked-by: Sergey Senozhatsky
-ss
Re: [PATCH v2] printk: fix deadlock when kernel panic
On (21/02/09 10:19), Petr Mladek wrote: > On Sat 2021-02-06 13:41:24, Muchun Song wrote: [..] > What about the following commit message? It uses imperative language > and explains that the patch just prevents the deadlock. It removes > some details. The diagram is better than many words. > > > printk_safe_flush_on_panic() caused the following deadlock on our server: > > CPU0: CPU1: > panic rcu_dump_cpu_stacks > kdump_nmi_shootdown_cpus nmi_trigger_cpumask_backtrace > register_nmi_handler(crash_nmi_callback) printk_safe_flush > __printk_safe_flush > > raw_spin_lock_irqsave(_lock) > // send NMI to other processors > apic_send_IPI_allbutself(NMI_VECTOR) > // NMI interrupt, > dead loop > crash_nmi_callback > printk_safe_flush_on_panic > printk_safe_flush > __printk_safe_flush > // deadlock > raw_spin_lock_irqsave(_lock) [..] I would also add to the commit message that it avoids the deadlock _in this particular case_ at expense of losing contents of printk_safe buffers. This looks important enough to be mentioned. -ss
Re: [External] Re: [PATCH v2] printk: fix deadlock when kernel panic
On (21/02/09 09:39), Petr Mladek wrote: > > > So then this never re-inits the safe_read_lock? > > Yes, but it will also not cause the deadlock. Right. > I prefer this approach. It is straightforward because it handles > read_lock the same way as logbuf_lock. I'm fine with that approach, but this needs to be in the commit message. Something like "lose printk_safe message when we think we will deadlock on printk_safe flush". -ss
Re: [PATCH v2 2/2] thermal: Move therm_throt there from x86/mce
Hi, Seems that the patch triggers some WARNs on my laptop. For every CPU: [0.003751] WARNING: CPU: 4 PID: 0 at arch/x86/kernel/irq.c:390 thermal_set_handler+0x12/0x25 [0.003751] Modules linked in: [0.003751] CPU: 4 PID: 0 Comm: swapper/4 Tainted: GW 5.11.0-rc6-next-20210208-3-g3ba4c4f662ad-dirty #1928 [0.003751] RIP: 0010:thermal_set_handler+0x12/0x25 [0.003751] RSP: :b5f0c00c7ed8 EFLAGS: 00010097 [0.003751] RAX: 0003 RBX: RCX: 01b2 [0.003751] RDX: RSI: 0003 RDI: 98410d00 [0.003751] RBP: 9e3c5fb11460 R08: R09: 0003007f [0.003751] R10: 9e3c5fb11480 R11: R12: 0428 [0.003751] R13: R14: R15: [0.003751] FS: () GS:9e3c5fb0() knlGS: [0.003751] CS: 0010 DS: ES: CR0: 80050033 [0.003751] CR2: CR3: 00039ee0a001 CR4: 001706a0 [0.003751] Call Trace: [0.003751] intel_init_thermal+0x16d/0x1c7 [0.003751] identify_cpu+0x249/0x329 [0.003751] identify_secondary_cpu+0x15/0x8c [0.003751] smp_store_cpu_info+0x3f/0x48 [0.003751] start_secondary+0x42/0xfd [0.003751] secondary_startup_64_no_verify+0xb0/0xbb -ss
Re: [PATCH] printk: avoid prb_first_valid_seq() where possible
On (21/02/05 15:23), John Ogness wrote:
> If message sizes average larger than expected (more than 32
> characters), the data_ring will wrap before the desc_ring. Once the
> data_ring wraps, it will start invalidating descriptors. These
> invalid descriptors hang around until they are eventually recycled
> when the desc_ring wraps. Readers do not care about invalid
> descriptors, but they still need to iterate past them. If the
> average message size is much larger than 32 characters, then there
> will be many invalid descriptors preceding the valid descriptors.
>
> The function prb_first_valid_seq() always begins at the oldest
> descriptor and searches for the first valid descriptor. This can
> be rather expensive for the above scenario. And, in fact, because
> of its heavy usage in /dev/kmsg, there have been reports of long
> delays and even RCU stalls.
>
> For code that does not need to search from the oldest record,
> replace prb_first_valid_seq() usage with prb_read_valid_*()
> functions, which provide a start sequence number to search from.
>
> Fixes: 896fbe20b4e2333fb55 ("printk: use the lockless ringbuffer")
> Reported-by: kernel test robot
> Reported-by: J. Avila
> Signed-off-by: John Ogness
Acked-by: Sergey Senozhatsky
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5a95c688621f..035aae771ea1 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -735,9 +735,9 @@ static ssize_t devkmsg_read(struct file *file, char
> __user *buf,
> logbuf_lock_irq();
> }
>
> - if (user->seq < prb_first_valid_seq(prb)) {
> + if (r->info->seq != user->seq) {
> /* our last seen message is gone, return error and reset */
> - user->seq = prb_first_valid_seq(prb);
Yeah, I can see how this pattern can be expensive, it would have been
less obvious and harder to spot had it been something like this
valid_seq = prb_first_valid_seq(prb);
if (user->seq < valid_seq) {
user->seq = valid_seq;
...
}
Great analysis, John.
I wonder if Intel test robot measures all test execution times; I do
recall "we saw N% performance improvement after patch P" emails, but
not sure if all of the tests are being tracked.
-ss
Re: [External] Re: [PATCH v2] printk: fix deadlock when kernel panic
On (21/02/08 16:49), Muchun Song wrote: > On Mon, Feb 8, 2021 at 2:38 PM Sergey Senozhatsky > wrote: > > > > On (21/02/06 13:41), Muchun Song wrote: > > > We found a deadlock bug on our server when the kernel panic. It can be > > > described in the following diagram. > > > > > > CPU0: CPU1: > > > panic rcu_dump_cpu_stacks > > > kdump_nmi_shootdown_cpus > > > nmi_trigger_cpumask_backtrace > > > register_nmi_handler(crash_nmi_callback) printk_safe_flush > > > __printk_safe_flush > > > > > > raw_spin_lock_irqsave(_lock) > > > // send NMI to other processors > > > apic_send_IPI_allbutself(NMI_VECTOR) > > > // NMI interrupt, > > > dead loop > > > crash_nmi_callback > > > > At what point does this decrement num_online_cpus()? Any chance that > > panic CPU can apic_send_IPI_allbutself() and printk_safe_flush_on_panic() > > before num_online_cpus() becomes 1? > > I took a closer look at the code. IIUC, It seems that there is no point > which decreases num_online_cpus. So then this never re-inits the safe_read_lock? if (num_online_cpus() > 1) return; debug_locks_off(); raw_spin_lock_init(_read_lock); -ss
Re: [PATCH] printk: avoid prb_first_valid_seq() where possible
On (21/02/05 15:23), John Ogness wrote:
> If message sizes average larger than expected (more than 32
> characters), the data_ring will wrap before the desc_ring. Once the
> data_ring wraps, it will start invalidating descriptors. These
> invalid descriptors hang around until they are eventually recycled
> when the desc_ring wraps. Readers do not care about invalid
> descriptors, but they still need to iterate past them. If the
> average message size is much larger than 32 characters, then there
> will be many invalid descriptors preceding the valid descriptors.
>
> The function prb_first_valid_seq() always begins at the oldest
> descriptor and searches for the first valid descriptor. This can
> be rather expensive for the above scenario. And, in fact, because
> of its heavy usage in /dev/kmsg, there have been reports of long
> delays and even RCU stalls.
>
> For code that does not need to search from the oldest record,
> replace prb_first_valid_seq() usage with prb_read_valid_*()
> functions, which provide a start sequence number to search from.
>
> Fixes: 896fbe20b4e2333fb55 ("printk: use the lockless ringbuffer")
> Reported-by: kernel test robot
Can we please also ask the kernel test robot to test this patch?
-ss
Re: [PATCH v2] printk: fix deadlock when kernel panic
On (21/02/06 13:41), Muchun Song wrote: > We found a deadlock bug on our server when the kernel panic. It can be > described in the following diagram. > > CPU0: CPU1: > panic rcu_dump_cpu_stacks > kdump_nmi_shootdown_cpus nmi_trigger_cpumask_backtrace > register_nmi_handler(crash_nmi_callback) printk_safe_flush > __printk_safe_flush > > raw_spin_lock_irqsave(_lock) > // send NMI to other processors > apic_send_IPI_allbutself(NMI_VECTOR) > // NMI interrupt, > dead loop > crash_nmi_callback At what point does this decrement num_online_cpus()? Any chance that panic CPU can apic_send_IPI_allbutself() and printk_safe_flush_on_panic() before num_online_cpus() becomes 1? > printk_safe_flush_on_panic > printk_safe_flush > __printk_safe_flush > // deadlock > raw_spin_lock_irqsave(_lock) -ss
[PATCHv2 2/3] media: uvcvideo: add ROI auto controls
From: Sergey Senozhatsky
This patch adds support for Region of Interest bmAutoControls.
ROI control is a compound data type:
Control Selector CT_REGION_OF_INTEREST_CONTROL
Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
wLength 10
Offset FieldSize
0wROI_Top 2
2wROI_Left2
4wROI_Bottom 2
6wROI_Right 2
8bmAutoControls 2 (Bitmap)
uvc_control_mapping, however, can handle only s32 data type at the
moment: ->get() returns s32 value, ->set() accepts s32 value; while
v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
Hence ROI control handling is split into two patches:
a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
separately, by the means of selection API.
Signed-off-by: Sergey Senozhatsky
---
.../media/v4l/ext-ctrls-camera.rst| 25 +++
drivers/media/usb/uvc/uvc_ctrl.c | 19 ++
include/uapi/linux/usb/video.h| 1 +
include/uapi/linux/v4l2-controls.h| 9 +++
4 files changed, 54 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index c05a2d2c675d..1593c999c8e2 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -653,6 +653,31 @@ enum v4l2_scene_mode -
||
++
+``V4L2_CID_REGION_OF_INTEREST_AUTO (bitmask)``
+This determines which, if any, on board features should track to the
+Region of Interest.
+
+.. flat-table::
+:header-rows: 0
+:stub-columns: 0
+
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
+ - Auto Exposure.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
+ - Auto Iris.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
+ - Auto White Balance.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
+ - Auto Focus.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
+ - Auto Face Detect.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
+ - Auto Detect and Track.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
+ - Image Stabilization.
+* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
+ - Higher Quality.
+
.. [#f1]
This control may be changed to a menu control in the future, if more
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b3dde98499f4..5502fe540519 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -355,6 +355,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
.flags = UVC_CTRL_FLAG_GET_CUR
| UVC_CTRL_FLAG_AUTO_UPDATE,
},
+ {
+ .entity = UVC_GUID_UVC_CAMERA,
+ .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
+ .index = 21,
+ .size = 10,
+ .flags = UVC_CTRL_FLAG_SET_CUR | UVC_CTRL_FLAG_GET_CUR
+ | UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+ | UVC_CTRL_FLAG_GET_DEF
+ },
};
static const struct uvc_menu_info power_line_frequency_controls[] = {
@@ -753,6 +762,16 @@ static const struct uvc_control_mapping
uvc_ctrl_mappings[] = {
.v4l2_type = V4L2_CTRL_TYPE_BOOLEAN,
.data_type = UVC_CTRL_DATA_TYPE_BOOLEAN,
},
+ {
+ .id = V4L2_CID_REGION_OF_INTEREST_AUTO,
+ .name = "Region of Interest (auto)",
+ .entity = UVC_GUID_UVC_CAMERA,
+ .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
+ .size = 16,
+ .offset = 64,
+ .v4l2_type = V4L2_CTRL_TYPE_BITMASK,
+ .data_type = UVC_CTRL_DATA_TYPE_BITMASK,
+ },
};
/*
diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
index d854cb19c42c..c87624962896 100644
--- a/include/uapi/linux/usb/video.h
+++ b/include/uapi/linux/usb/video.h
@@ -104,6 +104,7 @@
#define UVC_CT_ROLL_ABSOLUTE_CONTROL 0x0f
#define UVC_CT_ROLL_RELATIVE_CONTROL 0x10
#define UVC_CT_PRIVACY_CONTROL 0x11
+#define UVC_CT_REGION_OF_INTEREST_CONTROL 0x14
/* A.9.5. Processing Unit Control Selectors */
#define UVC_PU_CONTROL_UNDEFINED 0x00
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux
[PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
From: Sergey Senozhatsky Document new v4l2-selection target which will be used for the Region of Interest v4l2 control. Signed-off-by: Sergey Senozhatsky --- .../media/v4l/selection-api-configuration.rst | 23 +++ .../media/v4l/v4l2-selection-targets.rst | 21 + include/uapi/linux/v4l2-common.h | 8 +++ 3 files changed, 52 insertions(+) diff --git a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst index fee49bf1a1c0..9f69d71803f6 100644 --- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst +++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst @@ -135,3 +135,26 @@ and the height of rectangles obtained using ``V4L2_SEL_TGT_CROP`` and ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the scaling is applied. The application can compute the scaling ratios using these values. + +Configuration of Region of Interest (ROI) += + +The range of coordinates of the top left corner, width and height of +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. +It is recommended for the driver developers to put the top/left corner +at position ``(0,0)``. The rectangle's coordinates are in global sensor +coordinates. The units are in pixels and independent of the field of view. +They are not impacted by any cropping or scaling that is currently being +used. + +The top left corner, width and height of the Region of Interest area +currently being employed by the device is given by the +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system +as ``V4L2_SEL_TGT_ROI_BOUNDS``. + +In order to change active ROI top left, width and height coordinates +use ``V4L2_SEL_TGT_ROI`` target. + +Each capture device has a default ROI rectangle, given by the +``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall set the ROI rectangle +to the default when the driver is first loaded, but not later. diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst index e877ebbdb32e..cb3809418fa6 100644 --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst @@ -69,3 +69,24 @@ of the two interfaces they are used. modified by hardware. - Yes - No +* - ``V4L2_SEL_TGT_ROI_CURRENT`` + - 0x0200 + - Current Region of Interest rectangle. + - Yes + - No +* - ``V4L2_SEL_TGT_ROI_DEFAULT`` + - 0x0201 + - Suggested Region of Interest rectangle. + - Yes + - No +* - ``V4L2_SEL_TGT_ROI_BOUNDS`` + - 0x0202 + - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit + inside the ROI bounds rectangle. + - Yes + - No +* - ``V4L2_SEL_TGT_ROI`` + - 0x0203 + - Sets the new Region of Interest rectangle. + - Yes + - No diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h index 7d21c1634b4d..d0c108fba638 100644 --- a/include/uapi/linux/v4l2-common.h +++ b/include/uapi/linux/v4l2-common.h @@ -78,6 +78,14 @@ #define V4L2_SEL_TGT_COMPOSE_BOUNDS0x0102 /* Current composing area plus all padding pixels */ #define V4L2_SEL_TGT_COMPOSE_PADDED0x0103 +/* Current Region of Interest area */ +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 +/* Default Region of Interest area */ +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 +/* Region of Interest bounds */ +#define V4L2_SEL_TGT_ROI_BOUNDS0x0202 +/* Set Region of Interest area */ +#define V4L2_SEL_TGT_ROI 0x0203 /* Selection flags */ #define V4L2_SEL_FLAG_GE (1 << 0) -- 2.30.0
[PATCHv2 0/3] Add UVC 1.5 Region Of Interest control to uvcvideo
Hello, RFC This patch set adds UVC 1.5 Region of Interest support. v1->v2: - Address Laurent's comments Sergey Senozhatsky (3): media: v4l UAPI docs: document ROI selection targets media: uvcvideo: add ROI auto controls media: uvcvideo: add UVC 1.5 ROI control .../media/v4l/ext-ctrls-camera.rst| 25 +++ .../media/v4l/selection-api-configuration.rst | 23 +++ .../media/v4l/v4l2-selection-targets.rst | 21 +++ drivers/media/usb/uvc/uvc_ctrl.c | 19 +++ drivers/media/usb/uvc/uvc_v4l2.c | 143 +- include/uapi/linux/usb/video.h| 1 + include/uapi/linux/v4l2-common.h | 8 + include/uapi/linux/v4l2-controls.h| 9 ++ 8 files changed, 246 insertions(+), 3 deletions(-) -- 2.30.0
[PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control
From: Sergey Senozhatsky
This patch implements parts of UVC 1.5 Region of Interest (ROI)
control, using the uvcvideo selection API.
There are several things to mention here.
First, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL; and ROI rectangle
coordinates "must be within the current Digital Window as specified
by the CT_WINDOW control." (4.2.2.1.20 Digital Region of Interest
(ROI) Control.) This is not entirely clear if we need to implement
CT_DIGITAL_WINDOW_CONTROL. ROI is naturally limited by: ROI GET_MIN
and GET_MAX rectangles. Besides, the H/W that I'm playing with
implements ROI, but doesn't implement CT_DIGITAL_WINDOW_CONTROL,
so WINDOW_CONTROL is probably optional.
Second, the patch doesn't implement all of the ROI requests.
Namely, SEL_TGT_BOUNDS for ROI implements GET_MAX (that is maximal
ROI rectangle area). GET_MIN is not implemented (as of now) since
it's not very clear if user space would need such information.
Signed-off-by: Sergey Senozhatsky
---
drivers/media/usb/uvc/uvc_v4l2.c | 143 ++-
1 file changed, 140 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..71b4577196e5 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1139,14 +1139,60 @@ static int uvc_ioctl_querymenu(struct file *file, void
*fh,
return uvc_query_v4l2_menu(chain, qm);
}
-static int uvc_ioctl_g_selection(struct file *file, void *fh,
-struct v4l2_selection *sel)
+/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
+struct uvc_roi_rect {
+ __u16 top;
+ __u16 left;
+ __u16 bottom;
+ __u16 right;
+};
+
+static int uvc_ioctl_g_roi_target(struct file *file, void *fh,
+ struct v4l2_selection *sel)
{
struct uvc_fh *handle = fh;
struct uvc_streaming *stream = handle->stream;
+ struct uvc_roi_rect *roi;
+ u8 query;
+ int ret;
- if (sel->type != stream->type)
+ switch (sel->target) {
+ case V4L2_SEL_TGT_ROI_DEFAULT:
+ query = UVC_GET_DEF;
+ break;
+ case V4L2_SEL_TGT_ROI_CURRENT:
+ query = UVC_GET_CUR;
+ break;
+ case V4L2_SEL_TGT_ROI_BOUNDS:
+ query = UVC_GET_MAX;
+ break;
+ default:
return -EINVAL;
+ }
+
+ roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
+ if (!roi)
+ return -ENOMEM;
+
+ ret = uvc_query_ctrl(stream->dev, query, 1, stream->dev->intfnum,
+UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
+sizeof(struct uvc_roi_rect));
+ if (!ret) {
+ sel->r.left = roi->left;
+ sel->r.top = roi->top;
+ sel->r.width= roi->right;
+ sel->r.height = roi->bottom;
+ }
+
+ kfree(roi);
+ return ret;
+}
+
+static int uvc_ioctl_g_sel_target(struct file *file, void *fh,
+ struct v4l2_selection *sel)
+{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
switch (sel->target) {
case V4L2_SEL_TGT_CROP_DEFAULT:
@@ -1173,6 +1219,96 @@ static int uvc_ioctl_g_selection(struct file *file, void
*fh,
return 0;
}
+static int uvc_ioctl_g_selection(struct file *file, void *fh,
+struct v4l2_selection *sel)
+{
+ struct uvc_fh *handle = fh;
+ struct uvc_streaming *stream = handle->stream;
+
+ if (sel->type != stream->type)
+ return -EINVAL;
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ return uvc_ioctl_g_sel_target(file, fh, sel);
+ case V4L2_SEL_TGT_ROI_CURRENT:
+ case V4L2_SEL_TGT_ROI_DEFAULT:
+ case V4L2_SEL_TGT_ROI_BOUNDS:
+ return uvc_ioctl_g_roi_target(file, fh, sel);
+ }
+
+ return -EINVAL;
+}
+
+static bool validate_roi_bounds(struct uvc_streaming *stream,
+ struct v4l2_selection *sel)
+{
+ bool ok = true;
+
+ if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
+ sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
+ return false;
+
+ /* perhaps also can test against ROI GET_MAX */
+
+ mutex_lock(>mutex);
+ if ((u16)sel->r.width > stream->cur_frame->wWidth)
+ ok = false;
+ if ((u16)sel->r.height > stream->cur_frame->wHeight)
+ ok = false;
+ mutex_unlock(&
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On (21/02/02 15:36), Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed. This is useful for
> debugging purposes.
>
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
>
> Signed-off-by: Timur Tabi
Looks good to me.
Acked-by: Sergey Senozhatsky
-ss
Re: [PATCH 1/3] printk: use CONFIG_CONSOLE_LOGLEVEL_* directly
On (21/02/02 16:02), Masahiro Yamada wrote: > > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > used in , which is included from most of source files. > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > arch/x86/platform/uv/uv_nmi.c > drivers/firmware/efi/libstub/efi-stub-helper.c > drivers/tty/sysrq.c > kernel/printk/printk.c > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > kernel, it is enough to recompile those 4 files. Do you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT so often that it becomes a problem? -ss
Re: [PATCH 3/3] printk: move CONSOLE_EXT_LOG_MAX to kernel/printk/printk.c
On (21/02/02 09:50), John Ogness wrote: > > The only consequences could be out-of-tree modules breaking, > but do we care about that? Yeah, I don't think anyone does. So we are fine here. -ss
Re: [PATCH 2/3] printk: hard-code CONSOLE_LOGLEVEL_MIN in printk.c
On (21/02/02 16:02), Masahiro Yamada wrote:
> include/linux/printk.h | 1 -
> kernel/printk/printk.c | 2 +-
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index fd34b3aa2f90..ceaf0486c01c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -48,7 +48,6 @@ static inline const char *printk_skip_headers(const char
> *buffer)
>
> /* We show everything that is MORE important than this.. */
> #define CONSOLE_LOGLEVEL_SILENT 0 /* Mum's the word */
> -#define CONSOLE_LOGLEVEL_MIN 1 /* Minimum loglevel we let people use */
> #define CONSOLE_LOGLEVEL_DEBUG 10 /* issue debug messages */
> #define CONSOLE_LOGLEVEL_MOTORMOUTH 15 /* You can't shut this one up */
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 92b93340905c..7b50298d52e3 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -63,7 +63,7 @@
> int console_printk[4] = {
> CONFIG_CONSOLE_LOGLEVEL_DEFAULT,/* console_loglevel */
> CONFIG_MESSAGE_LOGLEVEL_DEFAULT,/* default_message_loglevel */
> - CONSOLE_LOGLEVEL_MIN, /* minimum_console_loglevel */
> + 1, /* minimum_console_loglevel */
> CONFIG_CONSOLE_LOGLEVEL_DEFAULT,/* default_console_loglevel */
I personally don't think that this improves code readability.
-ss
[PATCH] media: uvcvideo: initial UVC 1.5 region of interest support
This patch implements parts of UVC 1.5 Region of Interest (ROI)
control, using the uvcvideo selection API.
There are several things to mention here.
First, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL; and ROI rectangle
coordinates "must be within the current Digital Window as specified
by the CT_WINDOW control." (4.2.2.1.20 Digital Region of Interest
(ROI) Control.) This is not entirely clear if we need to implement
CT_DIGITAL_WINDOW_CONTROL. ROI is naturally limited by:
a) ROI GET_MIN and GET_MAX rectangles
b) current image crop
c) stream->cur_frame->wWidth/stream->cur_frame->wHeight
Second, ROI control is a compound data type:
Control Selector CT_REGION_OF_INTEREST_CONTROL
Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
wLength 10
Offset FieldSize
0wROI_Top 2
2wROI_Left2
4wROI_Bottom 2
6wROI_Right 2
8bmAutoControls 2 (Bitmap)
While uvc_control_mapping can handle only s32 data at the moment:
->get() returns s32 value, ->set() accepts s32 value; while v4l2_ctrl
maximum/minimum/default_value can hold only s64 values.
Therefore ROI control handling is split into two parts:
a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
by the means of selection API.
Third, the patch adds new selection targets in order to handle ROI
control requests, but it doesn't implement all of the requests.
Namely, SEL_TGT_BOUNDS for ROI implements GET_MAX (that is maximal
ROI rectangle area). GET_MIN is not implemented ( as of now) since
it's not very clear if user space would need such information.
Signed-off-by: Sergey Senozhatsky
---
drivers/media/usb/uvc/uvc_ctrl.c | 32 +++
drivers/media/usb/uvc/uvc_v4l2.c | 143 -
include/uapi/linux/usb/video.h | 1 +
include/uapi/linux/v4l2-common.h | 10 ++
include/uapi/linux/v4l2-controls.h | 1 +
5 files changed, 184 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b3dde98499f4..4e55a0922f15 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -355,6 +355,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
.flags = UVC_CTRL_FLAG_GET_CUR
| UVC_CTRL_FLAG_AUTO_UPDATE,
},
+ {
+ .entity = UVC_GUID_UVC_CAMERA,
+ .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
+ .index = 21,
+ .size = 10,
+ .flags = UVC_CTRL_FLAG_SET_CUR | UVC_CTRL_FLAG_GET_CUR
+ | UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+ | UVC_CTRL_FLAG_GET_DEF
+ },
};
static const struct uvc_menu_info power_line_frequency_controls[] = {
@@ -370,6 +379,17 @@ static const struct uvc_menu_info exposure_auto_controls[]
= {
{ 8, "Aperture Priority Mode" },
};
+static struct uvc_menu_info roi_auto_controls[] = {
+ { 0, "Auto Exposure" },
+ { 1, "Auto Iris" },
+ { 2, "Auto White Balance" },
+ { 3, "Auto Focus" },
+ { 4, "Auto Face Detect" },
+ { 5, "Auto Detect and Track" },
+ { 6, "Image Stabilization" },
+ { 7, "Higher Quality" },
+};
+
static s32 uvc_ctrl_get_zoom(struct uvc_control_mapping *mapping,
u8 query, const u8 *data)
{
@@ -753,6 +773,18 @@ static const struct uvc_control_mapping
uvc_ctrl_mappings[] = {
.v4l2_type = V4L2_CTRL_TYPE_BOOLEAN,
.data_type = UVC_CTRL_DATA_TYPE_BOOLEAN,
},
+ {
+ .id = V4L2_CID_REGION_OF_INTEREST_CONTROLS,
+ .name = "Region of Interest (controls)",
+ .entity = UVC_GUID_UVC_CAMERA,
+ .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
+ .size = 16,
+ .offset = 64,
+ .v4l2_type = V4L2_CTRL_TYPE_MENU,
+ .data_type = UVC_CTRL_DATA_TYPE_BITMASK,
+ .menu_info = roi_auto_controls,
+ .menu_count = ARRAY_SIZE(roi_auto_controls),
+ },
};
/*
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..71b4577196e5 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1139,14 +1139,60 @@ static int uvc_ioctl_querymenu(struct file *file, void
*fh,
return uvc_query_v4l2_menu(chain, qm);
}
-static int uvc_ioctl_g_selection(struct file *file, void *fh,
-
Re: [printk] b031a684bf: INFO:rcu_tasks_detected_stalls_on_tasks
On (21/01/27 22:28), John Ogness wrote:
>
> # modprobe rcutorture onoff_interval=3 onoff_holdoff=30 torture_type=tasks
>
> (Those are the same modprobe parameters used by the lkp job.)
>
> After about a minute I see:
>
> [ 47.268292] tasks-torture: rcu_torture_read_exit: Start of episode
> [ 51.273365] tasks-torture: rcu_torture_read_exit: End of episode
> [ 55.823306] smpboot: do_boot_cpu failed(-1) to wakeup CPU#0
> [ 55.824350] tasks-torture:torture_onoff task: online 0 failed: errno -5
> [ 55.830661] tasks-torture:torture_onoff task: online 0 failed: errno -5
> [ 55.848524] tasks-torture:torture_onoff task: online 0 failed: errno -5
Just out of curious, this is seen only with the printk commit in
question applied? do_boot_cpu() doesn't seem to call a lot of printk-s,
only pr_debug("Setting warm reset code and vector.\n"), as far as I can
tell.
I can't repro this on my arm64 board. Maybe it's too slow. Race condition?
-ss
Re: [printk] b031a684bf: INFO:rcu_tasks_detected_stalls_on_tasks
On (21/01/22 17:21), Petr Mladek wrote: [..] > > [ 952.271861] ? firmware_map_remove > > (kbuild/src/consumer/kernel/sched/core.c:4411) > > [ 952.272870] ? ksys_write (kbuild/src/consumer/fs/read_write.c:661) > > [ 952.273709] schedule (kbuild/src/consumer/include/linux/thread_info.h:84 > > (discriminator 1) kbuild/src/consumer/include/linux/sched.h:1897 > > (discriminator 1) kbuild/src/consumer/kernel/sched/core.c:4608 > > (discriminator 1)) > > [ 952.274495] exit_to_user_mode_prepare > > (kbuild/src/consumer/kernel/entry/common.c:160 > > kbuild/src/consumer/kernel/entry/common.c:191) > > [ 952.275516] syscall_exit_to_user_mode > > (kbuild/src/consumer/arch/x86/include/asm/jump_label.h:41 > > kbuild/src/consumer/arch/x86/include/asm/nospec-branch.h:288 > > kbuild/src/consumer/arch/x86/include/asm/entry-common.h:80 > > kbuild/src/consumer/kernel/entry/common.c:133 > > kbuild/src/consumer/kernel/entry/common.c:268) > > > [ 952.276650] entry_INT80_compat > > (kbuild/src/consumer/arch/x86/entry/entry_64_compat.S:412) > > [ 952.277673] RIP: 0023:0xf7f93a02 > > [ 952.278497] RSP: 002b:ff8db2f4 EFLAGS: 0246 ORIG_RAX: > > 0004 > > [ 952.280235] RAX: 0001 RBX: 0001 RCX: > > 5666f220 > > [ 952.281813] RDX: 0001 RSI: f7f59d60 RDI: > > 0001 > > [ 952.283347] RBP: 5666f220 R08: R09: > > > > [ 952.284919] R10: R11: R12: > > > > [ 952.286507] R13: R14: R15: > > > > [ 952.294673] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.328047] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.336658] > > [ 352.572231] > > [ 98.585525] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.336693] > > [ 952.359450] > > [ 352.594642] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.359483] > > [ 952.377678] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.409955] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.412504] > > [ 352.610470] > > [ 98.605699] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.412531] > > [ 952.444293] > > [ 352.625170] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.444328] > > [ 952.453669] tasks-torture:torture_onoff task: online 0 failed: errno -5 > > [ 952.480340] > > [ 352.638172] > > > > To reproduce: > > > > # build kernel > > cd linux > > cp config-5.10.0-rc5-gb031a684bfd0 .config > > make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 olddefconfig prepare > > modules_prepare bzImage > > > > git clone https://github.com/intel/lkp-tests.git > > cd lkp-tests > > bin/lkp qemu -k job-script # job-script is attached in > > this email > > I am still struggling with lkp-tests. I have never used them before > and have troubles to get it working. It asks for a newer qemu at > the moment. It means that I am still not able to reproduce it :-/ > > I wonder if there is a way to reproduce this without lpk-tests > framework. AFAIU this is just 'modprobe rcutorture' -ss
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On (21/01/26 20:29), John Ogness wrote: > > On 2021-01-26, Steven Rostedt wrote: > > And even if we make this a boot time option, perhaps we should still > > include that nasty dmesg notice, which will let people know that the > > kernel has unhashed values. > > +1 > > The notice would probably be the main motivation for distros/users to > avoid unhashed values unless truly debugging. Which is what we want. +1 for boot param with a scary name and dmesg WARNING WARNING WARNING that should look even scarier. Timur, do you have time to take a look and submit a patch? -ss
Re: [PATCH 0/2] Fix BUG: Invalid wait context in hrtimer_interrupt()
On (21/01/23 23:37), Qais Yousef wrote: > > I hit a pr_warn() inside hrtimer_interrupt() which lead to a BUG: Invalid wait > context splat. > > The problem wasn't reproducible but I think the cause is obvious, printk can't > be called from interrupt context. > > AFAICU printk_deferred() is safe from NMI, so I assumed it is safe to be > called > from hrtimer_interrupt() too. Adding a pr_warn_once() inside > hrtimer_interrupt() in a location where it is always hit produces the BUG > splat. Replacing it with pr_warn_deferred_once() generates the printk warning > without any splat. Could you please post the lockdep splat? Why is it invalid? Is this... -rt kernel, perhaps? -ss
Re: [PATCH 0/2] Fix BUG: Invalid wait context in hrtimer_interrupt()
On (21/01/26 14:59), Qais Yousef wrote:
> On 01/26/21 13:46, Sergey Senozhatsky wrote:
> > On (21/01/23 23:37), Qais Yousef wrote:
[..]
> I reported it here:
> https://lore.kernel.org/lkml/20210114124512.mg3vexudfmoadef5@e107158-lin/
>
> # [67628.388606] hrtimer: interrupt took 304720 ns
> [67628.393546]
> [67628.393550] =
> [67628.393554] [ BUG: Invalid wait context ]
> [67628.393557] 5.11.0-rc3-00019-g86be331946f7 #37 Not tainted
> [67628.393560] -
> [67628.393563] sugov:0/192 is trying to lock:
> [67628.393566] 000800b1d898 (_lock_key){-.-.}-{3:3}, at:
> pl011_console_write+0x138/0x218
> [67628.393581] other info that might help us debug this:
> [67628.393584] context-{2:2}
> [67628.393586] 4 locks held by sugov:0/192:
> [67628.393589] #0: 0008059cb720
> (_policy->work_lock){+.+.}-{4:4}, at: sugov_work+0x58/0x88
> [67628.393603] #1: 800015446f20 (prepare_lock){+.+.}-{4:4}, at:
> clk_prepare_lock+0x34/0xb0
> [67628.393618] #2: 8000152aaa60 (console_lock){+.+.}-{0:0}, at:
> vprintk_emit+0x12c/0x310
> [67628.393632] #3: 8000152aab88 (console_owner){-.-.}-{0:0}, at:
> console_unlock+0x190/0x6d8
> [67628.393646] stack backtrace:
> [67628.393649] CPU: 0 PID: 192 Comm: sugov:0 Not tainted
> 5.11.0-rc3-00019-g86be331946f7 #37
> [67628.393653] Hardware name: ARM Juno development board (r2) (DT)
> [67628.393656] Call trace:
> [67628.393659] dump_backtrace+0x0/0x1b0
> [67628.393661] show_stack+0x20/0x70
> [67628.393664] dump_stack+0xf8/0x168
> [67628.393666] __lock_acquire+0x964/0x1c88
> [67628.393669] lock_acquire+0x26c/0x500
> [67628.393671] _raw_spin_lock+0x64/0x88
> [67628.393674] pl011_console_write+0x138/0x218
> [67628.393677] console_unlock+0x2c4/0x6d8
> [67628.393680] vprintk_emit+0x134/0x310
> [67628.393682] vprintk_default+0x40/0x50
> [67628.393685] vprintk_func+0xfc/0x2b0
> [67628.393687] printk+0x68/0x90
> [67628.393690] hrtimer_interrupt+0x24c/0x250
> [67628.393693] arch_timer_handler_phys+0x3c/0x50
> [67628.393695] handle_percpu_devid_irq+0xd8/0x460
> [67628.393698] generic_handle_irq+0x38/0x50
> [67628.393701] __handle_domain_irq+0x6c/0xc8
> [67628.393704] gic_handle_irq+0xb0/0xf0
> [67628.393706] el1_irq+0xb4/0x180
> [67628.393709] _raw_spin_unlock_irqrestore+0x58/0xa8
> [67628.393712] hrtimer_start_range_ns+0x1b0/0x420
> [67628.393715] msg_submit+0x100/0x108
> [67628.393717] mbox_send_message+0x84/0x128
> [67628.393720] scpi_send_message+0x11c/0x2a8
> [67628.393723] scpi_dvfs_set_idx+0x48/0x70
> [67628.393726] scpi_dvfs_set_rate+0x60/0x78
> [67628.393728] clk_change_rate+0x184/0x8a8
> [67628.393731] clk_core_set_rate_nolock+0x1c0/0x280
> [67628.393734] clk_set_rate+0x40/0x98
> [67628.393736] scpi_cpufreq_set_target+0x40/0x68
> [67628.393739] __cpufreq_driver_target+0x1a0/0x5d0
Thanks.
> > Why is it invalid? Is this... -rt kernel, perhaps?
>
> No I was running on Linus master at the time.
>
> AFAIU printk will hold the console lock which could sleep in interrupt
> context.
>
> Did I miss something?
printk() is not permitted to sleep/schedule/etc and it never does.
Generally it should be OK to call it from IRQ (module recursion paths).
-ss
Re: [printk] b031a684bf: INFO:rcu_tasks_detected_stalls_on_tasks
On (21/01/22 16:13), kernel test robot wrote: [..] > > ++++ > || 6b916706f8 | b031a684bf | > ++++ > | boot_successes | 66 | 18 | > | boot_failures | 2 | 26 | > | INFO:rcu_sched_detected_stalls_on_CPUs/tasks | 1 || > | RIP:enqueue_hrtimer| 1 || > | RIP:__memset | 1 || > | RIP:clear_page_rep | 2 | 2 | > | BUG:kernel_hang_in_boot_stage | 2 || > | INFO:rcu_sched_self-detected_stall_on_CPU | 1 | 4 | > | INFO:rcu_tasks_detected_stalls_on_tasks| 0 | 22 | > | RIP:kernel_init_free_pages | 0 | 1 | > | IP-Config:Auto-configuration_of_network_failed | 0 | 3 | > | RIP:zone_page_state| 0 | 1 | > ++++ A side note: > # printk and dmesg options > # > CONFIG_PRINTK_TIME=y > # CONFIG_PRINTK_CALLER is not set > CONFIG_CONSOLE_LOGLEVEL_DEFAULT=7 > CONFIG_CONSOLE_LOGLEVEL_QUIET=4 Could you please keep CONFIG_PRINTK_CALLER enabled at all times? -ss
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On (21/01/19 01:47), Matthew Wilcox wrote: [..] > > > So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when > > CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise? > > Distros enable CONFIG_DEBUG_KERNEL. Oh, I see. > If you want to add CONFIG_DEBUG_LEAK_ADDRESSES, then that's great, > and you won't even have to change users, you can just change how %p > behaves. I like the name. config dependent behaviour of %p wouldn't be new, well, to some extent, e.g. XFS does something similar (see below). I don't think Linus will be sold on this, however. fs/xfs/xfs_linux.h: /* * Starting in Linux 4.15, the %p (raw pointer value) printk modifier * prints a hashed version of the pointer to avoid leaking kernel * pointers into dmesg. If we're trying to debug the kernel we want the * raw values, so override this behavior as best we can. */ #ifdef DEBUG # define PTR_FMT "%px" #else # define PTR_FMT "%p" #endif And then they just use it as xfs_alert(mp, "%s: bad inode magic number, dip = "ptr_fmt", dino bp = "ptr_fmt", ino = %ld", __func__, dip, bp, in_f->ilf_ino); -ss
Re: [PATCH] printk: fix buffer overflow potential for print_text()
On (21/01/19 10:00), John Ogness wrote: > On 2021-01-19, Sergey Senozhatsky wrote: > > John, how did you spot these problems? > > I am preparing my series to remove the logbuf_lock, which also refactors > and consolidates code from syslog_print_all() and > kmsg_dump_get_buffer(). While testing/verifying my series, I noticed the > these oddities in the semantics and decided I should research where they > came from and if they were actually necessary. Any chance you can put those tests somewhere public so that we can run them regularly? (say, before Petr sends out a pull request to Linus.) > I wouldn't say the oddities are necessary (in fact, they are quite > annoying), but we have decided to keep them in out of fear of breaking > out-of-tree modules and/or interesting userspace code. Sure. > One positive effect of the rework is that we are finding these oddities > and documenting them. Absolutely agree. -ss
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On (21/01/18 13:03), Timur Tabi wrote: > On 1/18/21 12:26 PM, Matthew Wilcox wrote: > > Don't make it easy. And don't make it look like they're doing > > something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK > > by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too. > > DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far. > > It's already extremely easy to replace %p with %px in your own printks, so I > don't really understand your argument. I like the idea of a more radical name, e.g. DUMP_PREFIX_RAW_POINTERS or something similar. > Seriously, this patch should not be so contentious. If you want hashed > addresses, then nothing changes. If you need unhashed addresses while > debugging, then use DUMP_PREFIX_UNHASHED. Just like you can use %px in > printk. I never use %p in my printks, but then I never submit code upstream > that prints addresses, hashed or unhashed. So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise? -ss
Re: [PATCH] printk: fix buffer overflow potential for print_text()
On (21/01/15 13:07), Petr Mladek wrote:
> On Fri 2021-01-15 13:04:37, Petr Mladek wrote:
> > On Thu 2021-01-14 18:10:12, John Ogness wrote:
> > > Before commit b6cf8b3f3312 ("printk: add lockless ringbuffer"),
> > > msg_print_text() would only write up to size-1 bytes into the
> > > provided buffer. Some callers expect this behavior and append
> > > a terminator to returned string. In particular:
> > >
> > > arch/powerpc/xmon/xmon.c:dump_log_buf()
> > > arch/um/kernel/kmsg_dump.c:kmsg_dumper_stdout()
> > >
> > > msg_print_text() has been replaced by record_print_text(), which
> > > currently fills the full size of the buffer. This causes a
> > > buffer overflow for the above callers.
> > >
> > > Change record_print_text() so that it will only use size-1 bytes
> > > for text data. Also, for paranoia sakes, add a terminator after
> > > the text data.
> > >
> > > And finally, document this behavior so that it is clear that only
> > > size-1 bytes are used and a terminator is added.
> > >
> > > Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> > > Signed-off-by: John Ogness
John, how did you spot these problems?
FWIW, Acked-by: Sergey Senozhatsky
> I forgot one thing. We should add stable here:
>
> Cc: sta...@vger.kernel.org # 5.10+
Good point.
-ss
Re: [PATCH] printk: ringbuffer: fix line counting
On (21/01/13 15:48), John Ogness wrote:
>
> Counting text lines in a record simply involves counting the number
> of newline characters (+1). However, it is searching the full data
> block for newline characters, even though the text data can be (and
> often is) a subset of that area. Since the extra area in the data
> block was never initialized, the result is that extra newlines may
> be seen and counted.
>
> Restrict newline searching to the text data length.
>
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness
Acked-by: Sergey Senozhatsky
-ss
Re: [PATCH] printk: fix kmsg_dump_get_buffer length calulations
On (21/01/13 17:50), John Ogness wrote:
>
> kmsg_dump_get_buffer() uses @syslog to determine if the syslog
> prefix should be written to the buffer. However, when calculating
> the maximum number of records that can fit into the buffer, it
> always counts the bytes from the syslog prefix.
>
> Use @syslog when calculating the maximum number of records that can
> fit into the buffer.
>
> Fixes: e2ae715d66bf ("kmsg - kmsg_dump() use iterator to receive log buffer
> content")
> Signed-off-by: John Ogness
Acked-by: Sergey Senozhatsky
-ss
Re: [PATCH] mm/zsmalloc.c: Convert to use kmem_cache_zalloc in cache_alloc_zspage()
On (21/01/14 07:00), Miaohe Lin wrote: > > We always memset the zspage allocated via cache_alloc_zspage. So it's more > convenient to use kmem_cache_zalloc in cache_alloc_zspage than caller do it > manually. > > Signed-off-by: Miaohe Lin Reviewed-by: Sergey Senozhatsky -ss
Re: madvise(MADV_REMOVE) deadlocks on shmem THP
On (21/01/13 20:31), Hugh Dickins wrote:
> > We are running into lockups during the memory pressure tests on our
> > boards, which essentially NMI panic them. In short the test case is
> >
> > - THP shmem
> > echo advise > /sys/kernel/mm/transparent_hugepage/shmem_enabled
> >
> > - And a user-space process doing madvise(MADV_HUGEPAGE) on new mappings,
> > and madvise(MADV_REMOVE) when it wants to remove the page range
> >
> > The problem boils down to the reverse locking chain:
> > kswapd does
> >
> > lock_page(page) -> down_read(page->mapping->i_mmap_rwsem)
> >
> > madvise() process does
> >
> > down_write(page->mapping->i_mmap_rwsem) -> lock_page(page)
> >
> >
> >
> > CPU0 CPU1
> >
> > kswapd vfs_fallocate()
> > shrink_node()
> > shmem_fallocate()
> > shrink_active_list()
> > unmap_mapping_range()
> >page_referenced() << lock page:PG_locked >>
> > unmap_mapping_pages() << down_write(mapping->i_mmap_rwsem) >>
> > rmap_walk_file()
> > zap_page_range_single()
> > down_read(mapping->i_mmap_rwsem) << W-locked on CPU1>>
> > unmap_page_range()
> > rwsem_down_read_failed()
> > __split_huge_pmd()
> >__rwsem_down_read_failed_common()
> > __lock_page() << PG_locked on CPU0 >>
> > schedule()
> > wait_on_page_bit_common()
> >
> > io_schedule()
>
> Very interesting, Sergey: many thanks for this report.
Thanks for the quick feedback.
> There is no doubt that kswapd is right in its lock ordering:
> __split_huge_pmd() is in the wrong to be attempting lock_page().
>
> Which used not to be done, but was added in 5.8's c444eb564fb1 ("mm:
> thp: make the THP mapcount atomic against __split_huge_pmd_locked()").
Hugh, I forgot to mention, we are facing these issues on 4.19.
Let me check if (maybe) we have cherry picked c444eb564fb1.
-ss
madvise(MADV_REMOVE) deadlocks on shmem THP
Hi, We are running into lockups during the memory pressure tests on our boards, which essentially NMI panic them. In short the test case is - THP shmem echo advise > /sys/kernel/mm/transparent_hugepage/shmem_enabled - And a user-space process doing madvise(MADV_HUGEPAGE) on new mappings, and madvise(MADV_REMOVE) when it wants to remove the page range The problem boils down to the reverse locking chain: kswapd does lock_page(page) -> down_read(page->mapping->i_mmap_rwsem) madvise() process does down_write(page->mapping->i_mmap_rwsem) -> lock_page(page) CPU0 CPU1 kswapd vfs_fallocate() shrink_node() shmem_fallocate() shrink_active_list() unmap_mapping_range() page_referenced() << lock page:PG_locked >> unmap_mapping_pages() << down_write(mapping->i_mmap_rwsem) >> rmap_walk_file() zap_page_range_single() down_read(mapping->i_mmap_rwsem) << W-locked on CPU1>> unmap_page_range() rwsem_down_read_failed() __split_huge_pmd() __rwsem_down_read_failed_common() __lock_page() << PG_locked on CPU0 >> schedule() wait_on_page_bit_common() io_schedule() -ss
Re: [stable] ext4 fscrypt_get_encryption_info() circular locking dependency
Hello, Eric, sorry for the delay. On (20/12/11 10:03), Eric Biggers wrote: > > [..] > > > > [ 1598.658233] __rwsem_down_read_failed_common+0x186/0x201 > > [ 1598.658235] call_rwsem_down_read_failed+0x14/0x30 > > [ 1598.658238] down_read+0x2e/0x45 > > [ 1598.658240] rmap_walk_file+0x73/0x1ce > > [ 1598.658242] page_referenced+0x10d/0x154 > > [ 1598.658247] shrink_active_list+0x1d4/0x475 > > [ 1598.658250] shrink_node+0x27e/0x661 > > [ 1598.658254] try_to_free_pages+0x425/0x7ec > > [ 1598.658258] __alloc_pages_nodemask+0x80b/0x1514 > > [ 1598.658279] __do_page_cache_readahead+0xd4/0x1a9 > > [ 1598.658282] filemap_fault+0x346/0x573 > > [ 1598.658287] ext4_filemap_fault+0x31/0x44 > > Could you provide some more information about what is causing these actual > lockups for you? Are there more stack traces? I think I have some leads, and, just like you said, this deos not appear to be ext4 related. A likely root cause for the lockups I'm observing, is that kswapd and virtio_balloon have reverse locking order for THP pages: down_write(mapping->i_mmap_rwsem) --> page->PG_locked vs page->PG_locked --> down_read(mapping->i_mmap_rwsem) -ss
Re: ARC no console output (was Re: [PATCH 1/2] init/console: Use ttynull as a fallback when there is no console)
On (21/01/07 09:58), Vineet Gupta wrote: > On 1/7/21 9:04 AM, Petr Mladek wrote: > > On Thu 2021-01-07 08:43:16, Vineet Gupta wrote: > > > Hi John, > > > > > > On 1/7/21 1:02 AM, John Ogness wrote: > > > > Hi Vineet, > > > > > > > > On 2021-01-06, Vineet Gupta wrote: > > > > > This breaks ARC booting (no output on console). > > > > > > > > Could you provide the kernel boot arguments that you use? This series is > > > > partly about addressing users that have used boot arguments that are > > > > technically incorrect (even if had worked). Seeing the boot arguments of > > > > users that are not experiencing problems may help to reveal some of the > > > > unusual console usages until now. > > > > > > > > > Kernel command line: earlycon=uart8250,mmio32,0xf0005000,115200n8 > > > console=ttyS0,115200n8 debug print-fatal-signals=1 > > > > This is strange, the problematic patch should use ttynull > > only as a fallback. It should not be used when a particular console > > is defined on the command line. > > What happens in my case is console_on_rootfs() doesn't find /dev/console and > switching to ttynull. /dev is not present because devtmpfs doesn't automount > for initramfs. I wonder if we'll move the nulltty fallback logic into printk code [1] will it fix the problem? [1] https://lore.kernel.org/lkml/X6x%2FAxD1qanC6evJ@jagdpanzerIV.localdomain/ -ss
Re: [PATCH 1/1] Revert "init/console: Use ttynull as a fallback when there is no console"
On (21/01/07 17:44), Petr Mladek wrote:
>
> This reverts commit 757055ae8dedf5333af17b3b5b4b70ba9bc9da4e.
>
> The commit caused that ttynull was used as the default console
> on many systems. It happened when there was no console configured
> on the command line and ttynull_init() was the first initcall
> calling register_console().
>
> The commit fixed a historical problem that have been there for ages.
> The primary motivation was the commit 3cffa06aeef7ece30f6
> ("printk/console: Allow to disable console output by using console=""
> or console=null"). It provided a clean solution
> for a workaround that was widely used and worked only by chance.
>
> This revert causes that the console="" or console=null command line
> options will again work only by chance. These options will cause that
> a particular console will be preferred and the default (tty) ones
> will not get enabled. There will be no console registered at
> all. As a result there won't be stdin, stdout, and stderr for
> the init process. But it worked exactly this way even before.
>
> The proper solution has to fulfill many conditions:
>
> + Register ttynull only when explicitly required or as
> the ultimate fallback.
>
> + ttynull must get associated with /dev/console but it must
> not become preferred console when used as a fallback.
> Especially, it must still be possible to replace it
> by a better console later.
>
> Such a change requires clean up of the register_console() code.
> Otherwise, it would be even harder to follow. Especially, the use
> of has_preferred_console and CON_CONSDEV flag is tricky. The clean
> up is risky. The ordering of consoles is not well defined. And
> any changes tend to break existing user settings.
>
> Do the revert at the least risky solution for now.
>
> Signed-off-by: Petr Mladek
Acked-by: Sergey Senozhatsky
-ss
Re: [PATCH] tty/serial/8250: make touch watchdog settable
On (21/01/06 17:28), chenzhen (R) wrote: > On 21/1/6 15:33, Sergey Senozhatsky wrote: > > On (21/01/06 14:46), chenzhen wrote: > >> Since 54f19b4a6(tty/serial/8250: Touch NMI watchdog in wait_for_xmitr), > >> serial8250 > >> will always touch watchdog in write and wait_for_xmitr. However, > >> serial8250 may > >> become low speed thus take a long time to print. In this process, nmi and > >> soft > >> watchdog on current CPU will be invalid. > >> > >> To resolve this problem, add a cmdline option "tty_watchdog_enable" to > >> control > >> the touchdog in serial8250. > > Sorry, I don't understand - what does this fix? > > > > -ss > > . > > It fixes that if serial8250 is low speed on some machine, when it > writes for a long time, NMI/softlockup watchdog will never bark and > potential rlock will not be detected. So an option to control the > touchdog in serial8250 may help. This is the intention. Suppose that serial8250 triggers the lockup watchdog, how are you going to fix it? If you need to write N bytes to the slow 8250 port then there is nothing you can do about it; triggering watchdog from the 8250 write path is certainly not going to help. If you have an excessive number of printk()-s then reduce the number of spurious printk()-s (ratelimit, etc). -ss
Re: [PATCH v1] panic: push panic() messages to the console even from the MCE nmi handler
On (21/01/04 16:15), “William Roche wrote:
[..]
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 332736a..eb90cc0 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -166,6 +166,15 @@ static void panic_print_sys_info(void)
> ftrace_dump(DUMP_ALL);
> }
>
> +/*
> + * Force flush messages to the console.
> + */
> +static void panic_flush_to_console(void)
> +{
> + printk_safe_flush_on_panic();
> + console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> +}
You must debug_locks_off() as the very first step here. But see below.
> /**
> * panic - halt the system
> * @fmt: The text string to print
> @@ -247,7 +256,7 @@ void panic(const char *fmt, ...)
>* Bypass the panic_cpu check and call __crash_kexec directly.
>*/
> if (!_crash_kexec_post_notifiers) {
> - printk_safe_flush_on_panic();
> + panic_flush_to_console();
> __crash_kexec(NULL);
It's dangerous to call console_flush_on_panic() before we stop secondary
CPUs. console_flush_on_panic() ignores the state console_sem, so if any
of the secondary is currently printing something on the consoles you'll
get corrupted messages - we use `static char buffer` for messages we
push to consoles.
Another issue is that with this panic_flush_to_console() call console_sem
can end up being locked once (by secondary CPU) and unlocked twice (by
second and panic CPUs) [*]
> /*
> @@ -271,9 +280,8 @@ void panic(const char *fmt, ...)
>*/
> atomic_notifier_call_chain(_notifier_list, 0, buf);
>
> - /* Call flush even twice. It tries harder with a single online CPU */
> - printk_safe_flush_on_panic();
> kmsg_dump(KMSG_DUMP_PANIC);
> + panic_flush_to_console();
Why?
> /*
>* If you doubt kdump always works fine in any situation,
> @@ -298,7 +306,7 @@ void panic(const char *fmt, ...)
>* buffer. Try to acquire the lock then release it regardless of the
>* result. The release will also print the buffers out. Locks debug
>* should be disabled to avoid reporting bad unlock balance when
> - * panic() is not being callled from OOPS.
> + * panic() is not being called from OOPS.
>*/
> debug_locks_off();
> console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> @@ -314,6 +322,7 @@ void panic(const char *fmt, ...)
>* We can't use the "normal" timers since we just panicked.
>*/
> pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
> + panic_flush_to_console();
[*] So this
> for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
> touch_nmi_watchdog();
> @@ -347,6 +356,7 @@ void panic(const char *fmt, ...)
> disabled_wait();
> #endif
> pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
> + panic_flush_to_console();
[*] and this are both very interesting points.
Those extra console_flush_on_panic() calls indicate that normal printk()
cannot succeed in locking the console_sem so it doesn't try to
console_unlock(): either because we killed the secondary CPU while it
was holding the lock, or because we locked it once and unlocked it twice.
I think it would make sense to just re-init console_sem, so that normal
printk()-s will have chance to grab the console_sem lock and then we don't
need any extra panic_flush_to_console() calls. Maybe we can do something
like this
---
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ffdd0dc7ec6d..4bd2e29c8cc0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2638,6 +2638,7 @@ void console_flush_on_panic(enum con_flush_mode mode)
* context and we don't want to get preempted while flushing,
* ensure may_schedule is cleared.
*/
+ sema_init(_sem, 1);
console_trylock();
console_may_schedule = 0;
Re: [stable] ext4 fscrypt_get_encryption_info() circular locking dependency
On (20/12/11 13:08), Sergey Senozhatsky wrote:
> >
> > How interested are you in having this fixed? Did you encounter an actual
> > deadlock or just the lockdep report?
>
Got one more. fscrypt_get_encryption_info() again, but from ext4_lookup().
[ 162.840909] kswapd0/80 is trying to acquire lock:
[ 162.840912] 78ea628f (jbd2_handle){}, at:
start_this_handle+0x1f9/0x859
[ 162.840919]
but task is already holding lock:
[ 162.840922] 314ed5a0 (fs_reclaim){+.+.}, at:
__fs_reclaim_acquire+0x5/0x2f
[ 162.840929]
which lock already depends on the new lock.
[ 162.840932]
the existing dependency chain (in reverse order) is:
[ 162.840934]
-> #2 (fs_reclaim){+.+.}:
[ 162.840940]kmem_cache_alloc_trace+0x44/0x28b
[ 162.840944]mempool_create_node+0x46/0x92
[ 162.840947]fscrypt_initialize+0xa0/0xbf
[ 162.840950]fscrypt_get_encryption_info+0xa4/0x774
[ 162.840953]fscrypt_setup_filename+0x99/0x2d1
[ 162.840956]__fscrypt_prepare_lookup+0x25/0x6b
[ 162.840960]ext4_lookup+0x1b2/0x323
[ 162.840963]path_openat+0x9a5/0x156d
[ 162.840966]do_filp_open+0x97/0x13e
[ 162.840970]do_sys_open+0x128/0x3a3
[ 162.840973]do_syscall_64+0x6f/0x22a
[ 162.840977]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 162.840979]
-> #1 (fscrypt_init_mutex){+.+.}:
[ 162.840983]mutex_lock_nested+0x20/0x26
[ 162.840986]fscrypt_initialize+0x20/0xbf
[ 162.840989]fscrypt_get_encryption_info+0xa4/0x774
[ 162.840992]fscrypt_inherit_context+0xbe/0xe6
[ 162.840995]__ext4_new_inode+0x11ee/0x1631
[ 162.840999]ext4_mkdir+0x112/0x416
[ 162.841002]vfs_mkdir2+0x135/0x1c6
[ 162.841004]do_mkdirat+0xc3/0x138
[ 162.841007]do_syscall_64+0x6f/0x22a
[ 162.841011]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 162.841012]
-> #0 (jbd2_handle){}:
[ 162.841017]start_this_handle+0x21c/0x859
[ 162.841019]jbd2__journal_start+0xa2/0x282
[ 162.841022]ext4_release_dquot+0x58/0x93
[ 162.841025]dqput+0x196/0x1ec
[ 162.841028]__dquot_drop+0x8d/0xb2
[ 162.841032]ext4_clear_inode+0x22/0x8c
[ 162.841035]ext4_evict_inode+0x127/0x662
[ 162.841038]evict+0xc0/0x241
[ 162.841041]
Re: [stable] ext4 fscrypt_get_encryption_info() circular locking dependency
On (20/12/10 19:48), Eric Biggers wrote: > > > > [ 133.454836] Chain exists of: > > jbd2_handle --> fscrypt_init_mutex --> fs_reclaim > > > > [ 133.454840] Possible unsafe locking scenario: > > > > [ 133.454841]CPU0CPU1 > > [ 133.454843] > > [ 133.454844] lock(fs_reclaim); > > [ 133.454846]lock(fscrypt_init_mutex); > > [ 133.454848]lock(fs_reclaim); > > [ 133.454850] lock(jbd2_handle); > > [ 133.454851] > > This actually got fixed by the patch series > https://lkml.kernel.org/linux-fscrypt/20200913083620.170627-1-ebigg...@kernel.org/ > which went into 5.10. The more recent patch to remove ext4_dir_open() isn't > related. > > It's a hard patch series to backport. Backporting it to 5.4 would be somewhat > feasible, while 4.19 would be very difficult as there have been a lot of other > fscrypt commits which would heavily conflict with cherry-picks. > > How interested are you in having this fixed? Did you encounter an actual > deadlock or just the lockdep report? Difficult to say. On one hand 'yes' I see lockups on my devices (4.19 kernel); I can't tell at the moment what's the root cause. So on the other hand 'no' I can't say that it's because of ext4_dir_open(). What I saw so far involved ext4, kswapd, khugepaged and lots of other things. [ 1598.655901] INFO: task khugepaged:66 blocked for more than 122 seconds. [ 1598.655914] Call Trace: [ 1598.655920] __schedule+0x506/0x1240 [ 1598.655924] ? kvm_zap_rmapp+0x52/0x69 [ 1598.655927] schedule+0x3f/0x78 [ 1598.655929] __rwsem_down_read_failed_common+0x186/0x201 [ 1598.655933] call_rwsem_down_read_failed+0x14/0x30 [ 1598.655936] down_read+0x2e/0x45 [ 1598.655939] rmap_walk_file+0x73/0x1ce [ 1598.655941] page_referenced+0x10d/0x154 [ 1598.655948] shrink_active_list+0x1d4/0x475 [..] [ 1598.655986] INFO: task kswapd0:79 blocked for more than 122 seconds. [ 1598.655993] Call Trace: [ 1598.655995] __schedule+0x506/0x1240 [ 1598.655998] schedule+0x3f/0x78 [ 1598.656000] __rwsem_down_read_failed_common+0x186/0x201 [ 1598.656003] call_rwsem_down_read_failed+0x14/0x30 [ 1598.656006] down_read+0x2e/0x45 [ 1598.656008] rmap_walk_file+0x73/0x1ce [ 1598.656010] page_referenced+0x10d/0x154 [ 1598.656015] shrink_active_list+0x1d4/0x475 [..] [ 1598.658233] __rwsem_down_read_failed_common+0x186/0x201 [ 1598.658235] call_rwsem_down_read_failed+0x14/0x30 [ 1598.658238] down_read+0x2e/0x45 [ 1598.658240] rmap_walk_file+0x73/0x1ce [ 1598.658242] page_referenced+0x10d/0x154 [ 1598.658247] shrink_active_list+0x1d4/0x475 [ 1598.658250] shrink_node+0x27e/0x661 [ 1598.658254] try_to_free_pages+0x425/0x7ec [ 1598.658258] __alloc_pages_nodemask+0x80b/0x1514 [ 1598.658279] __do_page_cache_readahead+0xd4/0x1a9 [ 1598.658282] filemap_fault+0x346/0x573 [ 1598.658287] ext4_filemap_fault+0x31/0x44 -ss
[stable] ext4 fscrypt_get_encryption_info() circular locking dependency
Hi,
I got the following lockdep splat the other day, while running some
tests on 4.19. I didn't test other stable kernels, but it seems that
5.4 should also have similar problem.
As far as I can tell, ext4_dir_open() has been removed quite recently:
https://www.mail-archive.com/linux-f2fs-devel@lists.sourceforge.net/msg18727.html
Eric, Ted, Jaegeuk, how difficult would it be to remove ext4_dir_open()
from the stable kernels? (I'm interested in ext4 in particular, I
understand that other filesystems may also need similar patches)
[ 133.454721] kswapd0/79 is trying to acquire lock:
[ 133.454724] a815a55f (jbd2_handle){}, at:
start_this_handle+0x1f9/0x859
[ 133.454730]
but task is already holding lock:
[ 133.454731] 106bd5a3 (fs_reclaim){+.+.}, at:
__fs_reclaim_acquire+0x5/0x2f
[ 133.454736]
which lock already depends on the new lock.
[ 133.454739]
the existing dependency chain (in reverse order) is:
[ 133.454740]
-> #2 (fs_reclaim){+.+.}:
[ 133.454745]kmem_cache_alloc_trace+0x44/0x28b
[ 133.454748]mempool_create_node+0x46/0x92
[ 133.454750]fscrypt_initialize+0xa0/0xbf
[ 133.454752]fscrypt_get_encryption_info+0xa4/0x774
[ 133.454754]ext4_dir_open+0x1b/0x2d
[ 133.454757]do_dentry_open+0x144/0x36d
[ 133.454759]path_openat+0x2d7/0x156d
[ 133.454762]do_filp_open+0x97/0x13e
[ 133.454764]do_sys_open+0x128/0x3a3
[ 133.454766]do_syscall_64+0x6f/0x22a
[ 133.454769]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 133.454771]
-> #1 (fscrypt_init_mutex){+.+.}:
[ 133.454774]mutex_lock_nested+0x20/0x26
[ 133.454776]fscrypt_initialize+0x20/0xbf
[ 133.454778]fscrypt_get_encryption_info+0xa4/0x774
[ 133.454780]fscrypt_inherit_context+0xbe/0xe6
[ 133.454782]__ext4_new_inode+0x11ee/0x1631
[ 133.454785]ext4_mkdir+0x112/0x416
[ 133.454787]vfs_mkdir2+0x135/0x1c6
[ 133.454789]do_mkdirat+0xc3/0x138
[ 133.454791]do_syscall_64+0x6f/0x22a
[ 133.454793]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 133.454795]
-> #0 (jbd2_handle){}:
[ 133.454798]start_this_handle+0x21c/0x859
[ 133.454800]jbd2__journal_start+0xa2/0x282
[ 133.454802]ext4_release_dquot+0x58/0x93
[ 133.454804]dqput+0x196/0x1ec
[ 133.454806]__dquot_drop+0x8d/0xb2
[ 133.454809]ext4_clear_inode+0x22/0x8c
[ 133.454811]ext4_evict_inode+0x127/0x662
[ 133.454813]evict+0xc0/0x241
[ 133.454816]dispose_list+0x36/0x54
[ 133.454818]prune_icache_sb+0x56/0x76
[ 133.454820]super_cache_scan+0x13a/0x19c
[ 133.454822]shrink_slab+0x39a/0x572
[ 133.454824]shrink_node+0x3f8/0x63b
[ 133.454826]balance_pgdat+0x1bd/0x326
[ 133.454828]kswapd+0x2ad/0x510
[ 133.454831]kthread+0x14d/0x155
[ 133.454833]ret_from_fork+0x24/0x50
[ 133.454834]
other info that might help us debug this:
[ 133.454836] Chain exists of:
jbd2_handle --> fscrypt_init_mutex --> fs_reclaim
[ 133.454840] Possible unsafe locking scenario:
[ 133.454841]CPU0CPU1
[ 133.454843]
[ 133.454844] lock(fs_reclaim);
[ 133.454846]lock(fscrypt_init_mutex);
[ 133.454848]lock(fs_reclaim);
[ 133.454850] lock(jbd2_handle);
[ 133.454851]
*** DEADLOCK ***
[ 133.454854] 3 locks held by kswapd0/79:
[ 133.454855] #0: 106bd5a3 (fs_reclaim){+.+.}, at:
__fs_reclaim_acquire+0x5/0x2f
[ 133.454859] #1: c230047b (shrinker_rwsem){}, at:
shrink_slab+0x3b/0x572
[ 133.454862] #2: ce797452 (>s_umount_key#45){}, at:
trylock_super+0x1b/0x47
[ 133.454866]
stack backtrace:
[ 133.454869] CPU: 6 PID: 79 Comm: kswapd0 Not tainted 4.19.161 #43
[ 133.454872] Call Trace:
[ 133.454877] dump_stack+0xbd/0x11d
[ 133.454880] ? print_circular_bug+0x2c1/0x2d4
[ 133.454883] __lock_acquire+0x1977/0x1981
[ 133.454886] ? start_this_handle+0x1f9/0x859
[ 133.454888] lock_acquire+0x1b7/0x202
[ 133.454890] ? start_this_handle+0x1f9/0x859
[ 133.454893] start_this_handle+0x21c/0x859
[ 133.454895] ? start_this_handle+0x1f9/0x859
[ 133.454897] ? kmem_cache_alloc+0x1d1/0x27d
[ 133.454900] jbd2__journal_start+0xa2/0x282
[ 133.454902] ? __ext4_journal_start_sb+0x10b/0x208
[ 133.454905] ext4_release_dquot+0x58/0x93
[ 133.454907] dqput+0x196/0x1ec
[ 133.454909] __dquot_drop+0x8d/0xb2
[ 133.454912] ? dquot_drop+0x27/0x43
[ 133.454914] ext4_clear_inode+0x22/0x8c
[ 133.454917] ext4_evict_inode+0x127/0x662
[ 133.454920] evict+0xc0/0x241
[ 133.454923] dispose_list+0x36/0x54
[ 133.454926]
