Re: [PATCH 1/2] PCI: rockchip: Work around missing device_type property in DT

[Rob Herring <robh@kernel.org>]

On Tue, Aug 18, 2020 at 1:03 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-08-18 18:48, Saravana Kannan wrote:
> > On Tue, Aug 18, 2020 at 10:34 AM Marc Zyngier <maz@kernel.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...