Re: [PATCH net-next 1/4] qed: Populate nvm image attribute shadow.

[Yuval Mintz <yuvalm@mellanox.com>]

On Mon, Mar 26, 2018 at 03:13:45AM -0700, Sudarsana Reddy Kalluru wrote:
> This patch add support for populating the flash image attributes.
s/add/adds/

[...]

> -int qed_mcp_bist_nvm_test_get_image_att(struct qed_hwfn *p_hwfn,
> -					struct qed_ptt *p_ptt,
> -					struct bist_nvm_image_att *p_image_att,
> +int qed_mcp_bist_nvm_get_image_att(struct qed_hwfn *p_hwfn,
> +				   struct qed_ptt *p_ptt,
> +				   struct bist_nvm_image_att *p_image_att,
>  					u32 image_index)

Indentation seems broken.

>  
> +int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
> +{
> +	struct qed_nvm_image_info *nvm_info = &p_hwfn->nvm_info;
> +	struct qed_ptt *p_ptt;
> +	int rc;
> +	u32 i;
> +
> +	p_ptt = qed_ptt_acquire(p_hwfn);
> +	if (!p_ptt) {
> +		DP_ERR(p_hwfn, "failed to acquire ptt\n");
> +		return -EBUSY;
> +	}
> +
> +	/* Acquire from MFW the amount of available images */
> +	nvm_info->num_images = 0;
> +	rc = qed_mcp_bist_nvm_get_num_images(p_hwfn,
> +					     p_ptt, &nvm_info->num_images);
> +	if (rc == -EOPNOTSUPP) {
> +		DP_INFO(p_hwfn, "DRV_MSG_CODE_BIST_TEST is not supported\n");
> +		goto out;
> +	} else if ((rc != 0) || (nvm_info->num_images == 0)) {

rc || !nvm_info->num_images

> +		DP_ERR(p_hwfn, "Failed getting number of images\n");
> +		goto err0;
> +	}
> +
> +	nvm_info->image_att =
> +	    kmalloc(nvm_info->num_images * sizeof(struct bist_nvm_image_att),
> +		    GFP_KERNEL);

Indentation can be better than this.

[...]

> --- a/drivers/net/ethernet/qlogic/qed/qed_selftest.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_selftest.c
> @@ -125,10 +125,11 @@ int qed_selftest_nvram(struct qed_dev *cdev)
>  	}
>  
>  	/* Acquire from MFW the amount of available images */
> -	rc = qed_mcp_bist_nvm_test_get_num_images(p_hwfn, p_ptt, &num_images);
> +	rc = qed_mcp_bist_nvm_get_num_images(p_hwfn, p_ptt, &num_images);
>  	if (rc || !num_images) {
>  		DP_ERR(p_hwfn, "Failed getting number of images\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto err0;

Well, this one is a bug fix [Failure flow currently leaks a PTT entry].
If you don't want to treat it as one that's fine, but I think it
deserves its own patch in the series.

>  	}
>  
>  	/* Iterate over images and validate CRC */
> @@ -136,8 +137,8 @@ int qed_selftest_nvram(struct qed_dev *cdev)
>  		/* This mailbox returns information about the image required for
>  		 * reading it.
>  		 */
> -		rc = qed_mcp_bist_nvm_test_get_image_att(p_hwfn, p_ptt,
> -							 &image_att, i);
> +		rc = qed_mcp_bist_nvm_get_image_att(p_hwfn, p_ptt,
> +						    &image_att, i);
>  		if (rc) {
>  			DP_ERR(p_hwfn,
>  			       "Failed getting image index %d attributes\n",
> -- 
> 1.8.3.1
>