On 01/16/2013 10:56 PM, Sebastian Andrzej Siewior wrote:
> * Suzuki K. Poulose | 2013-01-11 12:12:11 [+0530]:
>
> only some stylish comments?
>
>> diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
>> index eaea3c1..58935c0 100644
>> --- a/kexec/arch/ppc/kexec-uImage-ppc.c
>> +++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>> @@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
>> );
>> }
>>
>> +char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
> char *slurp_ramdisk_ppc(const char *filename, off_t *r_size)
> ?
OK
>
>> +{
>> + struct Image_info img;
>> + off_t size;
>> + const unsigned char *buf =
>> + (const unsigned char *)slurp_file(filename, &size);
>
> is the const cast required here?
Yes, the uImage_probe() expects (const unsigned char *) as the first
argument.
>
>> + /* Check if this is a uImage RAMDisk */
>> + if (buf != (void*)0 &&
> this should be NULL if at all
>
OK
>> + uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK) {
> and maybe splitted in more than one line
>
OK
>> + if (uImage_load(buf, size, &img) != 0)
>> + die ("uImage: Reading %ld bytes from %s failed\n", size, filename);
> no space between the function.
>
OK
>> + buf = img.buf;
>> + size = img.len;
>> + }
>> +
>> + *r_size = size;
>> + return buf;
>> +}
>> +
>> int uImage_ppc_probe(const char *buf, off_t len)
>> {
>> int type;
>> @@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
>> blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr, &dtb_addr);
>>
>> if (ramdisk) {
>> - seg_buf = slurp_file(ramdisk, &seg_size);
>> + seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
>
> I'm not sure but this kinda breaks the case where someone was using a
> plain file without the uImage header. I don't know if someone used this
> at all but it seems we have code for this. The way you load the uImage
> is very generic and I would expect to load it the same way on ARM or SH.
>
If the ramdisk doesn't have uImage header, the slurp_ramdisk_ppc()
defaults to the original behaviour. i.e, it just does a slurp_file()
as it used to do earlier.
> One thing that is different compared to kernel: If the compression is
> set to GZ on a ramdisk image you should not uncompress it. I think that
> uImage_load() decompresses the .gz part for you but I am not sure.
>
Did you mean, uImage_load() shouldn't do a decompression for ramdisk ?
We rely on uImage_load to get what is required.
>> /* Load ramdisk at top of memory */
>> hole_addr = add_buffer(info, seg_buf, seg_size, seg_size,
>> 0, dtb_addr + blob_size, max_addr, -1);
>>
>
Thanks for the detailed review.
Suzuki
> Sebastian
>
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
