Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
and pci_epf_test_alloc_space().
Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
but leaks on subsequent unbind by not calling pci_epf_free_space().
Signed-off-by: Alan Mikhak <[email protected]>
Reviewed-by: Paul Walmsley <[email protected]>
---
drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 27806987e93b..96156a537922 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
static int pci_epf_test_set_bar(struct pci_epf *epf)
{
- int bar;
+ int bar, add;
int ret;
struct pci_epf_bar *epf_bar;
struct pci_epc *epc = epf->epc;
@@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
epc_features = epf_test->epc_features;
- for (bar = BAR_0; bar <= BAR_5; bar++) {
+ for (bar = BAR_0; bar <= BAR_5; bar += add) {
epf_bar = &epf->bar[bar];
+ /*
+ * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
+ * if the specific implementation required a 64-bit BAR,
+ * even if we only requested a 32-bit BAR.
+ */
+ add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
if (!!(epc_features->reserved_bar & (1 << bar)))
continue;
@@ -413,13 +419,6 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
if (bar == test_reg_bar)
return ret;
}
- /*
- * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
- * if the specific implementation required a 64-bit BAR,
- * even if we only requested a 32-bit BAR.
- */
- if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
- bar++;
}
return 0;
@@ -431,7 +430,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
struct device *dev = &epf->dev;
struct pci_epf_bar *epf_bar;
void *base;
- int bar;
+ int bar, add;
enum pci_barno test_reg_bar = epf_test->test_reg_bar;
const struct pci_epc_features *epc_features;
@@ -445,8 +444,10 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
}
epf_test->reg[test_reg_bar] = base;
- for (bar = BAR_0; bar <= BAR_5; bar++) {
+ for (bar = BAR_0; bar <= BAR_5; bar += add) {
epf_bar = &epf->bar[bar];
+ add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
+
if (bar == test_reg_bar)
continue;
@@ -459,8 +460,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
dev_err(dev, "Failed to allocate space for BAR%d\n",
bar);
epf_test->reg[bar] = base;
- if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
- bar++;
}
return 0;
--
2.7.4
+Bjorn Helgaas, +Gustavo Pimentel, +Wen Yang, +Kangjie Lu
On Thu, May 23, 2019 at 2:55 PM Alan Mikhak <[email protected]> wrote:
>
> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
> and pci_epf_test_alloc_space().
>
> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
> but leaks on subsequent unbind by not calling pci_epf_free_space().
>
> Signed-off-by: Alan Mikhak <[email protected]>
> Reviewed-by: Paul Walmsley <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 27806987e93b..96156a537922 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>
> static int pci_epf_test_set_bar(struct pci_epf *epf)
> {
> - int bar;
> + int bar, add;
> int ret;
> struct pci_epf_bar *epf_bar;
> struct pci_epc *epc = epf->epc;
> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>
> epc_features = epf_test->epc_features;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + /*
> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> + * if the specific implementation required a 64-bit BAR,
> + * even if we only requested a 32-bit BAR.
> + */
> + add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
>
> if (!!(epc_features->reserved_bar & (1 << bar)))
> continue;
> @@ -413,13 +419,6 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> if (bar == test_reg_bar)
> return ret;
> }
> - /*
> - * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> - * if the specific implementation required a 64-bit BAR,
> - * even if we only requested a 32-bit BAR.
> - */
> - if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> }
>
> return 0;
> @@ -431,7 +430,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> struct device *dev = &epf->dev;
> struct pci_epf_bar *epf_bar;
> void *base;
> - int bar;
> + int bar, add;
> enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> const struct pci_epc_features *epc_features;
>
> @@ -445,8 +444,10 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> }
> epf_test->reg[test_reg_bar] = base;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
> +
> if (bar == test_reg_bar)
> continue;
>
> @@ -459,8 +460,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> dev_err(dev, "Failed to allocate space for BAR%d\n",
> bar);
> epf_test->reg[bar] = base;
> - if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> }
>
> return 0;
> --
> 2.7.4
>
Hi,
On 24/05/19 3:25 AM, Alan Mikhak wrote:
> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
> and pci_epf_test_alloc_space().
>
> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
> but leaks on subsequent unbind by not calling pci_epf_free_space().
>
> Signed-off-by: Alan Mikhak <[email protected]>
> Reviewed-by: Paul Walmsley <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 27806987e93b..96156a537922 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>
> static int pci_epf_test_set_bar(struct pci_epf *epf)
> {
> - int bar;
> + int bar, add;
> int ret;
> struct pci_epf_bar *epf_bar;
> struct pci_epc *epc = epf->epc;
> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>
> epc_features = epf_test->epc_features;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + /*
> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> + * if the specific implementation required a 64-bit BAR,
> + * even if we only requested a 32-bit BAR.
> + */
set_bar shouldn't set PCI_BASE_ADDRESS_MEM_TYPE_64. If a platform supports only
64-bit BAR, that should be specified in epc_features bar_fixed_64bit member.
Thanks
Kishon
Hi,
On 24/05/19 5:25 AM, Alan Mikhak wrote:
> +Bjorn Helgaas, +Gustavo Pimentel, +Wen Yang, +Kangjie Lu
>
> On Thu, May 23, 2019 at 2:55 PM Alan Mikhak <[email protected]> wrote:
>>
>> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
>> and pci_epf_test_alloc_space().
>>
>> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
>> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
>> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
>> but leaks on subsequent unbind by not calling pci_epf_free_space().
>>
>> Signed-off-by: Alan Mikhak <[email protected]>
>> Reviewed-by: Paul Walmsley <[email protected]>
>> ---
>> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index 27806987e93b..96156a537922 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>>
>> static int pci_epf_test_set_bar(struct pci_epf *epf)
>> {
>> - int bar;
>> + int bar, add;
>> int ret;
>> struct pci_epf_bar *epf_bar;
>> struct pci_epc *epc = epf->epc;
>> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>>
>> epc_features = epf_test->epc_features;
>>
>> - for (bar = BAR_0; bar <= BAR_5; bar++) {
>> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
>> epf_bar = &epf->bar[bar];
>> + /*
>> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
>> + * if the specific implementation required a 64-bit BAR,
>> + * even if we only requested a 32-bit BAR.
>> + */
set_bar shouldn't set PCI_BASE_ADDRESS_MEM_TYPE_64. If a platform supports only
64-bit BAR, that should be specified in epc_features bar_fixed_64bit member.
Thanks
Kishon
Hi Kishon,
Yes. This change is still applicable even when the platform specifies
that it only supports 64-bit BARs by setting the bar_fixed_64bit
member of epc_features.
The issue being fixed is this: If the 'continue' statement is executed
within the loop, the loop index 'bar' needs to advanced by two, not
one, when the BAR is 64-bit. Otherwise the next loop iteration will be
on an odd BAR which doesn't exist.
The PCI_BASE_ADDRESS_MEM_TYPE_64 flag in epf_bar->flag reflects the
value set by the platform in the bar_fixed_64bit member of
epc_features.
This patch moves the checking of PCI_BASE_ADDRESS_MEM_TYPE_64 in
epf_bar->flags to before the 'continue' statement to advance the 'bar'
loop index accordingly. The comment you see about 'pci_epc_set_bar()'
preceding the moved code is the original comment and was also moved
along with the code.
Regards,
Alan Mikhak
On Fri, May 24, 2019 at 1:51 AM Kishon Vijay Abraham I <[email protected]> wrote:
>
> Hi,
>
> On 24/05/19 5:25 AM, Alan Mikhak wrote:
> > +Bjorn Helgaas, +Gustavo Pimentel, +Wen Yang, +Kangjie Lu
> >
> > On Thu, May 23, 2019 at 2:55 PM Alan Mikhak <[email protected]> wrote:
> >>
> >> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
> >> and pci_epf_test_alloc_space().
> >>
> >> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
> >> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
> >> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
> >> but leaks on subsequent unbind by not calling pci_epf_free_space().
> >>
> >> Signed-off-by: Alan Mikhak <[email protected]>
> >> Reviewed-by: Paul Walmsley <[email protected]>
> >> ---
> >> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
> >> 1 file changed, 12 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> index 27806987e93b..96156a537922 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> >>
> >> static int pci_epf_test_set_bar(struct pci_epf *epf)
> >> {
> >> - int bar;
> >> + int bar, add;
> >> int ret;
> >> struct pci_epf_bar *epf_bar;
> >> struct pci_epc *epc = epf->epc;
> >> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> >>
> >> epc_features = epf_test->epc_features;
> >>
> >> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> >> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> >> epf_bar = &epf->bar[bar];
> >> + /*
> >> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> >> + * if the specific implementation required a 64-bit BAR,
> >> + * even if we only requested a 32-bit BAR.
> >> + */
>
> set_bar shouldn't set PCI_BASE_ADDRESS_MEM_TYPE_64. If a platform supports only
> 64-bit BAR, that should be specified in epc_features bar_fixed_64bit member.
>
> Thanks
> Kishon
On Fri, May 24, 2019 at 11:50:41AM -0700, Alan Mikhak wrote:
> Hi Kishon,
>
> Yes. This change is still applicable even when the platform specifies
> that it only supports 64-bit BARs by setting the bar_fixed_64bit
> member of epc_features.
>
> The issue being fixed is this: If the 'continue' statement is executed
> within the loop, the loop index 'bar' needs to advanced by two, not
> one, when the BAR is 64-bit. Otherwise the next loop iteration will be
> on an odd BAR which doesn't exist.
>
> The PCI_BASE_ADDRESS_MEM_TYPE_64 flag in epf_bar->flag reflects the
> value set by the platform in the bar_fixed_64bit member of
> epc_features.
>
> This patch moves the checking of PCI_BASE_ADDRESS_MEM_TYPE_64 in
> epf_bar->flags to before the 'continue' statement to advance the 'bar'
> loop index accordingly. The comment you see about 'pci_epc_set_bar()'
> preceding the moved code is the original comment and was also moved
> along with the code.
@Kishon, I would need your ACK to merge this patch.
Thanks,
Lorenzo
> Regards,
> Alan Mikhak
>
> On Fri, May 24, 2019 at 1:51 AM Kishon Vijay Abraham I <[email protected]> wrote:
> >
> > Hi,
> >
> > On 24/05/19 5:25 AM, Alan Mikhak wrote:
> > > +Bjorn Helgaas, +Gustavo Pimentel, +Wen Yang, +Kangjie Lu
> > >
> > > On Thu, May 23, 2019 at 2:55 PM Alan Mikhak <[email protected]> wrote:
> > >>
> > >> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
> > >> and pci_epf_test_alloc_space().
> > >>
> > >> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
> > >> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
> > >> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
> > >> but leaks on subsequent unbind by not calling pci_epf_free_space().
> > >>
> > >> Signed-off-by: Alan Mikhak <[email protected]>
> > >> Reviewed-by: Paul Walmsley <[email protected]>
> > >> ---
> > >> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
> > >> 1 file changed, 12 insertions(+), 13 deletions(-)
> > >>
> > >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > >> index 27806987e93b..96156a537922 100644
> > >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > >> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> > >>
> > >> static int pci_epf_test_set_bar(struct pci_epf *epf)
> > >> {
> > >> - int bar;
> > >> + int bar, add;
> > >> int ret;
> > >> struct pci_epf_bar *epf_bar;
> > >> struct pci_epc *epc = epf->epc;
> > >> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> > >>
> > >> epc_features = epf_test->epc_features;
> > >>
> > >> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> > >> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> > >> epf_bar = &epf->bar[bar];
> > >> + /*
> > >> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> > >> + * if the specific implementation required a 64-bit BAR,
> > >> + * even if we only requested a 32-bit BAR.
> > >> + */
> >
> > set_bar shouldn't set PCI_BASE_ADDRESS_MEM_TYPE_64. If a platform supports only
> > 64-bit BAR, that should be specified in epc_features bar_fixed_64bit member.
> >
> > Thanks
> > Kishon
Hi Alan,
On 25/05/19 12:20 AM, Alan Mikhak wrote:
> Hi Kishon,
>
> Yes. This change is still applicable even when the platform specifies
> that it only supports 64-bit BARs by setting the bar_fixed_64bit
> member of epc_features.
>
> The issue being fixed is this: If the 'continue' statement is executed
> within the loop, the loop index 'bar' needs to advanced by two, not
> one, when the BAR is 64-bit. Otherwise the next loop iteration will be
> on an odd BAR which doesn't exist.
IIUC you are fixing the case where the BAR is "reserved" (specified in
epc_features) and is also a 64-bit BAR?
If 2 consecutive BARs are marked as reserved in reserved_bar of epc_features,
the result should be the same right?
Thanks
Kishon
>
> The PCI_BASE_ADDRESS_MEM_TYPE_64 flag in epf_bar->flag reflects the
> value set by the platform in the bar_fixed_64bit member of
> epc_features.
>
> This patch moves the checking of PCI_BASE_ADDRESS_MEM_TYPE_64 in
> epf_bar->flags to before the 'continue' statement to advance the 'bar'
> loop index accordingly. The comment you see about 'pci_epc_set_bar()'
> preceding the moved code is the original comment and was also moved
> along with the code.
>
> Regards,
> Alan Mikhak
>
> On Fri, May 24, 2019 at 1:51 AM Kishon Vijay Abraham I <[email protected]> wrote:
>>
>> Hi,
>>
>> On 24/05/19 5:25 AM, Alan Mikhak wrote:
>>> +Bjorn Helgaas, +Gustavo Pimentel, +Wen Yang, +Kangjie Lu
>>>
>>> On Thu, May 23, 2019 at 2:55 PM Alan Mikhak <[email protected]> wrote:
>>>>
>>>> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
>>>> and pci_epf_test_alloc_space().
>>>>
>>>> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
>>>> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
>>>> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
>>>> but leaks on subsequent unbind by not calling pci_epf_free_space().
>>>>
>>>> Signed-off-by: Alan Mikhak <[email protected]>
>>>> Reviewed-by: Paul Walmsley <[email protected]>
>>>> ---
>>>> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
>>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> index 27806987e93b..96156a537922 100644
>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>>>>
>>>> static int pci_epf_test_set_bar(struct pci_epf *epf)
>>>> {
>>>> - int bar;
>>>> + int bar, add;
>>>> int ret;
>>>> struct pci_epf_bar *epf_bar;
>>>> struct pci_epc *epc = epf->epc;
>>>> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>>>>
>>>> epc_features = epf_test->epc_features;
>>>>
>>>> - for (bar = BAR_0; bar <= BAR_5; bar++) {
>>>> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
>>>> epf_bar = &epf->bar[bar];
>>>> + /*
>>>> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
>>>> + * if the specific implementation required a 64-bit BAR,
>>>> + * even if we only requested a 32-bit BAR.
>>>> + */
>>
>> set_bar shouldn't set PCI_BASE_ADDRESS_MEM_TYPE_64. If a platform supports only
>> 64-bit BAR, that should be specified in epc_features bar_fixed_64bit member.
>>
>> Thanks
>> Kishon
On Thu, May 30, 2019 at 9:37 PM Kishon Vijay Abraham I <[email protected]> wrote:
>
> Hi Alan,
>
> On 25/05/19 12:20 AM, Alan Mikhak wrote:
> > Hi Kishon,
> >
> > Yes. This change is still applicable even when the platform specifies
> > that it only supports 64-bit BARs by setting the bar_fixed_64bit
> > member of epc_features.
> >
> > The issue being fixed is this: If the 'continue' statement is executed
> > within the loop, the loop index 'bar' needs to advanced by two, not
> > one, when the BAR is 64-bit. Otherwise the next loop iteration will be
> > on an odd BAR which doesn't exist.
>
> IIUC you are fixing the case where the BAR is "reserved" (specified in
> epc_features) and is also a 64-bit BAR?
Correct. If BAR0 is specified in epc_features as reserved and also
64-bit, the loop would skip BAR0 by executing the 'continue'
statement. In this scenario, BAR1 doesn't exist since the 64-bit BAR0
spans the two 32-bit BAR0 and BAR1. The loop index 'bar' would be
advanced by 2 in this case so on the next iteration the loop would
process BAR2.
> If 2 consecutive BARs are marked as reserved in reserved_bar of epc_features,
> the result should be the same right?
If both BAR0 and BAR2 are reserved and also 64-bit, the loop would
check BAR0 on its first iteration and skip BAR0 and BAR1, check BAR2
on its second iteration and skip BAR2 and BAR3, then check BAR4 on its
third iteration. If BAR4 is also 64-bit and reserved, the loop would
skip BAR4 and BAR5 and that would be the final iteration of the loop.
Regards,
Alan
On 24/05/19 3:25 AM, Alan Mikhak wrote:
> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
> and pci_epf_test_alloc_space().
>
> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
> but leaks on subsequent unbind by not calling pci_epf_free_space().
>
> Signed-off-by: Alan Mikhak <[email protected]>
> Reviewed-by: Paul Walmsley <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 27806987e93b..96156a537922 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>
> static int pci_epf_test_set_bar(struct pci_epf *epf)
> {
> - int bar;
> + int bar, add;
> int ret;
> struct pci_epf_bar *epf_bar;
> struct pci_epc *epc = epf->epc;
> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>
> epc_features = epf_test->epc_features;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + /*
> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> + * if the specific implementation required a 64-bit BAR,
> + * even if we only requested a 32-bit BAR.
> + */
> + add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
>
> if (!!(epc_features->reserved_bar & (1 << bar)))
> continue;
> @@ -413,13 +419,6 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> if (bar == test_reg_bar)
> return ret;
> }
> - /*
> - * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> - * if the specific implementation required a 64-bit BAR,
> - * even if we only requested a 32-bit BAR.
> - */
> - if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> }
>
> return 0;
> @@ -431,7 +430,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> struct device *dev = &epf->dev;
> struct pci_epf_bar *epf_bar;
> void *base;
> - int bar;
> + int bar, add;
> enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> const struct pci_epc_features *epc_features;
>
> @@ -445,8 +444,10 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> }
> epf_test->reg[test_reg_bar] = base;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
> +
> if (bar == test_reg_bar)
> continue;
>
> @@ -459,8 +460,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> dev_err(dev, "Failed to allocate space for BAR%d\n",
> bar);
> epf_test->reg[bar] = base;
> - if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> }
>
> return 0;
>
On Thu, May 23, 2019 at 02:55:40PM -0700, Alan Mikhak wrote:
> Always skip odd bar when skipping 64bit BARs in pci_epf_test_set_bar()
> and pci_epf_test_alloc_space().
>
> Otherwise, pci_epf_test_set_bar() will call pci_epc_set_bar() on odd loop
> index when skipping reserved 64bit BAR. Moreover, pci_epf_test_alloc_space()
> will call pci_epf_alloc_space() on bind for odd loop index when BAR is 64bit
> but leaks on subsequent unbind by not calling pci_epf_free_space().
>
> Signed-off-by: Alan Mikhak <[email protected]>
> Reviewed-by: Paul Walmsley <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
Applied to pci/endpoint for v5.3, thanks.
Lorenzo
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 27806987e93b..96156a537922 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -389,7 +389,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>
> static int pci_epf_test_set_bar(struct pci_epf *epf)
> {
> - int bar;
> + int bar, add;
> int ret;
> struct pci_epf_bar *epf_bar;
> struct pci_epc *epc = epf->epc;
> @@ -400,8 +400,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>
> epc_features = epf_test->epc_features;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + /*
> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> + * if the specific implementation required a 64-bit BAR,
> + * even if we only requested a 32-bit BAR.
> + */
> + add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
>
> if (!!(epc_features->reserved_bar & (1 << bar)))
> continue;
> @@ -413,13 +419,6 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> if (bar == test_reg_bar)
> return ret;
> }
> - /*
> - * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> - * if the specific implementation required a 64-bit BAR,
> - * even if we only requested a 32-bit BAR.
> - */
> - if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> }
>
> return 0;
> @@ -431,7 +430,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> struct device *dev = &epf->dev;
> struct pci_epf_bar *epf_bar;
> void *base;
> - int bar;
> + int bar, add;
> enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> const struct pci_epc_features *epc_features;
>
> @@ -445,8 +444,10 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> }
> epf_test->reg[test_reg_bar] = base;
>
> - for (bar = BAR_0; bar <= BAR_5; bar++) {
> + for (bar = BAR_0; bar <= BAR_5; bar += add) {
> epf_bar = &epf->bar[bar];
> + add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
> +
> if (bar == test_reg_bar)
> continue;
>
> @@ -459,8 +460,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> dev_err(dev, "Failed to allocate space for BAR%d\n",
> bar);
> epf_test->reg[bar] = base;
> - if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> - bar++;
> }
>
> return 0;
> --
> 2.7.4
>