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