On Wed, 23 Sep 2020 18:01:01 -0700 David Awogbemila wrote:
> @@ -518,6 +521,49 @@ int gve_adminq_describe_device(struct gve_priv *priv)
> priv->rx_desc_cnt = priv->rx_pages_per_qpl;
> }
> priv->default_num_queues = be16_to_cpu(descriptor->default_num_queues);
> + dev_opt = (void *)(descriptor + 1);
> +
> + num_options = be16_to_cpu(descriptor->num_device_options);
> + for (i = 0; i < num_options; i++) {
> + u16 option_length = be16_to_cpu(dev_opt->option_length);
> + u16 option_id = be16_to_cpu(dev_opt->option_id);
> +
> + if ((void *)dev_opt + sizeof(*dev_opt) + option_length > (void
> *)descriptor +
> + be16_to_cpu(descriptor->total_length)) {
Can you calculate an void *end pointer before the loop and avoid this
very long condition?
> + dev_err(&priv->dev->dev,
> + "options exceed device_descriptor's total
> length.\n");
> + err = -EINVAL;
> + goto free_device_descriptor;
> + }
> +
> + switch (option_id) {
> + case GVE_DEV_OPT_ID_RAW_ADDRESSING:
> + /* If the length or feature mask doesn't match,
> + * continue without enabling the feature.
> + */
> + if (option_length != GVE_DEV_OPT_LEN_RAW_ADDRESSING ||
> + be32_to_cpu(dev_opt->feat_mask) !=
> + GVE_DEV_OPT_FEAT_MASK_RAW_ADDRESSING) {
Apply the byteswap to the constant so it's done at compilation time.
> + dev_info(&priv->pdev->dev,
> + "Raw addressing device option not
> enabled, length or features mask did not match expected.\n");
dev_warn(), also do yourself a favor and print what the values were.
> + priv->raw_addressing = false;
> + } else {
> + dev_info(&priv->pdev->dev,
> + "Raw addressing device option
> enabled.\n");
> + priv->raw_addressing = true;
Does this really need to be printed on every boot?
> + }
> + break;
> + default:
> + /* If we don't recognize the option just continue
> + * without doing anything.
> + */
> + dev_info(&priv->pdev->dev,
> + "Unrecognized device option 0x%hx not
> enabled.\n",
dev_dbg()
> + option_id);
alignment is off
> + break;
> + }
> + dev_opt = (void *)dev_opt + sizeof(*dev_opt) + option_length;
> + }
