linux-kernel - Re: [PATCH V9 2/5] PCI: Create device tree node for selected devices

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 26 Jun 2023 13:11:55 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Lizhi Hou <lizhi.hou@....com>
Cc:     linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, robh@...nel.org, max.zhen@....com,
        sonal.santan@....com, stefano.stabellini@...inx.com
Subject: Re: [PATCH V9 2/5] PCI: Create device tree node for selected devices

On Mon, Jun 26, 2023 at 10:34:05AM -0700, Lizhi Hou wrote:
> On 6/21/23 13:22, Bjorn Helgaas wrote:

>     Added an of_pci_make_dev_node() interface that can be used to create
>     device tree node for PCI devices.
> 
>     Added a PCI_DYNAMIC_OF_NODES config option. When the option is turned
> on,
>     the kernel will generate device tree nodes for PCI bridges
> unconditionally.
> 
>     Initially, the basic properties are added for the dynamically generated
>     device tree nodes which include #address-cells, #size-cells,
> device_type,
>     compatible, ranges, reg.

s/Added/Add/ (twice, mentioned before).

The commit log should say what the *patch* does, not what *you* did.

> > > @@ -501,8 +501,10 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
> > >   		 * to rely on this function (you ship a firmware that doesn't
> > >   		 * create device nodes for all PCI devices).
> > >   		 */
> > > -		if (ppnode)
> > > +		if (ppnode && of_property_present(ppnode, "interrupt-map"))
> >
> > Maybe this deserves a comment?  The connection between "interrupt-map"
> > and the rest of this patch isn't obvious to me.
> > 
> > Also, it looks like this happens for *everybody*, regardless of
> > PCI_DYNAMIC_OF_NODES, which seems a little suspect.  If it's an
> > unrelated bug fix it should be a different patch.
> 
> This is not a bug fix. The check will distinguish between device tree nodes
> automatically created for pci bridges by this patch with those created by a
> DT based system. With this patch, device tree nodes are created for pci
> bridges, thus ppnode here will be non-zero and we will break out of the
> loop. In order to still use pci_swizzle_interrupt_pin(), checking
> “interrupt-map” for ppnode is added here.
> 
> After thinking about this more, using “interrupt-map” property may not be
> correct for the cases where ppnode is not dynamically generated and it does
> not have “interrupt-map”. So, I would introduce a new property “dynamic” for
> pci bridge nodes generated dynamically. And change the code to: if (ppnode
> && of_property_present(ppnode, "dynamic")).
> 
> Does this make sense?

Makes a lot more sense to me than relying on some unrelated and
undocumented property.  Probably still would benefit from an #ifdef.

Rob might have an opinion on whether "dynamic" makes sense from a
DT perspective.

Bjorn