Re: [PATCH 3/3] media: mediatek: vcodedc: Fix Wcast-function-type-strict warnings

["Nícolas F. R. A. Prado" <nfraprado@collabora.com>]

On Sun, Feb 04, 2024 at 01:55:49PM -0700, Nathan Chancellor wrote:
> On Fri, Feb 02, 2024 at 03:15:46PM -0500, Nícolas F. R. A. Prado wrote:
> > On Fri, Feb 02, 2024 at 01:58:05PM +0100, AngeloGioacchino Del Regno wrote:
> > > Il 01/02/24 23:25, Sami Tolvanen ha scritto:
> > > > On Thu, Feb 1, 2024 at 10:17 PM Nathan Chancellor <nathan@kernel.org> wrote:
> > > > > 
> > > > > On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
> > > > > > Building with LLVM=1 throws the following warning:
> > > > > > drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> > > > > > 
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > 
> > > > > I am not positive because I don't have any hardware to test this driver
> > > > > but I suspect this patch is just hiding the warning without actually
> > > > > addressing the issue that it is pointing out.
> > > > 
> > > > Agreed, this won't fix the issue. The correct solution is to drop the
> > > > cast and change the handler type to match the pointer type (i.e. use
> > > > const void* for the first argument).
> > > > 
> > > 
> > > Even though I agree that the correct solution is to change the handler's type,
> > > I think that having a test on the actual hardware done is still valuable.
> > > 
> > > We scheduled a job on KernelCI to test this commit on our integration kernel,
> > > you'll get results for ChromeOS' tast decoders (MT8195 only) and Fluster tests
> > > on MT8183/8186/8192/8195.
> > > 
> > > 
> > > The results should be available in a couple of hours here, relative to
> > > commit `49955a84129dbe1f94fedf729690efcf28513828` on our tree:
> > > https://chromeos.kernelci.org/job/collabora-chromeos-kernel/branch/for-kernelci/
> > > 
> > > P.S.: If they don't, feel free to ping me or Nicolas (added to the loop) about it.
> > 
> > Hi,
> > 
> > the results are available at 
> > 
> > https://chromeos.kernelci.org/test/job/collabora-chromeos-kernel/branch/for-kernelci/kernel/v6.8-rc2-3109-g49955a84129d/
> > 
> > (You need to type "decoder" into the search bar to limit the results to only the
> > decoder tests)
> > 
> > The only regressions I see are due to infrastructure error or broken test
> > unrelated to this change (v4l2-decoder-conformance-h264-frext test on
> > MT8195-Tomato, and cros-tast-decoder-v4l2-sl-h264 test on MT8183-Juniper)
> > 
> > Otherwise, all platforms (MT8183/8186/8192/8195) and video codecs
> > (VP8/VP9/H264/H265/AV1) seem unaffected.
> > 
> > Note that these are GCC builds.
> 
> Thank you for running the tests to make sure this series does not
> regress anything. If possible, it would be good to try and build with
> LLVM and enable kernel Control Flow Integrity (kCFI, CONFIG_CFI_CLANG)
> to see if my theory that this is currently broken is correct. I have
> prebuilt LLVM toolchains on kernel.org but if it is too much of a
> hassle, I would not worry about it.

We don't have a way in KernelCI to run a one-off test, instead we need to make a
PR adding a build definition that will be triggered for every update on a given
tree. The results I reported above were for configurations that we already had
in place.

That said, maybe we should be running with LLVM and CONFIG_CFI_CLANG enabled on
a regular basis, if you think it is useful as a general tool for identifying
issues. I'd also be interested in hearing more from you on what other kind of
clang configs you think might be good to enable in CI to provide value to the
community.

Thanks,
Nícolas