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

[Jeremy Kerr]

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