* Re: [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register
2019-07-25 19:25 [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register Sumit Saxena
@ 2019-07-25 11:58 ` Koenig, Christian
2019-08-07 23:01 ` Bjorn Helgaas
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Koenig, Christian @ 2019-07-25 11:58 UTC (permalink / raw)
To: Sumit Saxena, helgaas; +Cc: linux-pci, stable
Am 25.07.19 um 21:25 schrieb Sumit Saxena:
> In Resize BAR control register, bits[8:12] represents size of BAR.
> As per PCIe specification, below is encoded values in register bits
> to actual BAR size table:
>
> Bits BAR size
> 0 1 MB
> 1 2 MB
> 2 4 MB
> 3 8 MB
> --
>
> For 1 MB BAR size, BAR size bits should be set to 0 but incorrectly
> these bits are set to "1f". Latest megaraid_sas and mpt3sas adapters
> which support Resizable BAR with 1 MB BAR size fails to initialize
> during system resume from S3 sleep.
>
> Fix: Correctly calculate BAR size bits for Resize BAR control register.
>
> V2:
> -Simplified calculation of BAR size bits as suggested by Christian Koenig.
>
> CC: stable@vger.kernel.org # v4.16+
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
> Fixes: d3252ace0bc652a1a244455556b6a549f969bf99 ("PCI: Restore resized BAR state on resume")
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1ac27..e59921296125 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> res = pdev->resource + bar_idx;
> - size = order_base_2((resource_size(res) >> 20) | 1) - 1;
> + size = order_base_2(resource_size(res) >> 20);
> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register
2019-07-25 19:25 [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register Sumit Saxena
2019-07-25 11:58 ` Koenig, Christian
@ 2019-08-07 23:01 ` Bjorn Helgaas
2019-08-08 7:01 ` Koenig, Christian
2019-08-07 23:17 ` Bjorn Helgaas
2019-08-07 23:27 ` Bjorn Helgaas
3 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2019-08-07 23:01 UTC (permalink / raw)
To: Sumit Saxena; +Cc: Christian.Koenig, linux-pci, stable
On Fri, Jul 26, 2019 at 12:55:52AM +0530, Sumit Saxena wrote:
> In Resize BAR control register, bits[8:12] represents size of BAR.
> As per PCIe specification, below is encoded values in register bits
> to actual BAR size table:
>
> Bits BAR size
> 0 1 MB
> 1 2 MB
> 2 4 MB
> 3 8 MB
> --
>
> For 1 MB BAR size, BAR size bits should be set to 0 but incorrectly
> these bits are set to "1f". Latest megaraid_sas and mpt3sas adapters
> which support Resizable BAR with 1 MB BAR size fails to initialize
> during system resume from S3 sleep.
>
> Fix: Correctly calculate BAR size bits for Resize BAR control register.
>
> V2:
> -Simplified calculation of BAR size bits as suggested by Christian Koenig.
>
> CC: stable@vger.kernel.org # v4.16+
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
> Fixes: d3252ace0bc652a1a244455556b6a549f969bf99 ("PCI: Restore resized BAR state on resume")
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
> drivers/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1ac27..e59921296125 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> res = pdev->resource + bar_idx;
> - size = order_base_2((resource_size(res) >> 20) | 1) - 1;
> + size = order_base_2(resource_size(res) >> 20);
Since BAR sizes are always powers of 2, wouldn't this be simpler as:
size = ilog2(resource_size(res)) - 20;
which nicely matches the table in PCIe r5.0, sec 7.8.6.3?
> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> --
> 2.18.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register
2019-08-07 23:01 ` Bjorn Helgaas
@ 2019-08-08 7:01 ` Koenig, Christian
2019-08-08 12:31 ` Bjorn Helgaas
0 siblings, 1 reply; 7+ messages in thread
From: Koenig, Christian @ 2019-08-08 7:01 UTC (permalink / raw)
To: Bjorn Helgaas, Sumit Saxena; +Cc: linux-pci, stable
Am 08.08.19 um 01:01 schrieb Bjorn Helgaas:
> On Fri, Jul 26, 2019 at 12:55:52AM +0530, Sumit Saxena wrote:
>> In Resize BAR control register, bits[8:12] represents size of BAR.
>> As per PCIe specification, below is encoded values in register bits
>> to actual BAR size table:
>>
>> Bits BAR size
>> 0 1 MB
>> 1 2 MB
>> 2 4 MB
>> 3 8 MB
>> --
>>
>> For 1 MB BAR size, BAR size bits should be set to 0 but incorrectly
>> these bits are set to "1f". Latest megaraid_sas and mpt3sas adapters
>> which support Resizable BAR with 1 MB BAR size fails to initialize
>> during system resume from S3 sleep.
>>
>> Fix: Correctly calculate BAR size bits for Resize BAR control register.
>>
>> V2:
>> -Simplified calculation of BAR size bits as suggested by Christian Koenig.
>>
>> CC: stable@vger.kernel.org # v4.16+
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
>> Fixes: d3252ace0bc652a1a244455556b6a549f969bf99 ("PCI: Restore resized BAR state on resume")
>> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
>> ---
>> drivers/pci/pci.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 29ed5ec1ac27..e59921296125 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
>> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
>> bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
>> res = pdev->resource + bar_idx;
>> - size = order_base_2((resource_size(res) >> 20) | 1) - 1;
>> + size = order_base_2(resource_size(res) >> 20);
> Since BAR sizes are always powers of 2, wouldn't this be simpler as:
>
> size = ilog2(resource_size(res)) - 20;
>
> which nicely matches the table in PCIe r5.0, sec 7.8.6.3?
Yeah, that should obviously work as well.
We would have a serious problem in the resource management if the
resource size is smaller than 1MB or not a power of two.
Feel free to add my r-b.
Regards,
Christian.
>
>> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
>> ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
>> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
>> --
>> 2.18.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register
2019-08-08 7:01 ` Koenig, Christian
@ 2019-08-08 12:31 ` Bjorn Helgaas
0 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2019-08-08 12:31 UTC (permalink / raw)
To: Koenig, Christian; +Cc: Sumit Saxena, linux-pci, stable
On Thu, Aug 08, 2019 at 07:01:03AM +0000, Koenig, Christian wrote:
> Am 08.08.19 um 01:01 schrieb Bjorn Helgaas:
> > On Fri, Jul 26, 2019 at 12:55:52AM +0530, Sumit Saxena wrote:
> >> In Resize BAR control register, bits[8:12] represents size of BAR.
> >> As per PCIe specification, below is encoded values in register bits
> >> to actual BAR size table:
> >>
> >> Bits BAR size
> >> 0 1 MB
> >> 1 2 MB
> >> 2 4 MB
> >> 3 8 MB
> >> --
> >>
> >> For 1 MB BAR size, BAR size bits should be set to 0 but incorrectly
> >> these bits are set to "1f". Latest megaraid_sas and mpt3sas adapters
> >> which support Resizable BAR with 1 MB BAR size fails to initialize
> >> during system resume from S3 sleep.
> >>
> >> Fix: Correctly calculate BAR size bits for Resize BAR control register.
> >>
> >> V2:
> >> -Simplified calculation of BAR size bits as suggested by Christian Koenig.
> >>
> >> CC: stable@vger.kernel.org # v4.16+
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
> >> Fixes: d3252ace0bc652a1a244455556b6a549f969bf99 ("PCI: Restore resized BAR state on resume")
> >> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> >> ---
> >> drivers/pci/pci.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index 29ed5ec1ac27..e59921296125 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> >> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> >> bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> >> res = pdev->resource + bar_idx;
> >> - size = order_base_2((resource_size(res) >> 20) | 1) - 1;
> >> + size = order_base_2(resource_size(res) >> 20);
> > Since BAR sizes are always powers of 2, wouldn't this be simpler as:
> >
> > size = ilog2(resource_size(res)) - 20;
> >
> > which nicely matches the table in PCIe r5.0, sec 7.8.6.3?
>
> Yeah, that should obviously work as well.
>
> We would have a serious problem in the resource management if the
> resource size is smaller than 1MB or not a power of two.
Yes, definitely. Resizable BARs are required by spec to be 1MB or
larger, but this does niggle at me a little bit, too. It probably
saves a few bits in pci_dev to recompute this at restore-time, but
honestly, I think it would be more obviously correct to just do the
simple-minded thing of saving and restoring the entire register.
> Feel free to add my r-b.
Done, thanks!
> Regards,
> Christian.
>
> >
> >> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> >> ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> >> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> >> --
> >> 2.18.1
> >>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register
2019-07-25 19:25 [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register Sumit Saxena
2019-07-25 11:58 ` Koenig, Christian
2019-08-07 23:01 ` Bjorn Helgaas
@ 2019-08-07 23:17 ` Bjorn Helgaas
2019-08-07 23:27 ` Bjorn Helgaas
3 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2019-08-07 23:17 UTC (permalink / raw)
To: Sumit Saxena; +Cc: Christian.Koenig, linux-pci, stable
On Fri, Jul 26, 2019 at 12:55:52AM +0530, Sumit Saxena wrote:
> In Resize BAR control register, bits[8:12] represents size of BAR.
> As per PCIe specification, below is encoded values in register bits
> to actual BAR size table:
>
> Bits BAR size
> 0 1 MB
> 1 2 MB
> 2 4 MB
> 3 8 MB
> --
>
> For 1 MB BAR size, BAR size bits should be set to 0 but incorrectly
> these bits are set to "1f". Latest megaraid_sas and mpt3sas adapters
> which support Resizable BAR with 1 MB BAR size fails to initialize
> during system resume from S3 sleep.
>
> Fix: Correctly calculate BAR size bits for Resize BAR control register.
>
> V2:
> -Simplified calculation of BAR size bits as suggested by Christian Koenig.
>
> CC: stable@vger.kernel.org # v4.16+
Also, d3252ace0bc6 ("PCI: Restore resized BAR state on resume") didn't
appear until v4.19. I updated this to "v4.19+"; let me know if that's
wrong.
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
> Fixes: d3252ace0bc652a1a244455556b6a549f969bf99 ("PCI: Restore resized BAR state on resume")
I updated this to conventional format as above (12-char SHA1).
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
> drivers/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1ac27..e59921296125 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> res = pdev->resource + bar_idx;
> - size = order_base_2((resource_size(res) >> 20) | 1) - 1;
> + size = order_base_2(resource_size(res) >> 20);
> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> --
> 2.18.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register
2019-07-25 19:25 [PATCH V2] PCI: set BAR size bits correctly in Resize BAR control register Sumit Saxena
` (2 preceding siblings ...)
2019-08-07 23:17 ` Bjorn Helgaas
@ 2019-08-07 23:27 ` Bjorn Helgaas
3 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2019-08-07 23:27 UTC (permalink / raw)
To: Sumit Saxena; +Cc: Christian.Koenig, linux-pci, stable
On Fri, Jul 26, 2019 at 12:55:52AM +0530, Sumit Saxena wrote:
> In Resize BAR control register, bits[8:12] represents size of BAR.
> As per PCIe specification, below is encoded values in register bits
> to actual BAR size table:
>
> Bits BAR size
> 0 1 MB
> 1 2 MB
> 2 4 MB
> 3 8 MB
> --
>
> For 1 MB BAR size, BAR size bits should be set to 0 but incorrectly
> these bits are set to "1f". Latest megaraid_sas and mpt3sas adapters
> which support Resizable BAR with 1 MB BAR size fails to initialize
> during system resume from S3 sleep.
>
> Fix: Correctly calculate BAR size bits for Resize BAR control register.
>
> V2:
> -Simplified calculation of BAR size bits as suggested by Christian Koenig.
>
> CC: stable@vger.kernel.org # v4.16+
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
> Fixes: d3252ace0bc652a1a244455556b6a549f969bf99 ("PCI: Restore resized BAR state on resume")
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
I applied this to pci/misc for v5.4 with the tweaks I mentioned (see
below).
Christian, I didn't add your Reviewed-by since I changed the patch,
but I'll be glad to update the branch if you take another look.
> ---
> drivers/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1ac27..e59921296125 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> res = pdev->resource + bar_idx;
> - size = order_base_2((resource_size(res) >> 20) | 1) - 1;
> + size = order_base_2(resource_size(res) >> 20);
> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
commit 614b04644b57
Author: Sumit Saxena <sumit.saxena@broadcom.com>
Date: Fri Jul 26 00:55:52 2019 +0530
PCI: Set BAR size bits correctly in Resize BAR control register
In a Resizable BAR Control Register, bits 13:8 control the size of the BAR.
The encoded values of these bits are as follows (see PCIe r5.0, sec
7.8.6.3):
Value BAR size
0 1 MB (2^20 bytes)
1 2 MB (2^21 bytes)
2 4 MB (2^22 bytes)
...
43 8 EB (2^63 bytes)
Previously we incorrectly set the BAR size bits for a 1 MB BAR to 0x1f
instead of 0, so devices that support that size, e.g., new megaraid_sas and
mpt3sas adapters, fail to initialize during resume from S3 sleep.
Correctly calculate the BAR size bits for Resizable BAR control registers.
Link: https://lore.kernel.org/r/20190725192552.24295-1-sumit.saxena@broadcom.com
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203939
Fixes: d3252ace0bc6 ("PCI: Restore resized BAR state on resume")
Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: stable@vger.kernel.org # v4.19+
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index da3241bb4479..5836eb576d96 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1438,7 +1438,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
res = pdev->resource + bar_idx;
- size = order_base_2((resource_size(res) >> 20) | 1) - 1;
+ size = ilog2(resource_size(res)) - 20;
ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
^ permalink raw reply related [flat|nested] 7+ messages in thread