1. Patchew
  2. linux
  3. [PATCH 0/6] TPMI update for new defines and permissions
  4. View patch

[PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

Srinivas Pandruvada posted 6 patches 2 years ago
[PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by Srinivas Pandruvada 2 years ago 
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;
 
 	name = intel_tpmi_name(pfs->pfs_header.tpmi_id);
 	if (!name)
-- 
2.41.0
Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by Ilpo Järvinen 2 years ago 
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.  

-- 
 i.
Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by srinivas pandruvada 2 years ago 
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
Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by Ilpo Järvinen 2 years ago 
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?).

-- 
 i.
Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by Andy Shevchenko 2 years ago 
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?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by srinivas pandruvada 2 years ago 
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

>
Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features
Posted by srinivas pandruvada 2 years ago 
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
> 
> > 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.