diff mbox series

fpga: dfl: pci: gracefully handle misconfigured port entries

Message ID 20210420172740.707259-1-matthew.gerlach@linux.intel.com (mailing list archive)
State New
Headers show
Series fpga: dfl: pci: gracefully handle misconfigured port entries | expand

Commit Message

Matthew Gerlach April 20, 2021, 5:27 p.m. UTC 
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Gracefully ignore misconfigured port entries encountered in
incorrect FPGA images.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Tom Rix April 20, 2021, 6:49 p.m. UTC | #1 
On 4/20/21 10:27 AM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Gracefully ignore misconfigured port entries encountered in
> incorrect FPGA images.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>   drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index b44523e..660d3b6 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -212,6 +212,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
Does something similar need to be added to find_dfls_by_vsec ?
>   	int port_num, bar, i, ret = 0;
>   	resource_size_t start, len;
>   	void __iomem *base;
> +	int bars = 0;
>   	u32 offset;
>   	u64 v;
>   
> @@ -228,6 +229,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>   	if (dfl_feature_is_fme(base)) {
>   		start = pci_resource_start(pcidev, 0);
>   		len = pci_resource_len(pcidev, 0);
> +		bars |= BIT(0);
>   
>   		dfl_fpga_enum_info_add_dfl(info, start, len);
>   
> @@ -253,9 +255,21 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>   			 */
>   			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
>   			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> +			if (bars & BIT(bar)) {
> +				dev_warn(&pcidev->dev, "skipping bad port BAR %d\n", bar);
> +				continue;
> +			}
> +
>   			start = pci_resource_start(pcidev, bar) + offset;
> -			len = pci_resource_len(pcidev, bar) - offset;
> +			len = pci_resource_len(pcidev, bar);
> +			if (offset >= len) {
> +				dev_warn(&pcidev->dev, "bad port offset %u >= %pa\n",
> +					 offset, &len);

why %pa,&len for instead of %u,len ?

Tom

> +				continue;
> +			}
>   
> +			len -= offset;
> +			bars |= BIT(bar);
>   			dfl_fpga_enum_info_add_dfl(info, start, len);
>   		}
>   	} else if (dfl_feature_is_port(base)) {
Matthew Gerlach April 20, 2021, 7:19 p.m. UTC | #2 
On Tue, 20 Apr 2021, Tom Rix wrote:

>
> On 4/20/21 10:27 AM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> 
>> Gracefully ignore misconfigured port entries encountered in
>> incorrect FPGA images.
>> 
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>   drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index b44523e..660d3b6 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -212,6 +212,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
> Does something similar need to be added to find_dfls_by_vsec ?
>>   	int port_num, bar, i, ret = 0;
>>   	resource_size_t start, len;
>>   	void __iomem *base;
>> +	int bars = 0;
>>   	u32 offset;
>>   	u64 v;
>>   @@ -228,6 +229,7 @@ static int find_dfls_by_default(struct pci_dev 
>> *pcidev,
>>   	if (dfl_feature_is_fme(base)) {
>>   		start = pci_resource_start(pcidev, 0);
>>   		len = pci_resource_len(pcidev, 0);
>> +		bars |= BIT(0);
>>     		dfl_fpga_enum_info_add_dfl(info, start, len);
>>   @@ -253,9 +255,21 @@ static int find_dfls_by_default(struct pci_dev 
>> *pcidev,
>>   			 */
>>   			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
>>   			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
>> +			if (bars & BIT(bar)) {
>> +				dev_warn(&pcidev->dev, "skipping bad port BAR 
>> %d\n", bar);
>> +				continue;
>> +			}
>> +
>>   			start = pci_resource_start(pcidev, bar) + offset;
>> -			len = pci_resource_len(pcidev, bar) - offset;
>> +			len = pci_resource_len(pcidev, bar);
>> +			if (offset >= len) {
>> +				dev_warn(&pcidev->dev, "bad port offset %u >= 
>> %pa\n",
>> +					 offset, &len);
>
> why %pa,&len for instead of %u,len ?
>
> Tom

Hi Tom,

The variable len is of type resource_size_t, and I am following what it 
says to do in Documentation/core-api/printk-formats.rst:

Physical address types phys_addr_t
----------------------------------

::

         %pa[p]  0x01234567 or 0x0123456789abcdef

For printing a phys_addr_t type (and its derivatives, such as
resource_size_t) which can vary based on build options, regardless of the
width of the CPU data path.

Passed by reference.

Matthew
>
>> +				continue;
>> +			}
>>   +			len -= offset;
>> +			bars |= BIT(bar);
>>   			dfl_fpga_enum_info_add_dfl(info, start, len);
>>   		}
>>   	} else if (dfl_feature_is_port(base)) {
>
>
Wu, Hao April 21, 2021, 5:25 a.m. UTC | #3 
> Subject: [PATCH] fpga: dfl: pci: gracefully handle misconfigured port entries
> 
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Gracefully ignore misconfigured port entries encountered in
> incorrect FPGA images.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index b44523e..660d3b6 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -212,6 +212,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>  	int port_num, bar, i, ret = 0;
>  	resource_size_t start, len;
>  	void __iomem *base;
> +	int bars = 0;
>  	u32 offset;
>  	u64 v;
> 
> @@ -228,6 +229,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>  	if (dfl_feature_is_fme(base)) {
>  		start = pci_resource_start(pcidev, 0);
>  		len = pci_resource_len(pcidev, 0);
> +		bars |= BIT(0);
> 
>  		dfl_fpga_enum_info_add_dfl(info, start, len);
> 
> @@ -253,9 +255,21 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>  			 */
>  			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
>  			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> +			if (bars & BIT(bar)) {
> +				dev_warn(&pcidev->dev, "skipping bad port
> BAR %d\n", bar);
> +				continue;
> +			}

Will it be a real problem that multiple ports are inside one BAR but different offsets?

Hao

> +
>  			start = pci_resource_start(pcidev, bar) + offset;
> -			len = pci_resource_len(pcidev, bar) - offset;
> +			len = pci_resource_len(pcidev, bar);
> +			if (offset >= len) {
> +				dev_warn(&pcidev->dev, "bad port
> offset %u >= %pa\n",
> +					 offset, &len);
> +				continue;
> +			}
> 
> +			len -= offset;
> +			bars |= BIT(bar);
>  			dfl_fpga_enum_info_add_dfl(info, start, len);
>  		}
>  	} else if (dfl_feature_is_port(base)) {
> --
> 1.8.3.1
Matthew Gerlach April 21, 2021, 5:06 p.m. UTC | #4 
On Wed, 21 Apr 2021, Wu, Hao wrote:

>> Subject: [PATCH] fpga: dfl: pci: gracefully handle misconfigured port entries
>>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Gracefully ignore misconfigured port entries encountered in
>> incorrect FPGA images.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index b44523e..660d3b6 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -212,6 +212,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>>  	int port_num, bar, i, ret = 0;
>>  	resource_size_t start, len;
>>  	void __iomem *base;
>> +	int bars = 0;
>>  	u32 offset;
>>  	u64 v;
>>
>> @@ -228,6 +229,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>>  	if (dfl_feature_is_fme(base)) {
>>  		start = pci_resource_start(pcidev, 0);
>>  		len = pci_resource_len(pcidev, 0);
>> +		bars |= BIT(0);
>>
>>  		dfl_fpga_enum_info_add_dfl(info, start, len);
>>
>> @@ -253,9 +255,21 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
>>  			 */
>>  			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
>>  			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
>> +			if (bars & BIT(bar)) {
>> +				dev_warn(&pcidev->dev, "skipping bad port
>> BAR %d\n", bar);
>> +				continue;
>> +			}
>
> Will it be a real problem that multiple ports are inside one BAR but different offsets?
>
> Hao

I don't think multiple ports within a single BAR is something that has 
been supported in the past.  The genesis for this patch was a 
misconfigured port entry pointing to BAR0.  BAR0 had already been mapped 
for the FME and remapping BAR0 failed resulting in enumeration failure.

Matthew

>
>> +
>>  			start = pci_resource_start(pcidev, bar) + offset;
>> -			len = pci_resource_len(pcidev, bar) - offset;
>> +			len = pci_resource_len(pcidev, bar);
>> +			if (offset >= len) {
>> +				dev_warn(&pcidev->dev, "bad port
>> offset %u >= %pa\n",
>> +					 offset, &len);
>> +				continue;
>> +			}
>>
>> +			len -= offset;
>> +			bars |= BIT(bar);
>>  			dfl_fpga_enum_info_add_dfl(info, start, len);
>>  		}
>>  	} else if (dfl_feature_is_port(base)) {
>> --
>> 1.8.3.1
>
>
Moritz Fischer April 26, 2021, 1:21 a.m. UTC | #5 
On Tue, Apr 20, 2021 at 12:19:42PM -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 20 Apr 2021, Tom Rix wrote:
> 
> > 
> > On 4/20/21 10:27 AM, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > Gracefully ignore misconfigured port entries encountered in
> > > incorrect FPGA images.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > >   drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
> > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > > index b44523e..660d3b6 100644
> > > --- a/drivers/fpga/dfl-pci.c
> > > +++ b/drivers/fpga/dfl-pci.c
> > > @@ -212,6 +212,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
> > Does something similar need to be added to find_dfls_by_vsec ?
> > >   	int port_num, bar, i, ret = 0;
> > >   	resource_size_t start, len;
> > >   	void __iomem *base;
> > > +	int bars = 0;
> > >   	u32 offset;
> > >   	u64 v;
> > >   @@ -228,6 +229,7 @@ static int find_dfls_by_default(struct pci_dev
> > > *pcidev,
> > >   	if (dfl_feature_is_fme(base)) {
> > >   		start = pci_resource_start(pcidev, 0);
> > >   		len = pci_resource_len(pcidev, 0);
> > > +		bars |= BIT(0);
> > >     		dfl_fpga_enum_info_add_dfl(info, start, len);
> > >   @@ -253,9 +255,21 @@ static int find_dfls_by_default(struct
> > > pci_dev *pcidev,
> > >   			 */
> > >   			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
> > >   			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> > > +			if (bars & BIT(bar)) {
> > > +				dev_warn(&pcidev->dev, "skipping bad port BAR %d\n", bar);
> > > +				continue;
> > > +			}
> > > +
> > >   			start = pci_resource_start(pcidev, bar) + offset;
> > > -			len = pci_resource_len(pcidev, bar) - offset;
> > > +			len = pci_resource_len(pcidev, bar);
> > > +			if (offset >= len) {
> > > +				dev_warn(&pcidev->dev, "bad port offset %u >= %pa\n",
> > > +					 offset, &len);
Do you want pa or px here?

> > 
> > why %pa,&len for instead of %u,len ?
> > 
> > Tom
> 
> Hi Tom,
> 
> The variable len is of type resource_size_t, and I am following what it says
> to do in Documentation/core-api/printk-formats.rst:
> 
> Physical address types phys_addr_t
> ----------------------------------
> 
> ::
> 
>         %pa[p]  0x01234567 or 0x0123456789abcdef
> 
> For printing a phys_addr_t type (and its derivatives, such as
> resource_size_t) which can vary based on build options, regardless of the
> width of the CPU data path.
> 
> Passed by reference.
> 
> Matthew
> > 
> > > +				continue;
> > > +			}
> > >   +			len -= offset;
> > > +			bars |= BIT(bar);
> > >   			dfl_fpga_enum_info_add_dfl(info, start, len);
> > >   		}
> > >   	} else if (dfl_feature_is_port(base)) {
> > 
> > 

- Moritz
Wu, Hao April 26, 2021, 2:39 a.m. UTC | #6 
> On Wed, 21 Apr 2021, Wu, Hao wrote:
> 
> >> Subject: [PATCH] fpga: dfl: pci: gracefully handle misconfigured port entries
> >>
> >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>
> >> Gracefully ignore misconfigured port entries encountered in
> >> incorrect FPGA images.
> >>
> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >> ---
> >>  drivers/fpga/dfl-pci.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> >> index b44523e..660d3b6 100644
> >> --- a/drivers/fpga/dfl-pci.c
> >> +++ b/drivers/fpga/dfl-pci.c
> >> @@ -212,6 +212,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
> >>  	int port_num, bar, i, ret = 0;
> >>  	resource_size_t start, len;
> >>  	void __iomem *base;
> >> +	int bars = 0;
> >>  	u32 offset;
> >>  	u64 v;
> >>
> >> @@ -228,6 +229,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
> >>  	if (dfl_feature_is_fme(base)) {
> >>  		start = pci_resource_start(pcidev, 0);
> >>  		len = pci_resource_len(pcidev, 0);
> >> +		bars |= BIT(0);
> >>
> >>  		dfl_fpga_enum_info_add_dfl(info, start, len);
> >>
> >> @@ -253,9 +255,21 @@ static int find_dfls_by_default(struct pci_dev
> *pcidev,
> >>  			 */
> >>  			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
> >>  			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> >> +			if (bars & BIT(bar)) {
> >> +				dev_warn(&pcidev->dev, "skipping bad port
> >> BAR %d\n", bar);
> >> +				continue;
> >> +			}
> >
> > Will it be a real problem that multiple ports are inside one BAR but different
> offsets?
> >
> > Hao
> 
> I don't think multiple ports within a single BAR is something that has
> been supported in the past.  The genesis for this patch was a
> misconfigured port entry pointing to BAR0.  BAR0 had already been mapped
> for the FME and remapping BAR0 failed resulting in enumeration failure.

could some products put the port in BAR0? Or multiple ports in one BAR.
As we consider this is a common driver can be reused and maintained for
a long time, so I hope that we don't put limitation setup only for some 
specific products.

Hao

> 
> Matthew
> 
> >
> >> +
> >>  			start = pci_resource_start(pcidev, bar) + offset;
> >> -			len = pci_resource_len(pcidev, bar) - offset;
> >> +			len = pci_resource_len(pcidev, bar);
> >> +			if (offset >= len) {
> >> +				dev_warn(&pcidev->dev, "bad port
> >> offset %u >= %pa\n",
> >> +					 offset, &len);
> >> +				continue;
> >> +			}
> >>
> >> +			len -= offset;
> >> +			bars |= BIT(bar);
> >>  			dfl_fpga_enum_info_add_dfl(info, start, len);
> >>  		}
> >>  	} else if (dfl_feature_is_port(base)) {
> >> --
> >> 1.8.3.1
> >
> >
diff mbox series

Patch

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index b44523e..660d3b6 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -212,6 +212,7 @@  static int find_dfls_by_default(struct pci_dev *pcidev,
 	int port_num, bar, i, ret = 0;
 	resource_size_t start, len;
 	void __iomem *base;
+	int bars = 0;
 	u32 offset;
 	u64 v;
 
@@ -228,6 +229,7 @@  static int find_dfls_by_default(struct pci_dev *pcidev,
 	if (dfl_feature_is_fme(base)) {
 		start = pci_resource_start(pcidev, 0);
 		len = pci_resource_len(pcidev, 0);
+		bars |= BIT(0);
 
 		dfl_fpga_enum_info_add_dfl(info, start, len);
 
@@ -253,9 +255,21 @@  static int find_dfls_by_default(struct pci_dev *pcidev,
 			 */
 			bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
 			offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
+			if (bars & BIT(bar)) {
+				dev_warn(&pcidev->dev, "skipping bad port BAR %d\n", bar);
+				continue;
+			}
+
 			start = pci_resource_start(pcidev, bar) + offset;
-			len = pci_resource_len(pcidev, bar) - offset;
+			len = pci_resource_len(pcidev, bar);
+			if (offset >= len) {
+				dev_warn(&pcidev->dev, "bad port offset %u >= %pa\n",
+					 offset, &len);
+				continue;
+			}
 
+			len -= offset;
+			bars |= BIT(bar);
 			dfl_fpga_enum_info_add_dfl(info, start, len);
 		}
 	} else if (dfl_feature_is_port(base)) {