Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Mar 2022 23:25:21 +0000
From:   Wei Liu <wei.liu@kernel.org>
To:     Iouri Tarassov <iourit@linux.microsoft.com>
Cc:     kys@microsoft.com, haiyangz@microsoft.com, sthemmin@microsoft.com,
        wei.liu@kernel.org, linux-hyperv@vger.kernel.org,
        linux-kernel@vger.kernel.org, spronovo@microsoft.com,
        spronovo@linux.microsoft.com, gregkh@linuxfoundation.org
Subject: Re: [PATCH v3 04/30] drivers: hv: dxgkrnl: Creation of dxgadapter
 object
Message-ID: <20220301232521.74utynmxngknd7qy@liuwe-devbox-debian-v2>
References: <719fe06b7cbe9ac12fa4a729e810e3383ab421c1.1646163378.git.iourit@linux.microsoft.com>
 <03e9d393c08a348ca60539bb5f8d2dd6b5afaf0f.1646163378.git.iourit@linux.microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <03e9d393c08a348ca60539bb5f8d2dd6b5afaf0f.1646163378.git.iourit@linux.microsoft.com>
Precedence: bulk

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.