| Message ID | 20170809151818.24423-1-hegdevasant@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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
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
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*/
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(-)