diff mbox series

[v3,3/5] platform/x86: think-lmi: Correct NVME password handling

Message ID 20230526171658.3886-3-mpearson-lenovo@squebb.ca
State New
Headers
Series [v3,1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings |

Commit Message

Mark Pearson May 26, 2023, 5:16 p.m. UTC 
  NVME passwords identifier have been standardised across the Lenovo
systems and now use udrp and adrp (user and admin level) instead of
unvp and mnvp.

This should apparently be backwards compatible.

Also cleaned up so the index is set to a default of 1 rather than 0
as this just makes more sense (there is no device 0).

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2 & v3: None. Version bumped in series

 drivers/platform/x86/think-lmi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Ilpo Järvinen May 29, 2023, 12:03 p.m. UTC | #1 
On Fri, 26 May 2023, Mark Pearson wrote:

> NVME passwords identifier have been standardised across the Lenovo
> systems and now use udrp and adrp (user and admin level) instead of
> unvp and mnvp.
> 
> This should apparently be backwards compatible.
> 
> Also cleaned up so the index is set to a default of 1 rather than 0
> as this just makes more sense (there is no device 0).

These two sound entirely separate changes. If that's the case, please 
make own patch from the send change.

Hmm, index_store() still allows 0, is that also related here? Please check 
also ABI documentation as index default seems to be mentioned there as 
well.
Mark Pearson May 29, 2023, 2:58 p.m. UTC | #2 
Thanks Ilpo

On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
> On Fri, 26 May 2023, Mark Pearson wrote:
>
>> NVME passwords identifier have been standardised across the Lenovo
>> systems and now use udrp and adrp (user and admin level) instead of
>> unvp and mnvp.
>> 
>> This should apparently be backwards compatible.
>> 
>> Also cleaned up so the index is set to a default of 1 rather than 0
>> as this just makes more sense (there is no device 0).
>
> These two sound entirely separate changes. If that's the case, please 
> make own patch from the send change.

Ack. It was all related to the index setting and seemed trivial so I lumped together but I can split.
This patch series is turning into a good learning exercise for my git skills :) (which are limited)

>
> Hmm, index_store() still allows 0, is that also related here? Please check 
> also ABI documentation as index default seems to be mentioned there as 
> well.
>

I'd rather not limit it so 0 isn't allowed in case our BIOS team does something weird in the future; but right now 1 is the default so it makes more sense.

Well spotted on the ABI documentation - I had completely missed that. I will address that as well.

> -- 
>  i.

Thanks for the review
Mark
Ilpo Järvinen May 29, 2023, 3:41 p.m. UTC | #3 
On Mon, 29 May 2023, Mark Pearson wrote:
> On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
> > On Fri, 26 May 2023, Mark Pearson wrote:
> >
> >> NVME passwords identifier have been standardised across the Lenovo
> >> systems and now use udrp and adrp (user and admin level) instead of
> >> unvp and mnvp.
> >> 
> >> This should apparently be backwards compatible.
> >> 
> >> Also cleaned up so the index is set to a default of 1 rather than 0
> >> as this just makes more sense (there is no device 0).
> >
> > These two sound entirely separate changes. If that's the case, please 
> > make own patch from the send change.
> 
> Ack. It was all related to the index setting and seemed trivial so I 
> lumped together but I can split. This patch series is turning into a 
> good learning exercise for my git skills :) (which are limited)
>
> > Hmm, index_store() still allows 0, is that also related here? Please check 
> > also ABI documentation as index default seems to be mentioned there as 
> > well.
> >
> 
> I'd rather not limit it so 0 isn't allowed in case our BIOS team does 
> something weird in the future; but right now 1 is the default so it 
> makes more sense.

Sure, do what you feel makes sense here. I was just pointing out the 
perceived inconsistency in case it wasn't intentional.

It might be useful to add one sentence into changelog about the reasoning 
so it can be found easier later on (effectively the paragraph you wrote 
above with small tweaks is enough I think).
Mark Pearson May 29, 2023, 4:06 p.m. UTC | #4 
On Mon, May 29, 2023, at 11:41 AM, Ilpo Järvinen wrote:
> On Mon, 29 May 2023, Mark Pearson wrote:
>> On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
>> > On Fri, 26 May 2023, Mark Pearson wrote:
>> >
>> >> NVME passwords identifier have been standardised across the Lenovo
>> >> systems and now use udrp and adrp (user and admin level) instead of
>> >> unvp and mnvp.
>> >> 
>> >> This should apparently be backwards compatible.
>> >> 
>> >> Also cleaned up so the index is set to a default of 1 rather than 0
>> >> as this just makes more sense (there is no device 0).
>> >
>> > These two sound entirely separate changes. If that's the case, please 
>> > make own patch from the send change.
>> 
>> Ack. It was all related to the index setting and seemed trivial so I 
>> lumped together but I can split. This patch series is turning into a 
>> good learning exercise for my git skills :) (which are limited)
>>
>> > Hmm, index_store() still allows 0, is that also related here? Please check 
>> > also ABI documentation as index default seems to be mentioned there as 
>> > well.
>> >
>> 
>> I'd rather not limit it so 0 isn't allowed in case our BIOS team does 
>> something weird in the future; but right now 1 is the default so it 
>> makes more sense.
>
> Sure, do what you feel makes sense here. I was just pointing out the 
> perceived inconsistency in case it wasn't intentional.
>
> It might be useful to add one sentence into changelog about the reasoning 
> so it can be found easier later on (effectively the paragraph you wrote 
> above with small tweaks is enough I think).

Ack - will do. Thanks

Mark
diff mbox series

Patch

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c7e98fbe7c3d..1c02958035ad 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -456,9 +456,9 @@  static ssize_t new_password_store(struct kobject *kobj,
 				sprintf(pwd_type, "mhdp%d", setting->index);
 		} else if (setting == tlmi_priv.pwd_nvme) {
 			if (setting->level == TLMI_LEVEL_USER)
-				sprintf(pwd_type, "unvp%d", setting->index);
+				sprintf(pwd_type, "udrp%d", setting->index);
 			else
-				sprintf(pwd_type, "mnvp%d", setting->index);
+				sprintf(pwd_type, "adrp%d", setting->index);
 		} else {
 			sprintf(pwd_type, "%s", setting->pwd_type);
 		}
@@ -1524,6 +1524,10 @@  static int tlmi_analyze(void)
 		if (!tlmi_priv.pwd_nvme)
 			goto fail_clear_attr;
 
+		/* Set default hdd/nvme index to 1 as there is no device 0 */
+		tlmi_priv.pwd_hdd->index = 1;
+		tlmi_priv.pwd_nvme->index = 1;
+
 		if (tlmi_priv.pwdcfg.core.password_state & TLMI_HDD_PWD) {
 			/* Check if PWD is configured and set index to first drive found */
 			if (tlmi_priv.pwdcfg.ext.hdd_user_password ||