Re: [PATCH v3 2/5] platform/x86: think-lmi: Correct System password interface

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

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.

-- 
 i.