[Skiboot] [RESEND PATCH v1 02/11]ibm-fsp/firenze: nest feature detection

[Joel Stanley]

On Sun, 2015-07-05 at 22:25 +0530, Madhavan Srinivasan wrote:
> diff --git a/hw/nest.c b/hw/nest.c
> new file mode 100644
> index 0000000..fd22a0e
> --- /dev/null
> +++ b/hw/nest.c

> +struct ima_catalog_page_0 *page0_ptr;
> +struct page0_offsets *pg0_offsets;
> +
> +int load_catalogue_lid()
> +{
> +	struct proc_chip *chip = get_chip(pir_to_chip_id(this_cpu()->pir));

I think get_chip(this_cpu()->chip_id) will get you the same thing with
one less function call?

> +	size_t size = NEST_CATALOGUE_SIZE;
> +	int rc=0, loaded;
> +
> +	pg0_offsets = (struct page0_offsets *)malloc(
> +					sizeof(struct page0_offsets));

You don't need to cast the void * that malloc returns (same goes for all
the other times you call malloc in these patches).

> +	if (!pg0_offsets) {
> +		prerror("Nest_IMA: No mem for pg0_offsets structure\n");;
> +		rc = OPAL_NO_MEM;
> +		return rc;

you could just return OPAL_NO_MEM?

> +	}
> +
> +	pg0_offsets->page0 = (char *)malloc(NEST_CATALOGUE_SIZE);
> +	if (!pg0_offsets->page0) {
> +		prerror("Nest_IMA: No mem for catalogue lid \n");
> +		rc = OPAL_NO_MEM;
> +		return rc;

Here too.

> +	}
> +
> +	loaded = start_preload_resource (RESOURCE_ID_CATALOGUE,
> +						RESOURCE_SUBID_NONE,
> +						pg0_offsets->page0, &size);
> +	if (loaded == OPAL_SUCCESS)
> +		loaded = wait_for_resource_loaded(RESOURCE_ID_CATALOGUE,
> +							RESOURCE_SUBID_NONE);
> +
> +	if (loaded != OPAL_SUCCESS) {
> +		prerror("Nest_IMA: Error loading catalogue lid. index=%x\n",
> +							(int)chip->type);

You shouldn't need to cast to int.

Why are you printing the chip type here?
 
> +		rc = OPAL_RESOURCE;
> +		free(pg0_offsets->page0);
> +		free(pg0_offsets);
> +		return rc;

return OPAL_RESOURCE.

> +	}
> +
> +	/*
> +	 * Now that we have loaded the catalogue. Check for Chip event support
> +	 */
> +	page0_ptr = (struct ima_catalog_page_0 *) PAGE0(pg0_offsets);
> +	if (page0_ptr->chip_group_offset == 0) {
> +		prerror("Nest_IMA: Not Supported \n");
> +		return OPAL_UNSUPPORTED;
> +	}
> +
> +	/*
> +	 * Lets save some entry points to help out search
> +	 */
> +	GROUP_ENTRY(pg0_offsets) = PAGE0(pg0_offsets) +
> +					(page0_ptr->group_data_offs * 4096);
> +	EVENT_ENTRY(pg0_offsets) = PAGE0(pg0_offsets) +
> +					(page0_ptr->event_data_offs * 4096);
> +	CHIP_EVENT_ENTRY(pg0_offsets) = EVENT_ENTRY(pg0_offsets) +
> +					(page0_ptr->chip_event_offset);
> +	CHIP_GROUP_ENTRY(pg0_offsets) = GROUP_ENTRY(pg0_offsets) +
> +					(page0_ptr->chip_group_offset);
> +
> +	return rc;

This is the only time you return RC, and you haven't changed it from the
initalised value. Make it return OPAL_SUCCESS. Now you can drop the 'rc'
variable.

> +}
> +
> +/*
> + * powerpc Nest instrumentation support
> + */
> +void nest_ima_init(void)
> +{
> +	if (load_catalogue_lid()) {
> +		printf("IMA Catalog lid failed to load, Exiting \n");
> +		return;
> +	}
> +
> +	return;
> +}