| Message ID | 20230526171658.3886-2-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3,1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings | expand |
On Fri, 26 May 2023, Mark Pearson wrote: > The system password identification was incorrect. This means that if > the password was enabled it wouldn't be detected correctly; and setting > it would not work. > Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in > sync with Lenovo documentation. > > Correct these mistakes. > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> Missing Fixes tag? > --- > Changes in v2: > - Updated define name to be SMP_PWD instead of SYS_PWD > - Clarified in comments what each password type is. > Changes in v3: None. Version bump with rest of series > > drivers/platform/x86/think-lmi.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 2745224f62ab..c7e98fbe7c3d 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -168,11 +168,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support"); > */ > #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4" > > -#define TLMI_POP_PWD (1 << 0) > -#define TLMI_PAP_PWD (1 << 1) > -#define TLMI_HDD_PWD (1 << 2) > -#define TLMI_SYS_PWD (1 << 3) > -#define TLMI_CERT (1 << 7) > +#define TLMI_POP_PWD (1 << 0) /* Supervisor */ > +#define TLMI_PAP_PWD (1 << 1) /* Power-on */ > +#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */ > +#define TLMI_SMP_PWD (1 << 6) /* System Management */ > +#define TLMI_CERT (1 << 7) /* Certificate Based */ Whe you're adding Fixes tag, please make this change minimal by just adding TLMI_SMP_PWD. The rest of these define changes are a good too but it's unrelated to the actual fix so they should be in a separate patch. And once you move it into own change, convert to BIT() while at it.
Thanks Ilpo On Mon, May 29, 2023, at 7:36 AM, Ilpo Järvinen wrote: > On Fri, 26 May 2023, Mark Pearson wrote: > >> The system password identification was incorrect. This means that if >> the password was enabled it wouldn't be detected correctly; and setting >> it would not work. >> Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in >> sync with Lenovo documentation. >> >> Correct these mistakes. >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > > Missing Fixes tag? Yes - will add. > >> --- >> Changes in v2: >> - Updated define name to be SMP_PWD instead of SYS_PWD >> - Clarified in comments what each password type is. >> Changes in v3: None. Version bump with rest of series >> >> drivers/platform/x86/think-lmi.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> index 2745224f62ab..c7e98fbe7c3d 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -168,11 +168,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support"); >> */ >> #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4" >> >> -#define TLMI_POP_PWD (1 << 0) >> -#define TLMI_PAP_PWD (1 << 1) >> -#define TLMI_HDD_PWD (1 << 2) >> -#define TLMI_SYS_PWD (1 << 3) >> -#define TLMI_CERT (1 << 7) >> +#define TLMI_POP_PWD (1 << 0) /* Supervisor */ >> +#define TLMI_PAP_PWD (1 << 1) /* Power-on */ >> +#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */ >> +#define TLMI_SMP_PWD (1 << 6) /* System Management */ >> +#define TLMI_CERT (1 << 7) /* Certificate Based */ > > Whe you're adding Fixes tag, please make this change minimal by just > adding TLMI_SMP_PWD. > > The rest of these define changes are a good too but it's unrelated to the > actual fix so they should be in a separate patch. And once you move it > into own change, convert to BIT() while at it. > I was asked previously to clarify what SMP stood for so added the comment and it seemed odd to only clarify one and not the others. Can I push back on this request. Doing two separate patches for just that doesn't make sense to me. Thanks for the review Mark
On Mon, 29 May 2023, Mark Pearson wrote: > Thanks Ilpo > > On Mon, May 29, 2023, at 7:36 AM, Ilpo Järvinen wrote: > > On Fri, 26 May 2023, Mark Pearson wrote: > > > >> The system password identification was incorrect. This means that if > >> the password was enabled it wouldn't be detected correctly; and setting > >> it would not work. > >> Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in > >> sync with Lenovo documentation. > >> > >> Correct these mistakes. > >> > >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > > > > Missing Fixes tag? > > Yes - will add. > > > > >> --- > >> Changes in v2: > >> - Updated define name to be SMP_PWD instead of SYS_PWD > >> - Clarified in comments what each password type is. > >> Changes in v3: None. Version bump with rest of series > >> > >> drivers/platform/x86/think-lmi.c | 14 +++++++------- > >> 1 file changed, 7 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > >> index 2745224f62ab..c7e98fbe7c3d 100644 > >> --- a/drivers/platform/x86/think-lmi.c > >> +++ b/drivers/platform/x86/think-lmi.c > >> @@ -168,11 +168,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support"); > >> */ > >> #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4" > >> > >> -#define TLMI_POP_PWD (1 << 0) > >> -#define TLMI_PAP_PWD (1 << 1) > >> -#define TLMI_HDD_PWD (1 << 2) > >> -#define TLMI_SYS_PWD (1 << 3) > >> -#define TLMI_CERT (1 << 7) > >> +#define TLMI_POP_PWD (1 << 0) /* Supervisor */ > >> +#define TLMI_PAP_PWD (1 << 1) /* Power-on */ > >> +#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */ > >> +#define TLMI_SMP_PWD (1 << 6) /* System Management */ > >> +#define TLMI_CERT (1 << 7) /* Certificate Based */ > > > > Whe you're adding Fixes tag, please make this change minimal by just > > adding TLMI_SMP_PWD. > > > > The rest of these define changes are a good too but it's unrelated to the > > actual fix so they should be in a separate patch. And once you move it > > into own change, convert to BIT() while at it. > > I was asked previously to clarify what SMP stood for so added the > comment and it seemed odd to only clarify one and not the others. > Can I push back on this request. Doing two separate patches for just > that doesn't make sense to me. I did not mean removing TLMI_SMP_PWD's comment from this patch just to add it in the another but the comments to the other bits which should go into their own patch. The thing here is that fixes should be made minimal to comply with stable rules.
On Mon, May 29, 2023, at 11:50 AM, Ilpo Järvinen wrote: > On Mon, 29 May 2023, Mark Pearson wrote: > >> Thanks Ilpo >> >> On Mon, May 29, 2023, at 7:36 AM, Ilpo Järvinen wrote: >> > On Fri, 26 May 2023, Mark Pearson wrote: >> > >> >> The system password identification was incorrect. This means that if >> >> the password was enabled it wouldn't be detected correctly; and setting >> >> it would not work. >> >> Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in >> >> sync with Lenovo documentation. >> >> >> >> Correct these mistakes. >> >> >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> > >> > Missing Fixes tag? >> >> Yes - will add. >> >> > >> >> --- >> >> Changes in v2: >> >> - Updated define name to be SMP_PWD instead of SYS_PWD >> >> - Clarified in comments what each password type is. >> >> Changes in v3: None. Version bump with rest of series >> >> >> >> drivers/platform/x86/think-lmi.c | 14 +++++++------- >> >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> >> index 2745224f62ab..c7e98fbe7c3d 100644 >> >> --- a/drivers/platform/x86/think-lmi.c >> >> +++ b/drivers/platform/x86/think-lmi.c >> >> @@ -168,11 +168,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support"); >> >> */ >> >> #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4" >> >> >> >> -#define TLMI_POP_PWD (1 << 0) >> >> -#define TLMI_PAP_PWD (1 << 1) >> >> -#define TLMI_HDD_PWD (1 << 2) >> >> -#define TLMI_SYS_PWD (1 << 3) >> >> -#define TLMI_CERT (1 << 7) >> >> +#define TLMI_POP_PWD (1 << 0) /* Supervisor */ >> >> +#define TLMI_PAP_PWD (1 << 1) /* Power-on */ >> >> +#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */ >> >> +#define TLMI_SMP_PWD (1 << 6) /* System Management */ >> >> +#define TLMI_CERT (1 << 7) /* Certificate Based */ >> > >> > Whe you're adding Fixes tag, please make this change minimal by just >> > adding TLMI_SMP_PWD. >> > >> > The rest of these define changes are a good too but it's unrelated to the >> > actual fix so they should be in a separate patch. And once you move it >> > into own change, convert to BIT() while at it. >> >> I was asked previously to clarify what SMP stood for so added the >> comment and it seemed odd to only clarify one and not the others. >> Can I push back on this request. Doing two separate patches for just >> that doesn't make sense to me. > > I did not mean removing TLMI_SMP_PWD's comment from this patch just to add > it in the another but the comments to the other bits which should go into > their own patch. The thing here is that fixes should be made minimal to > comply with stable rules. > OK....seems odd to me to be honest, but not something I'd lose sleep over. I'll do that in amongst all the other changes. Thanks Mark
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index 2745224f62ab..c7e98fbe7c3d 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -168,11 +168,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support"); */ #define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4" -#define TLMI_POP_PWD (1 << 0) -#define TLMI_PAP_PWD (1 << 1) -#define TLMI_HDD_PWD (1 << 2) -#define TLMI_SYS_PWD (1 << 3) -#define TLMI_CERT (1 << 7) +#define TLMI_POP_PWD (1 << 0) /* Supervisor */ +#define TLMI_PAP_PWD (1 << 1) /* Power-on */ +#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */ +#define TLMI_SMP_PWD (1 << 6) /* System Management */ +#define TLMI_CERT (1 << 7) /* Certificate Based */ #define to_tlmi_pwd_setting(kobj) container_of(kobj, struct tlmi_pwd_setting, kobj) #define to_tlmi_attr_setting(kobj) container_of(kobj, struct tlmi_attr_setting, kobj) @@ -1509,11 +1509,11 @@ static int tlmi_analyze(void) tlmi_priv.pwd_power->valid = true; if (tlmi_priv.opcode_support) { - tlmi_priv.pwd_system = tlmi_create_auth("sys", "system"); + tlmi_priv.pwd_system = tlmi_create_auth("smp", "system"); if (!tlmi_priv.pwd_system) goto fail_clear_attr; - if (tlmi_priv.pwdcfg.core.password_state & TLMI_SYS_PWD) + if (tlmi_priv.pwdcfg.core.password_state & TLMI_SMP_PWD) tlmi_priv.pwd_system->valid = true; tlmi_priv.pwd_hdd = tlmi_create_auth("hdd", "hdd");
The system password identification was incorrect. This means that if the password was enabled it wouldn't be detected correctly; and setting it would not work. Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in sync with Lenovo documentation. Correct these mistakes. Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- Changes in v2: - Updated define name to be SMP_PWD instead of SYS_PWD - Clarified in comments what each password type is. Changes in v3: None. Version bump with rest of series drivers/platform/x86/think-lmi.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)