[Skiboot] [PATCH 1/2] opal-prd: Disable pnor access interface on FSP system
Vasant Hegde
hegdevasant at linux.vnet.ibm.com
Fri Aug 11 00:31:41 AEST 2017
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
More information about the Skiboot
mailing list