| Message ID | 20230526171658.3886-3-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: > 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.
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
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).
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 --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 ||
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(-)