[PATCH] powernv: eeh: add buffer for P7IOC hub error data

[PATCH] powernv: eeh: add buffer for P7IOC hub error data

[Brian W Hart]

Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
a buffer for P7IOC hub diagnostic data.  Take care to inform OPAL of the
correct size for the buffer.

Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
---

I hope I've understood this correctly.  It looks to me like
ioda_eeh_hub_data is effectively asking OPAL to clobber its own
text (via 'data') when it makes the call to retrieve the hub data.

Added a hub diagnostic structure per-phb.  Perhaps the diagnostic
structure better belongs in the phb->diag union, but I wasn't sure whether
we'd need to carry the hub and PHB diag data at the same time.

 arch/powerpc/platforms/powernv/eeh-ioda.c | 4 ++--
 arch/powerpc/platforms/powernv/pci.h      | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 8184ef5..dfd9134 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -636,8 +636,8 @@ static void ioda_eeh_hub_diag(struct pci_controller *hose)
 	struct OpalIoP7IOCErrorData *data;
 	long rc;
 
-	data = (struct OpalIoP7IOCErrorData *)ioda_eeh_hub_diag;
-	rc = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
+	data = (struct OpalIoP7IOCErrorData *)&phb->p7ioc_err;
+	rc = opal_pci_get_hub_diag_data(phb->hub_id, data, sizeof *data);
 	if (rc != OPAL_SUCCESS) {
 		pr_warning("%s: Failed to get HUB#%llx diag-data (%ld)\n",
 			   __func__, phb->hub_id, rc);
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 911c24e..d1b6d8c 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -177,6 +177,8 @@ struct pnv_phb {
 		unsigned char			blob[PNV_PCI_DIAG_BUF_SIZE];
 		struct OpalIoP7IOCPhbErrorData	p7ioc;
 	} diag;
+
+	struct OpalIoP7IOCErrorData p7ioc_err;
 };
 
 extern struct pci_ops pnv_pci_ops;
-- 
1.8.3.1

Re: [PATCH] powernv: eeh: add buffer for P7IOC hub error data

[Gavin Shan]

On Thu, Dec 19, 2013 at 05:18:53PM -0600, Brian W Hart wrote:
>Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
>a buffer for P7IOC hub diagnostic data.  Take care to inform OPAL of the
>correct size for the buffer.
>
>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
>---
>
>I hope I've understood this correctly.  It looks to me like
>ioda_eeh_hub_data is effectively asking OPAL to clobber its own
>text (via 'data') when it makes the call to retrieve the hub data.
>

Yeah, we should have used following variable as HUB diag-data instead.

static char *hub_diag = NULL;

However, it's not safe to allocate page-sized buffer for "hub_diag".

>Added a hub diagnostic structure per-phb.  Perhaps the diagnostic
>structure better belongs in the phb->diag union, but I wasn't sure whether
>we'd need to carry the hub and PHB diag data at the same time.
>

Please put hub diag-data to struct pnv_phb::diag since we don't need
carry hub and PHB diag-data at same time. With it, please remove
variable "hub_diag" as well.

> arch/powerpc/platforms/powernv/eeh-ioda.c | 4 ++--
> arch/powerpc/platforms/powernv/pci.h      | 2 ++
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>index 8184ef5..dfd9134 100644
>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>@@ -636,8 +636,8 @@ static void ioda_eeh_hub_diag(struct pci_controller *hose)
> 	struct OpalIoP7IOCErrorData *data;
> 	long rc;
>
>-	data = (struct OpalIoP7IOCErrorData *)ioda_eeh_hub_diag;
>-	rc = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
>+	data = (struct OpalIoP7IOCErrorData *)&phb->p7ioc_err;
>+	rc = opal_pci_get_hub_diag_data(phb->hub_id, data, sizeof *data);
> 	if (rc != OPAL_SUCCESS) {
> 		pr_warning("%s: Failed to get HUB#%llx diag-data (%ld)\n",
> 			   __func__, phb->hub_id, rc);
>diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
>index 911c24e..d1b6d8c 100644
>--- a/arch/powerpc/platforms/powernv/pci.h
>+++ b/arch/powerpc/platforms/powernv/pci.h
>@@ -177,6 +177,8 @@ struct pnv_phb {
> 		unsigned char			blob[PNV_PCI_DIAG_BUF_SIZE];
> 		struct OpalIoP7IOCPhbErrorData	p7ioc;
> 	} diag;
>+
>+	struct OpalIoP7IOCErrorData p7ioc_err;
> };
>
> extern struct pci_ops pnv_pci_ops;

Thanks,
Gavin

Re: [PATCH] powernv: eeh: add buffer for P7IOC hub error data

[Brian W Hart]

On Fri, Dec 20, 2013 at 09:45:04AM +0800, Gavin Shan wrote:
> On Thu, Dec 19, 2013 at 05:18:53PM -0600, Brian W Hart wrote:
> >Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
> >a buffer for P7IOC hub diagnostic data.  Take care to inform OPAL of the
> >correct size for the buffer.
> >
> >Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
> >---
> >
> >I hope I've understood this correctly.  It looks to me like
> >ioda_eeh_hub_data is effectively asking OPAL to clobber its own
> >text (via 'data') when it makes the call to retrieve the hub data.
> >
> 
> Yeah, we should have used following variable as HUB diag-data instead.
> 
> static char *hub_diag = NULL;
> 
> However, it's not safe to allocate page-sized buffer for "hub_diag".
> 
> >Added a hub diagnostic structure per-phb.  Perhaps the diagnostic
> >structure better belongs in the phb->diag union, but I wasn't sure whether
> >we'd need to carry the hub and PHB diag data at the same time.
> >
> 
> Please put hub diag-data to struct pnv_phb::diag since we don't need
> carry hub and PHB diag-data at same time. With it, please remove
> variable "hub_diag" as well.

Thanks; will send another patch.