| Message ID | 169138864808.65607.6576358707894823512.stgit@jupiter (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v8,1/2] powerpc/rtas: Rename rtas_error_rc to rtas_generic_errno | expand |
Mahesh Salgaonkar <mahesh@linux.ibm.com> writes: > rtas_generic_errno() function will convert the generic rtas return codes > into errno. I don't see the point of renaming it, it just creates unnecessary churn. The existing name seems OK to me. ... > diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h > index 3abe15ac79db1..5572a0a2f6e18 100644 > --- a/arch/powerpc/include/asm/rtas.h > +++ b/arch/powerpc/include/asm/rtas.h > @@ -202,7 +202,9 @@ typedef struct { > #define RTAS_USER_REGION_SIZE (64 * 1024) > > /* RTAS return status codes */ > -#define RTAS_BUSY -2 /* RTAS Busy */ > +#define RTAS_HARDWARE_ERROR (-1) /* Hardware Error */ > +#define RTAS_BUSY (-2) /* RTAS Busy */ Are the brackets necessary? > +#define RTAS_INVALID_PARAMETER (-3) /* Invalid indicator/domain/sensor etc. */ > #define RTAS_EXTENDED_DELAY_MIN 9900 > #define RTAS_EXTENDED_DELAY_MAX 9905 > > @@ -212,6 +214,11 @@ typedef struct { > #define RTAS_THREADS_ACTIVE -9005 /* Multiple processor threads active */ > #define RTAS_OUTSTANDING_COPROC -9006 /* Outstanding coprocessor operations */ > > +/* statuses specific to get-sensor-state */ > +#define RTAS_SLOT_UNISOLATED (-9000) > +#define RTAS_SLOT_NOT_UNISOLATED (-9001) > +#define RTAS_SLOT_NOT_USABLE (-9002) These aren't specific to get-sensor-state. They're used by at least: ibm,manage-flash-image, ibm,activate-firmware, ibm,configure-connector, set-indicator etc. They have different meanings for those calls. I think you're best to just leave the constant values in rtas_error_rc(). > /* RTAS event classes */ > #define RTAS_INTERNAL_ERROR 0x80000000 /* set bit 0 */ > #define RTAS_EPOW_WARNING 0x40000000 /* set bit 1 */ > @@ -425,6 +432,7 @@ extern int rtas_set_indicator(int indicator, int index, int new_value); > extern int rtas_set_indicator_fast(int indicator, int index, int new_value); > extern void rtas_progress(char *s, unsigned short hex); > int rtas_ibm_suspend_me(int *fw_status); > +int rtas_generic_errno(int rtas_rc); > > struct rtc_time; > extern time64_t rtas_get_boot_time(void); > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index c087eeee320ff..80b6099e8ce20 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -1330,33 +1330,34 @@ bool __ref rtas_busy_delay(int status) > } > EXPORT_SYMBOL_GPL(rtas_busy_delay); > > -static int rtas_error_rc(int rtas_rc) > +int rtas_generic_errno(int rtas_rc) > { > int rc; > > switch (rtas_rc) { > - case -1: /* Hardware Error */ > - rc = -EIO; > - break; > - case -3: /* Bad indicator/domain/etc */ > - rc = -EINVAL; > - break; > - case -9000: /* Isolation error */ > - rc = -EFAULT; > - break; > - case -9001: /* Outstanding TCE/PTE */ > - rc = -EEXIST; > - break; > - case -9002: /* No usable slot */ > - rc = -ENODEV; > - break; > - default: > - pr_err("%s: unexpected error %d\n", __func__, rtas_rc); > - rc = -ERANGE; > - break; > + case RTAS_HARDWARE_ERROR: /* Hardware Error */ > + rc = -EIO; > + break; > + case RTAS_INVALID_PARAMETER: /* Bad indicator/domain/etc */ > + rc = -EINVAL; > + break; > + case RTAS_SLOT_UNISOLATED: /* Isolation error */ > + rc = -EFAULT; > + break; > + case RTAS_SLOT_NOT_UNISOLATED: /* Outstanding TCE/PTE */ > + rc = -EEXIST; > + break; > + case RTAS_SLOT_NOT_USABLE: /* No usable slot */ > + rc = -ENODEV; > + break; > + default: > + pr_err("%s: unexpected error %d\n", __func__, rtas_rc); > + rc = -ERANGE; > + break; > } > return rc; > } > +EXPORT_SYMBOL(rtas_generic_errno); Should be GPL. cheers
On 2023-08-15 13:52:14 Tue, Michael Ellerman wrote: > Mahesh Salgaonkar <mahesh@linux.ibm.com> writes: > > rtas_generic_errno() function will convert the generic rtas return codes > > into errno. > > I don't see the point of renaming it, it just creates unnecessary churn. > The existing name seems OK to me. Sure. Will revert back to existing name. > > ... > > > diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h > > index 3abe15ac79db1..5572a0a2f6e18 100644 > > --- a/arch/powerpc/include/asm/rtas.h > > +++ b/arch/powerpc/include/asm/rtas.h > > @@ -202,7 +202,9 @@ typedef struct { > > #define RTAS_USER_REGION_SIZE (64 * 1024) > > > > /* RTAS return status codes */ > > -#define RTAS_BUSY -2 /* RTAS Busy */ > > +#define RTAS_HARDWARE_ERROR (-1) /* Hardware Error */ > > +#define RTAS_BUSY (-2) /* RTAS Busy */ > > Are the brackets necessary? During v5 changset I received offline review comment to add brackets, hence continued here as well. I can take it away if Nathan is fine with it. > > > +#define RTAS_INVALID_PARAMETER (-3) /* Invalid indicator/domain/sensor etc. */ > > #define RTAS_EXTENDED_DELAY_MIN 9900 > > #define RTAS_EXTENDED_DELAY_MAX 9905 > > > > @@ -212,6 +214,11 @@ typedef struct { > > #define RTAS_THREADS_ACTIVE -9005 /* Multiple processor threads active */ > > #define RTAS_OUTSTANDING_COPROC -9006 /* Outstanding coprocessor operations */ > > > > +/* statuses specific to get-sensor-state */ > > +#define RTAS_SLOT_UNISOLATED (-9000) > > +#define RTAS_SLOT_NOT_UNISOLATED (-9001) > > +#define RTAS_SLOT_NOT_USABLE (-9002) > > These aren't specific to get-sensor-state. > > They're used by at least: ibm,manage-flash-image, ibm,activate-firmware, > ibm,configure-connector, set-indicator etc. > > They have different meanings for those calls. I think you're best to > just leave the constant values in rtas_error_rc(). Sure, I will leave them as constant in rtas_error_rc() and move these three #defines to drivers/pci/hotplug/rpaphp_pci.c in 2/2 patch where it makes sense. > > > /* RTAS event classes */ > > #define RTAS_INTERNAL_ERROR 0x80000000 /* set bit 0 */ > > #define RTAS_EPOW_WARNING 0x40000000 /* set bit 1 */ > > @@ -425,6 +432,7 @@ extern int rtas_set_indicator(int indicator, int index, int new_value); > > extern int rtas_set_indicator_fast(int indicator, int index, int new_value); > > extern void rtas_progress(char *s, unsigned short hex); > > int rtas_ibm_suspend_me(int *fw_status); > > +int rtas_generic_errno(int rtas_rc); > > > > struct rtc_time; > > extern time64_t rtas_get_boot_time(void); > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > > index c087eeee320ff..80b6099e8ce20 100644 > > --- a/arch/powerpc/kernel/rtas.c > > +++ b/arch/powerpc/kernel/rtas.c > > @@ -1330,33 +1330,34 @@ bool __ref rtas_busy_delay(int status) > > } > > EXPORT_SYMBOL_GPL(rtas_busy_delay); > > > > -static int rtas_error_rc(int rtas_rc) > > +int rtas_generic_errno(int rtas_rc) > > { > > int rc; > > > > switch (rtas_rc) { > > - case -1: /* Hardware Error */ > > - rc = -EIO; > > - break; > > - case -3: /* Bad indicator/domain/etc */ > > - rc = -EINVAL; > > - break; > > - case -9000: /* Isolation error */ > > - rc = -EFAULT; > > - break; > > - case -9001: /* Outstanding TCE/PTE */ > > - rc = -EEXIST; > > - break; > > - case -9002: /* No usable slot */ > > - rc = -ENODEV; > > - break; > > - default: > > - pr_err("%s: unexpected error %d\n", __func__, rtas_rc); > > - rc = -ERANGE; > > - break; > > + case RTAS_HARDWARE_ERROR: /* Hardware Error */ > > + rc = -EIO; > > + break; > > + case RTAS_INVALID_PARAMETER: /* Bad indicator/domain/etc */ > > + rc = -EINVAL; > > + break; > > + case RTAS_SLOT_UNISOLATED: /* Isolation error */ > > + rc = -EFAULT; > > + break; > > + case RTAS_SLOT_NOT_UNISOLATED: /* Outstanding TCE/PTE */ > > + rc = -EEXIST; > > + break; > > + case RTAS_SLOT_NOT_USABLE: /* No usable slot */ > > + rc = -ENODEV; > > + break; > > + default: > > + pr_err("%s: unexpected error %d\n", __func__, rtas_rc); > > + rc = -ERANGE; > > + break; > > } > > return rc; > > } > > +EXPORT_SYMBOL(rtas_generic_errno); > > Should be GPL. Will fix it in next revision. Thanks for your review.
Mahesh J Salgaonkar <mahesh@linux.ibm.com> writes: > On 2023-08-15 13:52:14 Tue, Michael Ellerman wrote: >> Mahesh Salgaonkar <mahesh@linux.ibm.com> writes: ... >> > diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h >> > index 3abe15ac79db1..5572a0a2f6e18 100644 >> > --- a/arch/powerpc/include/asm/rtas.h >> > +++ b/arch/powerpc/include/asm/rtas.h >> > @@ -202,7 +202,9 @@ typedef struct { >> > #define RTAS_USER_REGION_SIZE (64 * 1024) >> > >> > /* RTAS return status codes */ >> > -#define RTAS_BUSY -2 /* RTAS Busy */ >> > +#define RTAS_HARDWARE_ERROR (-1) /* Hardware Error */ >> > +#define RTAS_BUSY (-2) /* RTAS Busy */ >> >> Are the brackets necessary? > > During v5 changset I received offline review comment to add brackets, > hence continued here as well. I can take it away if Nathan is fine with > it. OK. I can't think of a context where the brackets are useful, but I'm probably just not thinking hard enough. I don't really mind adding them, I was just curious what the justification for them was. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Mahesh J Salgaonkar <mahesh@linux.ibm.com> writes: >> On 2023-08-15 13:52:14 Tue, Michael Ellerman wrote: >>> Mahesh Salgaonkar <mahesh@linux.ibm.com> writes: > ... >>> > diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h >>> > index 3abe15ac79db1..5572a0a2f6e18 100644 >>> > --- a/arch/powerpc/include/asm/rtas.h >>> > +++ b/arch/powerpc/include/asm/rtas.h >>> > @@ -202,7 +202,9 @@ typedef struct { >>> > #define RTAS_USER_REGION_SIZE (64 * 1024) >>> > >>> > /* RTAS return status codes */ >>> > -#define RTAS_BUSY -2 /* RTAS Busy */ >>> > +#define RTAS_HARDWARE_ERROR (-1) /* Hardware Error */ >>> > +#define RTAS_BUSY (-2) /* RTAS Busy */ >>> >>> Are the brackets necessary? >> >> During v5 changset I received offline review comment to add brackets, >> hence continued here as well. I can take it away if Nathan is fine with >> it. > > OK. I can't think of a context where the brackets are useful, but I'm > probably just not thinking hard enough. I don't really mind adding them, > I was just curious what the justification for them was. It was my (mistaken) suggestion -- they're not needed.
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h index 3abe15ac79db1..5572a0a2f6e18 100644 --- a/arch/powerpc/include/asm/rtas.h +++ b/arch/powerpc/include/asm/rtas.h @@ -202,7 +202,9 @@ typedef struct { #define RTAS_USER_REGION_SIZE (64 * 1024) /* RTAS return status codes */ -#define RTAS_BUSY -2 /* RTAS Busy */ +#define RTAS_HARDWARE_ERROR (-1) /* Hardware Error */ +#define RTAS_BUSY (-2) /* RTAS Busy */ +#define RTAS_INVALID_PARAMETER (-3) /* Invalid indicator/domain/sensor etc. */ #define RTAS_EXTENDED_DELAY_MIN 9900 #define RTAS_EXTENDED_DELAY_MAX 9905 @@ -212,6 +214,11 @@ typedef struct { #define RTAS_THREADS_ACTIVE -9005 /* Multiple processor threads active */ #define RTAS_OUTSTANDING_COPROC -9006 /* Outstanding coprocessor operations */ +/* statuses specific to get-sensor-state */ +#define RTAS_SLOT_UNISOLATED (-9000) +#define RTAS_SLOT_NOT_UNISOLATED (-9001) +#define RTAS_SLOT_NOT_USABLE (-9002) + /* RTAS event classes */ #define RTAS_INTERNAL_ERROR 0x80000000 /* set bit 0 */ #define RTAS_EPOW_WARNING 0x40000000 /* set bit 1 */ @@ -425,6 +432,7 @@ extern int rtas_set_indicator(int indicator, int index, int new_value); extern int rtas_set_indicator_fast(int indicator, int index, int new_value); extern void rtas_progress(char *s, unsigned short hex); int rtas_ibm_suspend_me(int *fw_status); +int rtas_generic_errno(int rtas_rc); struct rtc_time; extern time64_t rtas_get_boot_time(void); diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index c087eeee320ff..80b6099e8ce20 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -1330,33 +1330,34 @@ bool __ref rtas_busy_delay(int status) } EXPORT_SYMBOL_GPL(rtas_busy_delay); -static int rtas_error_rc(int rtas_rc) +int rtas_generic_errno(int rtas_rc) { int rc; switch (rtas_rc) { - case -1: /* Hardware Error */ - rc = -EIO; - break; - case -3: /* Bad indicator/domain/etc */ - rc = -EINVAL; - break; - case -9000: /* Isolation error */ - rc = -EFAULT; - break; - case -9001: /* Outstanding TCE/PTE */ - rc = -EEXIST; - break; - case -9002: /* No usable slot */ - rc = -ENODEV; - break; - default: - pr_err("%s: unexpected error %d\n", __func__, rtas_rc); - rc = -ERANGE; - break; + case RTAS_HARDWARE_ERROR: /* Hardware Error */ + rc = -EIO; + break; + case RTAS_INVALID_PARAMETER: /* Bad indicator/domain/etc */ + rc = -EINVAL; + break; + case RTAS_SLOT_UNISOLATED: /* Isolation error */ + rc = -EFAULT; + break; + case RTAS_SLOT_NOT_UNISOLATED: /* Outstanding TCE/PTE */ + rc = -EEXIST; + break; + case RTAS_SLOT_NOT_USABLE: /* No usable slot */ + rc = -ENODEV; + break; + default: + pr_err("%s: unexpected error %d\n", __func__, rtas_rc); + rc = -ERANGE; + break; } return rc; } +EXPORT_SYMBOL(rtas_generic_errno); int rtas_get_power_level(int powerdomain, int *level) { @@ -1370,7 +1371,7 @@ int rtas_get_power_level(int powerdomain, int *level) udelay(1); if (rc < 0) - return rtas_error_rc(rc); + return rtas_generic_errno(rc); return rc; } EXPORT_SYMBOL_GPL(rtas_get_power_level); @@ -1388,7 +1389,7 @@ int rtas_set_power_level(int powerdomain, int level, int *setlevel) } while (rtas_busy_delay(rc)); if (rc < 0) - return rtas_error_rc(rc); + return rtas_generic_errno(rc); return rc; } EXPORT_SYMBOL_GPL(rtas_set_power_level); @@ -1406,7 +1407,7 @@ int rtas_get_sensor(int sensor, int index, int *state) } while (rtas_busy_delay(rc)); if (rc < 0) - return rtas_error_rc(rc); + return rtas_generic_errno(rc); return rc; } EXPORT_SYMBOL_GPL(rtas_get_sensor); @@ -1424,7 +1425,7 @@ int rtas_get_sensor_fast(int sensor, int index, int *state) rc <= RTAS_EXTENDED_DELAY_MAX)); if (rc < 0) - return rtas_error_rc(rc); + return rtas_generic_errno(rc); return rc; } @@ -1466,7 +1467,7 @@ int rtas_set_indicator(int indicator, int index, int new_value) } while (rtas_busy_delay(rc)); if (rc < 0) - return rtas_error_rc(rc); + return rtas_generic_errno(rc); return rc; } EXPORT_SYMBOL_GPL(rtas_set_indicator); @@ -1488,7 +1489,7 @@ int rtas_set_indicator_fast(int indicator, int index, int new_value) rc <= RTAS_EXTENDED_DELAY_MAX)); if (rc < 0) - return rtas_error_rc(rc); + return rtas_generic_errno(rc); return rc; }
rtas_generic_errno() function will convert the generic rtas return codes into errno. Also, #define descriptive names for rtas return codes and use it instead of numeric values. Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com> --- (no changes since v7) Change in V7: - Until v6 there was only one patch with subject "PCI hotplug: rpaphp: Error out on busy status from get-sensor-state". Starting from v7, adding this new patch to introduce rtas_generic_errno() to handle generic rtas error codes. https://lore.kernel.org/all/20220429162545.GA79541@bhelgaas/ --- arch/powerpc/include/asm/rtas.h | 10 +++++++ arch/powerpc/kernel/rtas.c | 53 ++++++++++++++++++++------------------- 2 files changed, 36 insertions(+), 27 deletions(-)