| Message ID | 1439273796-25359-9-git-send-email-jlee@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tue 2015-08-11 14:16:28, Lee, Chun-Yi wrote: > For forwarding hibernation key from EFI stub to boot kernel, this patch > allocates setup data for carrying hibernation key, size and the status > of efi operating. > > Reviewed-by: Jiri Kosina <jkosina@suse.com> Jiri, are you sure you reviewed these? This is not really english, afaict, and efi/EFI should be spelled consistently. Could you try reviewing it again? Pointing out 10s of small bugs is quite boring... > unsigned long key_size; > unsigned long attributes; > + struct setup_data *setup_data, *hibernation_setup_data; > struct hibernation_keys *keys; > + unsigned long size = 0; > efi_status_t status; > > /* Allocate setup_data to carry keys */ > + size = sizeof(struct setup_data) + sizeof(struct hibernation_keys); > status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > - sizeof(struct hibernation_keys), &keys); > + size, &hibernation_setup_data); > if (status != EFI_SUCCESS) { > efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n"); > return; > } > > - memset(keys, 0, sizeof(struct hibernation_keys)); > + memset(hibernation_setup_data, 0, size); > + keys = (struct hibernation_keys *) hibernation_setup_data->data; > any chance to type stuff correctly so that casts are not neccessary? > +clean_fail: > + hibernation_setup_data->type = SETUP_HIBERNATION_KEYS; > + hibernation_setup_data->len = sizeof(struct hibernation_keys); > + hibernation_setup_data->next = 0; > + keys->hkey_status = efi_status_to_err(status); > + > + setup_data = (struct setup_data *)params->hdr.setup_data; > + while (setup_data && setup_data->next) > + setup_data = (struct setup_data *)setup_data->next; way too many casts here. Pavel
On Sat, Aug 15, 2015 at 07:07:38PM +0200, Pavel Machek wrote: > On Tue 2015-08-11 14:16:28, Lee, Chun-Yi wrote: > > For forwarding hibernation key from EFI stub to boot kernel, this patch > > allocates setup data for carrying hibernation key, size and the status > > of efi operating. > > > > Reviewed-by: Jiri Kosina <jkosina@suse.com> > > Jiri, are you sure you reviewed these? This is not really > english, afaict, and efi/EFI should be spelled consistently. > > Could you try reviewing it again? Pointing out 10s of small > bugs is quite boring... > It's my fault, I will find someone help to review my English in all patch description for next edition. > > unsigned long key_size; > > unsigned long attributes; > > + struct setup_data *setup_data, *hibernation_setup_data; > > struct hibernation_keys *keys; > > + unsigned long size = 0; > > efi_status_t status; > > > > /* Allocate setup_data to carry keys */ > > + size = sizeof(struct setup_data) + sizeof(struct hibernation_keys); > > status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > > - sizeof(struct hibernation_keys), &keys); > > + size, &hibernation_setup_data); > > if (status != EFI_SUCCESS) { > > efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n"); > > return; > > } > > > > - memset(keys, 0, sizeof(struct hibernation_keys)); > > + memset(hibernation_setup_data, 0, size); > > + keys = (struct hibernation_keys *) hibernation_setup_data->data; > > > > any chance to type stuff correctly so that casts are not > neccessary? > The data element defined in setup_data struct as u8: __u8 data[0]; So I use cast here. > > +clean_fail: > > + hibernation_setup_data->type = SETUP_HIBERNATION_KEYS; > > + hibernation_setup_data->len = sizeof(struct hibernation_keys); > > + hibernation_setup_data->next = 0; > > + keys->hkey_status = efi_status_to_err(status); > > + > > + setup_data = (struct setup_data *)params->hdr.setup_data; > > + while (setup_data && setup_data->next) > > + setup_data = (struct setup_data *)setup_data->next; > > way too many casts here. > > Pavel I follow setup_efi_pci32() and setup_efi_pci64(): while (data && data->next) data = (struct setup_data *)(unsigned long)data->next; That's better I also add (unsigned long) cast as setup_efi_pci. Do you have good suggestion let the above code more graceful? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 15 Aug 2015, Pavel Machek wrote: > > For forwarding hibernation key from EFI stub to boot kernel, this patch > > allocates setup data for carrying hibernation key, size and the status > > of efi operating. > > > > Reviewed-by: Jiri Kosina <jkosina@suse.com> > > Jiri, are you sure you reviewed these? This is not really > english, afaict, and efi/EFI should be spelled consistently. Yes, I did review the code and the fact that it is still in compliance with my original idea. I think you can blame me on not reviewing changelogs super-rigorously though, so all suggestions for improvements are absolutely welcome. Thanks,
Hi all. I've rejoined LKML, so I'll try to help with reviewing PM patches. I'd forgotten how much it is a case of sipping at a fire hydrant! Regards, Nigel On 17/08/15 07:23, Jiri Kosina wrote: > On Sat, 15 Aug 2015, Pavel Machek wrote: > >>> For forwarding hibernation key from EFI stub to boot kernel, this patch >>> allocates setup data for carrying hibernation key, size and the status >>> of efi operating. >>> >>> Reviewed-by: Jiri Kosina <jkosina@suse.com> >> Jiri, are you sure you reviewed these? This is not really >> english, afaict, and efi/EFI should be spelled consistently. > Yes, I did review the code and the fact that it is still in compliance > with my original idea. I think you can blame me on not reviewing > changelogs super-rigorously though, so all suggestions for improvements > are absolutely welcome. > > Thanks, > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 11 Aug, at 02:16:28PM, Lee, Chun-Yi wrote: > For forwarding hibernation key from EFI stub to boot kernel, this patch > allocates setup data for carrying hibernation key, size and the status > of efi operating. This could do with some more information, and include that the key is used to validate hibernate images. But now that I think about it, is there a reason this patch hasn't been merged with patch 6? The memory leak I mentioned in patch 6 becomes a non-issue in this one, so it would be good if these two could be squashed together. > Reviewed-by: Jiri Kosina <jkosina@suse.com> > Tested-by: Jiri Kosina <jkosina@suse.com> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com> > --- > arch/x86/boot/compressed/eboot.c | 26 +++++++++++++++++++++++--- > arch/x86/include/uapi/asm/bootparam.h | 1 + > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index 463aa9b..c838d09 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -1394,18 +1394,22 @@ static void setup_hibernation_keys(struct boot_params *params) > { > unsigned long key_size; > unsigned long attributes; > + struct setup_data *setup_data, *hibernation_setup_data; > struct hibernation_keys *keys; > + unsigned long size = 0; > efi_status_t status; One thing to be aware of is that eboot.c has mainly used the "reverse-christmas-tree" style of variable declarations, with longer lines first, and shorter ones following. I haven't mentioned it before because none of your changes seemed to be too different (and it's not a tree-wide convention), but the above setup_data line goes a bit too far. Could you try and keep them ordered, longest line first? > > /* Allocate setup_data to carry keys */ > + size = sizeof(struct setup_data) + sizeof(struct hibernation_keys); > status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > - sizeof(struct hibernation_keys), &keys); > + size, &hibernation_setup_data); > if (status != EFI_SUCCESS) { > efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n"); > return; > } > > - memset(keys, 0, sizeof(struct hibernation_keys)); > + memset(hibernation_setup_data, 0, size); > + keys = (struct hibernation_keys *) hibernation_setup_data->data; > > status = efi_call_early(get_variable, HIBERNATION_KEY, > &EFI_HIBERNATION_GUID, &attributes, > @@ -1419,7 +1423,8 @@ static void setup_hibernation_keys(struct boot_params *params) > if (status == EFI_SUCCESS) { > efi_printk(sys_table, "Cleaned existing hibernation key\n"); > status = EFI_NOT_FOUND; > - } > + } else > + goto clean_fail; Please add braces for the 'else' clause. Also, please include a comment stating that the reason you jump to the label instead of returning is so that the EFI status error code can be recorded in hibernation_setup_data. > } > > if (status != EFI_SUCCESS) { > @@ -1436,6 +1441,21 @@ static void setup_hibernation_keys(struct boot_params *params) > if (status != EFI_SUCCESS) > efi_printk(sys_table, "Failed to set hibernation key\n"); > } > + > +clean_fail: > + hibernation_setup_data->type = SETUP_HIBERNATION_KEYS; > + hibernation_setup_data->len = sizeof(struct hibernation_keys); > + hibernation_setup_data->next = 0; > + keys->hkey_status = efi_status_to_err(status); > + > + setup_data = (struct setup_data *)params->hdr.setup_data; > + while (setup_data && setup_data->next) > + setup_data = (struct setup_data *)setup_data->next; > + > + if (setup_data) > + setup_data->next = (unsigned long)hibernation_setup_data; > + else > + params->hdr.setup_data = (unsigned long)hibernation_setup_data; This label name is a little confusing because you reach it both when the EFI boot variable was successfully created and when a bogus EFI variable failed to be deleted, i.e. it's not always reached because of a failure. How about 'setup' or simply 'out' ?
On Fri, Aug 21, 2015 at 01:40:26PM +0100, Matt Fleming wrote: > On Tue, 11 Aug, at 02:16:28PM, Lee, Chun-Yi wrote: > > For forwarding hibernation key from EFI stub to boot kernel, this patch > > allocates setup data for carrying hibernation key, size and the status > > of efi operating. > > This could do with some more information, and include that the key is > used to validate hibernate images. > > But now that I think about it, is there a reason this patch hasn't > been merged with patch 6? The memory leak I mentioned in patch 6 > becomes a non-issue in this one, so it would be good if these two > could be squashed together. > OK, I will merge this patch with patch 6. Actually the sequence of patches are from the order of my developing. And, the purpose of code in this patch a bit different with patch 6, so I didn't merge them together. > > Reviewed-by: Jiri Kosina <jkosina@suse.com> > > Tested-by: Jiri Kosina <jkosina@suse.com> > > Signed-off-by: Lee, Chun-Yi <jlee@suse.com> > > --- > > arch/x86/boot/compressed/eboot.c | 26 +++++++++++++++++++++++--- > > arch/x86/include/uapi/asm/bootparam.h | 1 + > > 2 files changed, 24 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > > index 463aa9b..c838d09 100644 > > --- a/arch/x86/boot/compressed/eboot.c > > +++ b/arch/x86/boot/compressed/eboot.c > > @@ -1394,18 +1394,22 @@ static void setup_hibernation_keys(struct boot_params *params) > > { > > unsigned long key_size; > > unsigned long attributes; > > + struct setup_data *setup_data, *hibernation_setup_data; > > struct hibernation_keys *keys; > > + unsigned long size = 0; > > efi_status_t status; > > One thing to be aware of is that eboot.c has mainly used the > "reverse-christmas-tree" style of variable declarations, with longer > lines first, and shorter ones following. I haven't mentioned it before > because none of your changes seemed to be too different (and it's not > a tree-wide convention), but the above setup_data line goes a bit too > far. > > Could you try and keep them ordered, longest line first? > Sure, sorry for I didn't aware that before. > > > > /* Allocate setup_data to carry keys */ > > + size = sizeof(struct setup_data) + sizeof(struct hibernation_keys); > > status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > > - sizeof(struct hibernation_keys), &keys); > > + size, &hibernation_setup_data); > > if (status != EFI_SUCCESS) { > > efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n"); > > return; > > } > > > > - memset(keys, 0, sizeof(struct hibernation_keys)); > > + memset(hibernation_setup_data, 0, size); > > + keys = (struct hibernation_keys *) hibernation_setup_data->data; > > > > status = efi_call_early(get_variable, HIBERNATION_KEY, > > &EFI_HIBERNATION_GUID, &attributes, > > @@ -1419,7 +1423,8 @@ static void setup_hibernation_keys(struct boot_params *params) > > if (status == EFI_SUCCESS) { > > efi_printk(sys_table, "Cleaned existing hibernation key\n"); > > status = EFI_NOT_FOUND; > > - } > > + } else > > + goto clean_fail; > > Please add braces for the 'else' clause. Also, please include a > comment stating that the reason you jump to the label instead of > returning is so that the EFI status error code can be recorded in > hibernation_setup_data. > Thanks for suggestions, I will modify it. > > } > > > > if (status != EFI_SUCCESS) { > > @@ -1436,6 +1441,21 @@ static void setup_hibernation_keys(struct boot_params *params) > > if (status != EFI_SUCCESS) > > efi_printk(sys_table, "Failed to set hibernation key\n"); > > } > > + > > +clean_fail: > > + hibernation_setup_data->type = SETUP_HIBERNATION_KEYS; > > + hibernation_setup_data->len = sizeof(struct hibernation_keys); > > + hibernation_setup_data->next = 0; > > + keys->hkey_status = efi_status_to_err(status); > > + > > + setup_data = (struct setup_data *)params->hdr.setup_data; > > + while (setup_data && setup_data->next) > > + setup_data = (struct setup_data *)setup_data->next; > > + > > + if (setup_data) > > + setup_data->next = (unsigned long)hibernation_setup_data; > > + else > > + params->hdr.setup_data = (unsigned long)hibernation_setup_data; > > This label name is a little confusing because you reach it both when > the EFI boot variable was successfully created and when a bogus EFI > variable failed to be deleted, i.e. it's not always reached because of > a failure. > > How about 'setup' or simply 'out' ? > I will change the label to 'setup' that match with setting setup_data. > -- > Matt Fleming, Intel Open Source Technology Center Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 463aa9b..c838d09 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -1394,18 +1394,22 @@ static void setup_hibernation_keys(struct boot_params *params) { unsigned long key_size; unsigned long attributes; + struct setup_data *setup_data, *hibernation_setup_data; struct hibernation_keys *keys; + unsigned long size = 0; efi_status_t status; /* Allocate setup_data to carry keys */ + size = sizeof(struct setup_data) + sizeof(struct hibernation_keys); status = efi_call_early(allocate_pool, EFI_LOADER_DATA, - sizeof(struct hibernation_keys), &keys); + size, &hibernation_setup_data); if (status != EFI_SUCCESS) { efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n"); return; } - memset(keys, 0, sizeof(struct hibernation_keys)); + memset(hibernation_setup_data, 0, size); + keys = (struct hibernation_keys *) hibernation_setup_data->data; status = efi_call_early(get_variable, HIBERNATION_KEY, &EFI_HIBERNATION_GUID, &attributes, @@ -1419,7 +1423,8 @@ static void setup_hibernation_keys(struct boot_params *params) if (status == EFI_SUCCESS) { efi_printk(sys_table, "Cleaned existing hibernation key\n"); status = EFI_NOT_FOUND; - } + } else + goto clean_fail; } if (status != EFI_SUCCESS) { @@ -1436,6 +1441,21 @@ static void setup_hibernation_keys(struct boot_params *params) if (status != EFI_SUCCESS) efi_printk(sys_table, "Failed to set hibernation key\n"); } + +clean_fail: + hibernation_setup_data->type = SETUP_HIBERNATION_KEYS; + hibernation_setup_data->len = sizeof(struct hibernation_keys); + hibernation_setup_data->next = 0; + keys->hkey_status = efi_status_to_err(status); + + setup_data = (struct setup_data *)params->hdr.setup_data; + while (setup_data && setup_data->next) + setup_data = (struct setup_data *)setup_data->next; + + if (setup_data) + setup_data->next = (unsigned long)hibernation_setup_data; + else + params->hdr.setup_data = (unsigned long)hibernation_setup_data; } #else static void setup_hibernation_keys(struct boot_params *params) {} diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index ab456dc..8c88bc2 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -7,6 +7,7 @@ #define SETUP_DTB 2 #define SETUP_PCI 3 #define SETUP_EFI 4 +#define SETUP_HIBERNATION_KEYS 5 /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF