Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Christophe Kerello]

On 2/15/24 19:56, Krzysztof Kozlowski wrote:
> 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?

Hi Krzysztof,

I will check the return value of regmap_read API.

Regards,
Christophe Kerello.

> Best regards,
> Krzysztof