[PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Andrew Donnellan]

cxl_pci_enable_device_hook() is called when attempting to enable an AFU
sitting on a vPHB. At present, the state of the underlying CXL card's PCI
channel is only checked when it calls cxl_afu_check_and_enable() at the
very end, after it has already set DMA options and initialised a default
context.

Check the CXL card's link status before setting DMA options or initialising
a default context. If the link is down, print a warning and return
immediately.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
 drivers/misc/cxl/vphb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
index 6dd16a6..1972fb6 100644
--- a/drivers/misc/cxl/vphb.c
+++ b/drivers/misc/cxl/vphb.c
@@ -48,6 +48,11 @@ static bool cxl_pci_enable_device_hook(struct pci_dev *dev)
 
 	phb = pci_bus_to_host(dev->bus);
 	afu = (struct cxl_afu *)phb->private_data;
+
+	if (!cxl_adapter_link_ok(afu->adapter))
+		dev_warn(&dev->dev, "%s: Device link is down, refusing to enable AFU\n", __func__);
+		return false;
+
 	set_dma_ops(&dev->dev, &dma_direct_ops);
 	set_dma_offset(&dev->dev, PAGE_OFFSET);
 
-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Ian Munsie]

Excerpts from andrew.donnellan's message of 2015-09-04 16:20:24 +1000:
> +    if (!cxl_adapter_link_ok(afu->adapter))
> +        dev_warn(&dev->dev, "%s: Device link is down, refusing to enable AFU\n", __func__);
> +        return false;

Based on the indentation it looks like you meant to add some curly
brackets here.

-Ian

Re: [PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Andrew Donnellan]

On 07/09/15 09:58, Ian Munsie wrote:
> Excerpts from andrew.donnellan's message of 2015-09-04 16:20:24 +1000:
>> +    if (!cxl_adapter_link_ok(afu->adapter))
>> +        dev_warn(&dev->dev, "%s: Device link is down, refusing to enable AFU\n", __func__);
>> +        return false;
>
> Based on the indentation it looks like you meant to add some curly
> brackets here.

Indeed I did, and I'd even thought I'd tested it... this is why I 
shouldn't be allowed to submit patches at 4pm on Fridays...

Will submit V2.

(FWIW, I'm a firm believer in braces around single statements in 
conditionals to avoid mistakes like this, but CodingStyle disagrees...)


Andrew


-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Ian Munsie]

No worries. I've been caught out by something similar in the past when I
assumed that something like this:

if (foo)
	/*
	 * A big long
	 * multiline
	 * block comment!
	 */
	 do_something()

would surely already have curly brackets around it ;-)

Excerpts from andrew.donnellan's message of 2015-09-07 10:22:31 +1000:
> 
> On 07/09/15 09:58, Ian Munsie wrote:
> > Excerpts from andrew.donnellan's message of 2015-09-04 16:20:24 +1000:
> >> +    if (!cxl_adapter_link_ok(afu->adapter))
> >> +        dev_warn(&dev->dev, "%s: Device link is down, refusing to enable AFU\n", __func__);
> >> +        return false;
> >
> > Based on the indentation it looks like you meant to add some curly
> > brackets here.
> 
> Indeed I did, and I'd even thought I'd tested it... this is why I 
> shouldn't be allowed to submit patches at 4pm on Fridays...
> 
> Will submit V2.
> 
> (FWIW, I'm a firm believer in braces around single statements in 
> conditionals to avoid mistakes like this, but CodingStyle disagrees...)
> 
> 
> Andrew
> 

Re: [PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Michael Ellerman]

On Mon, 2015-09-07 at 16:33 +1000, Ian Munsie wrote:
> No worries. I've been caught out by something similar in the past when I
> assumed that something like this:
> 
> if (foo)
> 	/*
> 	 * A big long
> 	 * multiline
> 	 * block comment!
> 	 */
> 	 do_something()
> 
> would surely already have curly brackets around it ;-)

You guys can do whatever you like for the cxl code.

cheers

[PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Andrew Donnellan]

cxl_pci_enable_device_hook() is called when attempting to enable an AFU
sitting on a vPHB. At present, the state of the underlying CXL card's PCI
channel is only checked when it calls cxl_afu_check_and_enable() at the
very end, after it has already set DMA options and initialised a default
context.

Check the CXL card's link status before setting DMA options or initialising
a default context. If the link is down, print a warning and return
immediately.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
 drivers/misc/cxl/vphb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
index 6dd16a6..94b5208 100644
--- a/drivers/misc/cxl/vphb.c
+++ b/drivers/misc/cxl/vphb.c
@@ -48,6 +48,12 @@ static bool cxl_pci_enable_device_hook(struct pci_dev *dev)
 
 	phb = pci_bus_to_host(dev->bus);
 	afu = (struct cxl_afu *)phb->private_data;
+
+	if (!cxl_adapter_link_ok(afu->adapter)) {
+		dev_warn(&dev->dev, "%s: Device link is down, refusing to enable AFU\n", __func__);
+		return false;
+	}
+
 	set_dma_ops(&dev->dev, &dma_direct_ops);
 	set_dma_offset(&dev->dev, PAGE_OFFSET);
 
-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Ian Munsie]

That looks better :)

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

[PATCH v2] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Andrew Donnellan]

cxl_pci_enable_device_hook() is called when attempting to enable an AFU
sitting on a vPHB. At present, the state of the underlying CXL card's PCI
channel is only checked when it calls cxl_afu_check_and_enable() at the
very end, after it has already set DMA options and initialised a default
context.

Check the CXL card's link status before setting DMA options or initialising
a default context. If the link is down, print a warning and return
immediately.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
 drivers/misc/cxl/vphb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
index 6dd16a6..94b5208 100644
--- a/drivers/misc/cxl/vphb.c
+++ b/drivers/misc/cxl/vphb.c
@@ -48,6 +48,12 @@ static bool cxl_pci_enable_device_hook(struct pci_dev *dev)
 
 	phb = pci_bus_to_host(dev->bus);
 	afu = (struct cxl_afu *)phb->private_data;
+
+	if (!cxl_adapter_link_ok(afu->adapter)) {
+		dev_warn(&dev->dev, "%s: Device link is down, refusing to enable AFU\n", __func__);
+		return false;
+	}
+
 	set_dma_ops(&dev->dev, &dma_direct_ops);
 	set_dma_offset(&dev->dev, PAGE_OFFSET);
 
-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH v2] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Ian Munsie]

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Re: [PATCH v2] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Michael Ellerman]

On Mon, 2015-09-07 at 10:52 +1000, Andrew Donnellan wrote:
> cxl_pci_enable_device_hook() is called when attempting to enable an AFU
> sitting on a vPHB. At present, the state of the underlying CXL card's PCI
> channel is only checked when it calls cxl_afu_check_and_enable() at the
> very end, after it has already set DMA options and initialised a default
> context.
> 
> Check the CXL card's link status before setting DMA options or initialising
> a default context. If the link is down, print a warning and return
> immediately.

Is this a fix targetting 4.3 (current release) and/or stable, or something for
next?

cheers

Re: [PATCH v2] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Andrew Donnellan]

On 07/09/15 12:53, Michael Ellerman wrote:
> Is this a fix targetting 4.3 (current release) and/or stable, or something for
> next?

4.3 - this fix is related to cxl's EEH support which has been merged for 
4.3.

For the record, since I neglected to add this earlier:

---
Changes from v1:
- Insert braces so that "return false" is within if block


-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [v2] cxl: abort cxl_pci_enable_device_hook() if PCI channel is offline

[Michael Ellerman]

On Mon, 2015-07-09 at 00:52:58 UTC, Andrew Donnellan wrote:
> cxl_pci_enable_device_hook() is called when attempting to enable an AFU
> sitting on a vPHB. At present, the state of the underlying CXL card's PCI
> channel is only checked when it calls cxl_afu_check_and_enable() at the
> very end, after it has already set DMA options and initialised a default
> context.
> 
> Check the CXL card's link status before setting DMA options or initialising
> a default context. If the link is down, print a warning and return
> immediately.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/7d1647dc4ba0a61fec5381c1

cheers