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

[Vasant Hegde]

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