| Message ID | 20240415-fix-cocci-v1-0-477afb23728b@chromium.org |
|---|---|
| Headers | show |
| Series | media: Fix coccinelle warning/errors | expand |
Hi Ricardo, Thanks a lot. Reviewed-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> On 4/15/24 21:34, Ricardo Ribalda wrote: > link_freq does not fit in 32 bits. > > Found by cocci: > drivers/media/i2c/st-mipid02.c:329:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_s64 instead. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/i2c/st-mipid02.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/st-mipid02.c b/drivers/media/i2c/st-mipid02.c > index f250640729ca..93a40bfda1af 100644 > --- a/drivers/media/i2c/st-mipid02.c > +++ b/drivers/media/i2c/st-mipid02.c > @@ -326,7 +326,7 @@ static int mipid02_configure_from_rx_speed(struct mipid02_dev *bridge, > } > > dev_dbg(&client->dev, "detect link_freq = %lld Hz", link_freq); > - do_div(ui_4, link_freq); > + ui_4 = div64_s64(ui_4, link_freq); > bridge->r.clk_lane_reg1 |= ui_4 << 2; > > return 0; >
Hi Ricardo, On Wed, Apr 17, 2024 at 06:19:14PM +0200, Ricardo Ribalda wrote: > On Wed, 17 Apr 2024 at 17:51, Laurent Pinchart wrote: > > On Tue, Apr 16, 2024 at 11:47:17AM +0300, Dan Carpenter wrote: > > > In my opinion, it's better to just ignore old warnings. > > > > I agree. Whatever checkers we enable, whatever code we test, there will > > always be false positives. A CI system needs to be able to ignore those > > false positives and only warn about new issues. > > We already have support for that: > https://gitlab.freedesktop.org/linux-media/media-ci/-/tree/main/testdata/static?ref_type=heads Those are manually written filters. Would it be possible to reduce the manual step to flagging something as a false positive, and have a machine build the filters ? > But it would be great if those lists were as small as possible: > > - If we have a lot of warnings, two error messages can be combined and > will scape the filters > eg: > print(AAAA); > print(BBBB); > > AABBBAAB > > - The filters might hide new errors if they are too broad > > > Most of the patches in this series are simple and make a nicer code: > Eg the non return minmax() , > Other patches show real integer overflows. > > Now that the patches are ready, let's bite the bullet and try to > reduce our technical debt. > > > > When code is new the warnings are going to be mostly correct. The > > > original author is there and knows what the code does. Someone has > > > the hardware ready to test any changes. High value, low burden. > > > > > > When the code is old only the false positives are left. No one is > > > testing the code. It's low value, high burden. > > > > > > Plus it puts static checker authors in a difficult place because now > > > people have to work around our mistakes. It creates animosity. > > > > > > Now we have to hold ourselves to a much higher standard for false > > > positives. It sounds like I'm complaining and lazy, right? But Oleg > > > Drokin has told me previously that I spend too much time trying to > > > silence false positives instead of working on new code. He's has a > > > point which is that actually we have limited amount of time and we have > > > to make choices about what's the most useful thing we can do. > > > > > > So what I do and what the zero day bot does is we look at warnings one > > > time and we re-review old warnings whenever a file is changed. > > > > > > Kernel developers are very good at addressing static checker warnings > > > and fixing the real issues... People sometimes ask me to create a > > > database of warnings which I have reviewed but the answer is that > > > anything old can be ignored. As I write this, I've had a thought that > > > instead of a database of false positives maybe we should record a > > > database of real bugs to ensure that the fixes for anything real is > > > applied.
Hi Laurent On Thu, 18 Apr 2024 at 12:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Wed, Apr 17, 2024 at 06:19:14PM +0200, Ricardo Ribalda wrote: > > On Wed, 17 Apr 2024 at 17:51, Laurent Pinchart wrote: > > > On Tue, Apr 16, 2024 at 11:47:17AM +0300, Dan Carpenter wrote: > > > > In my opinion, it's better to just ignore old warnings. > > > > > > I agree. Whatever checkers we enable, whatever code we test, there will > > > always be false positives. A CI system needs to be able to ignore those > > > false positives and only warn about new issues. > > > > We already have support for that: > > https://gitlab.freedesktop.org/linux-media/media-ci/-/tree/main/testdata/static?ref_type=heads > > Those are manually written filters. Would it be possible to reduce the > manual step to flagging something as a false positive, and have a > machine build the filters ? > Do you expect that the list of exceptions will grow? I hope that once the CI is in place we will fix the warnings before they land in the tree. > > But it would be great if those lists were as small as possible: > > > > - If we have a lot of warnings, two error messages can be combined and > > will scape the filters > > eg: > > print(AAAA); > > print(BBBB); > > > AABBBAAB > > > > - The filters might hide new errors if they are too broad > > > > > > Most of the patches in this series are simple and make a nicer code: > > Eg the non return minmax() , > > Other patches show real integer overflows. > > > > Now that the patches are ready, let's bite the bullet and try to > > reduce our technical debt. > > > > > > When code is new the warnings are going to be mostly correct. The > > > > original author is there and knows what the code does. Someone has > > > > the hardware ready to test any changes. High value, low burden. > > > > > > > > When the code is old only the false positives are left. No one is > > > > testing the code. It's low value, high burden. > > > > > > > > Plus it puts static checker authors in a difficult place because now > > > > people have to work around our mistakes. It creates animosity. > > > > > > > > Now we have to hold ourselves to a much higher standard for false > > > > positives. It sounds like I'm complaining and lazy, right? But Oleg > > > > Drokin has told me previously that I spend too much time trying to > > > > silence false positives instead of working on new code. He's has a > > > > point which is that actually we have limited amount of time and we have > > > > to make choices about what's the most useful thing we can do. > > > > > > > > So what I do and what the zero day bot does is we look at warnings one > > > > time and we re-review old warnings whenever a file is changed. > > > > > > > > Kernel developers are very good at addressing static checker warnings > > > > and fixing the real issues... People sometimes ask me to create a > > > > database of warnings which I have reviewed but the answer is that > > > > anything old can be ignored. As I write this, I've had a thought that > > > > instead of a database of false positives maybe we should record a > > > > database of real bugs to ensure that the fixes for anything real is > > > > applied. > > -- > Regards, > > Laurent Pinchart
Hi Ricardo, On Thu, Apr 18, 2024 at 04:51:06PM +0200, Ricardo Ribalda wrote: > On Thu, 18 Apr 2024 at 12:53, Laurent Pinchart wrote: > > On Wed, Apr 17, 2024 at 06:19:14PM +0200, Ricardo Ribalda wrote: > > > On Wed, 17 Apr 2024 at 17:51, Laurent Pinchart wrote: > > > > On Tue, Apr 16, 2024 at 11:47:17AM +0300, Dan Carpenter wrote: > > > > > In my opinion, it's better to just ignore old warnings. > > > > > > > > I agree. Whatever checkers we enable, whatever code we test, there will > > > > always be false positives. A CI system needs to be able to ignore those > > > > false positives and only warn about new issues. > > > > > > We already have support for that: > > > https://gitlab.freedesktop.org/linux-media/media-ci/-/tree/main/testdata/static?ref_type=heads > > > > Those are manually written filters. Would it be possible to reduce the > > manual step to flagging something as a false positive, and have a > > machine build the filters ? > > Do you expect that the list of exceptions will grow? > > I hope that once the CI is in place we will fix the warnings before > they land in the tree. Any static checker is bound to produce false positives. Some of them can be addressed by improving the checker, others by modifying the source code, but in some cases the first option would be too difficult and the second would reduce readability of the code. I thus thing the list of accepted false positives will grow over time. > > > But it would be great if those lists were as small as possible: > > > > > > - If we have a lot of warnings, two error messages can be combined and > > > will scape the filters > > > eg: > > > print(AAAA); > > > print(BBBB); > > > > AABBBAAB > > > > > > - The filters might hide new errors if they are too broad > > > > > > > > > Most of the patches in this series are simple and make a nicer code: > > > Eg the non return minmax() , > > > Other patches show real integer overflows. > > > > > > Now that the patches are ready, let's bite the bullet and try to > > > reduce our technical debt. > > > > > > > > When code is new the warnings are going to be mostly correct. The > > > > > original author is there and knows what the code does. Someone has > > > > > the hardware ready to test any changes. High value, low burden. > > > > > > > > > > When the code is old only the false positives are left. No one is > > > > > testing the code. It's low value, high burden. > > > > > > > > > > Plus it puts static checker authors in a difficult place because now > > > > > people have to work around our mistakes. It creates animosity. > > > > > > > > > > Now we have to hold ourselves to a much higher standard for false > > > > > positives. It sounds like I'm complaining and lazy, right? But Oleg > > > > > Drokin has told me previously that I spend too much time trying to > > > > > silence false positives instead of working on new code. He's has a > > > > > point which is that actually we have limited amount of time and we have > > > > > to make choices about what's the most useful thing we can do. > > > > > > > > > > So what I do and what the zero day bot does is we look at warnings one > > > > > time and we re-review old warnings whenever a file is changed. > > > > > > > > > > Kernel developers are very good at addressing static checker warnings > > > > > and fixing the real issues... People sometimes ask me to create a > > > > > database of warnings which I have reviewed but the answer is that > > > > > anything old can be ignored. As I write this, I've had a thought that > > > > > instead of a database of false positives maybe we should record a > > > > > database of real bugs to ensure that the fixes for anything real is > > > > > applied.
Btw, here is the output from my check (based on linux next from a few days ago). There are some false positives because Smatch parses __pow10() incorrectly etc but it's mostly correct. regards, dan carpenter drivers/mtd/spi-nor/core.c:2896 spi_nor_late_init_params() warn: unnecessary div64_u64(): divisor = '2-255' drivers/mtd/spi-nor/core.c:3409 spi_nor_set_mtd_eraseregions() warn: unnecessary div64_u64(): divisor = '1-u32max' drivers/hwmon/occ/common.c:467 occ_get_powr_avg() warn: unnecessary div64_u64(): divisor = '1-u32max' drivers/hwmon/occ/common.c:702 occ_store_caps_user() warn: unnecessary div64_u64(): divisor = '1000000' drivers/hwmon/scmi-hwmon.c:67 scmi_hwmon_scale() warn: unnecessary div64_u64(): divisor = '1,10' drivers/infiniband/hw/cxgb4/device.c:160 wr_log_show() warn: unnecessary div64_u64(): divisor = '1000' drivers/infiniband/hw/cxgb4/device.c:161 wr_log_show() warn: unnecessary div64_u64(): divisor = '1000' drivers/clk/clk-versaclock7.c:743 vc7_get_apll_rate() warn: unnecessary div64_u64(): divisor = '2-31' drivers/clk/clk-versaclock7.c:794 vc7_calc_fod_1st_stage_rate() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/clk/clk-versaclock7.c:812 vc7_calc_fod_2nd_stage_rate() warn: unnecessary div64_u64(): divisor = '2-u32max' drivers/clk/clk-versaclock7.c:973 vc7_iod_recalc_rate() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/clk/clk-versaclock7.c:990 vc7_iod_round_rate() warn: unnecessary div64_u64(): divisor = '14-33554431' drivers/clk/clk-versaclock7.c:1016 vc7_iod_set_rate() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/clk/clk-si570.c:263 si570_round_rate() warn: unnecessary div64_u64(): divisor = '2' drivers/pci/setup-bus.c:1909 pci_bus_distribute_available_resources() warn: unnecessary div64_u64(): divisor = '1-u32max' drivers/pci/setup-bus.c:1910 pci_bus_distribute_available_resources() warn: unnecessary div64_u64(): divisor = '1-u32max' drivers/pci/setup-bus.c:1911 pci_bus_distribute_available_resources() warn: unnecessary div64_u64(): divisor = '1-u32max' drivers/pci/setup-bus.c:1914 pci_bus_distribute_available_resources() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/pci/setup-bus.c:1915 pci_bus_distribute_available_resources() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/pci/setup-bus.c:1916 pci_bus_distribute_available_resources() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/tty/serial/serial_core.c:431 uart_update_timeout() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/ptp/ptp_qoriq.c:224 ptp_qoriq_adjfine() warn: unnecessary div64_u64(): divisor = '2' drivers/ptp/ptp_dte.c:150 ptp_dte_adjfine() warn: unnecessary div64_u64(): divisor = '125000000' drivers/ptp/ptp_dte.c:152 ptp_dte_adjfine() warn: unnecessary div64_u64(): divisor = '125000000' drivers/gpu/drm/radeon/si_dpm.c:2200 si_calculate_power_efficiency_ratio() warn: unnecessary div64_u64(): divisor = '1000' drivers/gpu/drm/radeon/si_dpm.c:2202 si_calculate_power_efficiency_ratio() warn: unnecessary div64_u64(): divisor = '1-4294836225' drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c:356 rcar_mipi_dsi_pll_calc() warn: unnecessary div64_u64(): divisor = '0-255' drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c:361 rcar_mipi_dsi_pll_calc() warn: unnecessary div64_u64(): divisor = '0-u16max' drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/si_dpm.c:2351 si_calculate_power_efficiency_ratio() warn: unnecessary div64_u64(): divisor = '1000' drivers/gpu/drm/amd/amdgpu/../pm/legacy-dpm/si_dpm.c:2353 si_calculate_power_efficiency_ratio() warn: unnecessary div64_u64(): divisor = '1-4294836225' drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1673 amdgpu_ras_sysfs_badpages_read() warn: unnecessary div64_u64(): divisor = '28' drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1674 amdgpu_ras_sysfs_badpages_read() warn: unnecessary div64_u64(): divisor = '28' drivers/gpu/drm/amd/amdgpu/../display/dc/dc_dmub_srv.c:592 populate_subvp_cmd_drr_info() warn: unnecessary div64_u64(): divisor = '2' drivers/gpu/drm/amd/amdgpu/../display/dc/hwss/dce110/dce110_hwseq.c:807 dce110_edp_power_control() warn: unnecessary div64_u64(): divisor = '1000000' drivers/gpu/drm/amd/amdgpu/../display/dc/hwss/dce110/dce110_hwseq.c:812 dce110_edp_power_control() warn: unnecessary div64_u64(): divisor = '1000000' drivers/gpu/drm/amd/amdgpu/../display/dc/hwss/dce110/dce110_hwseq.c:935 dce110_edp_wait_for_T12() warn: unnecessary div64_u64(): divisor = '1000000' drivers/gpu/drm/amd/amdgpu/../display/dc/link/protocols/link_edp_panel_control.c:765 edp_setup_psr() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/dc/link/protocols/link_edp_panel_control.c:765 edp_setup_psr() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.c:800 kbps_to_peak_pbn() warn: unnecessary div64_u64(): divisor = '432000' drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_psr.c:160 amdgpu_dm_psr_enable() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_psr.c:160 amdgpu_dm_psr_enable() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:106 calc_duration_in_us_from_refresh_in_uhz() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:117 calc_duration_in_us_from_v_total() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:132 mod_freesync_calc_v_total_from_refresh() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:135 mod_freesync_calc_v_total_from_refresh() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:135 mod_freesync_calc_v_total_from_refresh() warn: unnecessary div64_u64(): divisor = '1000000' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:169 calc_v_total_from_duration() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:169 calc_v_total_from_duration() warn: unnecessary div64_u64(): divisor = '1000' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:201 update_v_total_for_static_ramp() warn: unnecessary div64_u64(): divisor = '1000000' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:200 update_v_total_for_static_ramp() warn: unnecessary div64_u64(): divisor = '1000-4467765' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:207 update_v_total_for_static_ramp() warn: unnecessary div64_u64(): divisor = '1000' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:214 update_v_total_for_static_ramp() warn: unnecessary div64_u64(): divisor = '16666' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:245 update_v_total_for_static_ramp() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:245 update_v_total_for_static_ramp() warn: unnecessary div64_u64(): divisor = '1000' drivers/gpu/drm/amd/amdgpu/../display/modules/freesync/freesync.c:1004 mod_freesync_build_vrr_params() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/acpi/cppc_acpi.c:1855 cppc_perf_to_khz() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/acpi/cppc_acpi.c:1863 cppc_perf_to_khz() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/acpi/cppc_acpi.c:1885 cppc_khz_to_perf() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/leds/rgb/leds-qcom-lpg.c:458 lpg_calc_freq() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/leds/rgb/leds-qcom-lpg.c:464 lpg_calc_freq() warn: unnecessary div64_u64(): divisor = '1024' drivers/md/dm-cache-policy-smq.c:1750 __smq_create() warn: unnecessary div64_u64(): divisor = '64-2097152' drivers/md/dm-verity-fec.c:764 verity_fec_ctr() warn: unnecessary div64_u64(): divisor = '512-130560' drivers/pwm/pwm-lpc32xx.c:48 lpc32xx_pwm_config() warn: unnecessary div64_u64(): divisor = 's32min-s32max' drivers/pwm/pwm-bcm-iproc.c:138 iproc_pwmc_apply() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/pwm/pwm-bcm-iproc.c:140 iproc_pwmc_apply() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/pwm/pwm-sifive.c:95 pwm_sifive_update_clock() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/pwm/pwm-spear.c:98 spear_pwm_config() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/pwm/pwm-spear.c:100 spear_pwm_config() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/media/i2c/st-vgxy61.c:482 get_pixel_rate() warn: unnecessary div64_u64(): divisor = '0-255' drivers/media/i2c/st-vgxy61.c:569 vgxy61_check_bw() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/media/i2c/ds90ub913.c:642 ub913_i2c_master_init() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/media/i2c/ds90ub913.c:643 ub913_i2c_master_init() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/media/i2c/ds90ub953.c:832 ub953_i2c_master_init() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/media/i2c/ds90ub953.c:833 ub953_i2c_master_init() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/media/platform/ti/vpe/sc.c:202 sc_config_scaler() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/media/dvb-core/dvb_demux.c:425 dvb_dmx_swfilter_packet() warn: unnecessary div64_u64(): divisor = '1024' drivers/scsi/sym53c8xx_2/sym_hipd.c:749 sym_prepare_setting() warn: unnecessary div64_u64(): divisor = '40000-2560000' drivers/video/fbdev/omap2/omapfb/dss/dsi.c:4773 dsi_vm_calc() warn: unnecessary div64_u64(): divisor = 's32min-s32max' drivers/video/fbdev/omap2/omapfb/dss/dsi.c:4780 dsi_vm_calc() warn: unnecessary div64_u64(): divisor = 's32min-s32max' drivers/cpufreq/cppc_cpufreq.c:124 cppc_scale_freq_workfn() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/pinctrl/ti/pinctrl-ti-iodelay.c:192 ti_iodelay_compute_dpe() warn: unnecessary div64_u64(): divisor = '176-34602480' drivers/net/wireless/intel/iwlwifi/mvm/ptp.c:55 iwl_mvm_ptp_get_adj_time() warn: unnecessary div64_u64(): divisor = '1000' drivers/net/ethernet/xilinx/xilinx_axienet_main.c:240 axienet_usec_to_timer() warn: unnecessary div64_u64(): divisor = '125000000' drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:407 bnxt_get_target_cycles() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c:1262 hw_atl_b0_adj_params_get() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/net/ethernet/engleder/tsnep_selftests.c:164 delay_base_time() warn: unnecessary div64_u64(): divisor = '62500,100000,200000,333333,400000,411854,500000,1000000,1500000,1700000,2000000,6000000' drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c:301 mlxsw_sp1_ptp_clock_init() warn: unnecessary div64_u64(): divisor = '3435819912' drivers/net/ethernet/mellanox/mlx5/core/en/htb.c:263 mlx5e_htb_convert_rate() warn: unnecessary div64_u64(): divisor = '1-u32max' drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c:519 find_target_cycles() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c:966 mlx5_init_overflow_period() warn: unnecessary div64_u64(): divisor = '0-u32max' drivers/net/ethernet/intel/ice/ice_ptp.c:1717 ice_ptp_cfg_clkout() warn: unnecessary div64_u64(): divisor = '1000000000' drivers/base/arch_topology.c:406 topology_init_cpu_capacity_cppc() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iolatency.c:441 check_scale_change() warn: unnecessary div64_u64(): divisor = '100' block/blk-iolatency.c:266 iolat_update_total_lat_avg() warn: unnecessary div64_u64(): divisor = '250000000' block/blk-iolatency.c:236 latency_sum_ok() warn: unnecessary div64_u64(): divisor = '10' block/blk-iolatency.c:955 iolatency_pd_stat() warn: unnecessary div64_u64(): divisor = '1000' block/blk-iolatency.c:956 iolatency_pd_stat() warn: unnecessary div64_u64(): divisor = '1000000' block/blk-iocost.c:706 abs_cost_to_cost() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:714 cost_to_abs_cost() warn: unnecessary div64_u64(): divisor = '65536' block/blk-iocost.c:797 ioc_refresh_period_us() warn: unnecessary div64_u64(): divisor = '100' block/blk-iocost.c:831 ioc_autop_idx() warn: unnecessary div64_u64(): divisor = '137438' block/blk-iocost.c:943 ioc_refresh_params_disk() warn: unnecessary div64_u64(): divisor = '1000000' block/blk-iocost.c:945 ioc_refresh_params_disk() warn: unnecessary div64_u64(): divisor = '1000000' block/blk-iocost.c:1015 ioc_adjust_base_vrate() warn: unnecessary div64_u64(): divisor = '100' block/blk-iocost.c:1018 ioc_adjust_base_vrate() warn: unnecessary div64_u64(): divisor = '100' block/blk-iocost.c:1030 ioc_adjust_base_vrate() warn: unnecessary div64_u64(): divisor = '100' block/blk-iocost.c:1365 iocg_kick_delay() warn: unnecessary div64_u64(): divisor = '1000000' block/blk-iocost.c:1618 ioc_lat_stat() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:1762 hweight_after_donation() warn: unnecessary div64_u64(): divisor = '65536' block/blk-iocost.c:1990 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-65536' block/blk-iocost.c:1999 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '65536' block/blk-iocost.c:2004 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:2009 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:2013 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:2016 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:2020 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:2044 transfer_surpluses() warn: unnecessary div64_u64(): divisor = '0-u32max' block/blk-iocost.c:2213 ioc_check_iocgs() warn: unnecessary div64_u64(): divisor = '65536' block/blk-iocost.c:2823 ioc_rqos_done() warn: unnecessary div64_u64(): divisor = '137' block/blk-iocost.c:3047 ioc_pd_stat() warn: unnecessary div64_u64(): divisor = '137438' fs/bcachefs/tests.c:788 btree_perf_test_thread() warn: unnecessary div64_u64(): divisor = '0-u32max' fs/bcachefs/alloc_background.c:2261 bch2_recalc_capacity() warn: unnecessary div64_u64(): divisor = '100' fs/btrfs/block-group.c:2793 calculate_global_root_id() warn: unnecessary div64_u64(): divisor = '134217728,1073741824' fs/nilfs2/the_nilfs.c:416 nilfs_max_segment_count() warn: unnecessary div64_u64(): divisor = '16-u32max' fs/f2fs/gc.c:568 atgc_lookup_victim() warn: unnecessary div64_u64(): divisor = '0-u32max' fs/f2fs/iostat.c:23 iostat_get_avg_bytes() warn: unnecessary div64_u64(): divisor = '0' kernel/trace/trace_benchmark.c:107 trace_do_benchmark() warn: unnecessary div64_u64(): divisor = '0,2-u32max' kernel/trace/trace_benchmark.c:129 trace_do_benchmark() warn: unnecessary div64_u64(): divisor = '1-u32max' kernel/trace/trace_events_hist.c:701 hist_field_get_div_fn() warn: unnecessary div64_u64(): divisor = '1-1048576' kernel/trace/trace_events_hist.c:5430 __get_percentage() warn: unnecessary div64_u64(): divisor = '10000' kernel/dma/map_benchmark.c:82 map_benchmark_thread() warn: unnecessary div64_u64(): divisor = '100' kernel/dma/map_benchmark.c:83 map_benchmark_thread() warn: unnecessary div64_u64(): divisor = '100' kernel/fork.c:1011 set_max_threads() warn: unnecessary div64_u64(): divisor = '131072' sound/usb/mixer_quirks.c:2353 snd_rme_current_freq_get() warn: unnecessary div64_u64(): divisor = '1-u32max' sound/soc/sof/ipc4-mtrace.c:432 ipc4_mtrace_enable() warn: unnecessary div64_u64(): divisor = '1000' sound/soc/sti/uniperif_player.c:182 uni_player_clk_set_rate() warn: unnecessary div64_u64(): divisor = '1000000' net/netfilter/xt_hashlimit.c:498 user2credits() warn: unnecessary div64_u64(): divisor = '32000' net/netfilter/xt_hashlimit.c:498 user2credits() warn: unnecessary div64_u64(): divisor = '31' net/netfilter/xt_hashlimit.c:500 user2credits() warn: unnecessary div64_u64(): divisor = '10000,1000000' net/ipv4/inetpeer.c:79 inet_initpeers() warn: unnecessary div64_u64(): divisor = '19200'
On 15/04/2024 20:34, Ricardo Ribalda wrote: > Use an API that resembles more the actual use of num_channels. > > Found by cocci: > drivers/media/usb/s2255/s2255drv.c:2362:5-24: WARNING: atomic_dec_and_test variation before object free at line 2363. > drivers/media/usb/s2255/s2255drv.c:1557:5-24: WARNING: atomic_dec_and_test variation before object free at line 1558. Hmm, that commit log needs more detail. "Convert from atomic_t to refcount_t because refcount_t has memory ordering guarantees which atomic does not, hence the WARNING for the free after the atomic dec." Something like that. I'll leave it up to yourself to decide if this warrants a Fixes: I don't think so myself because the previous code doesn't seem to matter to the decrement and free. --- bod
On 15/04/2024 20:34, Ricardo Ribalda wrote: > Simplifies the code. Found by cocci: > > drivers/media/common/saa7146/saa7146_hlp.c:125:36-37: WARNING opportunity for min() > drivers/media/common/saa7146/saa7146_hlp.c:154:41-42: WARNING opportunity for min() > drivers/media/common/saa7146/saa7146_hlp.c:286:35-36: WARNING opportunity for min() > drivers/media/common/saa7146/saa7146_hlp.c:289:35-36: WARNING opportunity for min() > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Tue, Apr 23, 2024 at 01:43:49PM +0100, Bryan O'Donoghue wrote: > On 15/04/2024 20:34, Ricardo Ribalda wrote: > > Use an API that resembles more the actual use of num_channels. > > > > Found by cocci: > > drivers/media/usb/s2255/s2255drv.c:2362:5-24: WARNING: atomic_dec_and_test variation before object free at line 2363. > > drivers/media/usb/s2255/s2255drv.c:1557:5-24: WARNING: atomic_dec_and_test variation before object free at line 1558. > > Hmm, that commit log needs more detail. > > "Convert from atomic_t to refcount_t because refcount_t has memory ordering > guarantees which atomic does not, hence the WARNING for the free after the > atomic dec." > The memory ordering rules are the same. They're basically identical. The difference is that if you decrement a refcount_t past zero it will trigger an error and refuse but atomic_t will merrily keep decrementing forever. With refcount_t you can still have a use after free bug but afterward the double free will trigger a warning. There are time where people use atomic_t to get unique number and don't care about wrapping so that's fine. But if it's reference counting then use refcount_t. There wouldn't be a Fixes tag in this case, because hopefully it's just hardenning and not a bugfix. regards, dan carpenter
On 15/04/2024 20:34, Ricardo Ribalda wrote: > Simplifies the code. > > Found by cocci: > drivers/media/usb/stk1160/stk1160-video.c:133:12-13: WARNING opportunity for min() > drivers/media/usb/stk1160/stk1160-video.c:176:13-14: WARNING opportunity for min() > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Show the commit log some love, then add Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> For patches 16 through 19. --- bod
Hi Dan On Tue, 23 Apr 2024 at 11:55, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > Btw, here is the output from my check (based on linux next from a few > days ago). There are some false positives because Smatch parses > __pow10() incorrectly etc but it's mostly correct. This looks pretty cool :) Are you planning to add this to smatch soon? Thanks!
After this set is applied, these are the only warnings left: drivers/media/pci/ivtv/ivtv-fileops.c:223:4-10: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:230:3-9: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:236:4-10: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:245:3-9: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:251:3-9: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:257:3-9: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:272:3-9: preceding lock on line 267 drivers/media/pci/ivtv/ivtv-fileops.c:598:4-10: preceding lock on line 627 drivers/media/pci/ivtv/ivtv-fileops.c:598:4-10: preceding lock on line 689 drivers/media/pci/ivtv/ivtv-fileops.c:606:3-9: preceding lock on line 627 drivers/media/pci/ivtv/ivtv-fileops.c:606:3-9: preceding lock on line 689 drivers/media/pci/ivtv/ivtv-fileops.c:648:3-9: preceding lock on line 627 drivers/media/pci/ivtv/ivtv-fileops.c:648:3-9: preceding lock on line 689 drivers/media/pci/ivtv/ivtv-fileops.c:692:4-10: preceding lock on line 689 drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2776 drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2786 drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2809 drivers/media/dvb-frontends/stv090x.c:799:1-7: preceding lock on line 768 drivers/media/usb/go7007/go7007-i2c.c:125:1-7: preceding lock on line 61 drivers/media/rc/imon.c:1167:1-7: preceding lock on line 1153 drivers/media/pci/cx18/cx18-scb.h:261:22-29: 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:77: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: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:154: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:171: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: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) drivers/media/platform/qcom/venus/hfi_cmds.h:201: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:220: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:230: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_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:1008:43-60: 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:1014:36-46: 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:1267: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_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/common/siano/smscoreapi.h:619:5-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/common/siano/smscoreapi.h:669:6-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/common/siano/smscoreapi.h:1049: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/common/siano/smscoreapi.h:1055: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/dvb-frontends/mxl5xx_defs.h:171: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/dvb-frontends/mxl5xx_defs.h:182: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/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) drivers/media/platform/xilinx/xilinx-dma.h:100:19-22: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) drivers/staging/media/atomisp/pci/atomisp_tpg.h:30:18-22: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays) CI tested: https://gitlab.freedesktop.org/linux-media/media-staging/-/commit/055b5211c68e721c3a7090be5373cf44859da1a7/pipelines?ref=ribalda%2Ftest-cocci Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Ricardo Ribalda (35): media: pci: mgb4: Refactor struct resources media: stb0899: Remove unreacheable code media: uvcvideo: Refactor iterators media: uvcvideo: Use max() macro media: go7007: Use min and max macros media: stm32-dcmipp: Remove redundant printk media: staging: sun6i-isp: Remove redundant printk media: dvb-frontends: tda18271c2dd: Remove casting during div media: v4l: async: refactor v4l2_async_create_ancillary_links staging: media: tegra-video: Use swap macro media: s2255: Use refcount_t instead of atomic_t for num_channels media: platform: mtk-mdp3: Use refcount_t for job_count media: common: saa7146: Use min macro media: dvb-frontends: drx39xyj: Use min macro media: netup_unidvb: Use min macro media: au0828: Use min macro media: flexcop-usb: Use min macro media: gspca: cpia1: Use min macro media: stk1160: Use min macro media: tegra-vde: Refactor timeout handling media: venus: Use div64_u64 media: i2c: st-mipid02: Use the correct div function media: dvb-frontends: tda10048: Use the right div media: tc358746: Use the correct div_ function media: venus: Use the correct div_ function media: venus: Refator return path media: dib0700: Refator return path media: usb: cx231xx: Refator return path media: i2c: rdacm20: Refator return path media: i2c: et8ek8: Refator return path media: cx231xx: Refator return path media: si4713: Refator return path media: ttpci: Refator return path media: hdpvr: Refator return path media: venus: Refator return path drivers/media/common/saa7146/saa7146_hlp.c | 8 +++---- drivers/media/dvb-frontends/drx39xyj/drxj.c | 9 +++----- drivers/media/dvb-frontends/stb0899_drv.c | 5 ----- drivers/media/dvb-frontends/tda10048.c | 3 +-- drivers/media/dvb-frontends/tda18271c2dd.c | 4 ++-- drivers/media/i2c/et8ek8/et8ek8_driver.c | 4 +++- drivers/media/i2c/rdacm20.c | 5 ++++- drivers/media/i2c/st-mipid02.c | 2 +- drivers/media/i2c/tc358746.c | 3 +-- drivers/media/pci/mgb4/mgb4_core.c | 4 ++-- drivers/media/pci/mgb4/mgb4_regs.c | 2 +- drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c | 2 +- drivers/media/pci/ttpci/budget-core.c | 5 ++++- .../media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 10 ++++----- .../media/platform/mediatek/mdp3/mtk-mdp3-core.c | 6 ++--- .../media/platform/mediatek/mdp3/mtk-mdp3-core.h | 2 +- .../media/platform/mediatek/mdp3/mtk-mdp3-m2m.c | 6 ++--- drivers/media/platform/nvidia/tegra-vde/h264.c | 6 ++--- drivers/media/platform/qcom/venus/vdec.c | 15 +++++++------ drivers/media/platform/qcom/venus/venc.c | 19 +++++++++------- .../platform/st/stm32/stm32-dcmipp/dcmipp-core.c | 5 +---- drivers/media/radio/si4713/radio-usb-si4713.c | 8 +++++-- drivers/media/usb/au0828/au0828-video.c | 5 +---- drivers/media/usb/b2c2/flexcop-usb.c | 5 +---- drivers/media/usb/cx231xx/cx231xx-i2c.c | 16 +++++++++---- drivers/media/usb/cx231xx/cx231xx-video.c | 10 +++++++-- drivers/media/usb/dvb-usb/dib0700_core.c | 4 +++- drivers/media/usb/go7007/go7007-fw.c | 4 ++-- drivers/media/usb/gspca/cpia1.c | 6 ++--- drivers/media/usb/hdpvr/hdpvr-control.c | 4 +++- drivers/media/usb/s2255/s2255drv.c | 20 ++++++++--------- drivers/media/usb/stk1160/stk1160-video.c | 10 ++------- drivers/media/usb/uvc/uvc_ctrl.c | 26 ++++++++++++---------- drivers/media/v4l2-core/v4l2-async.c | 8 +++---- drivers/staging/media/sunxi/sun6i-isp/sun6i_isp.c | 1 - drivers/staging/media/tegra-video/tegra20.c | 9 ++------ 36 files changed, 132 insertions(+), 129 deletions(-) --- base-commit: 71b3ed53b08d87212fbbe51bdc3bf44eb8c462f8 change-id: 20240415-fix-cocci-2df3ef22a6f7 Best regards,