| Message ID | 20191215182314.32208-1-pakki001@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm/ppi: replace assertion code with recovery in tpm_eval_dsm | expand |
On Sun, 2019-12-15 at 12:23 -0600, Aditya Pakki wrote: > In tpm_eval_dsm, BUG_ON on ppi_handle is used as an assertion. > By returning NULL to the callers, instead of crashing, the error > can be better handled. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> Thanks. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On Sun, Dec 15, 2019 at 12:23:14PM -0600, Aditya Pakki wrote: > In tpm_eval_dsm, BUG_ON on ppi_handle is used as an assertion. > By returning NULL to the callers, instead of crashing, the error > can be better handled. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > drivers/char/tpm/tpm_ppi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c > index b2dab941cb7f..4b6f6a9c0b48 100644 > +++ b/drivers/char/tpm/tpm_ppi.c > @@ -42,7 +42,9 @@ static inline union acpi_object * > tpm_eval_dsm(acpi_handle ppi_handle, int func, acpi_object_type type, > union acpi_object *argv4, u64 rev) > { > - BUG_ON(!ppi_handle); > + if (!ppi_handle) > + return NULL; If it can't happen the confusing if should either be omitted entirely or written as if (WARN_ON(!ppi_handle)) return NULL; Leaving it as apparently operational code just creates confusion for the reader that now has the task to figure out why ppi_handle can be null. I favour not including tests for impossible conditions. The kernel will crash immediately if ppi_handle is null anyhow. Jason
On Wed, 2019-12-18 at 09:45 -0400, Jason Gunthorpe wrote: > On Sun, Dec 15, 2019 at 12:23:14PM -0600, Aditya Pakki wrote: > > In tpm_eval_dsm, BUG_ON on ppi_handle is used as an assertion. > > By returning NULL to the callers, instead of crashing, the error > > can be better handled. > > > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > > drivers/char/tpm/tpm_ppi.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c > > index b2dab941cb7f..4b6f6a9c0b48 100644 > > +++ b/drivers/char/tpm/tpm_ppi.c > > @@ -42,7 +42,9 @@ static inline union acpi_object * > > tpm_eval_dsm(acpi_handle ppi_handle, int func, acpi_object_type type, > > union acpi_object *argv4, u64 rev) > > { > > - BUG_ON(!ppi_handle); > > + if (!ppi_handle) > > + return NULL; > > If it can't happen the confusing if should either be omitted entirely > or written as > > if (WARN_ON(!ppi_handle)) > return NULL; > > Leaving it as apparently operational code just creates confusion for > the reader that now has the task to figure out why ppi_handle can be > null. > > I favour not including tests for impossible conditions. The kernel > will crash immediately if ppi_handle is null anyhow. > > Jason Absolutely should be changed WARN_ON() as it never should happen. I'll update the patch before sending PR to Linus since I have it already applied. Thanks Jason for the remark! /Jarkko
diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c index b2dab941cb7f..4b6f6a9c0b48 100644 --- a/drivers/char/tpm/tpm_ppi.c +++ b/drivers/char/tpm/tpm_ppi.c @@ -42,7 +42,9 @@ static inline union acpi_object * tpm_eval_dsm(acpi_handle ppi_handle, int func, acpi_object_type type, union acpi_object *argv4, u64 rev) { - BUG_ON(!ppi_handle); + if (!ppi_handle) + return NULL; + return acpi_evaluate_dsm_typed(ppi_handle, &tpm_ppi_guid, rev, func, argv4, type); }
In tpm_eval_dsm, BUG_ON on ppi_handle is used as an assertion. By returning NULL to the callers, instead of crashing, the error can be better handled. Signed-off-by: Aditya Pakki <pakki001@umn.edu> --- drivers/char/tpm/tpm_ppi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)