Re: [PATCH 1/2] of: base: add support to get machine model name

From: Rob Herring <robh@kernel.org>
To: Frank Rowand <frowand.list@gmail.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH 1/2] of: base: add support to get machine model name
Date: Mon, 12 Dec 2016 09:17:27 -0600	[thread overview]
Message-ID: <CAL_JsqJvmiVn4hM0dVAWN0yDJxmmnDd3ACLb-Y=sP0Vc-NfSOg@mail.gmail.com> (raw)
In-Reply-To: <584B442D.4020104@gmail.com>

On Fri, Dec 9, 2016 at 5:54 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/09/16 08:03, Rob Herring wrote:
>> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 22/11/16 21:35, Rob Herring wrote:
>>>>
>>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>>>> wrote:
>>>
>>>
>>> [...]
>>>
>>>>>
>>>>> This patch adds a function that leads to conflating the "model" property
>>>>> and the "compatible" property. This leads to opaque, confusing and
>>>>> unclear
>>>>> code where ever it is used.   I think it is not good for the device tree
>>>>> framework to contribute to writing unclear code.
>>>>>
>>>>> Further, only two of the proposed users of this new function appear to
>>>>> be proper usage.  I do not think that the small amount of reduced lines
>>>>> of code is a good trade off for the reduced code clarity and for the
>>>>> potential for future mis-use of this function.
>>>>>
>>>>> Can I convince you to revert this patch?
>>>>
>>>>
>>>> Yes, I will revert.
>>
>> I looked at this again and the users. They are all informational, so
>
> A comment in the function docbook header stating that the intent of the
> returned value is for informational use only would make me happy.
>
> There is at least on proposed use in patch 2/2 that is not just
> informational.  init_octeon_system_type() sometimes uses the value of
> the model property to create the value of variable octeon_system_type.
> octeon_pcie_pcibios_map_irq() checks the value of octeon_system_type
> (via the function octeon_board_type_string()) to determine whether
> to apply a fixup:
>
> int __init octeon_pcie_pcibios_map_irq(const struct pci_dev *dev,
>                                        u8 slot, u8 pin)
> {
>         /*
>          * The EBH5600 board with the PCI to PCIe bridge mistakenly
>          * wires the first slot for both device id 2 and interrupt
>          * A. According to the PCI spec, device id 2 should be C. The
>          * following kludge attempts to fix this.
>          */
>         if (strstr(octeon_board_type_string(), "EBH5600") &&
>             dev->bus && dev->bus->parent) {

True, it is more than informational, but let's think about what would
have to happen for the change here to matter. We would have to have a
board with no model property. Then we'd have to have a compatible
string of "EBH5600" on a board which is not the same one as model
EBH5600 and wouldn't be valid anyway with no vendor prefix. IMO, this
code should be using of_machine_is_compatible() anyway. MIPS SoC and
board code is a mess anyway. Linus needs to yell at them.

Rob