[PATCH 0/3] media: Fix warnings building with LLVM=1

[PATCH 0/3] media: Fix warnings building with LLVM=1

[Ricardo Ribalda]

LLVM does check -Wcast-function-type-sctrict, which is triggered in a
couple of places in the media subsystem.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (3):
      media: pci: sta2x11: Fix Wcast-function-type-strict warnings
      media: usb: pvrusb2: Fix Wcast-function-type-strict warnings
      media: mediatek: vcodedc: Fix Wcast-function-type-strict warnings

 drivers/media/pci/sta2x11/sta2x11_vip.c                           | 5 +++--
 drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 6 ++++--
 drivers/media/usb/pvrusb2/pvrusb2-context.c                       | 5 +++--
 drivers/media/usb/pvrusb2/pvrusb2-dvb.c                           | 7 ++++---
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c                          | 7 ++++---
 5 files changed, 18 insertions(+), 12 deletions(-)
---
base-commit: 615d300648869c774bd1fe54b4627bb0c20faed4
change-id: 20240128-fix-clang-warnings-6c12920467cb

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH 1/3] media: pci: sta2x11: Fix Wcast-function-type-strict warnings

[Ricardo Ribalda]

Building with LLVM=1 throws the following warning:
drivers/media/pci/sta2x11/sta2x11_vip.c:1057:6: warning: cast from 'irqreturn_t (*)(int, struct sta2x11_vip *)' (aka 'enum irqreturn (*)(int, struct sta2x11_vip *)') to 'irq_handler_t' (aka 'enum irqreturn (*)(int, void *)') converts to incompatible function type [-Wcast-function-type-strict]

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/pci/sta2x11/sta2x11_vip.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
index e4cf9d63e926..0a3827575753 100644
--- a/drivers/media/pci/sta2x11/sta2x11_vip.c
+++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
@@ -757,7 +757,7 @@ static const struct video_device video_dev_template = {
 /**
  * vip_irq - interrupt routine
  * @irq: Number of interrupt ( not used, correct number is assumed )
- * @vip: local data structure containing all information
+ * @data: local data structure containing all information
  *
  * check for both frame interrupts set ( top and bottom ).
  * check FIFO overflow, but limit number of log messages after open.
@@ -767,9 +767,10 @@ static const struct video_device video_dev_template = {
  *
  * IRQ_HANDLED, interrupt done.
  */
-static irqreturn_t vip_irq(int irq, struct sta2x11_vip *vip)
+static irqreturn_t vip_irq(int irq, void *data)
 {
 	unsigned int status;
+	struct sta2x11_vip *vip = data;
 
 	status = reg_read(vip, DVP_ITS);
 

-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 1/3] media: pci: sta2x11: Fix Wcast-function-type-strict warnings

[Nathan Chancellor]

On Sun, Jan 28, 2024 at 02:12:20AM +0000, Ricardo Ribalda wrote:
> Building with LLVM=1 throws the following warning:
> drivers/media/pci/sta2x11/sta2x11_vip.c:1057:6: warning: cast from 'irqreturn_t (*)(int, struct sta2x11_vip *)' (aka 'enum irqreturn (*)(int, struct sta2x11_vip *)') to 'irq_handler_t' (aka 'enum irqreturn (*)(int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

I wonder if the media tree cares about reverse Christmas tree order for
variables?

> ---
>  drivers/media/pci/sta2x11/sta2x11_vip.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
> index e4cf9d63e926..0a3827575753 100644
> --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
> +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
> @@ -757,7 +757,7 @@ static const struct video_device video_dev_template = {
>  /**
>   * vip_irq - interrupt routine
>   * @irq: Number of interrupt ( not used, correct number is assumed )
> - * @vip: local data structure containing all information
> + * @data: local data structure containing all information
>   *
>   * check for both frame interrupts set ( top and bottom ).
>   * check FIFO overflow, but limit number of log messages after open.
> @@ -767,9 +767,10 @@ static const struct video_device video_dev_template = {
>   *
>   * IRQ_HANDLED, interrupt done.
>   */
> -static irqreturn_t vip_irq(int irq, struct sta2x11_vip *vip)
> +static irqreturn_t vip_irq(int irq, void *data)
>  {
>  	unsigned int status;
> +	struct sta2x11_vip *vip = data;
>  
>  	status = reg_read(vip, DVP_ITS);
>  
> 
> -- 
> 2.43.0.429.g432eaa2c6b-goog
> 

[PATCH 2/3] media: usb: pvrusb2: Fix Wcast-function-type-strict warnings

[Ricardo Ribalda]

Building with LLVM=1 throws the following warning:
drivers/media/usb/pvrusb2/pvrusb2-context.c:110:6: warning: cast from 'void (*)(struct pvr2_context *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:1070:30: warning: cast from 'void (*)(struct pvr2_v4l2_fh *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] drivers/media/usb/pvrusb2/pvrusb2-dvb.c:152:6: warning: cast from 'void (*)(struct pvr2_dvb_adapter *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/pvrusb2/pvrusb2-context.c | 5 +++--
 drivers/media/usb/pvrusb2/pvrusb2-dvb.c     | 7 ++++---
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c    | 7 ++++---
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
index 1764674de98b..16edabda191c 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
@@ -90,8 +90,9 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
 }
 
 
-static void pvr2_context_notify(struct pvr2_context *mp)
+static void pvr2_context_notify(void *data)
 {
+	struct pvr2_context *mp = data;
 	pvr2_context_set_notify(mp,!0);
 }
 
@@ -107,7 +108,7 @@ static void pvr2_context_check(struct pvr2_context *mp)
 			   "pvr2_context %p (initialize)", mp);
 		/* Finish hardware initialization */
 		if (pvr2_hdw_initialize(mp->hdw,
-					(void (*)(void *))pvr2_context_notify,
+					pvr2_context_notify,
 					mp)) {
 			mp->video_stream.stream =
 				pvr2_hdw_get_video_stream(mp->hdw);
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
index 26811efe0fb5..8b9f1a09bd53 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
@@ -88,8 +88,9 @@ static int pvr2_dvb_feed_thread(void *data)
 	return stat;
 }
 
-static void pvr2_dvb_notify(struct pvr2_dvb_adapter *adap)
+static void pvr2_dvb_notify(void *data)
 {
+	struct pvr2_dvb_adapter *adap = data;
 	wake_up(&adap->buffer_wait_data);
 }
 
@@ -148,8 +149,8 @@ static int pvr2_dvb_stream_do_start(struct pvr2_dvb_adapter *adap)
 		if (!(adap->buffer_storage[idx])) return -ENOMEM;
 	}
 
-	pvr2_stream_set_callback(pvr->video_stream.stream,
-				 (pvr2_stream_callback) pvr2_dvb_notify, adap);
+	pvr2_stream_set_callback(pvr->video_stream.stream, pvr2_dvb_notify,
+				 adap);
 
 	ret = pvr2_stream_set_buffer_count(stream, PVR2_DVB_BUFFER_COUNT);
 	if (ret < 0) return ret;
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index c04ab7258d64..590f80949bbf 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -1032,9 +1032,10 @@ static int pvr2_v4l2_open(struct file *file)
 	return 0;
 }
 
-
-static void pvr2_v4l2_notify(struct pvr2_v4l2_fh *fhp)
+static void pvr2_v4l2_notify(void *data)
 {
+	struct pvr2_v4l2_fh *fhp = data;
+
 	wake_up(&fhp->wait_data);
 }
 
@@ -1067,7 +1068,7 @@ static int pvr2_v4l2_iosetup(struct pvr2_v4l2_fh *fh)
 
 	hdw = fh->channel.mc_head->hdw;
 	sp = fh->pdi->stream->stream;
-	pvr2_stream_set_callback(sp,(pvr2_stream_callback)pvr2_v4l2_notify,fh);
+	pvr2_stream_set_callback(sp, pvr2_v4l2_notify, fh);
 	pvr2_hdw_set_stream_type(hdw,fh->pdi->config);
 	if ((ret = pvr2_hdw_set_streaming(hdw,!0)) < 0) return ret;
 	return pvr2_ioread_set_enabled(fh->rhp,!0);

-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 2/3] media: usb: pvrusb2: Fix Wcast-function-type-strict warnings

[Nathan Chancellor]

On Sun, Jan 28, 2024 at 02:12:21AM +0000, Ricardo Ribalda wrote:
> Building with LLVM=1 throws the following warning:
> drivers/media/usb/pvrusb2/pvrusb2-context.c:110:6: warning: cast from 'void (*)(struct pvr2_context *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:1070:30: warning: cast from 'void (*)(struct pvr2_v4l2_fh *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] drivers/media/usb/pvrusb2/pvrusb2-dvb.c:152:6: warning: cast from 'void (*)(struct pvr2_dvb_adapter *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/media/usb/pvrusb2/pvrusb2-context.c | 5 +++--
>  drivers/media/usb/pvrusb2/pvrusb2-dvb.c     | 7 ++++---
>  drivers/media/usb/pvrusb2/pvrusb2-v4l2.c    | 7 ++++---
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> index 1764674de98b..16edabda191c 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> @@ -90,8 +90,9 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
>  }
>  
>  
> -static void pvr2_context_notify(struct pvr2_context *mp)
> +static void pvr2_context_notify(void *data)
>  {
> +	struct pvr2_context *mp = data;
>  	pvr2_context_set_notify(mp,!0);
>  }
>  
> @@ -107,7 +108,7 @@ static void pvr2_context_check(struct pvr2_context *mp)
>  			   "pvr2_context %p (initialize)", mp);
>  		/* Finish hardware initialization */
>  		if (pvr2_hdw_initialize(mp->hdw,
> -					(void (*)(void *))pvr2_context_notify,
> +					pvr2_context_notify,
>  					mp)) {
>  			mp->video_stream.stream =
>  				pvr2_hdw_get_video_stream(mp->hdw);
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
> index 26811efe0fb5..8b9f1a09bd53 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
> @@ -88,8 +88,9 @@ static int pvr2_dvb_feed_thread(void *data)
>  	return stat;
>  }
>  
> -static void pvr2_dvb_notify(struct pvr2_dvb_adapter *adap)
> +static void pvr2_dvb_notify(void *data)
>  {
> +	struct pvr2_dvb_adapter *adap = data;
>  	wake_up(&adap->buffer_wait_data);
>  }
>  
> @@ -148,8 +149,8 @@ static int pvr2_dvb_stream_do_start(struct pvr2_dvb_adapter *adap)
>  		if (!(adap->buffer_storage[idx])) return -ENOMEM;
>  	}
>  
> -	pvr2_stream_set_callback(pvr->video_stream.stream,
> -				 (pvr2_stream_callback) pvr2_dvb_notify, adap);
> +	pvr2_stream_set_callback(pvr->video_stream.stream, pvr2_dvb_notify,
> +				 adap);
>  
>  	ret = pvr2_stream_set_buffer_count(stream, PVR2_DVB_BUFFER_COUNT);
>  	if (ret < 0) return ret;
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> index c04ab7258d64..590f80949bbf 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> @@ -1032,9 +1032,10 @@ static int pvr2_v4l2_open(struct file *file)
>  	return 0;
>  }
>  
> -
> -static void pvr2_v4l2_notify(struct pvr2_v4l2_fh *fhp)
> +static void pvr2_v4l2_notify(void *data)
>  {
> +	struct pvr2_v4l2_fh *fhp = data;
> +
>  	wake_up(&fhp->wait_data);
>  }
>  
> @@ -1067,7 +1068,7 @@ static int pvr2_v4l2_iosetup(struct pvr2_v4l2_fh *fh)
>  
>  	hdw = fh->channel.mc_head->hdw;
>  	sp = fh->pdi->stream->stream;
> -	pvr2_stream_set_callback(sp,(pvr2_stream_callback)pvr2_v4l2_notify,fh);
> +	pvr2_stream_set_callback(sp, pvr2_v4l2_notify, fh);
>  	pvr2_hdw_set_stream_type(hdw,fh->pdi->config);
>  	if ((ret = pvr2_hdw_set_streaming(hdw,!0)) < 0) return ret;
>  	return pvr2_ioread_set_enabled(fh->rhp,!0);
> 
> -- 
> 2.43.0.429.g432eaa2c6b-goog
> 

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

[Ricardo Ribalda]

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(-)

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

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

[Nathan Chancellor]

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
> 

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

[Sami Tolvanen]

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

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

[AngeloGioacchino Del Regno]

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

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

[Nícolas F. R. A. Prado]

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

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

[Nathan Chancellor]

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

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

[Nícolas F. R. A. Prado]

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