Re: [PATCH net-next v3 1/4] gve: Add support for raw addressing device option

[Jakub Kicinski <kuba@kernel.org>]

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;
> +	}