| Message ID | 1392418688-23895-6-git-send-email-aaron.f.brown@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2014-02-14 at 14:57 -0800, Aaron Brown wrote: > From: Mitch Williams <mitch.a.williams@intel.com> Hello. > Make sure errors are reported at the correct log level, quit printing > the function name every time, and make the messages more consistent in > format. > diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c [] > @@ -1939,14 +1939,14 @@ static void i40evf_init_task(struct work_struct *work) > adapter->flags &= ~I40EVF_FLAG_RESET_PENDING; > err = i40e_set_mac_type(hw); > if (err) { > - dev_info(&pdev->dev, "%s: set_mac_type failed: %d\n", > - __func__, err); > + dev_err(&pdev->dev, "Failed to set MAC type (%d)\n", > + err); > goto err; > } > err = i40evf_check_reset_complete(hw); > if (err) { > - dev_info(&pdev->dev, "%s: device is still in reset (%d).\n", > - __func__, err); > + dev_err(&pdev->dev, "Device is still in reset (%d).\n", > + err); Look at the lines above and below here. If you're going to be consistent, can you please remove the unnecessary period before the newline. > goto err; > } > hw->aq.num_arq_entries = I40EVF_AQ_LEN; > @@ -1956,14 +1956,14 @@ static void i40evf_init_task(struct work_struct *work) > > err = i40evf_init_adminq(hw); > if (err) { > - dev_info(&pdev->dev, "%s: init_adminq failed: %d\n", > - __func__, err); > + dev_err(&pdev->dev, "Failed to init Admin Queue (%d)\n", > + err); > goto err; > } > err = i40evf_send_api_ver(adapter); > if (err) { > - dev_info(&pdev->dev, "%s: unable to send to PF (%d)\n", > - __func__, err); > + dev_err(&pdev->dev, "Unable to send to PF (%d)\n", > + err); > i40evf_shutdown_adminq(hw); > goto err; > } [] > @@ -1998,8 +1998,7 @@ static void i40evf_init_task(struct work_struct *work) > sizeof(struct i40e_virtchnl_vsi_resource)); > adapter->vf_res = kzalloc(bufsz, GFP_KERNEL); > if (!adapter->vf_res) { > - dev_err(&pdev->dev, "%s: unable to allocate memory\n", > - __func__); > + dev_err(&pdev->dev, "Unable to allocate memory for vf resources.\n"); Unnecessary OOM. Allocs failures already have one and a dump_stack(). > @@ -2022,7 +2021,7 @@ static void i40evf_init_task(struct work_struct *work) > adapter->vsi_res = &adapter->vf_res->vsi_res[i]; > } > if (!adapter->vsi_res) { > - dev_info(&pdev->dev, "%s: no LAN VSI found\n", __func__); > + dev_err(&pdev->dev, "No LAN VSI found.\n"); Adding unnecessary periods. > @@ -2053,9 +2052,8 @@ static void i40evf_init_task(struct work_struct *work) > > /* The HW MAC address was set and/or determined in sw_init */ > if (!is_valid_ether_addr(adapter->hw.mac.addr)) { > - dev_info(&pdev->dev, > - "Invalid MAC address %pMAC, using random\n", > - adapter->hw.mac.addr); > + dev_info(&pdev->dev, "Invalid MAC address %pMAC, using random.\n", And here too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Joe Perches [mailto:joe@perches.com] > Sent: Friday, February 14, 2014 3:43 PM > To: Brown, Aaron F > Cc: davem@davemloft.net; Williams, Mitch A; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com; Brandeburg, Jesse > Subject: Re: [net-next 05/14] i40evf: fix up strings in init task > > On Fri, 2014-02-14 at 14:57 -0800, Aaron Brown wrote: > > From: Mitch Williams <mitch.a.williams@intel.com> > > Hello. > > > Make sure errors are reported at the correct log level, quit printing > > the function name every time, and make the messages more consistent in > > format. > > > diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c > b/drivers/net/ethernet/intel/i40evf/i40evf_main.c > [] > > @@ -1939,14 +1939,14 @@ static void i40evf_init_task(struct work_struct > *work) > > adapter->flags &= ~I40EVF_FLAG_RESET_PENDING; > > err = i40e_set_mac_type(hw); > > if (err) { > > - dev_info(&pdev->dev, "%s: set_mac_type failed: %d\n", > > - __func__, err); > > + dev_err(&pdev->dev, "Failed to set MAC type (%d)\n", > > + err); > > goto err; > > } > > err = i40evf_check_reset_complete(hw); > > if (err) { > > - dev_info(&pdev->dev, "%s: device is still in reset (%d).\n", > > - __func__, err); > > + dev_err(&pdev->dev, "Device is still in reset (%d).\n", > > + err); > > > Look at the lines above and below here. > > If you're going to be consistent, can you please > remove the unnecessary period before the newline. > > > goto err; > > } > > hw->aq.num_arq_entries = I40EVF_AQ_LEN; > > @@ -1956,14 +1956,14 @@ static void i40evf_init_task(struct work_struct > *work) > > > > err = i40evf_init_adminq(hw); > > if (err) { > > - dev_info(&pdev->dev, "%s: init_adminq failed: %d\n", > > - __func__, err); > > + dev_err(&pdev->dev, "Failed to init Admin Queue (%d)\n", > > + err); > > goto err; > > } > > err = i40evf_send_api_ver(adapter); > > if (err) { > > - dev_info(&pdev->dev, "%s: unable to send to PF (%d)\n", > > - __func__, err); > > + dev_err(&pdev->dev, "Unable to send to PF (%d)\n", > > + err); > > i40evf_shutdown_adminq(hw); > > goto err; > > } > > [] > > > @@ -1998,8 +1998,7 @@ static void i40evf_init_task(struct work_struct > *work) > > sizeof(struct i40e_virtchnl_vsi_resource)); > > adapter->vf_res = kzalloc(bufsz, GFP_KERNEL); > > if (!adapter->vf_res) { > > - dev_err(&pdev->dev, "%s: unable to allocate memory\n", > > - __func__); > > + dev_err(&pdev->dev, "Unable to allocate memory for vf > resources.\n"); > > Unnecessary OOM. > Allocs failures already have one and a dump_stack(). > > > @@ -2022,7 +2021,7 @@ static void i40evf_init_task(struct work_struct > *work) > > adapter->vsi_res = &adapter->vf_res->vsi_res[i]; > > } > > if (!adapter->vsi_res) { > > - dev_info(&pdev->dev, "%s: no LAN VSI found\n", __func__); > > + dev_err(&pdev->dev, "No LAN VSI found.\n"); > > Adding unnecessary periods. > > > @@ -2053,9 +2052,8 @@ static void i40evf_init_task(struct work_struct > *work) > > > > /* The HW MAC address was set and/or determined in sw_init */ > > if (!is_valid_ether_addr(adapter->hw.mac.addr)) { > > - dev_info(&pdev->dev, > > - "Invalid MAC address %pMAC, using random\n", > > - adapter->hw.mac.addr); > > + dev_info(&pdev->dev, "Invalid MAC address %pMAC, using > random.\n", > > And here too. > Joe, thanks for your review. We'll ditch the OOM message and I'll add a reminder to myself to scrub the rest of the driver for more of these. With regard to periods on the end of log messages, is this a hard rule? The grammar pedant in me likes to see periods on the end of sentences, so I tend to add them. However, I see that CodingStyle says, "Kernel messages do not have to be terminated with a period." Perhaps we should change that to say "should not" if it is considered a rule. Either way, we'll respin the patch. -Mitch -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-02-14 at 23:53 +0000, Williams, Mitch A wrote: > With regard to periods on the end of log messages, > is this a hard rule? No. > The grammar pedant in me likes to see periods on the end of sentences, Opinions vary. I think kernel logging output lines aren't sentences and are just notifications that don't need periods. If you look at a normal log, there are relatively few entries with periods. For instance, my current dmesg: $ dmesg | wc -l 1568 $ dmesg | grep "[^\.]\.$" | wc -l 84 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c index e9da5d5..682c341 100644 --- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c +++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c @@ -1939,14 +1939,14 @@ static void i40evf_init_task(struct work_struct *work) adapter->flags &= ~I40EVF_FLAG_RESET_PENDING; err = i40e_set_mac_type(hw); if (err) { - dev_info(&pdev->dev, "%s: set_mac_type failed: %d\n", - __func__, err); + dev_err(&pdev->dev, "Failed to set MAC type (%d)\n", + err); goto err; } err = i40evf_check_reset_complete(hw); if (err) { - dev_info(&pdev->dev, "%s: device is still in reset (%d).\n", - __func__, err); + dev_err(&pdev->dev, "Device is still in reset (%d).\n", + err); goto err; } hw->aq.num_arq_entries = I40EVF_AQ_LEN; @@ -1956,14 +1956,14 @@ static void i40evf_init_task(struct work_struct *work) err = i40evf_init_adminq(hw); if (err) { - dev_info(&pdev->dev, "%s: init_adminq failed: %d\n", - __func__, err); + dev_err(&pdev->dev, "Failed to init Admin Queue (%d)\n", + err); goto err; } err = i40evf_send_api_ver(adapter); if (err) { - dev_info(&pdev->dev, "%s: unable to send to PF (%d)\n", - __func__, err); + dev_err(&pdev->dev, "Unable to send to PF (%d)\n", + err); i40evf_shutdown_adminq(hw); goto err; } @@ -1977,13 +1977,13 @@ static void i40evf_init_task(struct work_struct *work) /* aq msg sent, awaiting reply */ err = i40evf_verify_api_ver(adapter); if (err) { - dev_err(&pdev->dev, "Unable to verify API version, error %d\n", + dev_err(&pdev->dev, "Unable to verify API version (%d)\n", err); goto err; } err = i40evf_send_vf_config_msg(adapter); if (err) { - dev_err(&pdev->dev, "Unable send config request, error %d\n", + dev_err(&pdev->dev, "Unable send config request (%d)\n", err); goto err; } @@ -1998,8 +1998,7 @@ static void i40evf_init_task(struct work_struct *work) sizeof(struct i40e_virtchnl_vsi_resource)); adapter->vf_res = kzalloc(bufsz, GFP_KERNEL); if (!adapter->vf_res) { - dev_err(&pdev->dev, "%s: unable to allocate memory\n", - __func__); + dev_err(&pdev->dev, "Unable to allocate memory for vf resources.\n"); goto err; } } @@ -2007,8 +2006,8 @@ static void i40evf_init_task(struct work_struct *work) if (err == I40E_ERR_ADMIN_QUEUE_NO_WORK) goto restart; if (err) { - dev_info(&pdev->dev, "%s: unable to get VF config (%d)\n", - __func__, err); + dev_err(&pdev->dev, "Unable to get VF config (%d)\n", + err); goto err_alloc; } adapter->state = __I40EVF_INIT_SW; @@ -2022,7 +2021,7 @@ static void i40evf_init_task(struct work_struct *work) adapter->vsi_res = &adapter->vf_res->vsi_res[i]; } if (!adapter->vsi_res) { - dev_info(&pdev->dev, "%s: no LAN VSI found\n", __func__); + dev_err(&pdev->dev, "No LAN VSI found.\n"); goto err_alloc; } @@ -2053,9 +2052,8 @@ static void i40evf_init_task(struct work_struct *work) /* The HW MAC address was set and/or determined in sw_init */ if (!is_valid_ether_addr(adapter->hw.mac.addr)) { - dev_info(&pdev->dev, - "Invalid MAC address %pMAC, using random\n", - adapter->hw.mac.addr); + dev_info(&pdev->dev, "Invalid MAC address %pMAC, using random.\n", + adapter->hw.mac.addr); random_ether_addr(adapter->hw.mac.addr); } memcpy(netdev->dev_addr, adapter->hw.mac.addr, netdev->addr_len);