In current code, the devres group for aggregate master is left open
after call to component_master_add_*(). This leads to problems when the
master does further managed allocations on its own. When any
participating driver calls component_del(), this leads to immediate
release of resources.
This came up when investigating a page fault occurring with i915 DRM
driver unbind with 5.15-rc1 kernel. The following sequence occurs:
i915_pci_remove()
-> intel_display_driver_unregister()
-> i915_audio_component_cleanup()
-> component_del()
-> component.c:take_down_master()
-> hdac_component_master_unbind() [via master->ops->unbind()]
-> devres_release_group(master->parent, NULL)
With older kernels this has not caused issues, but with audio driver
moving to use managed interfaces for more of its allocations, this no
longer works. Devres log shows following to occur:
component_master_add_with_match()
[ 126.886032] snd_hda_intel 0000:00:1f.3: DEVRES ADD 00000000323ccdc5 devm_component_match_release (24 bytes)
[ 126.886045] snd_hda_intel 0000:00:1f.3: DEVRES ADD 00000000865cdb29 grp< (0 bytes)
[ 126.886049] snd_hda_intel 0000:00:1f.3: DEVRES ADD 000000001b480725 grp< (0 bytes)
audio driver completes its PCI probe()
[ 126.892238] snd_hda_intel 0000:00:1f.3: DEVRES ADD 000000001b480725 pcim_iomap_release (48 bytes)
component_del() called() at DRM/i915 unbind()
[ 137.579422] i915 0000:00:02.0: DEVRES REL 00000000ef44c293 grp< (0 bytes)
[ 137.579445] snd_hda_intel 0000:00:1f.3: DEVRES REL 00000000865cdb29 grp< (0 bytes)
[ 137.579458] snd_hda_intel 0000:00:1f.3: DEVRES REL 000000001b480725 pcim_iomap_release (48 bytes)
So the "devres_release_group(master->parent, NULL)" ends up freeing the
pcim_iomap allocation. Upon next runtime resume, the audio driver will
cause a page fault as the iomap alloc was released without the driver
knowing about it.
Fix this issue by using the "struct master" pointer as identifier for
the devres group, and by closing the devres group after
the master->ops->bind() call is done. This allows devres allocations
done by the driver acting as master to be isolated from the binding state
of the aggregate driver. This modifies the logic originally introduced in
commit 9e1ccb4a7700 ("drivers/base: fix devres handling for master device")
Cc: stable(a)vger.kernel.org
BugLink: https://gitlab.freedesktop.org/drm/intel/-/issues/4136
Fixes: 9e1ccb4a7700 ("drivers/base: fix devres handling for master device")
Signed-off-by: Kai Vehmanen <kai.vehmanen(a)linux.intel.com>
Acked-by: Imre Deak <imre.deak(a)intel.com>
Acked-by: Russell King (Oracle) <rmk+kernel(a)armlinux.org.uk>
---
drivers/base/component.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
V3 changes:
- address feedback from Greg KH, add a Fixes tag and cc stable
V2 changes:
- after review form Imre and Russell, removing RFC tag
- rebased on top of 5.15-rc2 (V1 was on drm-tip)
- CI test results for V1 show that this patch fixes multiple
failures in i915 unbind and module reload tests:
https://patchwork.freedesktop.org/series/94889/
diff --git a/drivers/base/component.c b/drivers/base/component.c
index 5e79299f6c3f..870485cbbb87 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -246,7 +246,7 @@ static int try_to_bring_up_master(struct master *master,
return 0;
}
- if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
+ if (!devres_open_group(master->parent, master, GFP_KERNEL))
return -ENOMEM;
/* Found all components */
@@ -258,6 +258,7 @@ static int try_to_bring_up_master(struct master *master,
return ret;
}
+ devres_close_group(master->parent, NULL);
master->bound = true;
return 1;
}
@@ -282,7 +283,7 @@ static void take_down_master(struct master *master)
{
if (master->bound) {
master->ops->unbind(master->parent);
- devres_release_group(master->parent, NULL);
+ devres_release_group(master->parent, master);
master->bound = false;
}
}
base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
--
2.33.0
The mem-to-mem stateless decoder API specifies support for dynamic
resolution changes. In particular, the decoder should accept format
changes on the OUTPUT queue even when buffers have been allocated,
as long as it is not streaming.
Relax restrictions for S_FMT as described in the previous paragraph,
and as long as the codec format remains the same. This aligns it with
the Hantro and Cedrus decoders. This change was mostly based on commit
ae02d49493b5 ("media: hantro: Fix s_fmt for dynamic resolution changes").
Since rkvdec_s_fmt() is now just a wrapper around the output/capture
variants without any additional shared functionality, drop the wrapper
and call the respective functions directly.
Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Cc: <stable(a)vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wenst(a)chromium.org>
---
drivers/staging/media/rkvdec/rkvdec.c | 40 +++++++++++++--------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 7131156c1f2c..3f3f96488d74 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -280,31 +280,20 @@ static int rkvdec_try_output_fmt(struct file *file, void *priv,
return 0;
}
-static int rkvdec_s_fmt(struct file *file, void *priv,
- struct v4l2_format *f,
- int (*try_fmt)(struct file *, void *,
- struct v4l2_format *))
+static int rkvdec_s_capture_fmt(struct file *file, void *priv,
+ struct v4l2_format *f)
{
struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
struct vb2_queue *vq;
+ int ret;
- if (!try_fmt)
- return -EINVAL;
-
- vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
+ /* Change not allowed if queue is busy */
+ vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+ V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
if (vb2_is_busy(vq))
return -EBUSY;
- return try_fmt(file, priv, f);
-}
-
-static int rkvdec_s_capture_fmt(struct file *file, void *priv,
- struct v4l2_format *f)
-{
- struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
- int ret;
-
- ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_capture_fmt);
+ ret = rkvdec_try_capture_fmt(file, priv, f);
if (ret)
return ret;
@@ -319,9 +308,20 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
const struct rkvdec_coded_fmt_desc *desc;
struct v4l2_format *cap_fmt;
- struct vb2_queue *peer_vq;
+ struct vb2_queue *peer_vq, *vq;
int ret;
+ /*
+ * In order to support dynamic resolution change, the decoder admits
+ * a resolution change, as long as the pixelformat remains. Can't be
+ * done if streaming.
+ */
+ vq = v4l2_m2m_get_vq(m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
+ if (vb2_is_streaming(vq) ||
+ (vb2_is_busy(vq) &&
+ f->fmt.pix_mp.pixelformat != ctx->coded_fmt.fmt.pix_mp.pixelformat))
+ return -EBUSY;
+
/*
* Since format change on the OUTPUT queue will reset the CAPTURE
* queue, we can't allow doing so when the CAPTURE queue has buffers
@@ -331,7 +331,7 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
if (vb2_is_busy(peer_vq))
return -EBUSY;
- ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_output_fmt);
+ ret = rkvdec_try_output_fmt(file, priv, f);
if (ret)
return ret;
--
2.33.0.882.g93a45727a2-goog
The rkvdec H.264 decoder currently overrides sizeimage for the output
format. This causes issues when userspace requires and requests a larger
buffer, but ends up with one of insufficient size.
Instead, only provide a default size if none was requested. This fixes
the video_decode_accelerator_tests from Chromium failing on the first
frame due to insufficient buffer space. It also aligns the behavior
of the rkvdec driver with the Hantro and Cedrus drivers.
Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Cc: <stable(a)vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wenst(a)chromium.org>
---
drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 76e97cbe2512..951e19231da2 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
fmt->num_planes = 1;
- fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
- RKVDEC_H264_MAX_DEPTH_IN_BYTES;
+ if (!fmt->plane_fmt[0].sizeimage)
+ fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
+ RKVDEC_H264_MAX_DEPTH_IN_BYTES;
return 0;
}
--
2.33.0.882.g93a45727a2-goog
* Sasha Levin <sashal(a)kernel.org>:
> This is a note to let you know that I've just added the patch titled
>
> ext4: enforce buffer head state assertion in ext4_da_map_blocks
>
> to the 5.14-stable tree which can be found at:
> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
>
> The filename of the patch is:
> ext4-enforce-buffer-head-state-assertion-in-ext4_da_.patch
> and it can be found in the queue-5.14 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable(a)vger.kernel.org> know about it.
>
>
>
> commit b2838e02c515366e8452370fcda5baa2dcc8be68
> Author: Eric Whitney <enwlinux(a)gmail.com>
> Date: Thu Aug 19 10:49:27 2021 -0400
>
> ext4: enforce buffer head state assertion in ext4_da_map_blocks
>
> [ Upstream commit 948ca5f30e1df0c11eb5b0f410b9ceb97fa77ad9 ]
>
> Remove the code that re-initializes a buffer head with an invalid block
> number and BH_New and BH_Delay bits when a matching delayed and
> unwritten block has been found in the extent status cache. Replace it
> with assertions that verify the buffer head already has this state
> correctly set. The current code masked an inline data truncation bug
> that left stale entries in the extent status cache. With this change,
> generic/130 can be used to reproduce and detect that bug.
>
> Signed-off-by: Eric Whitney <enwlinux(a)gmail.com>
> Signed-off-by: Theodore Ts'o <tytso(a)mit.edu>
> Link: https://lore.kernel.org/r/20210819144927.25163-3-enwlinux@gmail.com
> Signed-off-by: Theodore Ts'o <tytso(a)mit.edu>
> Signed-off-by: Sasha Levin <sashal(a)kernel.org>
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index fc6ea56de77c..d204688b32a3 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1726,13 +1726,16 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
> }
>
> /*
> - * Delayed extent could be allocated by fallocate.
> - * So we need to check it.
> + * the buffer head associated with a delayed and not unwritten
> + * block found in the extent status cache must contain an
> + * invalid block number and have its BH_New and BH_Delay bits
> + * set, reflecting the state assigned when the block was
> + * initially delayed allocated
> */
> - if (ext4_es_is_delayed(&es) && !ext4_es_is_unwritten(&es)) {
> - map_bh(bh, inode->i_sb, invalid_block);
> - set_buffer_new(bh);
> - set_buffer_delay(bh);
> + if (ext4_es_is_delonly(&es)) {
> + BUG_ON(bh->b_blocknr != invalid_block);
> + BUG_ON(!buffer_new(bh));
> + BUG_ON(!buffer_delay(bh));
> return 0;
> }
>
This patch should not be added to the stable tree, as it will be reverted in
5.15.
There have been two reports of unexpected kernel panics triggered by this code
in kernels derived from 5.15-rc4, and the code will be removed for the time
being until the root cause can be determined and corrected in a future release.
Thanks,
Eric
* Sasha Levin <sashal(a)kernel.org>:
> This is a note to let you know that I've just added the patch titled
>
> ext4: enforce buffer head state assertion in ext4_da_map_blocks
>
> to the 5.4-stable tree which can be found at:
> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
>
> The filename of the patch is:
> ext4-enforce-buffer-head-state-assertion-in-ext4_da_.patch
> and it can be found in the queue-5.4 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable(a)vger.kernel.org> know about it.
>
>
>
> commit dd19180ca7482668952b8c51499e0676f825189b
> Author: Eric Whitney <enwlinux(a)gmail.com>
> Date: Thu Aug 19 10:49:27 2021 -0400
>
> ext4: enforce buffer head state assertion in ext4_da_map_blocks
>
> [ Upstream commit 948ca5f30e1df0c11eb5b0f410b9ceb97fa77ad9 ]
>
> Remove the code that re-initializes a buffer head with an invalid block
> number and BH_New and BH_Delay bits when a matching delayed and
> unwritten block has been found in the extent status cache. Replace it
> with assertions that verify the buffer head already has this state
> correctly set. The current code masked an inline data truncation bug
> that left stale entries in the extent status cache. With this change,
> generic/130 can be used to reproduce and detect that bug.
>
> Signed-off-by: Eric Whitney <enwlinux(a)gmail.com>
> Signed-off-by: Theodore Ts'o <tytso(a)mit.edu>
> Link: https://lore.kernel.org/r/20210819144927.25163-3-enwlinux@gmail.com
> Signed-off-by: Theodore Ts'o <tytso(a)mit.edu>
> Signed-off-by: Sasha Levin <sashal(a)kernel.org>
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index dcbd8ac8d471..af594b5e4f9f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1869,13 +1869,16 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
> }
>
> /*
> - * Delayed extent could be allocated by fallocate.
> - * So we need to check it.
> + * the buffer head associated with a delayed and not unwritten
> + * block found in the extent status cache must contain an
> + * invalid block number and have its BH_New and BH_Delay bits
> + * set, reflecting the state assigned when the block was
> + * initially delayed allocated
> */
> - if (ext4_es_is_delayed(&es) && !ext4_es_is_unwritten(&es)) {
> - map_bh(bh, inode->i_sb, invalid_block);
> - set_buffer_new(bh);
> - set_buffer_delay(bh);
> + if (ext4_es_is_delonly(&es)) {
> + BUG_ON(bh->b_blocknr != invalid_block);
> + BUG_ON(!buffer_new(bh));
> + BUG_ON(!buffer_delay(bh));
> return 0;
> }
>
This patch should not be added to the stable tree, as it will be reverted in
5.15.
There have been two reports of unexpected kernel panics triggered by this code
in kernels derived from 5.15-rc4, and the code will be removed for the time
being until the root cause can be determined and corrected in a future release.
Thanks,
Eric
* Sasha Levin <sashal(a)kernel.org>:
> This is a note to let you know that I've just added the patch titled
>
> ext4: enforce buffer head state assertion in ext4_da_map_blocks
>
> to the 5.10-stable tree which can be found at:
> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
>
> The filename of the patch is:
> ext4-enforce-buffer-head-state-assertion-in-ext4_da_.patch
> and it can be found in the queue-5.10 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable(a)vger.kernel.org> know about it.
>
>
>
> commit 38c3015b3b8b3a977ca7fa0da8de65c9d8cab2d2
> Author: Eric Whitney <enwlinux(a)gmail.com>
> Date: Thu Aug 19 10:49:27 2021 -0400
>
> ext4: enforce buffer head state assertion in ext4_da_map_blocks
>
> [ Upstream commit 948ca5f30e1df0c11eb5b0f410b9ceb97fa77ad9 ]
>
> Remove the code that re-initializes a buffer head with an invalid block
> number and BH_New and BH_Delay bits when a matching delayed and
> unwritten block has been found in the extent status cache. Replace it
> with assertions that verify the buffer head already has this state
> correctly set. The current code masked an inline data truncation bug
> that left stale entries in the extent status cache. With this change,
> generic/130 can be used to reproduce and detect that bug.
>
> Signed-off-by: Eric Whitney <enwlinux(a)gmail.com>
> Signed-off-by: Theodore Ts'o <tytso(a)mit.edu>
> Link: https://lore.kernel.org/r/20210819144927.25163-3-enwlinux@gmail.com
> Signed-off-by: Theodore Ts'o <tytso(a)mit.edu>
> Signed-off-by: Sasha Levin <sashal(a)kernel.org>
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 317aa1b90fb9..fce4fccb8641 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1727,13 +1727,16 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
> }
>
> /*
> - * Delayed extent could be allocated by fallocate.
> - * So we need to check it.
> + * the buffer head associated with a delayed and not unwritten
> + * block found in the extent status cache must contain an
> + * invalid block number and have its BH_New and BH_Delay bits
> + * set, reflecting the state assigned when the block was
> + * initially delayed allocated
> */
> - if (ext4_es_is_delayed(&es) && !ext4_es_is_unwritten(&es)) {
> - map_bh(bh, inode->i_sb, invalid_block);
> - set_buffer_new(bh);
> - set_buffer_delay(bh);
> + if (ext4_es_is_delonly(&es)) {
> + BUG_ON(bh->b_blocknr != invalid_block);
> + BUG_ON(!buffer_new(bh));
> + BUG_ON(!buffer_delay(bh));
> return 0;
> }
>
This patch should not be added to the stable tree, as it will be reverted in
5.15.
There have been two reports of unexpected kernel panics triggered by this code
in kernels derived from 5.15-rc4, and the code will be removed for the time
being until the root cause can be determined and corrected in a future release.
Thanks,
Eric