Re: [PATCH 1/3] nl80211: Fix checkpatch warnings

[Johannes Berg]

On Mon, 2016-05-30 at 10:30 +1000, Julian Calaby wrote:
> Hi All,
> 
> On Sun, May 29, 2016 at 1:30 PM, Kirtika Ruchandani
>  wrote:
> > 
> > This patch fixes the following checkpatch.pl warnings about
> > comments in nl80211.c :
> > - networking block comments don't use an empty '/*' line
> > - block comments use a trailing '*/' on a separate line
> > 
> > Signed-off-by: Kirtika Ruchandani 
> The change and logic behind it are sound, so it gets my:
> 
> Reviewed-by: Julian Calaby 
> 
> however I'm concerned that this file is a deliberate exception to the
> networking comment rules.
> 

It's kinda mixed, I never really enforced one style or the other ...
I'm kinda taking both now, with a slight preference towards the
networking style perhaps (to please davem :) )

That said, in general I'm not really sure of the value of all of these
patches - perhaps the kcalloc() one makes sense, not for checkpatch
reasons but rather for size limit/integer overflow reasons, and the
ether address assign for general readability, but overall ...

I'll have to make up my mind :)

johannes

Re: [PATCH 1/3] nl80211: Fix checkpatch warnings

[Julian Calaby]

Hi All,

On Sun, May 29, 2016 at 1:30 PM, Kirtika Ruchandani
 wrote:
> This patch fixes the following checkpatch.pl warnings about
> comments in nl80211.c :
> - networking block comments don't use an empty '/*' line
> - block comments use a trailing '*/' on a separate line
>
> Signed-off-by: Kirtika Ruchandani 

The change and logic behind it are sound, so it gets my:

Reviewed-by: Julian Calaby 

however I'm concerned that this file is a deliberate exception to the
networking comment rules.

Johannes?

Thanks,

Julian Calaby


> ---
>  net/wireless/nl80211.c | 129 
> +
>  1 file changed, 45 insertions(+), 84 deletions(-)
>
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index d759901..50a0de0 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -196,8 +196,7 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct 
> nlattr **attrs)
> return rdev;
>  }
>
> -/*
> - * This function returns a pointer to the driver
> +/* This function returns a pointer to the driver
>   * that the genl_info item that is passed refers to.
>   *
>   * The result of this can be a PTR_ERR and hence must
> @@ -1624,8 +1623,7 @@ static int nl80211_send_wiphy(struct 
> cfg80211_registered_device *rdev,
> goto nla_put_failure;
>
> features = rdev->wiphy.features;
> -   /*
> -* We can only add the per-channel limit information if the
> +   /* We can only add the per-channel limit information if the
>  * dump is split, otherwise it makes it too big. Therefore
>  * only advertise it in that case.
>  */
> @@ -1646,8 +1644,7 @@ static int nl80211_send_wiphy(struct 
> cfg80211_registered_device *rdev,
> rdev->wiphy.max_acl_mac_addrs))
> goto nla_put_failure;
>
> -   /*
> -* Any information below this point is only available to
> +   /* Any information below this point is only available to
>  * applications that can deal with it being split. This
>  * helps ensure that newly added capabilities don't break
>  * older tools by overrunning their buffers.
> @@ -1847,8 +1844,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, 
> struct netlink_callback *cb)
>  cb->nlh->nlmsg_seq,
>  NLM_F_MULTI, state);
> if (ret < 0) {
> -   /*
> -* If sending the wiphy data didn't fit 
> (ENOBUFS
> +   /* If sending the wiphy data didn't fit 
> (ENOBUFS
>  * or EMSGSIZE returned), this SKB is still
>  * empty (so it's not too big because another
>  * wiphy dataset is already in the skb) and
> @@ -1937,8 +1933,7 @@ static int parse_txq_params(struct nlattr *tb[],
>
>  static bool nl80211_can_set_dev_channel(struct wireless_dev *wdev)
>  {
> -   /*
> -* You can only set the channel explicitly for WDS interfaces,
> +   /* You can only set the channel explicitly for WDS interfaces,
>  * all others have their channel managed via their respective
>  * "establish a connection" command (connect, join, ...)
>  *
> @@ -2131,8 +2126,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, 
> struct genl_info *info)
>
> ASSERT_RTNL();
>
> -   /*
> -* Try to find the wiphy and netdev. Normally this
> +   /* Try to find the wiphy and netdev. Normally this
>  * function shouldn't need the netdev, but this is
>  * done for backward compatibility -- previously
>  * setting the channel was done per wiphy, but now
> @@ -2162,8 +2156,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, 
> struct genl_info *info)
> } else
> wdev = netdev->ieee80211_ptr;
>
> -   /*
> -* end workaround code, by now the rdev is available
> +   /* end workaround code, by now the rdev is available
>  * and locked, and wdev may or may not be NULL.
>  */
>
> @@ -2260,7 +2253,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, 
> struct genl_info *info)
> rx_ant = 
> nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_ANTENNA_RX]);
>
> /* reject antenna configurations which don't match the
> -* available antenna masks, except for the "all" mask */
> +* available antenna masks, except for the "all" mask
> +*/
> if ((~tx_ant && (tx_ant & 
> ~rdev->wiphy.available_antennas_tx)) ||
> (~rx_ant && (rx_ant & 
> 

[PATCH 1/3] nl80211: Fix checkpatch warnings

[Kirtika Ruchandani]

This patch fixes the following checkpatch.pl warnings about
comments in nl80211.c :
- networking block comments don't use an empty '/*' line
- block comments use a trailing '*/' on a separate line

Signed-off-by: Kirtika Ruchandani 
---
 net/wireless/nl80211.c | 129 +
 1 file changed, 45 insertions(+), 84 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d759901..50a0de0 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -196,8 +196,7 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct nlattr 
**attrs)
return rdev;
 }
 
-/*
- * This function returns a pointer to the driver
+/* This function returns a pointer to the driver
  * that the genl_info item that is passed refers to.
  *
  * The result of this can be a PTR_ERR and hence must
@@ -1624,8 +1623,7 @@ static int nl80211_send_wiphy(struct 
cfg80211_registered_device *rdev,
goto nla_put_failure;
 
features = rdev->wiphy.features;
-   /*
-* We can only add the per-channel limit information if the
+   /* We can only add the per-channel limit information if the
 * dump is split, otherwise it makes it too big. Therefore
 * only advertise it in that case.
 */
@@ -1646,8 +1644,7 @@ static int nl80211_send_wiphy(struct 
cfg80211_registered_device *rdev,
rdev->wiphy.max_acl_mac_addrs))
goto nla_put_failure;
 
-   /*
-* Any information below this point is only available to
+   /* Any information below this point is only available to
 * applications that can deal with it being split. This
 * helps ensure that newly added capabilities don't break
 * older tools by overrunning their buffers.
@@ -1847,8 +1844,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct 
netlink_callback *cb)
 cb->nlh->nlmsg_seq,
 NLM_F_MULTI, state);
if (ret < 0) {
-   /*
-* If sending the wiphy data didn't fit (ENOBUFS
+   /* If sending the wiphy data didn't fit (ENOBUFS
 * or EMSGSIZE returned), this SKB is still
 * empty (so it's not too big because another
 * wiphy dataset is already in the skb) and
@@ -1937,8 +1933,7 @@ static int parse_txq_params(struct nlattr *tb[],
 
 static bool nl80211_can_set_dev_channel(struct wireless_dev *wdev)
 {
-   /*
-* You can only set the channel explicitly for WDS interfaces,
+   /* You can only set the channel explicitly for WDS interfaces,
 * all others have their channel managed via their respective
 * "establish a connection" command (connect, join, ...)
 *
@@ -2131,8 +2126,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct 
genl_info *info)
 
ASSERT_RTNL();
 
-   /*
-* Try to find the wiphy and netdev. Normally this
+   /* Try to find the wiphy and netdev. Normally this
 * function shouldn't need the netdev, but this is
 * done for backward compatibility -- previously
 * setting the channel was done per wiphy, but now
@@ -2162,8 +2156,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct 
genl_info *info)
} else
wdev = netdev->ieee80211_ptr;
 
-   /*
-* end workaround code, by now the rdev is available
+   /* end workaround code, by now the rdev is available
 * and locked, and wdev may or may not be NULL.
 */
 
@@ -2260,7 +2253,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct 
genl_info *info)
rx_ant = 
nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_ANTENNA_RX]);
 
/* reject antenna configurations which don't match the
-* available antenna masks, except for the "all" mask */
+* available antenna masks, except for the "all" mask
+*/
if ((~tx_ant && (tx_ant & ~rdev->wiphy.available_antennas_tx)) 
||
(~rx_ant && (rx_ant & ~rdev->wiphy.available_antennas_rx)))
return -EINVAL;
@@ -2300,8 +2294,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct 
genl_info *info)
return -EINVAL;
 
if (frag_threshold != (u32) -1) {
-   /*
-* Fragments (apart from the last one) are required to
+   /* Fragments (apart from the last one) are required to
 * have even length. Make the fragmentation code