Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466831 > unrolled thread

[PATCH 0/3] hostap: Fine-tuning for a few functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/3] hostap: Fine-tuning for a few functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-20 18:50 +0200
    [PATCH 3/3] hostap: Delete unnecessary initialisations for the  variable "ret" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-20 18:50 +0200
    [PATCH 2/3] hostap: Delete an unnecessary jump label in  prism2_ioctl_priv_hostapd() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-20 18:50 +0200
      Re: [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd() Julian Calaby <julian.calaby@gmail.com> - 2016-08-21 03:50 +0200
    [PATCH 1/3] hostap: Use memdup_user() rather than duplicating its  implementation SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-20 18:50 +0200
    Re: [PATCH 0/3] hostap: Fine-tuning for a few functions Arend van Spriel <arend.vanspriel@broadcom.com> - 2016-08-20 21:30 +0200
      Re: [PATCH 0/3] hostap: Fine-tuning for a few functions Kalle Valo <kvalo@codeaurora.org> - 2016-08-22 17:50 +0200
        Re: [PATCH 0/3] hostap: Fine-tuning for a few functions Joe Perches <joe@perches.com> - 2016-08-22 18:30 +0200
        [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS Joe Perches <joe@perches.com> - 2016-08-22 20:20 +0200
          Re: [PATCH] checkpatch: See if modified files are marked obsolete in  MAINTAINERS SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-22 23:00 +0200
            Re: [PATCH] checkpatch: See if modified files are marked obsolete  in MAINTAINERS Joe Perches <joe@perches.com> - 2016-08-22 23:00 +0200
          Re: checkpatch: See if modified files are marked obsolete in  MAINTAINERS SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-23 09:30 +0200
            Re: checkpatch: See if modified files are marked obsolete in  MAINTAINERS Julia Lawall <julia.lawall@lip6.fr> - 2016-08-23 12:20 +0200

#1466831 — [PATCH 0/3] hostap: Fine-tuning for a few functions

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:35:43 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Use memdup_user()
  Delete an unnecessary jump label
  Delete unnecessary variable initialisations

 .../net/wireless/intersil/hostap/hostap_ioctl.c    | 36 ++++++++--------------
 1 file changed, 12 insertions(+), 24 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1466832 — [PATCH 3/3] hostap: Delete unnecessary initialisations for the variable "ret"

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:23:14 +0200

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning of four functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 5942917..c37b0bb 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -2895,7 +2895,7 @@ static int prism2_ioctl_priv_monitor(struct net_device *dev, int *i)
 {
 	struct hostap_interface *iface;
 	local_info_t *local;
-	int ret = 0;
+	int ret;
 	u32 mode;
 
 	iface = netdev_priv(dev);
@@ -3035,7 +3035,7 @@ static int ap_mac_cmd_ioctl(local_info_t *local, int *cmd)
 static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
 {
 	struct prism2_download_param *param;
-	int ret = 0;
+	int ret;
 
 	if (p->length < sizeof(struct prism2_download_param) ||
 	    p->length > 1024 || !p->pointer)
@@ -3791,7 +3791,7 @@ static int prism2_ioctl_scan_req(local_info_t *local,
 static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 {
 	struct prism2_hostapd_param *param;
-	int ret = 0;
+	int ret;
 	int ap_ioctl = 0;
 
 	if (p->length < sizeof(struct prism2_hostapd_param) ||
@@ -3954,7 +3954,7 @@ int hostap_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct iwreq *wrq = (struct iwreq *) ifr;
 	struct hostap_interface *iface;
 	local_info_t *local;
-	int ret = 0;
+	int ret;
 
 	iface = netdev_priv(dev);
 	local = iface->local;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1466833 — [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:21:29 +0200

Remove a jump label which is unneeded in this function at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 4e271f9..5942917 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 	}
 
 	if (ret == 1 || !ap_ioctl) {
-		if (copy_to_user(p->pointer, param, p->length)) {
+		if (copy_to_user(p->pointer, param, p->length))
 			ret = -EFAULT;
-			goto out;
-		} else if (ap_ioctl)
+		else if (ap_ioctl)
 			ret = 0;
 	}
 
- out:
 	kfree(param);
 	return ret;
 }
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1466891 — Re: [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

Hi Marcus,

On Sun, Aug 21, 2016 at 2:46 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
>
> Remove a jump label which is unneeded in this function at the end.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> index 4e271f9..5942917 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> @@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
>         }
>
>         if (ret == 1 || !ap_ioctl) {
> -               if (copy_to_user(p->pointer, param, p->length)) {
> +               if (copy_to_user(p->pointer, param, p->length))
>                         ret = -EFAULT;
> -                       goto out;
> -               } else if (ap_ioctl)
> +               else if (ap_ioctl)
>                         ret = 0;
>         }
>
> - out:

Does this change make any difference to the compiled code?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[toc] | [prev] | [next] | [standalone]

#1466834 — [PATCH 1/3] hostap: Use memdup_user() rather than duplicating its implementation

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:19:43 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 .../net/wireless/intersil/hostap/hostap_ioctl.c    | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 3e5fa78..4e271f9 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3041,14 +3041,9 @@ static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
 	    p->length > 1024 || !p->pointer)
 		return -EINVAL;
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(param, p->pointer, p->length)) {
-		ret = -EFAULT;
-		goto out;
-	}
+	param = memdup_user(p->pointer, p->length);
+	if (IS_ERR(param))
+		return PTR_ERR(param);
 
 	if (p->length < sizeof(struct prism2_download_param) +
 	    param->num_areas * sizeof(struct prism2_download_area)) {
@@ -3803,14 +3798,9 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 	    p->length > PRISM2_HOSTAPD_MAX_BUF_SIZE || !p->pointer)
 		return -EINVAL;
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(param, p->pointer, p->length)) {
-		ret = -EFAULT;
-		goto out;
-	}
+	param = memdup_user(p->pointer, p->length);
+	if (IS_ERR(param))
+		return PTR_ERR(param);
 
 	switch (param->cmd) {
 	case PRISM2_SET_ENCRYPTION:
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1466857

On 20-08-16 18:43, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:35:43 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.

Is it worth touching this old stuff especially when you are not making
any functional changes.

Regards,
Arend

> Markus Elfring (3):
>   Use memdup_user()
>   Delete an unnecessary jump label
>   Delete unnecessary variable initialisations
> 
>  .../net/wireless/intersil/hostap/hostap_ioctl.c    | 36 ++++++++--------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
> 

[toc] | [prev] | [next] | [standalone]

#1467746

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 20-08-16 18:43, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sat, 20 Aug 2016 18:35:43 +0200
>> 
>> A few update suggestions were taken into account
>> from static source code analysis.
>
> Is it worth touching this old stuff especially when you are not making
> any functional changes.

On the other hand if these patches break something this might be a good
way to get feedback if someone is really using this driver ;)

But yeah, not really sure what to do with these obsolete drivers like
hostap, ray_cs and wl3501.

-- 
Kalle Valo

[toc] | [prev] | [next] | [standalone]

#1467770

On Mon, 2016-08-22 at 18:49 +0300, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
[]
> But yeah, not really sure what to do with these obsolete drivers like
> hostap, ray_cs and wl3501.

Maybe marking sections obsolete in MAINTAINERS could
flag some "shouldn't touch this" warning for old code
in checkpatch.pl and/or get_maintainer.pl

[toc] | [prev] | [next] | [standalone]

#1467874 — [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

Use get_maintainer to check the status of individual files.
If "obsolete", suggest leaving the files alone.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4de3cc4..df5e9d9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -704,6 +704,16 @@ sub seed_camelcase_file {
 	}
 }
 
+sub is_maintained_obsolete {
+	my ($filename) = @_;
+
+	return 0 if (!(-e "$root/scripts/get_maintainer.pl"));
+
+	my $status = `perl $root/scripts/get_maintainer.pl --status --nom --nol --nogit --nogit-fallback $filename 2>&1`;
+
+	return $status =~ /obsolete/i;
+}
+
 my $camelcase_seeded = 0;
 sub seed_camelcase_includes {
 	return if ($camelcase_seeded);
@@ -2289,6 +2299,10 @@ sub process {
 		}
 
 		if ($found_file) {
+			if (is_maintained_obsolete($realfile)) {
+				WARN("OBSOLETE",
+				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
+			}
 			if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) {
 				$check = 1;
 			} else {
-- 
2.8.0.rc4.16.g56331f8

[toc] | [prev] | [next] | [standalone]

#1468040 — Re: [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

> @@ -2289,6 +2299,10 @@ sub process {
>  		}
>  
>  		if ($found_file) {
> +			if (is_maintained_obsolete($realfile)) {
> +				WARN("OBSOLETE",
> +				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
> +			}

How do you think about to avoid a double negation in such a warning message?

Would a wording like "… Only really necessary modifications please.\n"
be more useful here?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1468041 — Re: [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

On Mon, 2016-08-22 at 22:50 +0200, SF Markus Elfring wrote:
> > @@ -2289,6 +2299,10 @@ sub process {
> >  		}
> >  
> >  		if ($found_file) {
> > +			if (is_maintained_obsolete($realfile)) {
> > +				WARN("OBSOLETE",
> > +				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
> > +			}
> How do you think about to avoid a double negation in such a warning message?
> 
> Would a wording like "… Only really necessary modifications please.\n"
> be more useful here?

No, probably not.

[toc] | [prev] | [next] | [standalone]

#1468341 — Re: checkpatch: See if modified files are marked obsolete in MAINTAINERS

> Use get_maintainer to check the status of individual files.
> If "obsolete", suggest leaving the files alone.

Will another software system like the "kbuild test robot"
need any more fine-tuning for this change?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1468464 — Re: checkpatch: See if modified files are marked obsolete in MAINTAINERS

On Tue, 23 Aug 2016, SF Markus Elfring wrote:

> > Use get_maintainer to check the status of individual files.
> > If "obsolete", suggest leaving the files alone.
>
> Will another software system like the "kbuild test robot"
> need any more fine-tuning for this change?

It only works on files in which there have been commits, thus by
definition not obsolete.

julia


>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web