On Fri, May 26, 2023 at 10:00:19AM +0200, Julien Panis wrote: > > > > Special handling for 'count == 1' (or 'count < 2') makes sense indeed. > > > > > > I'm not sure that I fully understand, though. Will Smatch stop complaining > > > for both things that you mentioned above with a patch for handling 'count < 2' ? > > > Or will we consider either that warnings are handled, even if they are still there. > > No, don't do anything to silence the warning. Always fix the checker > > instead of working around it. > > > > Handling the "map->format.reg_bytes + map->format.pad_bytes + val_len" > > is kind of complicated but still easier than handling the: > > > > if (val_len % map->format.val_bytes) > > > > condition in regmap_raw_write() just because it's closer together. It's > > quite a bit of code to write to silence this false positive but it's > > something which could be done. > > > > Who must fix 'smart' ? Me ? Heh. I wish. I'll fix it. (Eventually. It's complicated). > > I run 'smart' on my side and did not see the warnings you mentioned. > Here are the commands I run: > - [path to]/smatch/smatch_scripts/build_kernel_data.sh > - [path to]/smatch/smatch_scripts/test_kernel.sh.sh > -> no warning for tps6594 mfd drivers in 'smatch_warns.txt' > [path to]/smatch/smatch_scripts/kchecker tps6594-i2c.c > -> no warning > [path to]/smatch/smatch_scripts/kchecker regmap.c > -> no warning related to tps6594 mfd driver > -> 2 errors for "uninitialized symbol 'ret'" in _regmap_raw_write/read() > > Maybe I did not use the tool properly (?) Sorry! These underflow flow check isn't released. I probably could though. It doesn't create too many false positives. See attached. drivers/clk/qcom/clk-rcg.c:618 clk_rcg_pixel_determine_rate() warn: 'request - delta' negative user limit promoted to high drivers/clk/qcom/clk-rcg2.c:617 clk_edp_pixel_determine_rate() warn: 'request - delta' negative user limit promoted to high drivers/clk/qcom/clk-rcg2.c:794 clk_pixel_determine_rate() warn: 'request - delta' negative user limit promoted to high drivers/mfd/tps6594-i2c.c:159 tps6594_i2c_write() warn: 'count - 2' negative user limit promoted to high drivers/gpu/drm/radeon/si.c:4594 si_vm_packet3_gfx_check() warn: 'pkt->count - 2' negative user limit promoted to high drivers/gpu/drm/radeon/si.c:4697 si_vm_packet3_compute_check() warn: 'pkt->count - 2' negative user limit promoted to high drivers/net/dsa/microchip/ksz8863_smi.c:66 ksz8863_mdio_write() warn: 'count - 4' negative user limit promoted to high net/wireless/chan.c:456 cfg80211_set_chans_dfs_state() warn: 'center_freq + bandwidth / 2 - 10' negative user limit promoted to high mm/page_isolation.c:558 start_isolate_page_range() warn: '(null)' negative user limit promoted to high I think the radeon and wireless warnings look like real bugs. The rest are false positives. I've looked at all these warnings before so they should have all been false positives... :/ regards, dan carpenter
/* * Copyright (C) 2018 Oracle. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * as published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt */ #include "smatch.h" #include "smatch_extra.h" #include "smatch_slist.h" static int my_id; static struct expression *warned; static void match_condition(struct expression *expr) { struct expression *right; struct range_list *rl; struct symbol *type; char *str; sval_t sval; if (warned == expr) return; if (expr->type != EXPR_COMPARE) return; switch (expr->op) { case '<': case SPECIAL_LTE: case SPECIAL_UNSIGNED_LT: case SPECIAL_UNSIGNED_LTE: break; default: return; } right = strip_expr(expr->right); if (right->type != EXPR_BINOP || right->op != '-') return; if (!get_user_rl(right->left, &rl)) return; if (!get_implied_value(right->right, &sval)) return; /* * Unfortunately subtracting 1 to underflow is often done deliberately. * It could be a bug or it could be not. Silence everything. */ if (sval.value == 1) return; if (sval_cmp(rl_min(rl), sval) >= 0) return; type = get_type(expr); if (!type) return; if (type_is_ptr(type)) return; if (!type_unsigned(type) || type_positive_bits(type) < 32) return; warned = expr; str = expr_to_str(right); sm_msg("warn: '%s' negative user limit promoted to high", str); free_string(str); } void check_negative_user_loop_promoted(int id) { my_id = id; add_hook(&match_condition, CONDITION_HOOK); }
