Re: [PATCH v3 06/30] drivers: hv: dxgkrnl: Enumerate and open dxgadapter objects

[Wei Liu <wei.liu@kernel.org>]

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.