[1/2] opal-prd: Disable pnor access interface on FSP system

Message ID 20170809151818.24423-1-hegdevasant@linux.vnet.ibm.com
State Accepted
Headers show

Commit Message

Vasant Hegde Aug. 9, 2017, 3:18 p.m.
On FSP system host does not have access to PNOR. Hence disable PNOR
access interfaces.

CC: Jeremy Kerr <jk@ozlabs.org>
CC: Daniel M Crowell <dcrowell@us.ibm.com>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 external/opal-prd/opal-prd.c | 14 ++++++++++----
 external/opal-prd/pnor.c     | 19 +++++++++++++++++++
 external/opal-prd/pnor.h     |  1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

Comments

Vasant Hegde Aug. 9, 2017, 3:24 p.m. | #1
On 08/09/2017 08:48 PM, Vasant Hegde wrote:
> On FSP system host does not have access to PNOR. Hence disable PNOR
> access interfaces.

Jeremy,

I've verified this patch on FSP system and with this patch I'm able to start 
opal-prd daemon.
Of course some more testing required.


-Vasant
Jeremy Kerr Aug. 10, 2017, 6:05 a.m. | #2
Hi Vasant,

> On FSP system host does not have access to PNOR. Hence disable PNOR
> access interfaces.

The concept looks good, but I'm not sure about the implementation:

> -	rc = pnor_init(&ctx->pnor);
> -	if (rc) {
> -		pr_log(LOG_ERR, "PNOR: Failed to open pnor: %m");
> -		goto out_close;
> +	if (pnor_available(&ctx->pnor)) {
> +		rc = pnor_init(&ctx->pnor);
> +		if (rc) {
> +			pr_log(LOG_ERR, "PNOR: Failed to open pnor: %m");
> +			goto out_close;
> +		}
> +	} else {
> +		/* Disable PNOR function pointers */
> +		hinterface.pnor_read = NULL;
> +		hinterface.pnor_write = NULL;
>  	}
>  
>  	ipmi_init(ctx);
> diff --git a/external/opal-prd/pnor.c b/external/opal-prd/pnor.c
> index 0e7e5c0..c032421 100644
> --- a/external/opal-prd/pnor.c
> +++ b/external/opal-prd/pnor.c
> @@ -32,6 +32,25 @@
>  #include "pnor.h"
>  #include "opal-prd.h"
>  
> +#define FDT_FLASH_PATH "/proc/device-tree/chosen/ibm,system-flash"
> +
> +bool pnor_available(struct pnor *pnor)
> +{
> +	/* --pnor is specified */
> +	if (pnor->path) {
> +		if (access(pnor->path, R_OK | W_OK) == 0)
> +			return true;
> +
> +		pr_log(LOG_ERR, "PNOR: Does not have permission to read pnor: %m");
> +		return false;
> +	}
> +
> +	if (access(FDT_FLASH_PATH, R_OK) == 0)
> +		return true;
> +
> +	return false;
> +}
> +

Here you're duplicating the check for the ibm,system-flash property in
the libflash code. Why not just use the return value of pnor_init there?
We'd want the following logic:

 - if no --pnor was specified (ie., pnor->path is NULL), and pnor_init()
   fails, then we continue without PNOR (and NULL-out the function
   pointers). We do want a message to be printed there though.

 - if --pnor *was* specified, and that specific device can't be opened,
   then fail immediately.

Unless there was a specific reason to re-check ibm,system-flash, I'm
inclined to just let dev_get_mtd() do the checking instead.

Cheers,


Jeremy
Vasant Hegde Aug. 10, 2017, 2:31 p.m. | #3
On 08/10/2017 11:35 AM, Jeremy Kerr wrote:
> Hi Vasant,
>
>> On FSP system host does not have access to PNOR. Hence disable PNOR
>> access interfaces.
>
> The concept looks good, but I'm not sure about the implementation:
>
>> -	rc = pnor_init(&ctx->pnor);
>> -	if (rc) {
>> -		pr_log(LOG_ERR, "PNOR: Failed to open pnor: %m");
>> -		goto out_close;
>> +	if (pnor_available(&ctx->pnor)) {
>> +		rc = pnor_init(&ctx->pnor);
>> +		if (rc) {
>> +			pr_log(LOG_ERR, "PNOR: Failed to open pnor: %m");
>> +			goto out_close;
>> +		}
>> +	} else {
>> +		/* Disable PNOR function pointers */
>> +		hinterface.pnor_read = NULL;
>> +		hinterface.pnor_write = NULL;
>>  	}
>>
>>  	ipmi_init(ctx);
>> diff --git a/external/opal-prd/pnor.c b/external/opal-prd/pnor.c
>> index 0e7e5c0..c032421 100644
>> --- a/external/opal-prd/pnor.c
>> +++ b/external/opal-prd/pnor.c
>> @@ -32,6 +32,25 @@
>>  #include "pnor.h"
>>  #include "opal-prd.h"
>>
>> +#define FDT_FLASH_PATH "/proc/device-tree/chosen/ibm,system-flash"
>> +
>> +bool pnor_available(struct pnor *pnor)
>> +{
>> +	/* --pnor is specified */
>> +	if (pnor->path) {
>> +		if (access(pnor->path, R_OK | W_OK) == 0)
>> +			return true;
>> +
>> +		pr_log(LOG_ERR, "PNOR: Does not have permission to read pnor: %m");
>> +		return false;
>> +	}
>> +
>> +	if (access(FDT_FLASH_PATH, R_OK) == 0)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>
> Here you're duplicating the check for the ibm,system-flash property in
> the libflash code. Why not just use the return value of pnor_init there?
> We'd want the following logic:

Yes. its kind of duplicate. But get_dev_mtd and file_init_path() is in common 
code and the error message we print inside these functions may confuse in this 
context.

Hence to keep it simple and to print proper message, I added another function. 
Of course with this I ended up adding few extra lines of code.

>
>  - if no --pnor was specified (ie., pnor->path is NULL), and pnor_init()
>    fails, then we continue without PNOR (and NULL-out the function
>    pointers). We do want a message to be printed there though.
>
>  - if --pnor *was* specified, and that specific device can't be opened,
>    then fail immediately.
>

Agree. That's what I'm doing. But in reverse order as I gave preference to user 
provided option.

> Unless there was a specific reason to re-check ibm,system-flash, I'm
> inclined to just let dev_get_mtd() do the checking instead.

Only reason was to print proper error message. Other we can just rely on 
pnor_init() as it handles both default path and --pnor.

Let me know if you still prefer to use existing one.

-Vasant
Jeremy Kerr Aug. 11, 2017, 4:49 a.m. | #4
Hi Vasant,

>> Here you're duplicating the check for the ibm,system-flash property in
>> the libflash code. Why not just use the return value of pnor_init there?
>> We'd want the following logic:
> 
> Yes. its kind of duplicate. But get_dev_mtd and file_init_path() is in
> common code and the error message we print inside these functions may
> confuse in this context.

OK, that makes sense.

One day, we might want to make the error logging in those common
functions a bit more flexible. But for now, this sounds like the best
approach.

Acked-by: Jeremy Kerr <jk@ozlabs.org>

Cheers,


Jeremy
Stewart Smith Aug. 15, 2017, 8:20 a.m. | #5
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
> On FSP system host does not have access to PNOR. Hence disable PNOR
> access interfaces.
>
> CC: Jeremy Kerr <jk@ozlabs.org>
> CC: Daniel M Crowell <dcrowell@us.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

thanks, series merged to master as of 6eb2c43e94c595bf6629f3da0e1c8f38dae8aa58

Patch

diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c
index 798d59c..a09a636 100644
--- a/external/opal-prd/opal-prd.c
+++ b/external/opal-prd/opal-prd.c
@@ -1949,10 +1949,16 @@  static int run_prd_daemon(struct opal_prd_ctx *ctx)
 
 	fixup_hinterface_table();
 
-	rc = pnor_init(&ctx->pnor);
-	if (rc) {
-		pr_log(LOG_ERR, "PNOR: Failed to open pnor: %m");
-		goto out_close;
+	if (pnor_available(&ctx->pnor)) {
+		rc = pnor_init(&ctx->pnor);
+		if (rc) {
+			pr_log(LOG_ERR, "PNOR: Failed to open pnor: %m");
+			goto out_close;
+		}
+	} else {
+		/* Disable PNOR function pointers */
+		hinterface.pnor_read = NULL;
+		hinterface.pnor_write = NULL;
 	}
 
 	ipmi_init(ctx);
diff --git a/external/opal-prd/pnor.c b/external/opal-prd/pnor.c
index 0e7e5c0..c032421 100644
--- a/external/opal-prd/pnor.c
+++ b/external/opal-prd/pnor.c
@@ -32,6 +32,25 @@ 
 #include "pnor.h"
 #include "opal-prd.h"
 
+#define FDT_FLASH_PATH "/proc/device-tree/chosen/ibm,system-flash"
+
+bool pnor_available(struct pnor *pnor)
+{
+	/* --pnor is specified */
+	if (pnor->path) {
+		if (access(pnor->path, R_OK | W_OK) == 0)
+			return true;
+
+		pr_log(LOG_ERR, "PNOR: Does not have permission to read pnor: %m");
+		return false;
+	}
+
+	if (access(FDT_FLASH_PATH, R_OK) == 0)
+		return true;
+
+	return false;
+}
+
 int pnor_init(struct pnor *pnor)
 {
 	int rc;
diff --git a/external/opal-prd/pnor.h b/external/opal-prd/pnor.h
index 4ff449c..28571af 100644
--- a/external/opal-prd/pnor.h
+++ b/external/opal-prd/pnor.h
@@ -23,5 +23,6 @@  extern int pnor_operation(struct pnor *pnor, const char *name,
 
 extern int pnor_init(struct pnor *pnor);
 extern void pnor_close(struct pnor *pnor);
+extern bool pnor_available(struct pnor *pnor);
 
 #endif /*PNOR_H*/