| Message ID | 20230925194338.966639-3-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | TPMI feature major/minor version check | expand |
On Mon, 25 Sep 2023, Srinivas Pandruvada wrote: > Parse major and minor version number from the version field. If there > is a mismatch for major version, exit from further processing for that > domain. > > If there is mismatch in minor version, driver continue to process with > an error message. This sentence sounds odd. > Minor version change doesn't change offsets and bit > structures of TPMI fields. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > .../x86/intel/speed_select_if/isst_tpmi_core.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > index 63faa2ea8327..37f17e229419 100644 > --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > @@ -30,7 +30,8 @@ > #include "isst_if_common.h" > > /* Supported SST hardware version by this driver */ > -#define ISST_HEADER_VERSION 1 > +#define ISST_MAJOR_VERSION 0 > +#define ISST_MINOR_VERSION 1 > > /* > * Used to indicate if value read from MMIO needs to get multiplied > @@ -352,12 +353,19 @@ static int sst_main(struct auxiliary_device *auxdev, struct tpmi_per_power_domai > pd_info->sst_header.cp_offset *= 8; > pd_info->sst_header.pp_offset *= 8; > > - if (pd_info->sst_header.interface_version != ISST_HEADER_VERSION) { > - dev_err(&auxdev->dev, "SST: Unsupported version:%x\n", > - pd_info->sst_header.interface_version); > + if (pd_info->sst_header.interface_version == TPMI_VERSION_INVALID) > + return -ENODEV; > + > + if (TPMI_MAJOR_VERSION(pd_info->sst_header.interface_version) != ISST_MAJOR_VERSION) { > + dev_err(&auxdev->dev, "SST: Unsupported major version:%lx\n", > + TPMI_MAJOR_VERSION(pd_info->sst_header.interface_version)); > return -ENODEV; > } > > + if (TPMI_MINOR_VERSION(pd_info->sst_header.interface_version) != ISST_MINOR_VERSION) > + dev_err(&auxdev->dev, "SST: Ignore: Unsupported minor version:%lx\n", > + TPMI_MINOR_VERSION(pd_info->sst_header.interface_version)); Why is this dev_err(), wouldn't dev_info() be more appropriate since after this patch it's no longer an error? > + > /* Read SST CP Header */ > *((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset); > >
Also the shortlog could be improved, IMO. E.g., Ignore minor version mismatch.
On Fri, 2023-09-29 at 17:28 +0300, Ilpo Järvinen wrote: > On Mon, 25 Sep 2023, Srinivas Pandruvada wrote: > > > Parse major and minor version number from the version field. If > > there > > is a mismatch for major version, exit from further processing for > > that > > domain. > > > > If there is mismatch in minor version, driver continue to process > > with > > an error message. > > This sentence sounds odd. What is the suggestion here? > > > Minor version change doesn't change offsets and bit > > structures of TPMI fields. > > > > Signed-off-by: Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> > > --- > > .../x86/intel/speed_select_if/isst_tpmi_core.c | 16 > > ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git > > a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > > b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > > index 63faa2ea8327..37f17e229419 100644 > > --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > > +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c > > @@ -30,7 +30,8 @@ > > #include "isst_if_common.h" > > > > /* Supported SST hardware version by this driver */ > > -#define ISST_HEADER_VERSION 1 > > +#define ISST_MAJOR_VERSION 0 > > +#define ISST_MINOR_VERSION 1 > > > > /* > > * Used to indicate if value read from MMIO needs to get > > multiplied > > @@ -352,12 +353,19 @@ static int sst_main(struct auxiliary_device > > *auxdev, struct tpmi_per_power_domai > > pd_info->sst_header.cp_offset *= 8; > > pd_info->sst_header.pp_offset *= 8; > > > > - if (pd_info->sst_header.interface_version != > > ISST_HEADER_VERSION) { > > - dev_err(&auxdev->dev, "SST: Unsupported > > version:%x\n", > > - pd_info->sst_header.interface_version); > > + if (pd_info->sst_header.interface_version == > > TPMI_VERSION_INVALID) > > + return -ENODEV; > > + > > + if (TPMI_MAJOR_VERSION(pd_info- > > >sst_header.interface_version) != ISST_MAJOR_VERSION) { > > + dev_err(&auxdev->dev, "SST: Unsupported major > > version:%lx\n", > > + TPMI_MAJOR_VERSION(pd_info- > > >sst_header.interface_version)); > > return -ENODEV; > > } > > > > + if (TPMI_MINOR_VERSION(pd_info- > > >sst_header.interface_version) != ISST_MINOR_VERSION) > > + dev_err(&auxdev->dev, "SST: Ignore: Unsupported > > minor version:%lx\n", > > + TPMI_MINOR_VERSION(pd_info- > > >sst_header.interface_version)); > > Why is this dev_err(), wouldn't dev_info() be more appropriate since > after this patch it's no longer an error? The distro run with minimum log level. So it is important that they notice and upgrade as they may be missing some new additions. Thanks, Srinivas > > > + > > /* Read SST CP Header */ > > *((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + > > pd_info->sst_header.cp_offset); > > > > >
On Fri, 2023-09-29 at 17:33 +0300, Ilpo Järvinen wrote: > Also the shortlog could be improved, IMO. E.g., Ignore minor version > mismatch. > This is not just ignoring minor version, important is error on major version change. Instead I can change to "Process major and minor version" or "Check major and minor version"
On Sat, 30 Sep 2023, srinivas pandruvada wrote: > On Fri, 2023-09-29 at 17:28 +0300, Ilpo Järvinen wrote: > > On Mon, 25 Sep 2023, Srinivas Pandruvada wrote: > > > > > Parse major and minor version number from the version field. If > > > there > > > is a mismatch for major version, exit from further processing for > > > that > > > domain. > > > > > > If there is mismatch in minor version, driver continue to process > > > with > > > an error message. > > > > This sentence sounds odd. > What is the suggestion here? These things sound wrong: It should be "continues" but I also find "to process" odd when lacking what it processes.
On Mon, 2023-10-02 at 16:49 +0300, Ilpo Järvinen wrote: > On Sat, 30 Sep 2023, srinivas pandruvada wrote: > > > On Fri, 2023-09-29 at 17:28 +0300, Ilpo Järvinen wrote: > > > On Mon, 25 Sep 2023, Srinivas Pandruvada wrote: > > > > > > > Parse major and minor version number from the version field. If > > > > there > > > > is a mismatch for major version, exit from further processing > > > > for > > > > that > > > > domain. > > > > > > > > If there is mismatch in minor version, driver continue to > > > > process > > > > with > > > > an error message. > > > > > > This sentence sounds odd. > > What is the suggestion here? > > These things sound wrong: It should be "continues" OK > but I also find "to > process" odd when lacking what it processes. Further processing means, whatever this driver is supposed to do. You don't want to fully explain again what this driver is doing. >
On Mon, 2 Oct 2023, srinivas pandruvada wrote: > On Mon, 2023-10-02 at 16:49 +0300, Ilpo Järvinen wrote: > > On Sat, 30 Sep 2023, srinivas pandruvada wrote: > > > > > On Fri, 2023-09-29 at 17:28 +0300, Ilpo Järvinen wrote: > > > > On Mon, 25 Sep 2023, Srinivas Pandruvada wrote: > > > > > > > > > Parse major and minor version number from the version field. If > > > > > there > > > > > is a mismatch for major version, exit from further processing > > > > > for > > > > > that > > > > > domain. > > > > > > > > > > If there is mismatch in minor version, driver continue to > > > > > process > > > > > with > > > > > an error message. > > > > > > > > This sentence sounds odd. > > > What is the suggestion here? > > > > These things sound wrong: It should be "continues" > OK > > > but I also find "to > > process" odd when lacking what it processes. > > Further processing means, whatever this driver is supposed to do. You > don't want to fully explain again what this driver is doing. Well, whatever. I'm not a native anyway so I cannot really be the defining authority how the language gets used so feel free to leave "to process" as is.
On Mon, 2023-10-02 at 17:24 +0300, Ilpo Järvinen wrote: > On Mon, 2 Oct 2023, srinivas pandruvada wrote: > > > On Mon, 2023-10-02 at 16:49 +0300, Ilpo Järvinen wrote: > > > On Sat, 30 Sep 2023, srinivas pandruvada wrote: > > > > > > > On Fri, 2023-09-29 at 17:28 +0300, Ilpo Järvinen wrote: > > > > > On Mon, 25 Sep 2023, Srinivas Pandruvada wrote: > > > > > > > > > > > Parse major and minor version number from the version > > > > > > field. If > > > > > > there > > > > > > is a mismatch for major version, exit from further > > > > > > processing > > > > > > for > > > > > > that > > > > > > domain. > > > > > > > > > > > > If there is mismatch in minor version, driver continue to > > > > > > process > > > > > > with > > > > > > an error message. > > > > > > > > > > This sentence sounds odd. > > > > What is the suggestion here? > > > > > > These things sound wrong: It should be "continues" > > OK > > > > > but I also find "to > > > process" odd when lacking what it processes. > > > > Further processing means, whatever this driver is supposed to do. > > You > > don't want to fully explain again what this driver is doing. > > Well, whatever. I'm not a native anyway so I cannot really be the > defining > authority how the language gets used so feel free to leave "to > process" > as is.' That is not my intention! I want to make it as good as possible. Let me come up with new description. Thanks, Srinivas >
diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c index 63faa2ea8327..37f17e229419 100644 --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c @@ -30,7 +30,8 @@ #include "isst_if_common.h" /* Supported SST hardware version by this driver */ -#define ISST_HEADER_VERSION 1 +#define ISST_MAJOR_VERSION 0 +#define ISST_MINOR_VERSION 1 /* * Used to indicate if value read from MMIO needs to get multiplied @@ -352,12 +353,19 @@ static int sst_main(struct auxiliary_device *auxdev, struct tpmi_per_power_domai pd_info->sst_header.cp_offset *= 8; pd_info->sst_header.pp_offset *= 8; - if (pd_info->sst_header.interface_version != ISST_HEADER_VERSION) { - dev_err(&auxdev->dev, "SST: Unsupported version:%x\n", - pd_info->sst_header.interface_version); + if (pd_info->sst_header.interface_version == TPMI_VERSION_INVALID) + return -ENODEV; + + if (TPMI_MAJOR_VERSION(pd_info->sst_header.interface_version) != ISST_MAJOR_VERSION) { + dev_err(&auxdev->dev, "SST: Unsupported major version:%lx\n", + TPMI_MAJOR_VERSION(pd_info->sst_header.interface_version)); return -ENODEV; } + if (TPMI_MINOR_VERSION(pd_info->sst_header.interface_version) != ISST_MINOR_VERSION) + dev_err(&auxdev->dev, "SST: Ignore: Unsupported minor version:%lx\n", + TPMI_MINOR_VERSION(pd_info->sst_header.interface_version)); + /* Read SST CP Header */ *((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset);
Parse major and minor version number from the version field. If there is a mismatch for major version, exit from further processing for that domain. If there is mismatch in minor version, driver continue to process with an error message. Minor version change doesn't change offsets and bit structures of TPMI fields. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- .../x86/intel/speed_select_if/isst_tpmi_core.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)