Re: [PATCH net-next 3/4] qed: Adapter flash update support.

[Yuval Mintz <yuvalm@mellanox.com>]

On Mon, Mar 26, 2018 at 03:13:47AM -0700, Sudarsana Reddy Kalluru wrote:
> This patch adds the required driver support for updating the flash or
> non volatile memory of the adapter. At highlevel, flash upgrade comprises
> of reading the flash images from the input file, validating the images and
> writing it to the respective paritions.

s/it/them/

[...]

> + *     /----------------------------------------------------------------------\
> + * 0B  |                       0x4 [command index]                            |
> + * 4B  | image_type     | Options        |  Number of register settings       |
> + * 8B  |                       Value                                          |
> + * 12B |                       Mask                                           |
> + * 16B |                       Offset                                         |
> + *     \----------------------------------------------------------------------/
> + * There can be several Value-Mask-Offset sets as specified by 'Number of...'.
> + * Options - 0'b - Calculate & Update CRC for image
> + */
> +static int qed_nvm_flash_image_access(struct qed_dev *cdev, const u8 **data,
> +				      bool *check_resp)
> +{
> +	struct qed_nvm_image_att nvm_image;
> +	struct qed_hwfn *p_hwfn;
> +	bool is_crc = false;
> +	u32 image_type;
> +	int rc = 0, i;
> +	u16 len;
> +
 +
> +	nvm_image.start_addr = p_hwfn->nvm_info.image_att[i].nvm_start_addr;
> +	nvm_image.length = p_hwfn->nvm_info.image_att[i].len;
> +
> +	DP_VERBOSE(cdev, NETIF_MSG_DRV,
> +		   "Read image %02x; type = %08x; NVM [%08x,...,%08x]\n",
> +		   **data, nvm_image.start_addr, image_type,
> +		   nvm_image.start_addr + nvm_image.length - 1);

Looks like 3rd and 4th printed parameters are flipped.


> +	(*data)++;
> +	is_crc = !!(**data);

If you'd actually want to be able to use the reserved bits
[forward-compatibility] in the future, you should check bit 0 instead of
checking the byte.

> +	(*data)++;
> +	len = *((u16 *)*data);
> +	*data += 2;

[...]

> +
> +/* Binary file format -
> + *     /----------------------------------------------------------------------\
> + * 0B  |                       0x3 [command index]                            |
> + * 4B  | b'0: check_response?   | b'1-127  reserved                           |
This shows there are 128 bits in a 4 byte field.

> + * 8B  | File-type |                   reserved                               |
> + *     \----------------------------------------------------------------------/
> + *     Start a new file of the provided type
> + */
> +static int qed_nvm_flash_image_file_start(struct qed_dev *cdev,
> +					  const u8 **data, bool *check_resp)
> +{
> +	int rc;
> +
> +	*data += 4;
> +	*check_resp = !!(**data);

Like above

> +	*data += 4;
> +
> +	DP_VERBOSE(cdev, NETIF_MSG_DRV,
> +		   "About to start a new file of type %02x\n", **data);
> +	rc = qed_mcp_nvm_put_file_begin(cdev, **data);
> +	*data += 4;
> +
> +	return rc;
> +}
> +
> +/* Binary file format -
> + *     /----------------------------------------------------------------------\
> + * 0B  |                       0x2 [command index]                            |
> + * 4B  |                       Length in bytes                                |
> + * 8B  | b'0: check_response?   | b'1-127  reserved                           |

Same as above

> + * 12B |                       Offset in bytes                                |
> + * 16B |                       Data ...                                       |
> + *     \----------------------------------------------------------------------/
> + *     Write data as part of a file that was previously started. Data should be
> + *     of length equal to that provided in the message
> + */
> +static int qed_nvm_flash_image_file_data(struct qed_dev *cdev,
> +					 const u8 **data, bool *check_resp)
> +{
> +	u32 offset, len;
> +	int rc;
> +
> +	*data += 4;
> +	len = *((u32 *)(*data));
> +	*data += 4;
> +	*check_resp = !!(**data);

Same as above

> +	*data += 4;
> +	offset = *((u32 *)(*data));
> +	*data += 4;
> +
> +	DP_VERBOSE(cdev, NETIF_MSG_DRV,
> +		   "About to write File-data: %08x bytes to offset %08x\n",
> +		   len, offset);
> +
> +	rc = qed_mcp_nvm_write(cdev, QED_PUT_FILE_DATA, offset,
> +			       (char *)(*data), len);
> +	*data += len;
> +
> +	return rc;
> +}

[...]

> +
> +static int qed_nvm_flash(struct qed_dev *cdev, const char *name)
> +{
> +	rc = qed_nvm_flash_image_validate(cdev, image, &data);
> +	if (rc)
> +		goto exit;
> +
> +	while (data < data_end) {
> +		bool check_resp = false;
> +
> +		/* Parse the actual command */
> +		cmd_type = *((u32 *)data);

What's the final format of the file? Is it LE?

> +		switch (cmd_type) {
> +		case QED_NVM_FLASH_CMD_FILE_DATA:
> +			rc = qed_nvm_flash_image_file_data(cdev, &data,
> +							   &check_resp);
> +			break;
> +		case QED_NVM_FLASH_CMD_FILE_START:
> +			rc = qed_nvm_flash_image_file_start(cdev, &data,
> +							    &check_resp);
> +			break;
> +		case QED_NVM_FLASH_CMD_NVM_CHANGE:
> +			rc = qed_nvm_flash_image_access(cdev, &data,
> +							&check_resp);
> +			break;
> +		default:
> +			DP_ERR(cdev, "Unknown command %08x\n", cmd_type);
> +			rc = -EINVAL;
> +			break;

Either goto or drop the print; You're getting from the next condition.

> +		}
> +
> +		if (rc) {
> +			DP_ERR(cdev, "Command %08x failed\n", cmd_type);
> +			goto exit;
> +		}
> +
> +		/* Check response if needed */
> +		if (check_resp) {
> +			u32 mcp_response = 0;
> +
> +			if (qed_mcp_nvm_resp(cdev, (u8 *)&mcp_response)) {
> +				DP_ERR(cdev, "Failed getting MCP response\n");
> +				rc = -EINVAL;
> +				goto exit;
> +			}
> +
> +			switch (mcp_response & FW_MSG_CODE_MASK) {
> +			case FW_MSG_CODE_OK:
> +			case FW_MSG_CODE_NVM_OK:
> +			case FW_MSG_CODE_NVM_PUT_FILE_FINISH_OK:
> +			case FW_MSG_CODE_PHY_OK:
> +				break;
> +			default:
> +				DP_ERR(cdev, "MFW returns error: %08x\n",
> +				       mcp_response);
> +				rc = -EINVAL;
> +				goto exit;
> +			}
> +		}
> +	}
> +
> +exit:
> +	release_firmware(image);
> +
> +	return rc;
> +}
> +