linux-kernel - Re: [PATCH 1/2] PCI: rockchip: Work around missing device

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]

Message-ID: <CAL_JsqLMqbGBKsh=0BuTUxhf5g6qC0TsOWK+xX8DdxpEGAbG5w@mail.gmail.com>
Date:   Tue, 18 Aug 2020 13:06:51 -0600
From:   Rob Herring <robh@...nel.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Saravana Kannan <saravanak@...gle.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        PCI <linux-pci@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Heiko Stuebner <heiko@...ech.de>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH 1/2] PCI: rockchip: Work around missing device_type
 property in DT

On Tue, Aug 18, 2020 at 1:03 PM Marc Zyngier <maz@...nel.org> wrote:
>
> On 2020-08-18 18:48, Saravana Kannan wrote:
> > On Tue, Aug 18, 2020 at 10:34 AM Marc Zyngier <maz@...nel.org> wrote:
>
> [...]
>
> >> OK. So how about something like this?
> >>
> >> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >> index 590493e04b01..a7a6ee599b14 100644
> >> --- a/drivers/of/address.c
> >> +++ b/drivers/of/address.c
> >> @@ -134,9 +134,13 @@ static int of_bus_pci_match(struct device_node
> >> *np)
> >>          * "pciex" is PCI Express
> >>          * "vci" is for the /chaos bridge on 1st-gen PCI powermacs
> >>          * "ht" is hypertransport
> >> +        *
> >> +        * If none of the device_type match, and that the node name is
> >> +        * "pcie", accept the device as PCI (with a warning).
> >>          */
> >>         return of_node_is_type(np, "pci") || of_node_is_type(np,
> >> "pciex") ||
> >> -               of_node_is_type(np, "vci") || of_node_is_type(np,
> >> "ht");
> >> +               of_node_is_type(np, "vci") || of_node_is_type(np,
> >> "ht") ||
> >> +               WARN_ON_ONCE(of_node_name_eq(np, "pcie"));
> >
> > I don't think we need the _ONCE. Otherwise, it'd warn only for the
> > first device that has this problem.
>
> Because probing devices doesn't necessarily occur once. Case in point,
> it takes *10 to 15* attempts for a rk3399 system such as mine to finally
> probe its PCIe device, thanks to the wonderful -EPROBE_DEFER.
>
> Do I want to see the same stack trace 10 (or more) times? No.
>
> > How about?
> > WARN(of_node_name_eq(np, "pcie"), "Missing device type in %pOF", np)
> >
> > That'll even tell them which node is bad.
>
> I explained my objections above. Spitting out the device node is
> useful, but there is no need to be exhaustive (if you're in a
> position to fix the DT, you can track all the broken instances
> for your device easily).
>
> I'm actually minded to tone it down even more, because the stack
> trace is meaningless to most users. See below for a revised patch.

LGTM.

>          M.
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 590493e04b01..b37bd9cc2810 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -128,15 +128,29 @@ static unsigned int of_bus_pci_get_flags(const
> __be32 *addr)
>    * PCI bus specific translator
>    */
>
> +static bool of_node_is_pcie(struct device_node *np)
> +{
> +       bool is_pcie = of_node_name_eq(np, "pcie");
> +
> +       if (is_pcie)
> +               pr_warn_once("%pOF: Missing device_type\n", np);
> +
> +       return is_pcie;
> +}
> +
>   static int of_bus_pci_match(struct device_node *np)
>   {
>         /*
>          * "pciex" is PCI Express
>          * "vci" is for the /chaos bridge on 1st-gen PCI powermacs
>          * "ht" is hypertransport
> +        *
> +        * If none of the device_type match, and that the node name is
> +        * "pcie", accept the device as PCI (with a warning).
>          */
>         return of_node_is_type(np, "pci") || of_node_is_type(np, "pciex") ||
> -               of_node_is_type(np, "vci") || of_node_is_type(np, "ht");
> +               of_node_is_type(np, "vci") || of_node_is_type(np, "ht") ||
> +               of_node_is_pcie(np);
>   }
>
>   static void of_bus_pci_count_cells(struct device_node *np,
>
> --
> Jazz is not dead. It just smells funny...