RE: [patch 4/4 v2] PS3: Add logical performance monitor driver support

["TakashiYamamoto" <TakashiA.Yamamoto@jp.sony.com>]

Hello,

I found a bug in our patch.

> +/**
> + * ps3_lpm_open - Open the logical performance monitor device.
> + * @tb_type: Specifies the type of trace buffer lv1 sould use for this lpm
> + *  instance, specified by one of enum ps3_lpm_tb_type.
> + * @tb_cache: Optional user supplied buffer to use as the trace buffer cache.
> + *  If NULL, the driver will allocate and manage an internal buffer.
> + *  Unused when when @tb_type is PS3_LPM_TB_TYPE_NONE.
> + * @tb_cache_size: The size in bytes of the user supplied @tb_cache buffer.
> + *  Unused when @tb_cache is NULL or @tb_type is PS3_LPM_TB_TYPE_NONE.
> + */
> +
> +int ps3_lpm_open(enum ps3_lpm_tb_type tb_type, void *tb_cache,
> +	u64 tb_cache_size)
> +{
> +	int result;
> +	u64 tb_size;
> +
> +	BUG_ON(!lpm_priv);
> +	BUG_ON(tb_type != PS3_LPM_TB_TYPE_NONE
> +		&& tb_type != PS3_LPM_TB_TYPE_INTERNAL);
> +
> +	if (tb_type == PS3_LPM_TB_TYPE_NONE && tb_cache)
> +		dev_dbg(sbd_core(), "%s:%u: bad in vals\n", __func__, __LINE__);
> +
> +	if (!atomic_add_unless(&lpm_priv->open, 1, 1)) {
> +		dev_dbg(sbd_core(), "%s:%u: busy\n", __func__, __LINE__);
> +		return -EBUSY;
> +	}
> +
> +	if (tb_type == PS3_LPM_TB_TYPE_NONE) {
> +		lpm_priv->tb_cache_internal = NULL;
> +		lpm_priv->tb_cache_size = 0;
> +		lpm_priv->tb_cache = NULL;
> +	} else if (tb_cache) {
> +		if (tb_cache != (void *)_ALIGN_UP((unsigned long)tb_cache, 128)
> +			|| tb_cache_size != _ALIGN_UP(tb_cache_size, 128)) {
> +			dev_err(sbd_core(), "%s:%u: unaligned tb_cache\n",
> +				__func__, __LINE__);
> +			result = -EINVAL;
> +			goto fail_align;
> +		}
> +		lpm_priv->tb_cache_internal = NULL;
> +		lpm_priv->tb_cache_size = tb_cache_size;
> +		lpm_priv->tb_cache = tb_cache;
> +	} else {
> +		/* tb_cache needs 128 byte alignment. */
> +		lpm_priv->tb_cache_size = PS3_LPM_DEFAULT_TB_CACHE_SIZE;
> +		lpm_priv->tb_cache_internal = kzalloc(tb_cache_size + 127,

The first parameter of kzalloc() is wrong.

		lpm_priv->tb_cache_internal = kzalloc(lpm_priv->tb_cache_size + 127,
                                                      ^^^^^^^^^^

> +			GFP_KERNEL);
> +		if (!lpm_priv->tb_cache_internal) {
> +			dev_err(sbd_core(), "%s:%u: alloc internal tb_cache "
> +				"failed\n", __func__, __LINE__);
> +			result = -ENOMEM;
> +			goto fail_malloc;
> +		}
> +		lpm_priv->tb_cache = (void *)_ALIGN_UP(
> +			(unsigned long)lpm_priv->tb_cache_internal, 128);
> +	}
> +
> +	result = lv1_construct_lpm(0, tb_type, 0, 0,
> +				ps3_mm_phys_to_lpar(__pa(lpm_priv->tb_cache)),
> +				lpm_priv->tb_cache_size, &lpm_priv->lpm_id,
> +				&lpm_priv->outlet_id, &tb_size);
> +
> +	if (result) {
> +		dev_err(sbd_core(), "%s:%u: lv1_construct_lpm failed: %s\n",
> +			__func__, __LINE__, ps3_result(result));
> +		result = -EINVAL;
> +		goto fail_construct;
> +	}
> +
> +	lpm_priv->shadow.pm_control = PS3_LPM_SHADOW_REG_INIT;
> +	lpm_priv->shadow.pm_start_stop = PS3_LPM_SHADOW_REG_INIT;
> +	lpm_priv->shadow.pm_interval = PS3_LPM_SHADOW_REG_INIT;
> +	lpm_priv->shadow.group_control = PS3_LPM_SHADOW_REG_INIT;
> +	lpm_priv->shadow.debug_bus_control = PS3_LPM_SHADOW_REG_INIT;
> +
> +	dev_dbg(sbd_core(), "%s:%u: lpm_id 0x%lx, outlet_id 0x%lx, "
> +		"tb_size 0x%lx\n", __func__, __LINE__, lpm_priv->lpm_id,
> +		lpm_priv->outlet_id, tb_size);
> +
> +	return 0;
> +
> +fail_construct:
> +	kfree(lpm_priv->tb_cache_internal);
> +	lpm_priv->tb_cache_internal = NULL;
> +fail_malloc:
> +fail_align:
> +	atomic_dec(&lpm_priv->open);
> +	return result;
> +}
> +EXPORT_SYMBOL_GPL(ps3_lpm_open);

Thanks.
Takashi Yamamoto.