| Message ID | 20240128-fix-clang-warnings-v1-3-1d946013a421@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: Fix warnings building with LLVM=1 | expand |
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. If handler is called through vpu_ipi_register() indirectly (which it obviously is if it is being passed down), there will be a CFI failure because the type of mtk_vcodec_ipi_handler is not the same as ipi_handler_t, as the comment mentions. Has this seen testing on real hardware with kCFI? > --- > drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c > index 9f6e4b59455d..781a0359afdc 100644 > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c > @@ -33,9 +33,11 @@ static int mtk_vcodec_vpu_set_ipi_register(struct mtk_vcodec_fw *fw, int id, > * The handler we receive takes a void * as its first argument. We > * cannot change this because it needs to be passed down to the rproc > * subsystem when SCP is used. VPU takes a const argument, which is > - * more constrained, so the conversion below is safe. > + * more constrained, so the conversion below is safe. We use the void > + * casting, to convince clang with -Wcast-function-type-sctrict that > + * this is safe. > */ > - ipi_handler_t handler_const = (ipi_handler_t)handler; > + ipi_handler_t handler_const = (ipi_handler_t)((void *)handler); > > return vpu_ipi_register(fw->pdev, id, handler_const, name, priv); > } > > -- > 2.43.0.429.g432eaa2c6b-goog >
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). Sami
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. Cheers, Angelo
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. Thanks, Nícolas
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. https://mirrors.edge.kernel.org/pub/tools/llvm/ Cheers, Nathan
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
diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c index 9f6e4b59455d..781a0359afdc 100644 --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c @@ -33,9 +33,11 @@ static int mtk_vcodec_vpu_set_ipi_register(struct mtk_vcodec_fw *fw, int id, * The handler we receive takes a void * as its first argument. We * cannot change this because it needs to be passed down to the rproc * subsystem when SCP is used. VPU takes a const argument, which is - * more constrained, so the conversion below is safe. + * more constrained, so the conversion below is safe. We use the void + * casting, to convince clang with -Wcast-function-type-sctrict that + * this is safe. */ - ipi_handler_t handler_const = (ipi_handler_t)handler; + ipi_handler_t handler_const = (ipi_handler_t)((void *)handler); return vpu_ipi_register(fw->pdev, id, handler_const, name, priv); }
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> --- drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)