Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

["Suzuki K. Poulose" <suzuki@in.ibm.com>]

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec