| Message ID | 20231128185605.3027653-3-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | TPMI update for new defines and permissions | expand |
On Tue, 28 Nov 2023, Srinivas Pandruvada wrote: > If some TPMI features are disabled, don't create auxiliary devices. In > this way feature drivers will not load. > > While creating auxiliary devices, call tpmi_read_feature_status() to > check feature state and return if the feature is disabled without > creating a device. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/platform/x86/intel/tpmi.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index c89aa4d14bea..4edaa182db04 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -604,9 +604,17 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev; > char feature_id_name[TPMI_FEATURE_NAME_LEN]; > struct intel_vsec_device *feature_vsec_dev; > + struct tpmi_feature_state feature_state; > struct resource *res, *tmp; > const char *name; > - int i; > + int i, ret; > + > + ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state); > + if (ret) > + return ret; > + > + if (!feature_state.enabled) > + return -EOPNOTSUPP; -ENODEV sounds more appropriate.
On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote: > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote: > > > If some TPMI features are disabled, don't create auxiliary devices. > > In > > this way feature drivers will not load. > > > > While creating auxiliary devices, call tpmi_read_feature_status() > > to > > check feature state and return if the feature is disabled without > > creating a device. > > > > Signed-off-by: Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> > > --- > > drivers/platform/x86/intel/tpmi.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > b/drivers/platform/x86/intel/tpmi.c > > index c89aa4d14bea..4edaa182db04 100644 > > --- a/drivers/platform/x86/intel/tpmi.c > > +++ b/drivers/platform/x86/intel/tpmi.c > > @@ -604,9 +604,17 @@ static int tpmi_create_device(struct > > intel_tpmi_info *tpmi_info, > > struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev; > > char feature_id_name[TPMI_FEATURE_NAME_LEN]; > > struct intel_vsec_device *feature_vsec_dev; > > + struct tpmi_feature_state feature_state; > > struct resource *res, *tmp; > > const char *name; > > - int i; > > + int i, ret; > > + > > + ret = tpmi_read_feature_status(tpmi_info, pfs- > > >pfs_header.tpmi_id, &feature_state); > > + if (ret) > > + return ret; > > + > > + if (!feature_state.enabled) > > + return -EOPNOTSUPP; > > -ENODEV sounds more appropriate. The -EOPNOTSUPP is returned matching the next return statement, which causes to continue to create devices which are supported and not disabled. Any other error is real device creation will causes driver modprobe to fail. Thanks, Srinivas
On Thu, 30 Nov 2023, srinivas pandruvada wrote: > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote: > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote: > > > > > If some TPMI features are disabled, don't create auxiliary devices. > > > In > > > this way feature drivers will not load. > > > > > > While creating auxiliary devices, call tpmi_read_feature_status() > > > to > > > check feature state and return if the feature is disabled without > > > creating a device. > > > > > > Signed-off-by: Srinivas Pandruvada > > > <srinivas.pandruvada@linux.intel.com> > > > --- > > > drivers/platform/x86/intel/tpmi.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > b/drivers/platform/x86/intel/tpmi.c > > > index c89aa4d14bea..4edaa182db04 100644 > > > --- a/drivers/platform/x86/intel/tpmi.c > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > @@ -604,9 +604,17 @@ static int tpmi_create_device(struct > > > intel_tpmi_info *tpmi_info, > > > struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev; > > > char feature_id_name[TPMI_FEATURE_NAME_LEN]; > > > struct intel_vsec_device *feature_vsec_dev; > > > + struct tpmi_feature_state feature_state; > > > struct resource *res, *tmp; > > > const char *name; > > > - int i; > > > + int i, ret; > > > + > > > + ret = tpmi_read_feature_status(tpmi_info, pfs- > > > >pfs_header.tpmi_id, &feature_state); > > > + if (ret) > > > + return ret; > > > + > > > + if (!feature_state.enabled) > > > + return -EOPNOTSUPP; > > > > -ENODEV sounds more appropriate. > > The -EOPNOTSUPP is returned matching the next return statement, which > causes to continue to create devices which are supported and not > disabled. Any other error is real device creation will causes driver > modprobe to fail. Oh, I see... I didn't look that deep into the code during my review (perhaps note that down into the commit message?).
On Thu, Nov 30, 2023 at 04:33:00PM +0200, Ilpo Järvinen wrote: > On Thu, 30 Nov 2023, srinivas pandruvada wrote: > > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote: > > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote: ... > > > > + if (!feature_state.enabled) > > > > + return -EOPNOTSUPP; > > > > > > -ENODEV sounds more appropriate. > > > > The -EOPNOTSUPP is returned matching the next return statement, which > > causes to continue to create devices which are supported and not > > disabled. Any other error is real device creation will causes driver > > modprobe to fail. > > Oh, I see... I didn't look that deep into the code during my review > (perhaps note that down into the commit message?). Maybe we should even use -ENOTSUPP (Linux internal error code), so it will be clear that it's _not_ going to user space?
On Thu, 2023-11-30 at 16:38 +0200, Andy Shevchenko wrote: > On Thu, Nov 30, 2023 at 04:33:00PM +0200, Ilpo Järvinen wrote: > > On Thu, 30 Nov 2023, srinivas pandruvada wrote: > > > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote: > > > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote: > > ... > > > > > > + if (!feature_state.enabled) > > > > > + return -EOPNOTSUPP; > > > > > > > > -ENODEV sounds more appropriate. > > > > > > The -EOPNOTSUPP is returned matching the next return statement, > > > which > > > causes to continue to create devices which are supported and not > > > disabled. Any other error is real device creation will causes > > > driver > > > modprobe to fail. > > > > Oh, I see... I didn't look that deep into the code during my review > > (perhaps note that down into the commit message?). > > Maybe we should even use -ENOTSUPP (Linux internal error code), so > it will be clear that it's _not_ going to user space? That will be better. I will change and resubmit. Thanks, Srinivas >
On Thu, 2023-11-30 at 10:00 -0500, srinivas pandruvada wrote: > On Thu, 2023-11-30 at 16:38 +0200, Andy Shevchenko wrote: > > On Thu, Nov 30, 2023 at 04:33:00PM +0200, Ilpo Järvinen wrote: > > > On Thu, 30 Nov 2023, srinivas pandruvada wrote: > > > > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote: > > > > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote: > > > > ... > > > > > > > > + if (!feature_state.enabled) > > > > > > + return -EOPNOTSUPP; > > > > > > > > > > -ENODEV sounds more appropriate. > > > > > > > > The -EOPNOTSUPP is returned matching the next return statement, > > > > which > > > > causes to continue to create devices which are supported and > > > > not > > > > disabled. Any other error is real device creation will causes > > > > driver > > > > modprobe to fail. > > > > > > Oh, I see... I didn't look that deep into the code during my > > > review > > > (perhaps note that down into the commit message?). > > > > Maybe we should even use -ENOTSUPP (Linux internal error code), so > > it will be clear that it's _not_ going to user space? > > That will be better. I will change and resubmit. The checkpatch gives error with this. WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #25: FILE: drivers/platform/x86/intel/tpmi.c:613: + return -ENOTSUPP; Thanks, Srinivas > > Thanks, > Srinivas > > > >
diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c index c89aa4d14bea..4edaa182db04 100644 --- a/drivers/platform/x86/intel/tpmi.c +++ b/drivers/platform/x86/intel/tpmi.c @@ -604,9 +604,17 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev; char feature_id_name[TPMI_FEATURE_NAME_LEN]; struct intel_vsec_device *feature_vsec_dev; + struct tpmi_feature_state feature_state; struct resource *res, *tmp; const char *name; - int i; + int i, ret; + + ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state); + if (ret) + return ret; + + if (!feature_state.enabled) + return -EOPNOTSUPP; name = intel_tpmi_name(pfs->pfs_header.tpmi_id); if (!name)
If some TPMI features are disabled, don't create auxiliary devices. In this way feature drivers will not load. While creating auxiliary devices, call tpmi_read_feature_status() to check feature state and return if the feature is disabled without creating a device. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/platform/x86/intel/tpmi.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)