diff mbox series

[v3,10/29] pci: Adjust dm_pci_read_bar32() to return errors correctly

Message ID 20200330171226.v3.10.I36321d5e30daf051900f01f6289dfc58439871ea@changeid
State Superseded
Delegated to: Bin Meng
Headers show
Series dm: Add programmatic generation of ACPI tables (part A) | expand

Commit Message

Simon Glass March 30, 2020, 11:12 p.m. UTC 
At present if reading a BAR returns 0xffffffff (e.g. the device is not
present) then the value is masked and a different value is returned.
This makes it harder to detect the problem when debugging.

Update the function to avoid masking in this case.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pci/pci-uclass.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko April 3, 2020, 11:22 a.m. UTC | #1 
On Mon, Mar 30, 2020 at 05:12:46PM -0600, Simon Glass wrote:
> At present if reading a BAR returns 0xffffffff (e.g. the device is not
> present) then the value is masked and a different value is returned.
> This makes it harder to detect the problem when debugging.

The above ('the device is not present') is actually not correct.
BAR is not mandatory register and detection is described in PCI spec.

To get device presence one may have check Vendor ID / Device ID pair rather
then BAR.

> Update the function to avoid masking in this case.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/pci/pci-uclass.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index ceb64517047..d2e10d6868a 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -1213,7 +1213,14 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int barnum)
>  
>  	bar = PCI_BASE_ADDRESS_0 + barnum * 4;
>  	dm_pci_read_config32(dev, bar, &addr);
> -	if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> +
> +	/*
> +	 * If we get an invalid address, return this so that comparisons with
> +	 * FDT_ADDR_T_NONE work correctly
> +	 */
> +	if (addr == 0xffffffff)
> +		return addr;
> +	else if (addr & PCI_BASE_ADDRESS_SPACE_IO)
>  		return addr & PCI_BASE_ADDRESS_IO_MASK;
>  	else
>  		return addr & PCI_BASE_ADDRESS_MEM_MASK;
> -- 
> 2.26.0.rc2.310.g2932bb562d-goog
>
Simon Glass April 8, 2020, 2:57 a.m. UTC | #2 
Hi Andy,

On Fri, 3 Apr 2020 at 05:22, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 30, 2020 at 05:12:46PM -0600, Simon Glass wrote:
> > At present if reading a BAR returns 0xffffffff (e.g. the device is not
> > present) then the value is masked and a different value is returned.
> > This makes it harder to detect the problem when debugging.
>
> The above ('the device is not present') is actually not correct.
> BAR is not mandatory register and detection is described in PCI spec.

What change are you suggesting here? I suggest 'not present' as an
example of why this might happen.

>
> To get device presence one may have check Vendor ID / Device ID pair rather
> then BAR.
>
> > Update the function to avoid masking in this case.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  drivers/pci/pci-uclass.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> > index ceb64517047..d2e10d6868a 100644
> > --- a/drivers/pci/pci-uclass.c
> > +++ b/drivers/pci/pci-uclass.c
> > @@ -1213,7 +1213,14 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int barnum)
> >
> >       bar = PCI_BASE_ADDRESS_0 + barnum * 4;
> >       dm_pci_read_config32(dev, bar, &addr);
> > -     if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> > +
> > +     /*
> > +      * If we get an invalid address, return this so that comparisons with
> > +      * FDT_ADDR_T_NONE work correctly
> > +      */
> > +     if (addr == 0xffffffff)
> > +             return addr;
> > +     else if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> >               return addr & PCI_BASE_ADDRESS_IO_MASK;
> >       else
> >               return addr & PCI_BASE_ADDRESS_MEM_MASK;
> > --
> > 2.26.0.rc2.310.g2932bb562d-goog
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Regards,
Simon
Andy Shevchenko April 8, 2020, 4:58 p.m. UTC | #3 
On Tue, Apr 07, 2020 at 08:57:20PM -0600, Simon Glass wrote:
> Hi Andy,
> 
> On Fri, 3 Apr 2020 at 05:22, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Mar 30, 2020 at 05:12:46PM -0600, Simon Glass wrote:
> > > At present if reading a BAR returns 0xffffffff (e.g. the device is not
> > > present) then the value is masked and a different value is returned.
> > > This makes it harder to detect the problem when debugging.
> >
> > The above ('the device is not present') is actually not correct.
> > BAR is not mandatory register and detection is described in PCI spec.
> 
> What change are you suggesting here? I suggest 'not present' as an
> example of why this might happen.

I suggest to follow PCI spec.
Thus, the code below is fragile and working by luck.

> > To get device presence one may have check Vendor ID / Device ID pair rather
> > then BAR.
> >
> > > Update the function to avoid masking in this case.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > > ---
> > >
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > >  drivers/pci/pci-uclass.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> > > index ceb64517047..d2e10d6868a 100644
> > > --- a/drivers/pci/pci-uclass.c
> > > +++ b/drivers/pci/pci-uclass.c
> > > @@ -1213,7 +1213,14 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int barnum)
> > >
> > >       bar = PCI_BASE_ADDRESS_0 + barnum * 4;
> > >       dm_pci_read_config32(dev, bar, &addr);
> > > -     if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> > > +
> > > +     /*
> > > +      * If we get an invalid address, return this so that comparisons with
> > > +      * FDT_ADDR_T_NONE work correctly
> > > +      */
> > > +     if (addr == 0xffffffff)
> > > +             return addr;
> > > +     else if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> > >               return addr & PCI_BASE_ADDRESS_IO_MASK;
> > >       else
> > >               return addr & PCI_BASE_ADDRESS_MEM_MASK;
> > > --
> > > 2.26.0.rc2.310.g2932bb562d-goog
> > >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> 
> Regards,
> Simon
Simon Glass April 8, 2020, 10:15 p.m. UTC | #4 
HI Andy,

On Wed, 8 Apr 2020 at 10:58, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Apr 07, 2020 at 08:57:20PM -0600, Simon Glass wrote:
> > Hi Andy,
> >
> > On Fri, 3 Apr 2020 at 05:22, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 05:12:46PM -0600, Simon Glass wrote:
> > > > At present if reading a BAR returns 0xffffffff (e.g. the device is not
> > > > present) then the value is masked and a different value is returned.
> > > > This makes it harder to detect the problem when debugging.
> > >
> > > The above ('the device is not present') is actually not correct.
> > > BAR is not mandatory register and detection is described in PCI spec.
> >
> > What change are you suggesting here? I suggest 'not present' as an
> > example of why this might happen.
>
> I suggest to follow PCI spec.
> Thus, the code below is fragile and working by luck.

I don't know what you are suggesting. This allows an error to be
reported in the common case and helps people discover mistakes in the
driver flow. What is your suggestion?

Rgards,
SImon


>
> > > To get device presence one may have check Vendor ID / Device ID pair rather
> > > then BAR.
> > >
> > > > Update the function to avoid masking in this case.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > > Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > > > ---
> > > >
> > > > Changes in v3: None
> > > > Changes in v2: None
> > > >
> > > >  drivers/pci/pci-uclass.c | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> > > > index ceb64517047..d2e10d6868a 100644
> > > > --- a/drivers/pci/pci-uclass.c
> > > > +++ b/drivers/pci/pci-uclass.c
> > > > @@ -1213,7 +1213,14 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int barnum)
> > > >
> > > >       bar = PCI_BASE_ADDRESS_0 + barnum * 4;
> > > >       dm_pci_read_config32(dev, bar, &addr);
> > > > -     if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> > > > +
> > > > +     /*
> > > > +      * If we get an invalid address, return this so that comparisons with
> > > > +      * FDT_ADDR_T_NONE work correctly
> > > > +      */
> > > > +     if (addr == 0xffffffff)
> > > > +             return addr;
> > > > +     else if (addr & PCI_BASE_ADDRESS_SPACE_IO)
> > > >               return addr & PCI_BASE_ADDRESS_IO_MASK;
> > > >       else
> > > >               return addr & PCI_BASE_ADDRESS_MEM_MASK;
> > > > --
> > > > 2.26.0.rc2.310.g2932bb562d-goog
> > > >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> >
> > Regards,
> > Simon
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
diff mbox series

Patch

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index ceb64517047..d2e10d6868a 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -1213,7 +1213,14 @@  u32 dm_pci_read_bar32(const struct udevice *dev, int barnum)
 
 	bar = PCI_BASE_ADDRESS_0 + barnum * 4;
 	dm_pci_read_config32(dev, bar, &addr);
-	if (addr & PCI_BASE_ADDRESS_SPACE_IO)
+
+	/*
+	 * If we get an invalid address, return this so that comparisons with
+	 * FDT_ADDR_T_NONE work correctly
+	 */
+	if (addr == 0xffffffff)
+		return addr;
+	else if (addr & PCI_BASE_ADDRESS_SPACE_IO)
 		return addr & PCI_BASE_ADDRESS_IO_MASK;
 	else
 		return addr & PCI_BASE_ADDRESS_MEM_MASK;