Re: [PATCH v3 03/12] PCI: of: Return -ENOENT if max-link-speed property is not found

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Apr 28, 2020 at 10:52:34AM -0500, Rob Herring wrote:
> On Mon, Apr 27, 2020 at 4:00 AM Pali Rohár <pali@kernel.org> wrote:
> > On Friday 24 April 2020 11:47:26 Rob Herring wrote:
> > > On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > OF API function of_property_read_u32() returns -EINVAL if property is
> > > > not found. Therefore this also happens with of_pci_get_max_link_speed(),
> > > > which also returns -EINVAL if the 'max-link-speed' property has invalid
> > > > value.
> > > >
> > > > Change the behaviour of of_pci_get_max_link_speed() to return -ENOENT
> > > > in case when the property does not exist and -EINVAL if it has invalid
> > > > value.
> > > >
> > > > Also interpret zero max-link-speed value of this property as invalid,
> > > > as the device tree bindings documentation specifies.
> > > >
> > > > Update pcie-tegra194 code to handle errors from this function like other
> > > > drivers - they do not distinguish between no value and invalid value.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-tegra194.c |  6 +++---
> > > >  drivers/pci/of.c                           | 15 +++++++++++----
> > > >  2 files changed, 14 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > index ae30a2fd3716..027bb41809f9 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > @@ -296,7 +296,7 @@ struct tegra_pcie_dw {
> > > >         u8 init_link_width;
> > > >         u32 msi_ctrl_int;
> > > >         u32 num_lanes;
> > > > -       u32 max_speed;
> > > > +       int max_speed;
> > > >         u32 cid;
> > > >         u32 cfg_link_cap_l1sub;
> > > >         u32 pcie_cap_base;
> > > > @@ -911,7 +911,7 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
> > > >         dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
> > > >
> > > >         /* Configure Max Speed from DT */
> > > > -       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
> > > > +       if (pcie->max_speed > 0) {
> > > >                 val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
> > > >                                         PCI_EXP_LNKCAP);
> > > >                 val &= ~PCI_EXP_LNKCAP_SLS;
> > > > @@ -1830,7 +1830,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
> > > >         dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
> > > >
> > > >         /* Configure Max Speed from DT */
> > > > -       if (pcie->max_speed && pcie->max_speed != -EINVAL) {
> > > > +       if (pcie->max_speed > 0) {
> > > >                 val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
> > > >                                         PCI_EXP_LNKCAP);
> > > >                 val &= ~PCI_EXP_LNKCAP_SLS;
> > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > index 81ceeaa6f1d5..19bf652256d8 100644
> > > > --- a/drivers/pci/of.c
> > > > +++ b/drivers/pci/of.c
> > > > @@ -584,15 +584,22 @@ EXPORT_SYMBOL_GPL(pci_parse_request_of_pci_ranges);
> > > >   *
> > > >   * @node: device tree node with the max link speed information
> > > >   *
> > > > - * Returns the associated max link speed from DT, or a negative value if the
> > > > - * required property is not found or is invalid.
> > > > + * Returns the associated max link speed from DT, -ENOENT if the required
> > > > + * property is not found or -EINVAL if the required property is invalid.
> > > >   */
> > > >  int of_pci_get_max_link_speed(struct device_node *node)
> > > >  {
> > > >         u32 max_link_speed;
> > > > +       int ret;
> > > > +
> > > > +       /* of_property_read_u32 returns -EINVAL if property does not exist */
> > > > +       ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);
> > > > +       if (ret == -EINVAL)
> > > > +               return -ENOENT;
> > >
> > > Generally, it's considered bad to change return values (though I guess
> > > this was happening. In hindsight, not present probably should have
> > > been -ENOENT. But it shouldn't really matter. The kernel should treat
> > > malformed as not present. It's not the kernel's job to validate the DT
> > > (the schema should and does now).
> >
> > Bjorn in review of V1 patch wrote that aardavark driver should at least
> > warn on DT error.
> 
> Yes, but I disagree. Just treat an error as not present as long as the
> driver can make progress. If something critical required is missing,
> then yes we should print an error and bail out.

That sounds good to me.

Bjorn