| Message ID | 164370438970.365087.16509597865935044130.stgit@localhost |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EFI: Reset system after capsule-on-disk | expand |
On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote: > Add a config option to reset system soon after processing capsule update > on disk. This is required in UEFI specification 2.9 Section 8.5.5 > "Delivery of Capsules via file on Mass Storage device" as; > > In all cases that a capsule is identified for processing the system is > restarted after capsule processing is completed. > > This also reports the result of each capsule update so that the user can > notice that the capsule update has been succeeded or not from console log. > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > --- > Changes in v2: > - Remove kconfig option to disable this feature. > - Use panic() instead of do_reset() so that if the reset fails, > the machine halt. > - Log the result of each capsule update always. > --- > lib/efi_loader/efi_capsule.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index 1ec7ea29ff..39bce714f7 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) > ret = efi_capsule_read_file(files[i], &capsule); > if (ret == EFI_SUCCESS) { > ret = efi_capsule_update_firmware(capsule); > - if (ret != EFI_SUCCESS) > - log_err("Applying capsule %ls failed\n", > - files[i]); > + log_err("Applying capsule %ls %s\n", > + files[i], > + ret == EFI_SUCCESS ? "succeeded" : "failed"); log_err()? log_info() is better, I think. > > /* create CapsuleXXXX */ > set_capsule_result(index, capsule, ret); > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) > free(files[i]); > free(files); > > + /* > + * UEFI spec requires to reset system after complete processing capsule > + * update on the storage. > + */ > + panic("Reboot after firmware update"); If CONFIG_PANIC_HANG is enabled, the system won't restart. It's not what we want here. -Takahiro Akashi > + > return ret; > } > #endif /* CONFIG_EFI_CAPSULE_ON_DISK */ >
Hi Takahiro, 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote: > > Add a config option to reset system soon after processing capsule update > > on disk. This is required in UEFI specification 2.9 Section 8.5.5 > > "Delivery of Capsules via file on Mass Storage device" as; > > > > In all cases that a capsule is identified for processing the system is > > restarted after capsule processing is completed. > > > > This also reports the result of each capsule update so that the user can > > notice that the capsule update has been succeeded or not from console log. > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > --- > > Changes in v2: > > - Remove kconfig option to disable this feature. > > - Use panic() instead of do_reset() so that if the reset fails, > > the machine halt. > > - Log the result of each capsule update always. > > --- > > lib/efi_loader/efi_capsule.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > index 1ec7ea29ff..39bce714f7 100644 > > --- a/lib/efi_loader/efi_capsule.c > > +++ b/lib/efi_loader/efi_capsule.c > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) > > ret = efi_capsule_read_file(files[i], &capsule); > > if (ret == EFI_SUCCESS) { > > ret = efi_capsule_update_firmware(capsule); > > - if (ret != EFI_SUCCESS) > > - log_err("Applying capsule %ls failed\n", > > - files[i]); > > + log_err("Applying capsule %ls %s\n", > > + files[i], > > + ret == EFI_SUCCESS ? "succeeded" : "failed"); > > log_err()? log_info() is better, I think. Hmm, would you think to use log_info() even if it is failed? Or should we have log_err(failure) and log_info(success)? > > > > > /* create CapsuleXXXX */ > > set_capsule_result(index, capsule, ret); > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) > > free(files[i]); > > free(files); > > > > + /* > > + * UEFI spec requires to reset system after complete processing capsule > > + * update on the storage. > > + */ > > + panic("Reboot after firmware update"); > > If CONFIG_PANIC_HANG is enabled, the system won't restart. > It's not what we want here. Indeed. Heinrich, what would you think if do_reset() doesn't work? (I think it is OK to get it back here, but needs a warning) Thank you, > > -Takahiro Akashi > > > + > > return ret; > > } > > #endif /* CONFIG_EFI_CAPSULE_ON_DISK */ > > -- Masami Hiramatsu
On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote: > Hi Takahiro, > > 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote: > > > Add a config option to reset system soon after processing capsule update > > > on disk. This is required in UEFI specification 2.9 Section 8.5.5 > > > "Delivery of Capsules via file on Mass Storage device" as; > > > > > > In all cases that a capsule is identified for processing the system is > > > restarted after capsule processing is completed. > > > > > > This also reports the result of each capsule update so that the user can > > > notice that the capsule update has been succeeded or not from console log. > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > --- > > > Changes in v2: > > > - Remove kconfig option to disable this feature. > > > - Use panic() instead of do_reset() so that if the reset fails, > > > the machine halt. > > > - Log the result of each capsule update always. > > > --- > > > lib/efi_loader/efi_capsule.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > index 1ec7ea29ff..39bce714f7 100644 > > > --- a/lib/efi_loader/efi_capsule.c > > > +++ b/lib/efi_loader/efi_capsule.c > > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) > > > ret = efi_capsule_read_file(files[i], &capsule); > > > if (ret == EFI_SUCCESS) { > > > ret = efi_capsule_update_firmware(capsule); > > > - if (ret != EFI_SUCCESS) > > > - log_err("Applying capsule %ls failed\n", > > > - files[i]); > > > + log_err("Applying capsule %ls %s\n", > > > + files[i], > > > + ret == EFI_SUCCESS ? "succeeded" : "failed"); > > > > log_err()? log_info() is better, I think. > > Hmm, would you think to use log_info() even if it is failed? Or should > we have log_err(failure) and log_info(success)? It is what I meant :) > > > > > > > > /* create CapsuleXXXX */ > > > set_capsule_result(index, capsule, ret); > > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) > > > free(files[i]); > > > free(files); > > > > > > + /* > > > + * UEFI spec requires to reset system after complete processing capsule > > > + * update on the storage. > > > + */ > > > + panic("Reboot after firmware update"); > > > > If CONFIG_PANIC_HANG is enabled, the system won't restart. > > It's not what we want here. > > Indeed. > Heinrich, what would you think if do_reset() doesn't work? > (I think it is OK to get it back here, but needs a warning) If (CONFIG_IS_ENABLED(SYSRESET)) { puts ("resetting ...\n"); sysreset_reset_walk(SYSRESET_WARM); } else { do_reset(...) halt(); } /* not reach here */ -Takahiro Akashi > Thank you, > > > > > -Takahiro Akashi > > > > > + > > > return ret; > > > } > > > #endif /* CONFIG_EFI_CAPSULE_ON_DISK */ > > > > > > > -- > Masami Hiramatsu
Hi Takahiro, 2022年2月2日(水) 13:15 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote: > > Hi Takahiro, > > > > 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > > > > > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote: > > > > Add a config option to reset system soon after processing capsule update > > > > on disk. This is required in UEFI specification 2.9 Section 8.5.5 > > > > "Delivery of Capsules via file on Mass Storage device" as; > > > > > > > > In all cases that a capsule is identified for processing the system is > > > > restarted after capsule processing is completed. > > > > > > > > This also reports the result of each capsule update so that the user can > > > > notice that the capsule update has been succeeded or not from console log. > > > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > --- > > > > Changes in v2: > > > > - Remove kconfig option to disable this feature. > > > > - Use panic() instead of do_reset() so that if the reset fails, > > > > the machine halt. > > > > - Log the result of each capsule update always. > > > > --- > > > > lib/efi_loader/efi_capsule.c | 12 +++++++++--- > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > index 1ec7ea29ff..39bce714f7 100644 > > > > --- a/lib/efi_loader/efi_capsule.c > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) > > > > ret = efi_capsule_read_file(files[i], &capsule); > > > > if (ret == EFI_SUCCESS) { > > > > ret = efi_capsule_update_firmware(capsule); > > > > - if (ret != EFI_SUCCESS) > > > > - log_err("Applying capsule %ls failed\n", > > > > - files[i]); > > > > + log_err("Applying capsule %ls %s\n", > > > > + files[i], > > > > + ret == EFI_SUCCESS ? "succeeded" : "failed"); > > > > > > log_err()? log_info() is better, I think. > > > > Hmm, would you think to use log_info() even if it is failed? Or should > > we have log_err(failure) and log_info(success)? > > It is what I meant :) OK. > > > > > > > > /* create CapsuleXXXX */ > > > > set_capsule_result(index, capsule, ret); > > > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) > > > > free(files[i]); > > > > free(files); > > > > > > > > + /* > > > > + * UEFI spec requires to reset system after complete processing capsule > > > > + * update on the storage. > > > > + */ > > > > + panic("Reboot after firmware update"); > > > > > > If CONFIG_PANIC_HANG is enabled, the system won't restart. > > > It's not what we want here. > > > > Indeed. > > Heinrich, what would you think if do_reset() doesn't work? > > (I think it is OK to get it back here, but needs a warning) > > If (CONFIG_IS_ENABLED(SYSRESET)) { > puts ("resetting ...\n"); > sysreset_reset_walk(SYSRESET_WARM); > } else { > do_reset(...) > halt(); > } > /* not reach here */ OK, and in both case we should we puts() some messages before reboot, right? Thank you, > > -Takahiro Akashi > > > > Thank you, > > > > > > > > -Takahiro Akashi > > > > > > > + > > > > return ret; > > > > } > > > > #endif /* CONFIG_EFI_CAPSULE_ON_DISK */ > > > > > > > > > > > > -- > > Masami Hiramatsu -- Masami Hiramatsu
On 2/2/22 05:15, AKASHI Takahiro wrote: > On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote: >> Hi Takahiro, >> >> 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>: >> >>> >>> On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote: >>>> Add a config option to reset system soon after processing capsule update >>>> on disk. This is required in UEFI specification 2.9 Section 8.5.5 >>>> "Delivery of Capsules via file on Mass Storage device" as; >>>> >>>> In all cases that a capsule is identified for processing the system is >>>> restarted after capsule processing is completed. >>>> >>>> This also reports the result of each capsule update so that the user can >>>> notice that the capsule update has been succeeded or not from console log. >>>> >>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> >>>> --- >>>> Changes in v2: >>>> - Remove kconfig option to disable this feature. >>>> - Use panic() instead of do_reset() so that if the reset fails, >>>> the machine halt. >>>> - Log the result of each capsule update always. >>>> --- >>>> lib/efi_loader/efi_capsule.c | 12 +++++++++--- >>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c >>>> index 1ec7ea29ff..39bce714f7 100644 >>>> --- a/lib/efi_loader/efi_capsule.c >>>> +++ b/lib/efi_loader/efi_capsule.c >>>> @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) >>>> ret = efi_capsule_read_file(files[i], &capsule); >>>> if (ret == EFI_SUCCESS) { >>>> ret = efi_capsule_update_firmware(capsule); >>>> - if (ret != EFI_SUCCESS) >>>> - log_err("Applying capsule %ls failed\n", >>>> - files[i]); >>>> + log_err("Applying capsule %ls %s\n", >>>> + files[i], >>>> + ret == EFI_SUCCESS ? "succeeded" : "failed"); >>> >>> log_err()? log_info() is better, I think. >> >> Hmm, would you think to use log_info() even if it is failed? Or should >> we have log_err(failure) and log_info(success)? > > It is what I meant :) > >>> >>>> >>>> /* create CapsuleXXXX */ >>>> set_capsule_result(index, capsule, ret); >>>> @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) >>>> free(files[i]); >>>> free(files); >>>> >>>> + /* >>>> + * UEFI spec requires to reset system after complete processing capsule >>>> + * update on the storage. >>>> + */ >>>> + panic("Reboot after firmware update"); >>> >>> If CONFIG_PANIC_HANG is enabled, the system won't restart. >>> It's not what we want here. >> >> Indeed. >> Heinrich, what would you think if do_reset() doesn't work? >> (I think it is OK to get it back here, but needs a warning) > > If (CONFIG_IS_ENABLED(SYSRESET)) { > puts ("resetting ...\n"); > sysreset_reset_walk(SYSRESET_WARM); do_reset() is implemented in many 25 places. drivers/sysreset/sysreset-uclass.c is just one of them. @Tom, @Simon: Is there a migration timeline to replace all other do_reset() implementations? A dummy implementation like in arch/riscv/lib/reset.c should not exist. The sysreset uclass handles the case of no sysreset driver already. Best regards Heinrich > } else { > do_reset(...) > halt(); > } > /* not reach here */ > > -Takahiro Akashi > > >> Thank you, >> >>> >>> -Takahiro Akashi >>> >>>> + >>>> return ret; >>>> } >>>> #endif /* CONFIG_EFI_CAPSULE_ON_DISK */ >>>> >> >> >> >> -- >> Masami Hiramatsu
On Thu, Feb 03, 2022 at 06:32:50PM +0100, Heinrich Schuchardt wrote: > On 2/2/22 05:15, AKASHI Takahiro wrote: > > On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote: > > > Hi Takahiro, > > > > > > 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > > > > > > > > On Tue, Feb 01, 2022 at 05:33:09PM +0900, Masami Hiramatsu wrote: > > > > > Add a config option to reset system soon after processing capsule update > > > > > on disk. This is required in UEFI specification 2.9 Section 8.5.5 > > > > > "Delivery of Capsules via file on Mass Storage device" as; > > > > > > > > > > In all cases that a capsule is identified for processing the system is > > > > > restarted after capsule processing is completed. > > > > > > > > > > This also reports the result of each capsule update so that the user can > > > > > notice that the capsule update has been succeeded or not from console log. > > > > > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > --- > > > > > Changes in v2: > > > > > - Remove kconfig option to disable this feature. > > > > > - Use panic() instead of do_reset() so that if the reset fails, > > > > > the machine halt. > > > > > - Log the result of each capsule update always. > > > > > --- > > > > > lib/efi_loader/efi_capsule.c | 12 +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > > index 1ec7ea29ff..39bce714f7 100644 > > > > > --- a/lib/efi_loader/efi_capsule.c > > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > > @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) > > > > > ret = efi_capsule_read_file(files[i], &capsule); > > > > > if (ret == EFI_SUCCESS) { > > > > > ret = efi_capsule_update_firmware(capsule); > > > > > - if (ret != EFI_SUCCESS) > > > > > - log_err("Applying capsule %ls failed\n", > > > > > - files[i]); > > > > > + log_err("Applying capsule %ls %s\n", > > > > > + files[i], > > > > > + ret == EFI_SUCCESS ? "succeeded" : "failed"); > > > > > > > > log_err()? log_info() is better, I think. > > > > > > Hmm, would you think to use log_info() even if it is failed? Or should > > > we have log_err(failure) and log_info(success)? > > > > It is what I meant :) > > > > > > > > > > > > > > > > /* create CapsuleXXXX */ > > > > > set_capsule_result(index, capsule, ret); > > > > > @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) > > > > > free(files[i]); > > > > > free(files); > > > > > > > > > > + /* > > > > > + * UEFI spec requires to reset system after complete processing capsule > > > > > + * update on the storage. > > > > > + */ > > > > > + panic("Reboot after firmware update"); > > > > > > > > If CONFIG_PANIC_HANG is enabled, the system won't restart. > > > > It's not what we want here. > > > > > > Indeed. > > > Heinrich, what would you think if do_reset() doesn't work? > > > (I think it is OK to get it back here, but needs a warning) > > > > If (CONFIG_IS_ENABLED(SYSRESET)) { > > puts ("resetting ...\n"); > > sysreset_reset_walk(SYSRESET_WARM); > > do_reset() is implemented in many 25 places. > drivers/sysreset/sysreset-uclass.c is just one of them. > > @Tom, @Simon: > Is there a migration timeline to replace all other do_reset() > implementations? > > A dummy implementation like in arch/riscv/lib/reset.c should not exist. > The sysreset uclass handles the case of no sysreset driver already. Not yet, please feel free to propose something, if it can't just be done outright, right now.
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 1ec7ea29ff..39bce714f7 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void) ret = efi_capsule_read_file(files[i], &capsule); if (ret == EFI_SUCCESS) { ret = efi_capsule_update_firmware(capsule); - if (ret != EFI_SUCCESS) - log_err("Applying capsule %ls failed\n", - files[i]); + log_err("Applying capsule %ls %s\n", + files[i], + ret == EFI_SUCCESS ? "succeeded" : "failed"); /* create CapsuleXXXX */ set_capsule_result(index, capsule, ret); @@ -1142,6 +1142,12 @@ efi_status_t efi_launch_capsules(void) free(files[i]); free(files); + /* + * UEFI spec requires to reset system after complete processing capsule + * update on the storage. + */ + panic("Reboot after firmware update"); + return ret; } #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
Add a config option to reset system soon after processing capsule update on disk. This is required in UEFI specification 2.9 Section 8.5.5 "Delivery of Capsules via file on Mass Storage device" as; In all cases that a capsule is identified for processing the system is restarted after capsule processing is completed. This also reports the result of each capsule update so that the user can notice that the capsule update has been succeeded or not from console log. Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org> --- Changes in v2: - Remove kconfig option to disable this feature. - Use panic() instead of do_reset() so that if the reset fails, the machine halt. - Log the result of each capsule update always. --- lib/efi_loader/efi_capsule.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)