[PATCH 0/6] Add improvements suggested by checkpatch for vmbus_drv

[PATCH 0/6] Add improvements suggested by checkpatch for vmbus_drv

[Matheus Castello]

This series fixes some warnings edmited by the checkpatch in the file
vmbus_drv.c. I thought it would be good to split each fix into a commit to
help with the review.

Matheus Castello (6):
  drivers: hv: Fix hyperv_record_panic_msg path on comment
  drivers: hv: vmbus: Replace symbolic permissions by octal permissions
  drivers: hv: vmbus: Fix checkpatch LINE_SPACING
  drivers: hv: vmbus: Fix checkpatch SPLIT_STRING
  drivers: hv: vmbus: Fix unnecessary OOM_MESSAGE
  drivers: hv: vmbus: Fix call msleep using < 20ms

 drivers/hv/vmbus_drv.c | 55 +++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

--
2.28.0

[PATCH 1/6] drivers: hv: Fix hyperv_record_panic_msg path on comment

[Matheus Castello]

Fix the kernel parameter path in the comment, in the documentation the
parameter is correct but if someone who is studying the code and see
this first can get confused and try to access the wrong path/parameter

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/hv/vmbus_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4fad3e6745e5..9ed7e3b1d654 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -55,7 +55,7 @@ int vmbus_interrupt;
 /*
  * Boolean to control whether to report panic messages over Hyper-V.
  *
- * It can be set via /proc/sys/kernel/hyperv/record_panic_msg
+ * It can be set via /proc/sys/kernel/hyperv_record_panic_msg
  */
 static int sysctl_record_panic_msg = 1;

--
2.28.0

[PATCH 2/6] drivers: hv: vmbus: Replace symbolic permissions by octal permissions

[Matheus Castello]

This fixed the below checkpatch issue:
WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
Consider using octal permissions '0444'.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/hv/vmbus_drv.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 9ed7e3b1d654..52c1407c1849 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1812,7 +1812,7 @@ static ssize_t channel_pending_show(struct vmbus_channel *channel,
 		       channel_pending(channel,
 				       vmbus_connection.monitor_pages[1]));
 }
-static VMBUS_CHAN_ATTR(pending, S_IRUGO, channel_pending_show, NULL);
+static VMBUS_CHAN_ATTR(pending, 0444, channel_pending_show, NULL);

 static ssize_t channel_latency_show(struct vmbus_channel *channel,
 				    char *buf)
@@ -1821,19 +1821,19 @@ static ssize_t channel_latency_show(struct vmbus_channel *channel,
 		       channel_latency(channel,
 				       vmbus_connection.monitor_pages[1]));
 }
-static VMBUS_CHAN_ATTR(latency, S_IRUGO, channel_latency_show, NULL);
+static VMBUS_CHAN_ATTR(latency, 0444, channel_latency_show, NULL);

 static ssize_t channel_interrupts_show(struct vmbus_channel *channel, char *buf)
 {
 	return sprintf(buf, "%llu\n", channel->interrupts);
 }
-static VMBUS_CHAN_ATTR(interrupts, S_IRUGO, channel_interrupts_show, NULL);
+static VMBUS_CHAN_ATTR(interrupts, 0444, channel_interrupts_show, NULL);

 static ssize_t channel_events_show(struct vmbus_channel *channel, char *buf)
 {
 	return sprintf(buf, "%llu\n", channel->sig_events);
 }
-static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
+static VMBUS_CHAN_ATTR(events, 0444, channel_events_show, NULL);

 static ssize_t channel_intr_in_full_show(struct vmbus_channel *channel,
 					 char *buf)
@@ -1872,7 +1872,7 @@ static ssize_t subchannel_monitor_id_show(struct vmbus_channel *channel,
 {
 	return sprintf(buf, "%u\n", channel->offermsg.monitorid);
 }
-static VMBUS_CHAN_ATTR(monitor_id, S_IRUGO, subchannel_monitor_id_show, NULL);
+static VMBUS_CHAN_ATTR(monitor_id, 0444, subchannel_monitor_id_show, NULL);

 static ssize_t subchannel_id_show(struct vmbus_channel *channel,
 				  char *buf)
--
2.28.0

[PATCH 3/6] drivers: hv: vmbus: Fix checkpatch LINE_SPACING

[Matheus Castello]

Fixed checkpatch warning: Missing a blank line after declarations
checkpatch(LINE_SPACING)

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/hv/vmbus_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 52c1407c1849..61d28c743263 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -156,6 +156,7 @@ static u32 channel_conn_id(struct vmbus_channel *channel,
 {
 	u8 monitor_group = channel_monitor_group(channel);
 	u8 monitor_offset = channel_monitor_offset(channel);
+
 	return monitor_page->parameter[monitor_group][monitor_offset].connectionid.u.id;
 }

@@ -550,6 +551,7 @@ static ssize_t vendor_show(struct device *dev,
 			   char *buf)
 {
 	struct hv_device *hv_dev = device_to_hv_device(dev);
+
 	return sprintf(buf, "0x%x\n", hv_dev->vendor_id);
 }
 static DEVICE_ATTR_RO(vendor);
@@ -559,6 +561,7 @@ static ssize_t device_show(struct device *dev,
 			   char *buf)
 {
 	struct hv_device *hv_dev = device_to_hv_device(dev);
+
 	return sprintf(buf, "0x%x\n", hv_dev->device_id);
 }
 static DEVICE_ATTR_RO(device);
--
2.28.0

[PATCH 4/6] drivers: hv: vmbus: Fix checkpatch SPLIT_STRING

[Matheus Castello]

Checkpatch emits WARNING: quoted string split across lines.
To keep the code clean and with the 80 column length indentation the
check and registration code for kmsg_dump_register has been transferred
to a new function hv_kmsg_dump_register.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/hv/vmbus_drv.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 61d28c743263..09d8236a51cf 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1387,6 +1387,23 @@ static struct kmsg_dumper hv_kmsg_dumper = {
 	.dump = hv_kmsg_dump,
 };

+static void hv_kmsg_dump_register(void)
+{
+	int ret;
+
+	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
+	if (hv_panic_page) {
+		ret = kmsg_dump_register(&hv_kmsg_dumper);
+		if (ret) {
+			pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
+			hv_free_hyperv_page((unsigned long)hv_panic_page);
+			hv_panic_page = NULL;
+		}
+	} else {
+		pr_err("Hyper-V: panic message page memory allocation failed");
+	}
+}
+
 static struct ctl_table_header *hv_ctl_table_hdr;

 /*
@@ -1477,21 +1494,8 @@ static int vmbus_bus_init(void)
 		 * capability is supported by the hypervisor.
 		 */
 		hv_get_crash_ctl(hyperv_crash_ctl);
-		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG) {
-			hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
-			if (hv_panic_page) {
-				ret = kmsg_dump_register(&hv_kmsg_dumper);
-				if (ret) {
-					pr_err("Hyper-V: kmsg dump register "
-						"error 0x%x\n", ret);
-					hv_free_hyperv_page(
-					    (unsigned long)hv_panic_page);
-					hv_panic_page = NULL;
-				}
-			} else
-				pr_err("Hyper-V: panic message page memory "
-					"allocation failed");
-		}
+		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
+			hv_kmsg_dump_register();

 		register_die_notifier(&hyperv_die_block);
 	}
--
2.28.0

[PATCH 5/6] drivers: hv: vmbus: Fix unnecessary OOM_MESSAGE

[Matheus Castello]

Fixed checkpatch warning: Possible unnecessary 'out of memory' message
checkpatch(OOM_MESSAGE)

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/hv/vmbus_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 09d8236a51cf..774b88dd0e15 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1989,10 +1989,8 @@ struct hv_device *vmbus_device_create(const guid_t *type,
 	struct hv_device *child_device_obj;

 	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
-	if (!child_device_obj) {
-		pr_err("Unable to allocate device object for child device\n");
+	if (!child_device_obj)
 		return NULL;
-	}

 	child_device_obj->channel = channel;
 	guid_copy(&child_device_obj->dev_type, type);
--
2.28.0

[PATCH 6/6] drivers: hv: vmbus: Fix call msleep using < 20ms

[Matheus Castello]

Fixed checkpatch warning: MSLEEP: msleep < 20ms can sleep for up to
20ms; see Documentation/timers/timers-howto.rst

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/hv/vmbus_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 774b88dd0e15..cf49c0c01206 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2382,7 +2382,7 @@ static int vmbus_bus_suspend(struct device *dev)
 		 * We wait here until the completion of any channel
 		 * offers that are currently in progress.
 		 */
-		msleep(1);
+		usleep_range(1000, 2000);
 	}

 	mutex_lock(&vmbus_connection.channel_mutex);
--
2.28.0

RE: [PATCH 1/6] drivers: hv: Fix hyperv_record_panic_msg path on comment

[Michael Kelley]

From: Matheus Castello <matheus@castello.eng.br>  Sent: Sunday, November 15, 2020 11:57 AM
> 
> Fix the kernel parameter path in the comment, in the documentation the
> parameter is correct but if someone who is studying the code and see
> this first can get confused and try to access the wrong path/parameter
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/hv/vmbus_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4fad3e6745e5..9ed7e3b1d654 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -55,7 +55,7 @@ int vmbus_interrupt;
>  /*
>   * Boolean to control whether to report panic messages over Hyper-V.
>   *
> - * It can be set via /proc/sys/kernel/hyperv/record_panic_msg
> + * It can be set via /proc/sys/kernel/hyperv_record_panic_msg
>   */
>  static int sysctl_record_panic_msg = 1;
> 
> --
> 2.28.0

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 2/6] drivers: hv: vmbus: Replace symbolic permissions by octal permissions

[Michael Kelley]

From: Matheus Castello <matheus@castello.eng.br> Sent: Sunday, November 15, 2020 11:58 AM
> 
> This fixed the below checkpatch issue:
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred.
> Consider using octal permissions '0444'.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/hv/vmbus_drv.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 9ed7e3b1d654..52c1407c1849 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1812,7 +1812,7 @@ static ssize_t channel_pending_show(struct vmbus_channel
> *channel,
>  		       channel_pending(channel,
>  				       vmbus_connection.monitor_pages[1]));
>  }
> -static VMBUS_CHAN_ATTR(pending, S_IRUGO, channel_pending_show, NULL);
> +static VMBUS_CHAN_ATTR(pending, 0444, channel_pending_show, NULL);
> 
>  static ssize_t channel_latency_show(struct vmbus_channel *channel,
>  				    char *buf)
> @@ -1821,19 +1821,19 @@ static ssize_t channel_latency_show(struct vmbus_channel
> *channel,
>  		       channel_latency(channel,
>  				       vmbus_connection.monitor_pages[1]));
>  }
> -static VMBUS_CHAN_ATTR(latency, S_IRUGO, channel_latency_show, NULL);
> +static VMBUS_CHAN_ATTR(latency, 0444, channel_latency_show, NULL);
> 
>  static ssize_t channel_interrupts_show(struct vmbus_channel *channel, char *buf)
>  {
>  	return sprintf(buf, "%llu\n", channel->interrupts);
>  }
> -static VMBUS_CHAN_ATTR(interrupts, S_IRUGO, channel_interrupts_show, NULL);
> +static VMBUS_CHAN_ATTR(interrupts, 0444, channel_interrupts_show, NULL);
> 
>  static ssize_t channel_events_show(struct vmbus_channel *channel, char *buf)
>  {
>  	return sprintf(buf, "%llu\n", channel->sig_events);
>  }
> -static VMBUS_CHAN_ATTR(events, S_IRUGO, channel_events_show, NULL);
> +static VMBUS_CHAN_ATTR(events, 0444, channel_events_show, NULL);
> 
>  static ssize_t channel_intr_in_full_show(struct vmbus_channel *channel,
>  					 char *buf)
> @@ -1872,7 +1872,7 @@ static ssize_t subchannel_monitor_id_show(struct
> vmbus_channel *channel,
>  {
>  	return sprintf(buf, "%u\n", channel->offermsg.monitorid);
>  }
> -static VMBUS_CHAN_ATTR(monitor_id, S_IRUGO, subchannel_monitor_id_show, NULL);
> +static VMBUS_CHAN_ATTR(monitor_id, 0444, subchannel_monitor_id_show, NULL);
> 
>  static ssize_t subchannel_id_show(struct vmbus_channel *channel,
>  				  char *buf)
> --
> 2.28.0

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 3/6] drivers: hv: vmbus: Fix checkpatch LINE_SPACING

[Michael Kelley]

From: Matheus Castello <matheus@castello.eng.br> Sent: Sunday, November 15, 2020 11:58 AM
> 
> Fixed checkpatch warning: Missing a blank line after declarations
> checkpatch(LINE_SPACING)
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/hv/vmbus_drv.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 52c1407c1849..61d28c743263 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -156,6 +156,7 @@ static u32 channel_conn_id(struct vmbus_channel *channel,
>  {
>  	u8 monitor_group = channel_monitor_group(channel);
>  	u8 monitor_offset = channel_monitor_offset(channel);
> +
>  	return monitor_page-
> >parameter[monitor_group][monitor_offset].connectionid.u.id;
>  }
> 
> @@ -550,6 +551,7 @@ static ssize_t vendor_show(struct device *dev,
>  			   char *buf)
>  {
>  	struct hv_device *hv_dev = device_to_hv_device(dev);
> +
>  	return sprintf(buf, "0x%x\n", hv_dev->vendor_id);
>  }
>  static DEVICE_ATTR_RO(vendor);
> @@ -559,6 +561,7 @@ static ssize_t device_show(struct device *dev,
>  			   char *buf)
>  {
>  	struct hv_device *hv_dev = device_to_hv_device(dev);
> +
>  	return sprintf(buf, "0x%x\n", hv_dev->device_id);
>  }
>  static DEVICE_ATTR_RO(device);
> --
> 2.28.0

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 4/6] drivers: hv: vmbus: Fix checkpatch SPLIT_STRING

[Michael Kelley]

From: Matheus Castello <matheus@castello.eng.br> Sent: Sunday, November 15, 2020 11:58 AM
> 
> Checkpatch emits WARNING: quoted string split across lines.
> To keep the code clean and with the 80 column length indentation the
> check and registration code for kmsg_dump_register has been transferred
> to a new function hv_kmsg_dump_register.
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/hv/vmbus_drv.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 61d28c743263..09d8236a51cf 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1387,6 +1387,23 @@ static struct kmsg_dumper hv_kmsg_dumper = {
>  	.dump = hv_kmsg_dump,
>  };
> 
> +static void hv_kmsg_dump_register(void)
> +{
> +	int ret;
> +
> +	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
> +	if (hv_panic_page) {
> +		ret = kmsg_dump_register(&hv_kmsg_dumper);
> +		if (ret) {
> +			pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
> +			hv_free_hyperv_page((unsigned long)hv_panic_page);
> +			hv_panic_page = NULL;
> +		}
> +	} else {
> +		pr_err("Hyper-V: panic message page memory allocation failed");
> +	}
> +}
> +

The above would be marginally better if organized as follows so that the
main execution path isn't in an "if" clause.  Also reduces indentation.

	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
	if (!hv_panic_page) {
		pr_err("Hyper-V: panic message page memory allocation failed");
		return;
	}
	ret = kmsg_dump_register(&hv_kmsg_dumper);
	if (ret) {
		pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
		hv_free_hyperv_page((unsigned long)hv_panic_page);
		hv_panic_page = NULL;
	}


>  static struct ctl_table_header *hv_ctl_table_hdr;
> 
>  /*
> @@ -1477,21 +1494,8 @@ static int vmbus_bus_init(void)
>  		 * capability is supported by the hypervisor.
>  		 */
>  		hv_get_crash_ctl(hyperv_crash_ctl);
> -		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG) {
> -			hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
> -			if (hv_panic_page) {
> -				ret = kmsg_dump_register(&hv_kmsg_dumper);
> -				if (ret) {
> -					pr_err("Hyper-V: kmsg dump register "
> -						"error 0x%x\n", ret);
> -					hv_free_hyperv_page(
> -					    (unsigned long)hv_panic_page);
> -					hv_panic_page = NULL;
> -				}
> -			} else
> -				pr_err("Hyper-V: panic message page memory "
> -					"allocation failed");
> -		}
> +		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
> +			hv_kmsg_dump_register();
> 
>  		register_die_notifier(&hyperv_die_block);
>  	}
> --
> 2.28.0

RE: [PATCH 6/6] drivers: hv: vmbus: Fix call msleep using < 20ms

[Michael Kelley]

From: Matheus Castello <matheus@castello.eng.br>  Sent: Sunday, November 15, 2020 11:58 AM
> 
> Fixed checkpatch warning: MSLEEP: msleep < 20ms can sleep for up to
> 20ms; see Documentation/timers/timers-howto.rst
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/hv/vmbus_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 774b88dd0e15..cf49c0c01206 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2382,7 +2382,7 @@ static int vmbus_bus_suspend(struct device *dev)
>  		 * We wait here until the completion of any channel
>  		 * offers that are currently in progress.
>  		 */
> -		msleep(1);
> +		usleep_range(1000, 2000);
>  	}
> 
>  	mutex_lock(&vmbus_connection.channel_mutex);
> --
> 2.28.0

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Re: [PATCH 5/6] drivers: hv: vmbus: Fix unnecessary OOM_MESSAGE

[Wei Liu]

On Sun, Nov 15, 2020 at 04:57:33PM -0300, Matheus Castello wrote:
> Fixed checkpatch warning: Possible unnecessary 'out of memory' message
> checkpatch(OOM_MESSAGE)
> 
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> ---
>  drivers/hv/vmbus_drv.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 09d8236a51cf..774b88dd0e15 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1989,10 +1989,8 @@ struct hv_device *vmbus_device_create(const guid_t *type,
>  	struct hv_device *child_device_obj;
> 
>  	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
> -	if (!child_device_obj) {
> -		pr_err("Unable to allocate device object for child device\n");
> +	if (!child_device_obj)

The generic OOM message would give you a stack dump but not as specific
/ clear as the message you deleted.

Also, the original intent of this check was to check for things like

    printk("Out of memory");

which was clearly redundant. The message we print here is not that.

See https://lkml.org/lkml/2014/6/10/382 .

Wei.

>  		return NULL;
> -	}
> 
>  	child_device_obj->channel = channel;
>  	guid_copy(&child_device_obj->dev_type, type);
> --
> 2.28.0
> 

Re: [PATCH 5/6] drivers: hv: vmbus: Fix unnecessary OOM_MESSAGE

[Wei Liu]

On Mon, Nov 16, 2020 at 11:21:48AM +0000, Wei Liu wrote:
> On Sun, Nov 15, 2020 at 04:57:33PM -0300, Matheus Castello wrote:
> > Fixed checkpatch warning: Possible unnecessary 'out of memory' message
> > checkpatch(OOM_MESSAGE)
> > 
> > Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> > ---
> >  drivers/hv/vmbus_drv.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 09d8236a51cf..774b88dd0e15 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1989,10 +1989,8 @@ struct hv_device *vmbus_device_create(const guid_t *type,
> >  	struct hv_device *child_device_obj;
> > 
> >  	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
> > -	if (!child_device_obj) {
> > -		pr_err("Unable to allocate device object for child device\n");
> > +	if (!child_device_obj)
> 
> The generic OOM message would give you a stack dump but not as specific
> / clear as the message you deleted.
> 
> Also, the original intent of this check was to check for things like
> 
>     printk("Out of memory");
> 
> which was clearly redundant. The message we print here is not that.
> 
> See https://lkml.org/lkml/2014/6/10/382 .
> 

In case my message is not clear, I think this pr_err should stay. ;-)

Wei.

Re: [PATCH 0/6] Add improvements suggested by checkpatch for vmbus_drv

[Wei Liu]

On Sun, Nov 15, 2020 at 04:57:28PM -0300, Matheus Castello wrote:
> This series fixes some warnings edmited by the checkpatch in the file
> vmbus_drv.c. I thought it would be good to split each fix into a commit to
> help with the review.
> 
> Matheus Castello (6):
>   drivers: hv: Fix hyperv_record_panic_msg path on comment
>   drivers: hv: vmbus: Replace symbolic permissions by octal permissions
>   drivers: hv: vmbus: Fix checkpatch LINE_SPACING
>   drivers: hv: vmbus: Fix checkpatch SPLIT_STRING
>   drivers: hv: vmbus: Fix unnecessary OOM_MESSAGE
>   drivers: hv: vmbus: Fix call msleep using < 20ms

I've pushed patch 1-3 and 6 to hyperv-next.

Patch 4 has a pending comment from Michael. Patch 5 can be dropped.

Wei.

Re: [PATCH 0/6] Add improvements suggested by checkpatch for vmbus_drv

[Matheus Castello]

Hi Wei,

Em 11/17/2020 7:58 AM, Wei Liu escreveu:
> On Sun, Nov 15, 2020 at 04:57:28PM -0300, Matheus Castello wrote:
>> This series fixes some warnings edmited by the checkpatch in the file
>> vmbus_drv.c. I thought it would be good to split each fix into a commit to
>> help with the review.
>>
>> Matheus Castello (6):
>>    drivers: hv: Fix hyperv_record_panic_msg path on comment
>>    drivers: hv: vmbus: Replace symbolic permissions by octal permissions
>>    drivers: hv: vmbus: Fix checkpatch LINE_SPACING
>>    drivers: hv: vmbus: Fix checkpatch SPLIT_STRING
>>    drivers: hv: vmbus: Fix unnecessary OOM_MESSAGE
>>    drivers: hv: vmbus: Fix call msleep using < 20ms
> 
> I've pushed patch 1-3 and 6 to hyperv-next.
> 
> Patch 4 has a pending comment from Michael. Patch 5 can be dropped.
>

Thanks for the review, got it and I agree with the drop of patch 5 
thanks to point it. I will send the v2 for the patch 4.

> Wei.
> 

Re: [PATCH 4/6] drivers: hv: vmbus: Fix checkpatch SPLIT_STRING

[Matheus Castello]

Hi Michael,

Em 11/15/2020 7:25 PM, Michael Kelley escreveu:
> From: Matheus Castello <matheus@castello.eng.br> Sent: Sunday, November 15, 2020 11:58 AM
>>
>> Checkpatch emits WARNING: quoted string split across lines.
>> To keep the code clean and with the 80 column length indentation the
>> check and registration code for kmsg_dump_register has been transferred
>> to a new function hv_kmsg_dump_register.
>>
>> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
>> ---
>>   drivers/hv/vmbus_drv.c | 34 +++++++++++++++++++---------------
>>   1 file changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 61d28c743263..09d8236a51cf 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -1387,6 +1387,23 @@ static struct kmsg_dumper hv_kmsg_dumper = {
>>   	.dump = hv_kmsg_dump,
>>   };
>>
>> +static void hv_kmsg_dump_register(void)
>> +{
>> +	int ret;
>> +
>> +	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
>> +	if (hv_panic_page) {
>> +		ret = kmsg_dump_register(&hv_kmsg_dumper);
>> +		if (ret) {
>> +			pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
>> +			hv_free_hyperv_page((unsigned long)hv_panic_page);
>> +			hv_panic_page = NULL;
>> +		}
>> +	} else {
>> +		pr_err("Hyper-V: panic message page memory allocation failed");
>> +	}
>> +}
>> +
> 
> The above would be marginally better if organized as follows so that the
> main execution path isn't in an "if" clause.  Also reduces indentation.
> 
> 	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
> 	if (!hv_panic_page) {
> 		pr_err("Hyper-V: panic message page memory allocation failed");
> 		return;
> 	}
> 	ret = kmsg_dump_register(&hv_kmsg_dumper);
> 	if (ret) {
> 		pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
> 		hv_free_hyperv_page((unsigned long)hv_panic_page);
> 		hv_panic_page = NULL;
> 	}
> 
> 

Thanks for the review, great I will use it on the v2.

>>   static struct ctl_table_header *hv_ctl_table_hdr;
>>
>>   /*
>> @@ -1477,21 +1494,8 @@ static int vmbus_bus_init(void)
>>   		 * capability is supported by the hypervisor.
>>   		 */
>>   		hv_get_crash_ctl(hyperv_crash_ctl);
>> -		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG) {
>> -			hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
>> -			if (hv_panic_page) {
>> -				ret = kmsg_dump_register(&hv_kmsg_dumper);
>> -				if (ret) {
>> -					pr_err("Hyper-V: kmsg dump register "
>> -						"error 0x%x\n", ret);
>> -					hv_free_hyperv_page(
>> -					    (unsigned long)hv_panic_page);
>> -					hv_panic_page = NULL;
>> -				}
>> -			} else
>> -				pr_err("Hyper-V: panic message page memory "
>> -					"allocation failed");
>> -		}
>> +		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
>> +			hv_kmsg_dump_register();
>>
>>   		register_die_notifier(&hyperv_die_block);
>>   	}
>> --
>> 2.28.0
> 

Re: [PATCH 4/6] drivers: hv: vmbus: Fix checkpatch SPLIT_STRING

[Joe Perches]

On Tue, 2020-11-24 at 21:54 -0300, Matheus Castello wrote:
> > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
[]
> > The above would be marginally better if organized as follows so that the
> > main execution path isn't in an "if" clause.  Also reduces indentation.
> > 
> > 	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
> > 	if (!hv_panic_page) {
> > 		pr_err("Hyper-V: panic message page memory allocation failed");

And nicer to add a terminating newline to the format like the pr_err below.

> > 		return;
> > 	}
> > 	ret = kmsg_dump_register(&hv_kmsg_dumper);
> > 	if (ret) {
> > 		pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
> > 		hv_free_hyperv_page((unsigned long)hv_panic_page);
> > 		hv_panic_page = NULL;
> > 	}

Re: [PATCH 4/6] drivers: hv: vmbus: Fix checkpatch SPLIT_STRING

[Matheus Castello]

Em 11/24/2020 11:56 PM, Joe Perches escreveu:
> On Tue, 2020-11-24 at 21:54 -0300, Matheus Castello wrote:
>>>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> []
>>> The above would be marginally better if organized as follows so that the
>>> main execution path isn't in an "if" clause.  Also reduces indentation.
>>>
>>> 	hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
>>> 	if (!hv_panic_page) {
>>> 		pr_err("Hyper-V: panic message page memory allocation failed");
> 
> And nicer to add a terminating newline to the format like the pr_err below.
>

Oops, sending v3. Thanks Joe Perches!

>>> 		return;
>>> 	}
>>> 	ret = kmsg_dump_register(&hv_kmsg_dumper);
>>> 	if (ret) {
>>> 		pr_err("Hyper-V: kmsg dump register error 0x%x\n", ret);
>>> 		hv_free_hyperv_page((unsigned long)hv_panic_page);
>>> 		hv_panic_page = NULL;
>>> 	}
> 
>