| Message ID | 20240116084240.14228-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: Fix mishandling of MEDIA_PAD_FL_MUST_CONNECT flag | expand |
Hi Sakari, On Tue, Jan 16, 2024 at 11:27:29AM +0000, Sakari Ailus wrote: > On Tue, Jan 16, 2024 at 10:42:34AM +0200, Laurent Pinchart wrote: > > When building pipelines by following links, the > > media_pipeline_explore_next_link() function only traverses enabled > > links. The remote pad of a disabled link is not added to the pipeline, > > and neither is the local pad. While the former is correct as disabled > > links should not be followed, not adding the local pad breaks processing > > of the MEDIA_PAD_FL_MUST_CONNECT flag. > > > > The MEDIA_PAD_FL_MUST_CONNECT flag is checked in the > > __media_pipeline_start() function that iterates over all pads after > > populating the pipeline. If the pad is not present, the check gets > > skipped, rendering it useless. > > > > Fix this by adding the local pad of all links regardless of their state, > > only skipping the remote pad for disabled links. > > > > Cc: Alexander Shiyan <eagle.alexander923@gmail.com> > > Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()") > > Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > Closes: https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Could you add Cc: stable when sending the PR? This should be applied to 6.1 > and later. I'll do so, far all patches in the series.
Hello, This patch series fixes mishandling of the MEDIA_PAD_FL_MUST_CONNECT flag. The issue has been introduced in commit ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()") when reworking the media controller pipeline validation code. It is known to cause crashes in the imx-mipi-csis driver (see [1]) and most likely affects multiple other drivers. Patch 1/7 and 2/7 fix the problem. Patches 3/7 to 7/7 fix an additional related issue affecting the imx8-isi driver. It is however not a regression introduced by commit ae219872834a. The problem causes a NULL pointer dereference when enabling streams if the pipeline is configured with the imx8-isi crossbar sink pad not being connected. Patch 3/7, initially submitted by Marek, fixes the issue in the imx8-isi driver itself. As the problem is very similar to what the MUST_CONNECT flag was designed to handled, I considered the bug to be better solved by using the MUST_CONNECT flag in the imx8-isi driver. This requires expanding the purpose of the flag (patch 6/7), after a few drive-by fixes and reworks (patches 4/7 and 5/7). Finally, patch 7/7 sets the MUST_CONNECT flag in the imx8-isi driver, effectively reverting the changes from patch 3/7. I have decided to still include patch 3/7 in the series, as in the even that the changes in patches 4/7 to 7/7 were to be reverted later (or simply require more discussions and new versions), patches 1/7 to 3/7 could be merged without delay and fix the issue for users. Compared to v1, minor review comments have been taken into account. As the approach seems to be generally welcome, I plan to send a pull request once v6.8-rc1 will be merged in the media tree. [1] https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de Laurent Pinchart (6): media: mc: Add local pad to pipeline regardless of the link state media: mc: Fix flags handling when creating pad links media: mc: Add num_links flag to media_pad media: mc: Rename pad variable to clarify intent media: mc: Expand MUST_CONNECT flag to always require an enabled link media: nxp: imx8-isi: Mark all crossbar sink pads as MUST_CONNECT Marek Vasut (1): media: nxp: imx8-isi: Check whether crossbar pad is non-NULL before access .../media/mediactl/media-types.rst | 11 +-- drivers/media/mc/mc-entity.c | 93 ++++++++++++++----- .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 4 +- include/media/media-entity.h | 2 + 4 files changed, 78 insertions(+), 32 deletions(-) base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a