Re: [PATCH v3 04/30] drivers: hv: dxgkrnl: Creation of dxgadapter object

[Wei Liu]

On Tue, Mar 01, 2022 at 11:45:51AM -0800, Iouri Tarassov wrote:
[...]
> +int dxgadapter_set_vmbus(struct dxgadapter *adapter, struct hv_device *hdev)
> +{
> +	int ret;
> +
> +	guid_to_luid(&hdev->channel->offermsg.offer.if_instance,
> +		     &adapter->luid);
> +	pr_debug("%s: %x:%x %p %pUb\n",
> +		    __func__, adapter->luid.b, adapter->luid.a, hdev->channel,
> +		    &hdev->channel->offermsg.offer.if_instance);
> +
> +	ret = dxgvmbuschannel_init(&adapter->channel, hdev);
> +	if (ret)
> +		goto cleanup;
> +
> +	adapter->channel.adapter = adapter;
> +	adapter->hv_dev = hdev;
> +
> +	ret = dxgvmb_send_open_adapter(adapter);
> +	if (ret < 0) {

if (ret) is simpler?

Please be consistent regarding how you check for errors.

> +		pr_err("dxgvmb_send_open_adapter failed: %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	ret = dxgvmb_send_get_internal_adapter_info(adapter);
> +	if (ret < 0)
> +		pr_err("get_internal_adapter_info failed: %d", ret);
> +
> +cleanup:
> +	if (ret)
> +		pr_debug("err: %s %d", __func__, ret);
> +	return ret;
> +}
> +
> +void dxgadapter_start(struct dxgadapter *adapter)
> +{
> +	struct dxgvgpuchannel *ch = NULL;
> +	struct dxgvgpuchannel *entry;
> +	int ret;
> +
> +	pr_debug("%s %x-%x",
> +		__func__, adapter->luid.a, adapter->luid.b);
> +
> +	/* Find the corresponding vGPU vm bus channel */
> +	list_for_each_entry(entry, &dxgglobal->vgpu_ch_list_head,
> +			    vgpu_ch_list_entry) {

The mutex is not acquired in this function.

I have not checked if this function is used elsewhere. But this is a
non-static function, it should have at least a comment on the locking
requirement?

If it is not needed elsewhere, please make it static or named it
dxgadapter_start_locked.


[...]
> +}
> +
> +void dxgadapter_stop(struct dxgadapter *adapter)

Same comment applies to this function as well.

Thanks,
Wei.