Received-SPF: pass (google.com: domain of linux-kernel+bounces-68223-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <7d5af347-749a-43e3-8321-445569f5b78c@foss.st.com>
Date: Fri, 16 Feb 2024 09:15:04 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support
Content-Language: en-US
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>
CC: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <devicetree@vger.kernel.org>
References: <20240212174822.77734-1-christophe.kerello@foss.st.com>
 <20240212174822.77734-7-christophe.kerello@foss.st.com>
 <989661f0-f539-43c3-a332-13c0e99ed7b9@linaro.org>
 <edbb5e6e-44c0-426b-9c97-87ea1eee1b4c@foss.st.com>
 <1e1ae38b-7f8c-44ba-9970-0929aaaa28a8@linaro.org>
 <a1badd8b-041b-495d-81cb-b264c687de80@foss.st.com>
 <9c36b6e5-262d-48b1-971c-b03d9edf7789@linaro.org>
From: Christophe Kerello <christophe.kerello@foss.st.com>
In-Reply-To: <9c36b6e5-262d-48b1-971c-b03d9edf7789@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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
>