RE: [Patch v2 2/3] Drivers: hv: add Azure Blob driver

[Long Li <longli@microsoft.com>]

> Subject: Re: [Patch v2 2/3] Drivers: hv: add Azure Blob driver
> 
> On Fri, Jun 25, 2021 at 11:30:19PM -0700, longli@linuxonhyperv.com wrote:
> > +#ifdef CONFIG_DEBUG_FS
> > +struct dentry *az_blob_debugfs_root;
> > +#endif
> 
> No need to keep this dentry, just look it up if you need it.

Will fix this.

> 
> > +
> > +static struct az_blob_device az_blob_dev;
> > +
> > +static int az_blob_ringbuffer_size = (128 * 1024);
> > +module_param(az_blob_ringbuffer_size, int, 0444);
> > +MODULE_PARM_DESC(az_blob_ringbuffer_size, "Ring buffer size
> > +(bytes)");
> 
> This is NOT the 1990's, please do not create new module parameters.
> Just make this work properly for everyone.

The default value is chosen so that it works best for most workloads while not taking too much system resources. It should work out of box for nearly all customers.

But what we see is that from time to time, some customers still want to change those values to work best for their workloads. It's hard to predict their workload. They have to recompile the kernel if there is no module parameter to do it. So the intent of this module parameter is that if the default value works for you, don't mess up with it.

> 
> > +#define AZ_ERR 0
> > +#define AZ_WARN 1
> > +#define AZ_DBG 2
> > +static int log_level = AZ_ERR;
> > +module_param(log_level, int, 0644);
> > +MODULE_PARM_DESC(log_level,
> > +	"Log level: 0 - Error (default), 1 - Warning, 2 - Debug.");
> 
> A single driver does not need a special debug/log level, use the system-wide
> functions and all will "just work"

Will fix this.

> 
> > +
> > +static uint device_queue_depth = 1024;
> > +module_param(device_queue_depth, uint, 0444);
> > +MODULE_PARM_DESC(device_queue_depth,
> > +	"System level max queue depth for this device");
> > +
> > +#define az_blob_log(level, fmt, args...)	\
> > +do {	\
> > +	if (level <= log_level)	\
> > +		pr_err("%s:%d " fmt, __func__, __LINE__, ##args);	\
> > +} while (0)
> > +
> > +#define az_blob_dbg(fmt, args...) az_blob_log(AZ_DBG, fmt, ##args)
> > +#define az_blob_warn(fmt, args...) az_blob_log(AZ_WARN, fmt, ##args)
> > +#define az_blob_err(fmt, args...) az_blob_log(AZ_ERR, fmt, ##args)
> 
> Again, no.
> 
> Just use dev_dbg(), dev_warn(), and dev_err() and there is no need for
> anything "special".  This is just one tiny driver, do not rewrite logic like this for
> no reason.
> 
> > +static void az_blob_remove_device(struct az_blob_device *dev) {
> > +	wait_event(dev->file_wait, list_empty(&dev->file_list));
> > +	misc_deregister(&az_blob_misc_device);
> > +#ifdef CONFIG_DEBUG_FS
> 
> No need for the #ifdef.
> 
> > +	debugfs_remove_recursive(az_blob_debugfs_root);
> > +#endif
> > +	/* At this point, we won't get any requests from user-mode */ }
> > +
> > +static int az_blob_create_device(struct az_blob_device *dev) {
> > +	int rc;
> > +	struct dentry *d;
> > +
> > +	rc = misc_register(&az_blob_misc_device);
> > +	if (rc) {
> > +		az_blob_err("misc_register failed rc %d\n", rc);
> > +		return rc;
> > +	}
> > +
> > +#ifdef CONFIG_DEBUG_FS
> 
> No need for the #ifdef
> 
> > +	az_blob_debugfs_root = debugfs_create_dir("az_blob", NULL);
> > +	if (!IS_ERR_OR_NULL(az_blob_debugfs_root)) {
> 
> No need to check this.
> 
> > +		d = debugfs_create_file("pending_requests", 0400,
> > +			az_blob_debugfs_root, NULL,
> > +			&az_blob_debugfs_fops);
> > +		if (IS_ERR_OR_NULL(d)) {
> 
> How can that be NULL?
> 
> No need to ever check any debugfs calls, please just make them and move on.

Will fix this.

Thank you,

Long

> 
> thanks,
> 
> greg k-h