On 15/02/2024 10:00, Christophe Kerello wrote: > > > On 2/14/24 11:07, Krzysztof Kozlowski wrote: >> On 13/02/2024 14:15, Christophe Kerello wrote: >>>>> + >>>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2) >>>>> + return 0; >>>>> + >>>>> + if (resource >= FMC2_MAX_RESOURCES) >>>>> + return -EINVAL; >>>>> + >>>>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr); >>> >>> Hi Krzysztof, >>> >>>> >>>> No checking of read value? >>>> >>> >>> No, it should never failed. >> >> And you tested that neither smatch, sparse nor Coverity report here >> warnings? >> > > Hi Krzysztof, > > There is a lot of driver in the Kernel that are using same > implementation, and I am surprised to not have had this comment 4 years > ago when the driver was introduced. Really? Care to give some pointers? Heh, I don't know what to respond to it. Either you say that my comment is incorrect or you say that it's okay to sneak poor code if no one notices? We can argue on the first, whether my comment is reasonable or not. But if you claim that previous poor choice of code is argument of bringing more of such poor choices, then we are done here. It's the oldest argument: someone did it that way, so I can do the same. Nope. > > So, how should I proceed? Shall I initialize all local variables used by > regmap_read? Or shall I check the return value of regmap_read? > And, as there is a lot of regmap_read call in this driver, shall I fix > them in a separate patch? regmap operations, depending on the regmap used, can fail. Most of the errors are result of static configuration, e.g. alignment, regmap in cache mode etc. Then certain regmap implementations can produce errors, which is not a static condition but dynamic. You have neither error checking nor value initialization. You risk here to have quite tricky to find, unnoticeable bugs, if there any mistake leading to regmap errors. Indeed neither smatch nor sparse report this as error currently, but maybe that's their limitation? Best regards, Krzysztof
