diff mbox series

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

Message ID 20200424153858.29744-4-pali@kernel.org
State New
Headers show
Series PCI: aardvark: Fix support for Turris MOX and Compex wifi cards | expand

Commit Message

Pali Rohár April 24, 2020, 3:38 p.m. UTC
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(-)

Comments

Rob Herring April 24, 2020, 4:47 p.m. UTC | #1
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).

Plus you are adding capability to distinguish not present and out of
bounds, but I don't see you using that?

If there's any error with max-link-speed, then just use the max speed
for the block which should be implied by the compatible string if
there's more than one.

Rob
Pali Rohár April 27, 2020, 9 a.m. UTC | #2
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.

And because max-link-speed is optional property, it is perfectly valid
when it is absent.

So without ability to distinguish between "property is not present in
DT" and "property is malformed" it is not possible to properly detect
this DT error.

> Plus you are adding capability to distinguish not present and out of
> bounds, but I don't see you using that?

It should have been used in next aardvark patch, but I forgot to include
needed code. My mistake.

> If there's any error with max-link-speed, then just use the max speed
> for the block which should be implied by the compatible string if
> there's more than one.
> 
> Rob
Rob Herring April 28, 2020, 3:52 p.m. UTC | #3
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.

> And because max-link-speed is optional property, it is perfectly valid
> when it is absent.
>
> So without ability to distinguish between "property is not present in
> DT" and "property is malformed" it is not possible to properly detect
> this DT error.

How would you have an error? Your DT has a schema to check it, right?
(Hint: convert your binding)

Think about it this way. If every driver has an error string for every
property, that's a lot of bloat to the kernel. If we really want to
print errors, we should define of_property_.*_optional() variants and
print errors in the core code.

Rob
Pali Rohár April 28, 2020, 4:01 p.m. UTC | #4
On Tuesday 28 April 2020 10:52:34 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.

Ok Rob, thank you for your input!

I understand your arguments about bloating and also Bjorn's concern
about DT error.

I have no problem with reverting these changes back and silently use
some default value. Just I need to know which option should I choose.

> > And because max-link-speed is optional property, it is perfectly valid
> > when it is absent.
> >
> > So without ability to distinguish between "property is not present in
> > DT" and "property is malformed" it is not possible to properly detect
> > this DT error.
> 
> How would you have an error? Your DT has a schema to check it, right?
> (Hint: convert your binding)

Schema is currently defined as human readable text file. So checking is
possible but only manually.

> Think about it this way. If every driver has an error string for every
> property, that's a lot of bloat to the kernel. If we really want to
> print errors, we should define of_property_.*_optional() variants and
> print errors in the core code.
Bjorn Helgaas April 28, 2020, 4:23 p.m. UTC | #5
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
Marek Behún April 28, 2020, 11:55 p.m. UTC | #6
On Tue, 28 Apr 2020 18:01:28 +0200
Pali Rohár <pali@kernel.org> wrote:

> > How would you have an error? Your DT has a schema to check it, right?
> > (Hint: convert your binding)  
> 
> Schema is currently defined as human readable text file. So checking is
> possible but only manually.

I think that's what Rob meant by "Hint: convert your binding", that we
should convert the pci-aardvardk dt binding to yaml.
diff mbox series

Patch

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;
+	else if (ret)
+		return -EINVAL;
 
-	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
-	    max_link_speed > 4)
+	if (max_link_speed == 0 || max_link_speed > 4)
 		return -EINVAL;
 
 	return max_link_speed;