linux-kernel - Re: [PATCH v3 8/8] memory: gpmc-omap: "gpmc,device-width" DT property is optional

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Sep 2021 12:48:19 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Roger Quadros <rogerq@...nel.org>, tony@...mide.com
Cc:     robh+dt@...nel.org, grygorii.strashko@...com, nm@...com,
        lokeshvutla@...com, nsekhar@...com, miquel.raynal@...tlin.com,
        devicetree@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 8/8] memory: gpmc-omap: "gpmc,device-width" DT property
 is optional

On 15/09/2021 11:11, Roger Quadros wrote:
> Hi Krzysztof,
> 
> On 07/09/2021 15:36, Krzysztof Kozlowski wrote:
>> On 07/09/2021 13:32, Roger Quadros wrote:
>>> Check for valid gpmc,device-width, nand-bus-width and bank-width
>>> at one place. Default to 8-bit width if none present.
>>
>> I don't understand the message in the context of the patch. The title
>> says one property is optional - that's it. The message says you
>> consolidate checks. How is this related to the title?
>>
>> The patch itself moves around checking of properties and reads
>> nand-bus-width *always*. It does not "check at one place" but rather
>> "check always". In the same time, the patch does not remove
>> gpmc,device-width check in other place.
>>
>> All three elements - the title, message and patch - do different things.
>> What did you want to achieve here? Can you help in clarifying it?
>>
> 
> OK I will explain it better in commit log in next revision. Let me explain here a bit.
> 
> Prior to this patch it was working like this
> 
> 	/* in gpmc_read_settings_dt() */
> 	s->device_width = 0;	/* invalid width, should be 1 for 8-bit, 2 for 16-bit */
> 	of_property_read_u32(np, "gpmc,device-width", s->device_width);
> 
> 	/* in gpmc_probe_generic_child () */
> 	if (of_device_is_compatible(child, "ti,omap2-nand")) {
> 		/* check for nand-bus-width, if absent set s->device_width to 1 (i.e. 8-bit) */
> 	} else {
> 		/* check for bank-width, if absent and s->device_width not set, error out */
> 	}
> 
> So that means if all three, "gpmc,device-width". "nand-bus-width" and "bank-width" are missing then
> it would create an error situation.
> 
> The patch is doing 3 things.
> 1) Make sure all DT checks related to bus width are being done at one place for better readability.

Not entirely. The gpmc,device-width is still done in the other place
because you did not remove it from the code. Unless you meant parsing of
gpmc,device-width not reading from DT? But then another round of checks
is in gpmc_cs_program_settings() so not in one place.

If you consolidate the checks to one place, I would expect the code to
be removed from other places, so from gpmc_cs_program_settings() and
gpmc_read_settings_dt(). Since this is not happening, the message
confuses me.

> 2) even if all 3 width properties are absent, we will not treat it as error and default to 8-bit.

This is not mentioned in commit msg.

> 3) check for nand-bus-width regardless of whether compatible to "ti,omap2-nand" or not.

Also not mentioned in commit msg.

Your commit reorganizes parsing and validating the child DT properties
but it does not change from "multiple place" to "one place".

At least I don't see it.

Best regards,
Krzysztof