RE: [PATCH v3 2/3] drivers: hv: vmbus: add test attributes to debugfs

[Michael Kelley <mikelley@microsoft.com>]

From: Branden Bonaby <brandonbonaby94@gmail.com> Sent: Tuesday, August 20, 2019 4:39 PM
> 
> Expose the test parameters as part of the debugfs channel attributes.
> We will control the testing state via these attributes.
> 
> Signed-off-by: Branden Bonaby <brandonbonaby94@gmail.com>
> ---
> Changes in v3:
>  - Change call to IS_ERR_OR_NULL, to IS_ERR.
> 
> Changes in v2:
>  - Move test attributes to debugfs.
>  - Wrap test code under #ifdef statements.
>  - Add new documentation file under Documentation/ABI/testing.
>  - Make commit message reflect the change from from sysfs to debugfs.
> 
>  Documentation/ABI/testing/debugfs-hyperv |  21 +++
>  MAINTAINERS                              |   1 +
>  drivers/hv/vmbus_drv.c                   | 167 +++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>  create mode 100644 Documentation/ABI/testing/debugfs-hyperv
> 
> diff --git a/Documentation/ABI/testing/debugfs-hyperv
> b/Documentation/ABI/testing/debugfs-hyperv
> new file mode 100644
> index 000000000000..b25f751fafa8
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-hyperv
> @@ -0,0 +1,21 @@
> +What:           /sys/kernel/debug/hyperv/<UUID>/fuzz_test_state
> +Date:           August 2019
> +KernelVersion:  5.3
> +Contact:        Branden Bonaby <brandonbonaby94@gmail.com>
> +Description:    Fuzz testing status of a vmbus device, whether its in an ON
> +                state or a OFF state

Document what values are actually returned?  

> +Users:          Debugging tools
> +
> +What:           /sys/kernel/debug/hyperv/<UUID>/delay/fuzz_test_buffer_interrupt_delay
> +Date:           August 2019
> +KernelVersion:  5.3
> +Contact:        Branden Bonaby <brandonbonaby94@gmail.com>
> +Description:    Fuzz testing buffer delay value between 0 - 1000

It would be helpful to document the units -- I think this is 0 to 1000
microseconds.

> +Users:          Debugging tools
> +
> +What:           /sys/kernel/debug/hyperv/<UUID>/delay/fuzz_test_message_delay
> +Date:           August 2019
> +KernelVersion:  5.3
> +Contact:        Branden Bonaby <brandonbonaby94@gmail.com>
> +Description:    Fuzz testing message delay value between 0 - 1000

Same here.

> +Users:          Debugging tools
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e81e60bd7c26..120284a8185f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7460,6 +7460,7 @@ F:	include/uapi/linux/hyperv.h
>  F:	include/asm-generic/mshyperv.h
>  F:	tools/hv/
>  F:	Documentation/ABI/stable/sysfs-bus-vmbus
> +F:	Documentation/ABI/testing/debugfs-hyperv
> 
>  HYPERBUS SUPPORT
>  M:	Vignesh Raghavendra <vigneshr@ti.com>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index ebd35fc35290..d2e47f04d172 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -919,6 +919,10 @@ static void vmbus_device_release(struct device *device)
>  	struct hv_device *hv_dev = device_to_hv_device(device);
>  	struct vmbus_channel *channel = hv_dev->channel;
> 
> +#ifdef CONFIG_HYPERV_TESTING
> +	hv_debug_rm_dev_dir(hv_dev);
> +#endif /* CONFIG_HYPERV_TESTING */

Same comment in as previous patch about #ifdef inline in the code,
and similarly for other occurrences in this patch.

> +
>  	mutex_lock(&vmbus_connection.channel_mutex);
>  	hv_process_channel_removal(channel);
>  	mutex_unlock(&vmbus_connection.channel_mutex);
> @@ -1727,6 +1731,9 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  		pr_err("Unable to register primary channeln");
>  		goto err_kset_unregister;
>  	}
> +#ifdef CONFIG_HYPERV_TESTING
> +	hv_debug_add_dev_dir(child_device_obj);
> +#endif /* CONFIG_HYPERV_TESTING */
> 
>  	return 0;
> 
> @@ -2086,6 +2093,159 @@ static void hv_crash_handler(struct pt_regs *regs)
>  	hyperv_cleanup();
>  };
> 
> +#ifdef CONFIG_HYPERV_TESTING
> +
> +struct dentry *hv_root;
> +
> +static int hv_debugfs_delay_get(void *data, u64 *val)
> +{
> +	*val = *(u32 *)data;
> +	return 0;
> +}
> +
> +static int hv_debugfs_delay_set(void *data, u64 val)
> +{
> +	if (val >= 1 && val <= 1000)
> +		*(u32 *)data = val;
> +	/*Best to not use else statement here since we want
> +	 * the delay to remain the same if val > 1000
> +	 */

The standard multi-line comment style would be:

	/*
	 * Best to not use else statement here since we want
	 * the delay to remain the same if val > 1000
	 */

> +	else if (val <= 0)
> +		*(u32 *)data = 0;

You could consider returning an error for an invalid
value (< 0, or > 1000).

> +	return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(hv_debugfs_delay_fops, hv_debugfs_delay_get,
> +			 hv_debugfs_delay_set, "%llu\n");
> +

Michael