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:   Wed, 2 Mar 2022 12:43:09 +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 06/30] drivers: hv: dxgkrnl: Enumerate and open
 dxgadapter objects
Message-ID: <20220302124309.w2zkzfaq3oinok3g@liuwe-devbox-debian-v2>
References: <719fe06b7cbe9ac12fa4a729e810e3383ab421c1.1646163378.git.iourit@linux.microsoft.com>
 <a282cd44ed35b33a1df866cd745f3464dcaac44f.1646163378.git.iourit@linux.microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a282cd44ed35b33a1df866cd745f3464dcaac44f.1646163378.git.iourit@linux.microsoft.com>
Precedence: bulk

On Tue, Mar 01, 2022 at 11:45:53AM -0800, Iouri Tarassov wrote:
[...]
> +void dxgadapter_remove_process(struct dxgprocess_adapter *process_info)
> +{
> +	pr_debug("%s %p %p", __func__,
> +		    process_info->adapter, process_info);

This is not very useful in a production system.

Keep in mind that this can flood dmesg when the debug level is cranked
up. The user is just perhaps trying to get information for other parts
of the kernel.

I would like to ask you to reduce the amount of pr_debug's in code, only
leave the most critical ones in place. There is ftrace and ebfp in the
kernel to help with observation at runtime.

> +	list_del(&process_info->adapter_process_list_entry);
> +	process_info->adapter_process_list_entry.next = NULL;
> +	process_info->adapter_process_list_entry.prev = NULL;

There is no need to explicitly set next and prev pointers to NULL. They
have been set to poison values by list_del. Please remove this kind of
code from all other places.

> +}
> +
>  int dxgadapter_acquire_lock_exclusive(struct dxgadapter *adapter)
>  {
>  	down_write(&adapter->core_lock);
> @@ -170,3 +198,52 @@ void dxgadapter_release_lock_shared(struct dxgadapter *adapter)
>  {
>  	up_read(&adapter->core_lock);
>  }
> +
[...]
> +struct dxgprocess_adapter *dxgprocess_get_adapter_info(struct dxgprocess
> +						       *process,
> +						       struct dxgadapter
> +						       *adapter)

Please document the locking requirement or renamed this function. I see
the list is manipulated with no lock held.

> +{
> +	struct dxgprocess_adapter *entry;
> +
> +	list_for_each_entry(entry, &process->process_adapter_list_head,
> +			    process_adapter_list_entry) {
> +		if (adapter == entry->adapter) {
> +			pr_debug("Found process info %p", entry);
> +			return entry;
> +		}
> +	}
> +	return NULL;
> +}

Thanks,
Wei.