[PATCH 1/2] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[PATCH 1/2] misc: pci_endpoint_test: Fix irq_type to convey the correct type

[Kunihiko Hayashi]

There are two variables that indicate the interrupt type to be used
in the next test execution, global "irq_type" and test->irq_type.

The former is referenced from pci_endpoint_test_get_irq() to preserve
the current type for ioctl(PCITEST_GET_IRQTYPE).

In pci_endpoint_test_request_irq(), since this global variable is
referenced when an error occurs, the unintended error message is
displayed.

And the type set in pci_endpoint_test_set_irq() isn't reflected in
the global "irq_type", so ioctl(PCITEST_GET_IRQTYPE) returns the previous
type. As a result, the wrong type will be displayed in "pcitest".

This patch fixes these two issues.

Fixes: b2ba9225e031 ("misc: pci_endpoint_test: Avoid using module parameter to determine irqtype")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/misc/pci_endpoint_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index e73b3078cdb6..854480921470 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -235,7 +235,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 	return true;
 
 fail:
-	switch (irq_type) {
+	switch (test->irq_type) {
 	case IRQ_TYPE_INTX:
 		dev_err(dev, "Failed to request IRQ %d for Legacy\n",
 			pci_irq_vector(pdev, i));
@@ -739,6 +739,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	if (!pci_endpoint_test_request_irq(test))
 		goto err;
 
+	irq_type = test->irq_type;
 	return true;
 
 err:
-- 
2.25.1

[PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Kunihiko Hayashi]

There are bar numbers that cannot be used on the endpoint.
So instead of SoC-specific conditions, add "reserved_bar" bar number
bitmap to the SoC data.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/misc/pci_endpoint_test.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 854480921470..e6a1a2916425 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -76,9 +76,6 @@
 #define PCI_DEVICE_ID_LS1088A			0x80c0
 #define PCI_DEVICE_ID_IMX8			0x0808
 
-#define is_am654_pci_dev(pdev)		\
-		((pdev)->device == PCI_DEVICE_ID_TI_AM654)
-
 #define PCI_DEVICE_ID_RENESAS_R8A774A1		0x0028
 #define PCI_DEVICE_ID_RENESAS_R8A774B1		0x002b
 #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
@@ -123,6 +120,7 @@ struct pci_endpoint_test {
 	struct miscdevice miscdev;
 	enum pci_barno test_reg_bar;
 	size_t alignment;
+	u32 reserved_bar;
 	const char *name;
 };
 
@@ -130,6 +128,7 @@ struct pci_endpoint_test_data {
 	enum pci_barno test_reg_bar;
 	size_t alignment;
 	int irq_type;
+	u32 reserved_bar;
 };
 
 static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
@@ -753,7 +752,6 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	int ret = -EINVAL;
 	enum pci_barno bar;
 	struct pci_endpoint_test *test = to_endpoint_test(file->private_data);
-	struct pci_dev *pdev = test->pdev;
 
 	mutex_lock(&test->mutex);
 
@@ -765,7 +763,7 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 		bar = arg;
 		if (bar > BAR_5)
 			goto ret;
-		if (is_am654_pci_dev(pdev) && bar == BAR_0)
+		if (BIT(bar) & test->reserved_bar)
 			goto ret;
 		ret = pci_endpoint_test_bar(test, bar);
 		break;
@@ -840,6 +838,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		test_reg_bar = data->test_reg_bar;
 		test->test_reg_bar = test_reg_bar;
 		test->alignment = data->alignment;
+		test->reserved_bar = data->reserved_bar;
 		irq_type = data->irq_type;
 	}
 
@@ -991,6 +990,7 @@ static const struct pci_endpoint_test_data am654_data = {
 	.test_reg_bar = BAR_2,
 	.alignment = SZ_64K,
 	.irq_type = IRQ_TYPE_MSI,
+	.reserved_bar = BIT(BAR_0),
 };
 
 static const struct pci_endpoint_test_data j721e_data = {
-- 
2.25.1

Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Niklas Cassel]

Hello Hayashisan,

On Mon, Dec 16, 2024 at 04:39:41PM +0900, Kunihiko Hayashi wrote:
> There are bar numbers that cannot be used on the endpoint.
> So instead of SoC-specific conditions, add "reserved_bar" bar number
> bitmap to the SoC data.

I think that it was mistake to put is_am654_pci_dev() checks in
pci_endpoint_test.c in the first place. However, let's not make the
situation worse by introducing a reserved_bar bitmap on the host side as
well.

IMO, we should not have any logic for this the host side at all.


Just like for am654, rk3588 has a BAR (BAR4) that should not be written by
pci_endpoint_test.c (as it exposes the ATU registers in BAR4, so if the
host writes this BAR, all address translation will be broken).

An EPC driver can mark a BAR as reserved and that is exactly was rk3588
does for BAR4:
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L300

Marking a BAR as reserved means that an EPF driver should not touch that
BAR at all.

However, this by itself is not enough if the BAR is enabled by default,
in that case we also need to disable the BAR for the host side to not
be able to write to it:
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L248-L249


If we look at am654, we can see that it does set BAR0 and BAR1 as reserved:
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pci-keystone.c#L967-L968

The problem is that am654 does not also disable these BARs by default.


If you look at most DWC based EPC drivers:
drivers/pci/controller/dwc/pci-dra7xx.c:                dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pci-imx6.c:          dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pci-layerscape-ep.c:         dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pcie-artpec6.c:              dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pcie-designware-plat.c:              dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pcie-dw-rockchip.c:          dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pcie-qcom-ep.c:              dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pcie-rcar-gen4.c:            dw_pcie_ep_reset_bar(pci, bar);
drivers/pci/controller/dwc/pcie-uniphier-ep.c:          dw_pcie_ep_reset_bar(pci, bar);

They call dw_pcie_ep_reset_bar() in EP init for all BARs.
(An EPF driver will be able to re-enable all BARs that are not marked as
reserved.)

am654 seems to be the exception here.


If you simply add code that disables all BARs by default in am654, you
should be able to remove these ugly is_am654_pci_dev() checks in the host
driver, and the host driver should not be able to write to these reserved
BARs, as they will never get enabled by pci-epf-test.c.


Kind regards,
Niklas

Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Kunihiko Hayashi]

Hi Niklas,

Thank you for your comment.

On 2024/12/17 17:19, Niklas Cassel wrote:
> Hello Hayashisan,
> 
> On Mon, Dec 16, 2024 at 04:39:41PM +0900, Kunihiko Hayashi wrote:
>> There are bar numbers that cannot be used on the endpoint.
>> So instead of SoC-specific conditions, add "reserved_bar" bar number
>> bitmap to the SoC data.
> 
> I think that it was mistake to put is_am654_pci_dev() checks in
> pci_endpoint_test.c in the first place. However, let's not make the
> situation worse by introducing a reserved_bar bitmap on the host side as
> well.
> 
> IMO, we should not have any logic for this the host side at all.

I also think that is_am654_pci_dev() isn't good solution, and I agree 
with you.

> Just like for am654, rk3588 has a BAR (BAR4) that should not be written by
> pci_endpoint_test.c (as it exposes the ATU registers in BAR4, so if the
> host writes this BAR, all address translation will be broken).
> 
> An EPC driver can mark a BAR as reserved and that is exactly was rk3588
> does for BAR4:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L300
> 
> Marking a BAR as reserved means that an EPF driver should not touch that
> BAR at all.
> 
> However, this by itself is not enough if the BAR is enabled by default,
> in that case we also need to disable the BAR for the host side to not
> be able to write to it:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L248-L249
> 
> 
> If we look at am654, we can see that it does set BAR0 and BAR1 as reserved:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pci-keystone.c#L967-L968
> 
> The problem is that am654 does not also disable these BARs by default.
> 
> 
> If you look at most DWC based EPC drivers:
> drivers/pci/controller/dwc/pci-dra7xx.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pci-imx6.c:          dw_pcie_ep_reset_bar(pci,
> bar);
> drivers/pci/controller/dwc/pci-layerscape-ep.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-artpec6.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-designware-plat.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-dw-rockchip.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-qcom-ep.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-rcar-gen4.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-uniphier-ep.c:
> dw_pcie_ep_reset_bar(pci, bar);
> 
> They call dw_pcie_ep_reset_bar() in EP init for all BARs.
> (An EPF driver will be able to re-enable all BARs that are not marked as
> reserved.)
> 
> am654 seems to be the exception here.

The endpoint driver including am654 should surely call 
dw_pcie_ep_reset_bar() to initialize BARs at first.

> If you simply add code that disables all BARs by default in am654, you
> should be able to remove these ugly is_am654_pci_dev() checks in the host
> driver, and the host driver should not be able to write to these reserved
> BARs, as they will never get enabled by pci-epf-test.c.

However, dw_pcie_ep_reset_bar() only clears BAR registers to 0x0. BAR 
doesn't have any "disabled" field, so I think that it means "32-bit, 
memory, non-prefetchable".

 
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L47-L52

And even if each endpoint driver marks "BAR_RESERVED" to the features, 
it is only referred to as excluded BARs when searching for free BARs. So 
the host will recognize this "reserved" BAR.

 
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/endpoint/pci-epc-core.c#L123-L124

I've not actually used am654, but for example, when I tried to execute 
BAR test on the other SoC that has inaccessible BAR (marked as 
"reserved"), AER reported an uncorrectable bus error.
This behavior depends on the bus connection to the BAR.

It seems that access to the BAR is unavoidable even if calling 
dw_pci_ep_reset_bar() and marking as BAR_RESERVED.

But I don't have any other idea to avoid access to reserved BARs,
so this patch [2/2] will be withdrawn.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Niklas Cassel]

On Thu, Dec 19, 2024 at 08:17:50PM +0900, Kunihiko Hayashi wrote:
> On 2024/12/17 17:19, Niklas Cassel wrote:
>
> > If you simply add code that disables all BARs by default in am654, you
> > should be able to remove these ugly is_am654_pci_dev() checks in the host
> > driver, and the host driver should not be able to write to these reserved
> > BARs, as they will never get enabled by pci-epf-test.c.
>
> However, dw_pcie_ep_reset_bar() only clears BAR registers to 0x0. BAR
> doesn't have any "disabled" field, so I think that it means "32-bit, memory,
> non-prefetchable".

From the DWC databook, 4.60a, section "6.1.2 BAR Details",
heading "Disabling a BAR".

"To disable a BAR (in any of the three schemes), your application can
write ‘0’ to the LSB of the BAR mask register."

dw_pcie_ep_reset_bar() calls __dw_pcie_ep_reset_bar(), which will
write a zero to the LSB of the BAR mask register:
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L50


>
>
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L47-L52
>
> And even if each endpoint driver marks "BAR_RESERVED" to the features, it is
> only referred to as excluded BARs when searching for free BARs. So the host
> will recognize this "reserved" BAR.

A BAR that has been disabled on the EP side, will not have a size/
be visible on host side.

Like I said, rk3588 calls dw_pcie_ep_reset_bar() on all BARs in EP init,
like most DWC based EPC drivers, and marks BAR4 as reserved.
This is how it looks on the host side during enumeration:

[   25.496645] pci 0000:01:00.0: [1d87:3588] type 00 class 0xff0000 PCIe Endpoint
[   25.497322] pci 0000:01:00.0: BAR 0 [mem 0x00000000-0x000fffff]
[   25.497863] pci 0000:01:00.0: BAR 1 [mem 0x00000000-0x000fffff]
[   25.498400] pci 0000:01:00.0: BAR 2 [mem 0x00000000-0x000fffff]
[   25.498937] pci 0000:01:00.0: BAR 3 [mem 0x00000000-0x000fffff]
[   25.499498] pci 0000:01:00.0: BAR 5 [mem 0x00000000-0x000fffff]
[   25.500036] pci 0000:01:00.0: ROM [mem 0x00000000-0x0000ffff pref]
[   25.500861] pci 0000:01:00.0: supports D1 D2
[   25.501240] pci 0000:01:00.0: PME# supported from D0 D1 D3hot

Likewise the looping in pci_endpoint_test.c will skip disabled BARs, e.g.:
https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/misc/pci_endpoint_test.c#L940-L943

Since test->bar[bar] will be NULL for BARs that are disabled.


Kind regards,
Niklas

Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Kunihiko Hayashi]

Hi Niklas,

On 2024/12/19 22:08, Niklas Cassel wrote:
> On Thu, Dec 19, 2024 at 08:17:50PM +0900, Kunihiko Hayashi wrote:
>> On 2024/12/17 17:19, Niklas Cassel wrote:
>>
>>> If you simply add code that disables all BARs by default in am654, you
>>> should be able to remove these ugly is_am654_pci_dev() checks in the
>>> host
>>> driver, and the host driver should not be able to write to these
>>> reserved
>>> BARs, as they will never get enabled by pci-epf-test.c.
>>
>> However, dw_pcie_ep_reset_bar() only clears BAR registers to 0x0. BAR
>> doesn't have any "disabled" field, so I think that it means "32-bit,
>> memory,
>> non-prefetchable".
> 
>  From the DWC databook, 4.60a, section "6.1.2 BAR Details",
> heading "Disabling a BAR".
> 
> "To disable a BAR (in any of the three schemes), your application can
> write ‘0’ to the LSB of the BAR mask register."
> 
> dw_pcie_ep_reset_bar() calls __dw_pcie_ep_reset_bar(), which will
> write a zero to the LSB of the BAR mask register:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L50

Now I missed it, and also find this description in the databook.

>>
>> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L47-L52
>>
>> And even if each endpoint driver marks "BAR_RESERVED" to the features, it
>> is
>> only referred to as excluded BARs when searching for free BARs. So the
>> host
>> will recognize this "reserved" BAR.
> 
> A BAR that has been disabled on the EP side, will not have a size/
> be visible on host side.
> 
> Like I said, rk3588 calls dw_pcie_ep_reset_bar() on all BARs in EP init,
> like most DWC based EPC drivers, and marks BAR4 as reserved.
> This is how it looks on the host side during enumeration:
> 
> [   25.496645] pci 0000:01:00.0: [1d87:3588] type 00 class 0xff0000 PCIe
> Endpoint
> [   25.497322] pci 0000:01:00.0: BAR 0 [mem 0x00000000-0x000fffff]
> [   25.497863] pci 0000:01:00.0: BAR 1 [mem 0x00000000-0x000fffff]
> [   25.498400] pci 0000:01:00.0: BAR 2 [mem 0x00000000-0x000fffff]
> [   25.498937] pci 0000:01:00.0: BAR 3 [mem 0x00000000-0x000fffff]
> [   25.499498] pci 0000:01:00.0: BAR 5 [mem 0x00000000-0x000fffff]
> [   25.500036] pci 0000:01:00.0: ROM [mem 0x00000000-0x0000ffff pref]
> [   25.500861] pci 0000:01:00.0: supports D1 D2
> [   25.501240] pci 0000:01:00.0: PME# supported from D0 D1 D3hot

Surely writing 0 to dbi2.BARn (BAR mask) register disables BARn.
I tried to do that using UniPhier SoC, and found disabling any BARs in
the same way.

On the other hand, some other SoCs might have BAR masks fixed by the DWC
IP configuration. These BARs will be exposed to the host even if the BAR
mask is set to 0. However, such case hasn't been upstreamed, so there is
no need to worry about them now.

The am654 driver just doesn't call dw_pcie_ep_reset_bar(), so this case
probably doesn't apply.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Niklas Cassel]

On Mon, Dec 23, 2024 at 08:51:42PM +0900, Kunihiko Hayashi wrote:
> On 2024/12/19 22:08, Niklas Cassel wrote:
> > On Thu, Dec 19, 2024 at 08:17:50PM +0900, Kunihiko Hayashi wrote:
> > > On 2024/12/17 17:19, Niklas Cassel wrote:

[...]

> On the other hand, some other SoCs might have BAR masks fixed by the DWC
> IP configuration. These BARs will be exposed to the host even if the BAR
> mask is set to 0. However, such case hasn't been upstreamed, so there is
> no need to worry about them now.

The three schemes are:
BARn_SIZING_SCHEME_N =“Fixed Mask” (0)
BARn_SIZING_SCHEME_N =“Programmable Mask” (1)
BARn_SIZING_SCHEME_N =“Resizable BAR” (2)

Considering that the text:
"To disable a BAR (in any of the three schemes), your application can
write ‘0’ to the LSB of the BAR mask register."

says "in any of the three schemes", I would expect writing 0 to BAR_MASK
should disable a BAR, even for a Fixed Mask/Fixed BAR.


Kind regards,
Niklas

Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Kunihiko Hayashi]

Hi Niklas,

On 2024/12/23 21:05, Niklas Cassel wrote:
> On Mon, Dec 23, 2024 at 08:51:42PM +0900, Kunihiko Hayashi wrote:
>> On 2024/12/19 22:08, Niklas Cassel wrote:
>>> On Thu, Dec 19, 2024 at 08:17:50PM +0900, Kunihiko Hayashi wrote:
>>>> On 2024/12/17 17:19, Niklas Cassel wrote:
> 
> [...]
> 
>> On the other hand, some other SoCs might have BAR masks fixed by the DWC
>> IP configuration. These BARs will be exposed to the host even if the BAR
>> mask is set to 0. However, such case hasn't been upstreamed, so there is
>> no need to worry about them now.
> 
> The three schemes are:
> BARn_SIZING_SCHEME_N =“Fixed Mask” (0)
> BARn_SIZING_SCHEME_N =“Programmable Mask” (1)
> BARn_SIZING_SCHEME_N =“Resizable BAR” (2)
> 
> Considering that the text:
> "To disable a BAR (in any of the three schemes), your application can
> write ‘0’ to the LSB of the BAR mask register."
> 
> says "in any of the three schemes", I would expect writing 0 to BAR_MASK
> should disable a BAR, even for a Fixed Mask/Fixed BAR.
The behavior in my case seemed suspicious "in any of the three schemes",
so I investigated and found it was an issue with specifying dbi2
address.

I confirmed disabing a BAR with writing 0 to the BAR mask.

Thank you for your explanation.
Again, this patch [2/2] will be withdrawn, and I expect that the
condition of the test and endpoint BAR reset for am654 will be fixed.

Thank you,

---
Best Regards
Kunihiko Hayashi