| Message ID | 20240507-cocci-flexarray-v2-0-7aea262cf065@chromium.org |
|---|---|
| Headers | show |
| Series | media: Fix the last set of coccinelle warnings | expand |
On Tue, 07 May 2024 16:27:06 +0000, Ricardo Ribalda wrote: > Replace all the single elements arrays with the element itself. > > Pahole shows the same padding and alignment for x86 and arm in both > situations. > > This fixes this cocci warning: > drivers/media/platform/allegro-dvt/nal-hevc.h:102:14-22: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) Thanks for the patch. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/platform/allegro-dvt/allegro-core.c | 6 +++--- > drivers/media/platform/allegro-dvt/nal-hevc.c | 11 +++-------- > drivers/media/platform/allegro-dvt/nal-hevc.h | 6 +++--- > 3 files changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/allegro-dvt/allegro-core.c b/drivers/media/platform/allegro-dvt/allegro-core.c > index da61f9beb6b4..369bd88cc0ae 100644 > --- a/drivers/media/platform/allegro-dvt/allegro-core.c > +++ b/drivers/media/platform/allegro-dvt/allegro-core.c > @@ -1852,14 +1852,14 @@ static ssize_t allegro_hevc_write_sps(struct allegro_channel *channel, > hrd->dpb_output_delay_length_minus1 = 30; > > hrd->bit_rate_scale = ffs(channel->bitrate_peak) - 6; > - hrd->vcl_hrd[0].bit_rate_value_minus1[0] = > + hrd->vcl_hrd[0].bit_rate_value_minus1 = > (channel->bitrate_peak >> (6 + hrd->bit_rate_scale)) - 1; > > cpb_size = v4l2_ctrl_g_ctrl(channel->mpeg_video_cpb_size) * 1000; > hrd->cpb_size_scale = ffs(cpb_size) - 4; > - hrd->vcl_hrd[0].cpb_size_value_minus1[0] = (cpb_size >> (4 + hrd->cpb_size_scale)) - 1; > + hrd->vcl_hrd[0].cpb_size_value_minus1 = (cpb_size >> (4 + hrd->cpb_size_scale)) - 1; > > - hrd->vcl_hrd[0].cbr_flag[0] = !v4l2_ctrl_g_ctrl(channel->mpeg_video_frame_rc_enable); > + hrd->vcl_hrd[0].cbr_flag = !v4l2_ctrl_g_ctrl(channel->mpeg_video_frame_rc_enable); > > size = nal_hevc_write_sps(&dev->plat_dev->dev, dest, n, sps); > > diff --git a/drivers/media/platform/allegro-dvt/nal-hevc.c b/drivers/media/platform/allegro-dvt/nal-hevc.c > index 9cdf2756e0a3..575089522df5 100644 > --- a/drivers/media/platform/allegro-dvt/nal-hevc.c > +++ b/drivers/media/platform/allegro-dvt/nal-hevc.c > @@ -210,14 +210,9 @@ static void nal_hevc_rbsp_vps(struct rbsp *rbsp, struct nal_hevc_vps *vps) > static void nal_hevc_rbsp_sub_layer_hrd_parameters(struct rbsp *rbsp, > struct nal_hevc_sub_layer_hrd_parameters *hrd) > { > - unsigned int i; > - unsigned int cpb_cnt = 1; > - > - for (i = 0; i < cpb_cnt; i++) { > - rbsp_uev(rbsp, &hrd->bit_rate_value_minus1[i]); > - rbsp_uev(rbsp, &hrd->cpb_size_value_minus1[i]); > - rbsp_bit(rbsp, &hrd->cbr_flag[i]); > - } > + rbsp_uev(rbsp, &hrd->bit_rate_value_minus1); > + rbsp_uev(rbsp, &hrd->cpb_size_value_minus1); > + rbsp_bit(rbsp, &hrd->cbr_flag); > } > > static void nal_hevc_rbsp_hrd_parameters(struct rbsp *rbsp, > diff --git a/drivers/media/platform/allegro-dvt/nal-hevc.h b/drivers/media/platform/allegro-dvt/nal-hevc.h > index eb46f12aae80..afa7a9d7d654 100644 > --- a/drivers/media/platform/allegro-dvt/nal-hevc.h > +++ b/drivers/media/platform/allegro-dvt/nal-hevc.h > @@ -97,9 +97,9 @@ struct nal_hevc_vps { > }; > > struct nal_hevc_sub_layer_hrd_parameters { > - unsigned int bit_rate_value_minus1[1]; > - unsigned int cpb_size_value_minus1[1]; > - unsigned int cbr_flag[1]; > + unsigned int bit_rate_value_minus1; > + unsigned int cpb_size_value_minus1; > + unsigned int cbr_flag; The struct is modeled after the specification in ITU-T H.265, which defines the fields as arrays. It's a limitation of the current implementation that only a single element is supported. Maybe replacing the hard coded values with a constant would be more appropriate to document this limitation. Michael > }; > > struct nal_hevc_hrd_parameters { > > -- > 2.45.0.rc1.225.g2a3ae87e7f-goog > >
Hi Michael On Wed, 8 May 2024 at 10:53, Michael Tretter <m.tretter@pengutronix.de> wrote: > > On Tue, 07 May 2024 16:27:06 +0000, Ricardo Ribalda wrote: > > Replace all the single elements arrays with the element itself. > > > > Pahole shows the same padding and alignment for x86 and arm in both > > situations. > > > > This fixes this cocci warning: > > drivers/media/platform/allegro-dvt/nal-hevc.h:102:14-22: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > > Thanks for the patch. > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/platform/allegro-dvt/allegro-core.c | 6 +++--- > > drivers/media/platform/allegro-dvt/nal-hevc.c | 11 +++-------- > > drivers/media/platform/allegro-dvt/nal-hevc.h | 6 +++--- > > 3 files changed, 9 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/platform/allegro-dvt/allegro-core.c b/drivers/media/platform/allegro-dvt/allegro-core.c > > index da61f9beb6b4..369bd88cc0ae 100644 > > --- a/drivers/media/platform/allegro-dvt/allegro-core.c > > +++ b/drivers/media/platform/allegro-dvt/allegro-core.c > > @@ -1852,14 +1852,14 @@ static ssize_t allegro_hevc_write_sps(struct allegro_channel *channel, > > hrd->dpb_output_delay_length_minus1 = 30; > > > > hrd->bit_rate_scale = ffs(channel->bitrate_peak) - 6; > > - hrd->vcl_hrd[0].bit_rate_value_minus1[0] = > > + hrd->vcl_hrd[0].bit_rate_value_minus1 = > > (channel->bitrate_peak >> (6 + hrd->bit_rate_scale)) - 1; > > > > cpb_size = v4l2_ctrl_g_ctrl(channel->mpeg_video_cpb_size) * 1000; > > hrd->cpb_size_scale = ffs(cpb_size) - 4; > > - hrd->vcl_hrd[0].cpb_size_value_minus1[0] = (cpb_size >> (4 + hrd->cpb_size_scale)) - 1; > > + hrd->vcl_hrd[0].cpb_size_value_minus1 = (cpb_size >> (4 + hrd->cpb_size_scale)) - 1; > > > > - hrd->vcl_hrd[0].cbr_flag[0] = !v4l2_ctrl_g_ctrl(channel->mpeg_video_frame_rc_enable); > > + hrd->vcl_hrd[0].cbr_flag = !v4l2_ctrl_g_ctrl(channel->mpeg_video_frame_rc_enable); > > > > size = nal_hevc_write_sps(&dev->plat_dev->dev, dest, n, sps); > > > > diff --git a/drivers/media/platform/allegro-dvt/nal-hevc.c b/drivers/media/platform/allegro-dvt/nal-hevc.c > > index 9cdf2756e0a3..575089522df5 100644 > > --- a/drivers/media/platform/allegro-dvt/nal-hevc.c > > +++ b/drivers/media/platform/allegro-dvt/nal-hevc.c > > @@ -210,14 +210,9 @@ static void nal_hevc_rbsp_vps(struct rbsp *rbsp, struct nal_hevc_vps *vps) > > static void nal_hevc_rbsp_sub_layer_hrd_parameters(struct rbsp *rbsp, > > struct nal_hevc_sub_layer_hrd_parameters *hrd) > > { > > - unsigned int i; > > - unsigned int cpb_cnt = 1; > > - > > - for (i = 0; i < cpb_cnt; i++) { > > - rbsp_uev(rbsp, &hrd->bit_rate_value_minus1[i]); > > - rbsp_uev(rbsp, &hrd->cpb_size_value_minus1[i]); > > - rbsp_bit(rbsp, &hrd->cbr_flag[i]); > > - } > > + rbsp_uev(rbsp, &hrd->bit_rate_value_minus1); > > + rbsp_uev(rbsp, &hrd->cpb_size_value_minus1); > > + rbsp_bit(rbsp, &hrd->cbr_flag); > > } > > > > static void nal_hevc_rbsp_hrd_parameters(struct rbsp *rbsp, > > diff --git a/drivers/media/platform/allegro-dvt/nal-hevc.h b/drivers/media/platform/allegro-dvt/nal-hevc.h > > index eb46f12aae80..afa7a9d7d654 100644 > > --- a/drivers/media/platform/allegro-dvt/nal-hevc.h > > +++ b/drivers/media/platform/allegro-dvt/nal-hevc.h > > @@ -97,9 +97,9 @@ struct nal_hevc_vps { > > }; > > > > struct nal_hevc_sub_layer_hrd_parameters { > > - unsigned int bit_rate_value_minus1[1]; > > - unsigned int cpb_size_value_minus1[1]; > > - unsigned int cbr_flag[1]; > > + unsigned int bit_rate_value_minus1; > > + unsigned int cpb_size_value_minus1; > > + unsigned int cbr_flag; > > The struct is modeled after the specification in ITU-T H.265, which > defines the fields as arrays. It's a limitation of the current > implementation that only a single element is supported. > > Maybe replacing the hard coded values with a constant would be more > appropriate to document this limitation. A define seems to convince coccinelle of our intentions :). I will upload the fix in v3 diff --git a/drivers/media/platform/allegro-dvt/nal-hevc.h b/drivers/media/platform/allegro-dvt/nal-hevc.h index eb46f12aae80..361e2f55c254 100644 --- a/drivers/media/platform/allegro-dvt/nal-hevc.h +++ b/drivers/media/platform/allegro-dvt/nal-hevc.h @@ -96,10 +96,11 @@ struct nal_hevc_vps { unsigned int extension_data_flag; }; +#define N_HRD_PARAMS 1 struct nal_hevc_sub_layer_hrd_parameters { - unsigned int bit_rate_value_minus1[1]; - unsigned int cpb_size_value_minus1[1]; - unsigned int cbr_flag[1]; + unsigned int bit_rate_value_minus1[N_HRD_PARAMS]; + unsigned int cpb_size_value_minus1[N_HRD_PARAMS]; + unsigned int cbr_flag[N_HRD_PARAMS]; }; struct nal_hevc_hrd_parameters { Thanks. > > Michael > > > }; > > > > struct nal_hevc_hrd_parameters { > > > > -- > > 2.45.0.rc1.225.g2a3ae87e7f-goog > > > > -- Ricardo Ribalda
On 07/05/2024 17:27, Ricardo Ribalda wrote: > This structures are not used, and have a single element array at the end > of them. Remove them. > > This fix the following cocci warnings: > drivers/media/platform/qcom/venus/hfi_helper.h:764:5-15: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_helper.h:1041:5-15: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_helper.h:1088:39-51: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_helper.h:1093:5-22: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_helper.h:1144:4-8: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_helper.h:1239:4-8: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_helper.h:1272:4-13: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_cmds.h:85:5-16: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_cmds.h:180:5-9: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > drivers/media/platform/qcom/venus/hfi_cmds.h:189:5-9: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> There's nothing inherently wrong with defining a protocol upfront in the form of a structure for future expansion. These structures are documentary of the host <-> firmware interface and are of use when implementing new features. I think these structures should just have the "[1] -> []" conversion done and be retained instead. --- bod
On 07/05/2024 17:27, Ricardo Ribalda wrote: > This field is never used, but if we remove it we would change the size > of the struct and can lead to behavior change. Stay on the safe side by > replacing the single element array with a single element field. > > This fixes the following cocci warning: > drivers/media/platform/qcom/venus/hfi_helper.h:1003:43-60: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/platform/qcom/venus/hfi_helper.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h > index 7c0edef263ae..eb0a4c64b7ef 100644 > --- a/drivers/media/platform/qcom/venus/hfi_helper.h > +++ b/drivers/media/platform/qcom/venus/hfi_helper.h > @@ -1000,7 +1000,7 @@ struct hfi_uncompressed_plane_constraints { > struct hfi_uncompressed_plane_info { > u32 format; > u32 num_planes; > - struct hfi_uncompressed_plane_constraints plane_constraints[1]; > + struct hfi_uncompressed_plane_constraints plane_constraints; > }; > > struct hfi_uncompressed_format_supported { > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 10/05/2024 00:35, Bryan O'Donoghue wrote: > I think these structures should just have the "[1] -> []" conversion > done and be retained instead. They won't have the same sizeof() then so ignore that thought. I still would suggest dropping the `something[1]` in favour of `something` because these structures document the protocol between host and firmware and therefore are useful even if unused in the code. --- bod
On 07/05/2024 17:27, Ricardo Ribalda wrote: > The single element array data[1] is never used. Replace it whit a > padding field of the same size. > > This fixes the following cocci error: > drivers/media/platform/qcom/venus/hfi_cmds.h:163:5-9: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h > index e1dd0ea2be1a..15271b3f2b49 100644 > --- a/drivers/media/platform/qcom/venus/hfi_cmds.h > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h > @@ -160,7 +160,7 @@ struct hfi_session_empty_buffer_uncompressed_plane0_pkt { > u32 input_tag; > u32 packet_buffer; > u32 extradata_buffer; > - u32 data[1]; > + u32 padding; > }; > > struct hfi_session_fill_buffer_pkt { > Its not padding - which is what we mean when we want to align something to a boundary - its data that we don't currently use. Please retain the namespace and do data[1] -> data. Once done. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 07/05/2024 17:27, Ricardo Ribalda wrote: > The single element array data[1] is never used. Replace it with a > padding field of the same size. > > This fixes the following cocci warning: > drivers/media/platform/qcom/venus/hfi_cmds.h:146:5-9: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h > index 15271b3f2b49..02e9a073d0c1 100644 > --- a/drivers/media/platform/qcom/venus/hfi_cmds.h > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h > @@ -143,7 +143,7 @@ struct hfi_session_empty_buffer_compressed_pkt { > u32 input_tag; > u32 packet_buffer; > u32 extradata_buffer; > - u32 data[1]; > + u32 padding; > }; > > struct hfi_session_empty_buffer_uncompressed_plane0_pkt { > Same comment as previous patch. `data` is what we use in this driver's namespace not padding and the protocol structures enumerate the content of the payload as data not padding. u32 data; Then Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- bod
On 07/05/2024 17:27, Ricardo Ribalda wrote: > - u32 data[1]; > + u32 extradata_size; A correct functional change but again please keep the name to `data` not `extradata_size` Then add Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- bod
Hi Bryan Thanks for your review On Fri, 10 May 2024 at 01:56, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 10/05/2024 00:35, Bryan O'Donoghue wrote: > > I think these structures should just have the "[1] -> []" conversion > > done and be retained instead. > > They won't have the same sizeof() then so ignore that thought. > > I still would suggest dropping the `something[1]` in favour of > `something` because these structures document the protocol between host > and firmware and therefore are useful even if unused in the code. The structures will be in the git log for the rest of the days. So if someone has to use them, they can recover them from there. Right now, they are not used and they are triggering a warning. I would argue that untested code is broken code. I'd rather remove the code. > > --- > bod
With this set we are done with all the cocci warning/errors. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v2: - Fix build error in ppc for siano - Link to v1: https://lore.kernel.org/r/20240507-cocci-flexarray-v1-0-4a421c21fd06@chromium.org --- Ricardo Ribalda (18): media: allegro: nal-hevc: Refactor nal_hevc_sub_layer_hrd_parameters media: xilinx: Refactor struct xvip_dma media: dvb-frontend/mxl5xx: Refactor struct MBIN_FILE_T media: dvb-frontend/mxl5xx: Use flex array for MBIN_SEGMENT_T media: pci: cx18: Use flex arrays for struct cx18_scb media: siano: Refactor struct sms_msg_data media: siano: Remove unused structures media: siano: Use flex arrays for sms_firmware media: venus: Remove unused structs media: venus: Use flex array for hfi_session_release_buffer_pkt media: venus: Refactor struct hfi_uncompressed_plane_info media: venus: Refactor struct hfi_session_get_property_pkt media: venus: Refactor struct hfi_uncompressed_format_supported media: venus: Refactor hfi_session_empty_buffer_uncompressed_plane0_pkt media: venus: Refactor hfi_session_empty_buffer_compressed_pkt media: venus: Refactor hfi_sys_get_property_pkt media: venus: Refactor hfi_session_fill_buffer_pkt media: venus: Refactor hfi_buffer_alloc_mode_supported drivers/media/common/siano/smscoreapi.c | 10 ++--- drivers/media/common/siano/smscoreapi.h | 18 +-------- drivers/media/common/siano/smsdvb-main.c | 4 +- drivers/media/common/siano/smsendian.c | 8 ++-- drivers/media/dvb-frontends/mxl5xx.c | 2 +- drivers/media/dvb-frontends/mxl5xx_defs.h | 4 +- drivers/media/pci/cx18/cx18-scb.h | 2 +- drivers/media/platform/allegro-dvt/allegro-core.c | 6 +-- drivers/media/platform/allegro-dvt/nal-hevc.c | 11 ++---- drivers/media/platform/allegro-dvt/nal-hevc.h | 6 +-- drivers/media/platform/qcom/venus/hfi_cmds.c | 16 ++++---- drivers/media/platform/qcom/venus/hfi_cmds.h | 46 +++++------------------ drivers/media/platform/qcom/venus/hfi_helper.h | 45 ++-------------------- drivers/media/platform/qcom/venus/hfi_parser.c | 2 +- drivers/media/platform/qcom/venus/hfi_venus.c | 2 +- drivers/media/platform/xilinx/xilinx-dma.c | 4 +- drivers/media/platform/xilinx/xilinx-dma.h | 2 +- 17 files changed, 53 insertions(+), 135 deletions(-) --- base-commit: e695668af8523b059127dfa8b261c76e7c9cde10 change-id: 20240507-cocci-flexarray-9a807a8e108e Best regards,