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

Groups > linux.kernel > #1482758 > unrolled thread

[PATCH 0/7] AGPGART: Fine-tuning for four function implementations

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/7] AGPGART: Fine-tuning for four function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:40 +0200
    [PATCH 6/7] AGPGART-UniNorth: Rename a jump label in  uninorth_create_gatt_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:50 +0200
      Re: [PATCH 6/7] AGPGART-UniNorth: Rename a jump label in  uninorth_create_gatt_table() kbuild test robot <lkp@intel.com> - 2016-09-15 19:40 +0200
        Re: [PATCH 6/7] AGPGART-UniNorth: Rename a jump label in  uninorth_create_gatt_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-15 20:30 +0200
    [PATCH 1/7] AGPGART: Use kmalloc_array() in  compat_agpioc_reserve_wrap() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:50 +0200
    [PATCH 4/7] AGPGART-SGI: Use kmalloc_array() in agp_sgi_init() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:50 +0200
    [PATCH 5/7] AGPGART-UniNorth: Use kmalloc_array() in  uninorth_create_gatt_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:50 +0200
    [PATCH 2/7] AGPGART: Use memdup_user() rather than duplicating its  implementation SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:50 +0200
    [PATCH 3/7] AGPGART: Rename jump labels in compat_agp_ioctl() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 22:50 +0200
    [PATCH 7/7] AGPGART-UniNorth: Delete an unnecessary check in  uninorth_create_gatt_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-13 23:00 +0200

#1482758 — [PATCH 0/7] AGPGART: Fine-tuning for four function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 22:34:56 +0200

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

Markus Elfring (7):
  Use kmalloc_array() in compat_agpioc_reserve_wrap()
  Use memdup_user() rather than duplicating its implementation
  Rename jump labels in compat_agp_ioctl()
  Use kmalloc_array() in agp_sgi_init()
  Use kmalloc_array() in uninorth_create_gatt_table()
  Rename a jump label in uninorth_create_gatt_table()
  Delete an unnecessary check in uninorth_create_gatt_table()

 drivers/char/agp/compat_ioctl.c | 31 +++++++++++++------------------
 drivers/char/agp/sgi-agp.c      |  6 +++---
 drivers/char/agp/uninorth-agp.c | 12 ++++++------
 3 files changed, 22 insertions(+), 27 deletions(-)

-- 
2.10.0

[toc] | [next] | [standalone]

#1482763 — [PATCH 6/7] AGPGART-UniNorth: Rename a jump label in uninorth_create_gatt_table()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 22:00:19 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/uninorth-agp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/char/agp/uninorth-agp.c b/drivers/char/agp/uninorth-agp.c
index 8d144cd..8b1eb72 100644
--- a/drivers/char/agp/uninorth-agp.c
+++ b/drivers/char/agp/uninorth-agp.c
@@ -406,7 +406,7 @@ static int uninorth_create_gatt_table(struct agp_bridge_data *bridge)
 						sizeof(*uninorth_priv.pages_arr),
 						GFP_KERNEL);
 	if (uninorth_priv.pages_arr == NULL)
-		goto enomem;
+		goto free_page_array;
 
 	table_end = table + ((PAGE_SIZE * (1 << page_order)) - 1);
 
@@ -436,8 +436,7 @@ static int uninorth_create_gatt_table(struct agp_bridge_data *bridge)
 		bridge->gatt_table[i] = scratch_value;
 
 	return 0;
-
-enomem:
+ free_page_array:
 	kfree(uninorth_priv.pages_arr);
 	if (table)
 		free_pages((unsigned long)table, page_order);
-- 
2.10.0

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

#1484436 — Re: [PATCH 6/7] AGPGART-UniNorth: Rename a jump label in uninorth_create_gatt_table()

[Multipart message — attachments visible in raw view] — view raw

Hi Markus,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.8-rc6 next-20160915]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/AGPGART-Fine-tuning-for-four-function-implementations/20160914-045406
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: powerpc-pmac32_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   drivers/char/agp/uninorth-agp.c: In function 'uninorth_create_gatt_table':
>> drivers/char/agp/uninorth-agp.c:426:3: error: label 'enomem' used but not defined
      goto enomem;
      ^

vim +/enomem +426 drivers/char/agp/uninorth-agp.c

e8a5f9001 Michel Dänzer   2009-08-04  420  	/* Need to clear out any dirty data still sitting in caches */
e8a5f9001 Michel Dänzer   2009-08-04  421  	flush_dcache_range((unsigned long)table,
79905ad50 Paul Mackerras  2010-06-01  422  			   (unsigned long)table_end + 1);
5ada62b10 Denis Kirjanov  2015-06-12  423  	bridge->gatt_table = vmap(uninorth_priv.pages_arr, (1 << page_order), 0, PAGE_KERNEL_NCG);
e8a5f9001 Michel Dänzer   2009-08-04  424  
e8a5f9001 Michel Dänzer   2009-08-04  425  	if (bridge->gatt_table == NULL)
e8a5f9001 Michel Dänzer   2009-08-04 @426  		goto enomem;
e8a5f9001 Michel Dänzer   2009-08-04  427  
6a12235c7 David Woodhouse 2009-07-29  428  	bridge->gatt_bus_addr = virt_to_phys(table);
^1da177e4 Linus Torvalds  2005-04-16  429  

:::::: The code at line 426 was first introduced by commit
:::::: e8a5f900148d058bce2d7bdce3d6bcbcb40267ec agp/uninorth: Simplify cache flushing.

:::::: TO: Michel Dänzer <daenzer@vmware.com>
:::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1484452 — Re: [PATCH 6/7] AGPGART-UniNorth: Rename a jump label in uninorth_create_gatt_table()

>    drivers/char/agp/uninorth-agp.c: In function 'uninorth_create_gatt_table':
>>> drivers/char/agp/uninorth-agp.c:426:3: error: label 'enomem' used but not defined
>       goto enomem;
>       ^
> 
> vim +/enomem +426 drivers/char/agp/uninorth-agp.c

This error message points a glitch out which was triggered by my update suggestion.
Unfortunately, it means that the proposed renaming of such an identifier was incomplete
at this source code place.

In which ways would you like to improve the shown software situation?

* Would you like to fix a single identifier anyhow directly?

* Do you expect that I should resend a corrected patch series as a whole package?

* Would it be sufficient to send another update only for the software module
  "AGPGART-UniNorth" a bit later?

* Can I worry about this programming mistake a bit less just because
  the suggested renaming would eventually be rejected?

Regards,
Markus

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

#1482765 — [PATCH 1/7] AGPGART: Use kmalloc_array() in compat_agpioc_reserve_wrap()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 17:51:32 +0200

Multiplications for the size determination of memory allocations
indicated that array data structures should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/compat_ioctl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/char/agp/compat_ioctl.c b/drivers/char/agp/compat_ioctl.c
index a48e05b..a2b69db 100644
--- a/drivers/char/agp/compat_ioctl.c
+++ b/drivers/char/agp/compat_ioctl.c
@@ -98,11 +98,15 @@ static int compat_agpioc_reserve_wrap(struct agp_file_private *priv, void __user
 		if (ureserve.seg_count >= 16384)
 			return -EINVAL;
 
-		usegment = kmalloc(sizeof(*usegment) * ureserve.seg_count, GFP_KERNEL);
+		usegment = kmalloc_array(ureserve.seg_count,
+					 sizeof(*usegment),
+					 GFP_KERNEL);
 		if (!usegment)
 			return -ENOMEM;
 
-		ksegment = kmalloc(sizeof(*ksegment) * kreserve.seg_count, GFP_KERNEL);
+		ksegment = kmalloc_array(kreserve.seg_count,
+					 sizeof(*ksegment),
+					 GFP_KERNEL);
 		if (!ksegment) {
 			kfree(usegment);
 			return -ENOMEM;
-- 
2.10.0

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

#1482767 — [PATCH 4/7] AGPGART-SGI: Use kmalloc_array() in agp_sgi_init()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 21:30:58 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/sgi-agp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
index 3051c73..798ad20 100644
--- a/drivers/char/agp/sgi-agp.c
+++ b/drivers/char/agp/sgi-agp.c
@@ -280,9 +280,9 @@ static int agp_sgi_init(void)
 	else
 		return 0;
 
-	sgi_tioca_agp_bridges = kmalloc(tioca_gart_found *
-					sizeof(struct agp_bridge_data *),
-					GFP_KERNEL);
+	sgi_tioca_agp_bridges = kmalloc_array(tioca_gart_found,
+					      sizeof(*sgi_tioca_agp_bridges),
+					      GFP_KERNEL);
 	if (!sgi_tioca_agp_bridges)
 		return -ENOMEM;
 
-- 
2.10.0

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

#1482768 — [PATCH 5/7] AGPGART-UniNorth: Use kmalloc_array() in uninorth_create_gatt_table()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 21:50:44 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/uninorth-agp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/agp/uninorth-agp.c b/drivers/char/agp/uninorth-agp.c
index fdced54..8d144cd 100644
--- a/drivers/char/agp/uninorth-agp.c
+++ b/drivers/char/agp/uninorth-agp.c
@@ -402,7 +402,9 @@ static int uninorth_create_gatt_table(struct agp_bridge_data *bridge)
 	if (table == NULL)
 		return -ENOMEM;
 
-	uninorth_priv.pages_arr = kmalloc((1 << page_order) * sizeof(struct page*), GFP_KERNEL);
+	uninorth_priv.pages_arr = kmalloc_array(1 << page_order,
+						sizeof(*uninorth_priv.pages_arr),
+						GFP_KERNEL);
 	if (uninorth_priv.pages_arr == NULL)
 		goto enomem;
 
-- 
2.10.0

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

#1482769 — [PATCH 2/7] AGPGART: Use memdup_user() rather than duplicating its implementation

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 21:00:44 +0200

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

* Try this copy operation before allocating memory for the local
  variable "ksegment".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/compat_ioctl.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/char/agp/compat_ioctl.c b/drivers/char/agp/compat_ioctl.c
index a2b69db..68d4dc7 100644
--- a/drivers/char/agp/compat_ioctl.c
+++ b/drivers/char/agp/compat_ioctl.c
@@ -98,11 +98,10 @@ static int compat_agpioc_reserve_wrap(struct agp_file_private *priv, void __user
 		if (ureserve.seg_count >= 16384)
 			return -EINVAL;
 
-		usegment = kmalloc_array(ureserve.seg_count,
-					 sizeof(*usegment),
-					 GFP_KERNEL);
-		if (!usegment)
-			return -ENOMEM;
+		usegment = memdup_user((void __user *) ureserve.seg_list,
+				       sizeof(*usegment) * ureserve.seg_count);
+		if (IS_ERR(usegment))
+			return PTR_ERR(usegment);
 
 		ksegment = kmalloc_array(kreserve.seg_count,
 					 sizeof(*ksegment),
@@ -112,13 +111,6 @@ static int compat_agpioc_reserve_wrap(struct agp_file_private *priv, void __user
 			return -ENOMEM;
 		}
 
-		if (copy_from_user(usegment, (void __user *) ureserve.seg_list,
-				   sizeof(*usegment) * ureserve.seg_count)) {
-			kfree(usegment);
-			kfree(ksegment);
-			return -EFAULT;
-		}
-
 		for (seg = 0; seg < ureserve.seg_count; seg++) {
 			ksegment[seg].pg_start = usegment[seg].pg_start;
 			ksegment[seg].pg_count = usegment[seg].pg_count;
-- 
2.10.0

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

#1482770 — [PATCH 3/7] AGPGART: Rename jump labels in compat_agp_ioctl()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 21:12:02 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/compat_ioctl.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/char/agp/compat_ioctl.c b/drivers/char/agp/compat_ioctl.c
index 68d4dc7..4220956 100644
--- a/drivers/char/agp/compat_ioctl.c
+++ b/drivers/char/agp/compat_ioctl.c
@@ -209,24 +209,24 @@ long compat_agp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	if ((agp_fe.current_controller == NULL) &&
 	    (cmd != AGPIOC_ACQUIRE32)) {
 		ret_val = -EINVAL;
-		goto ioctl_out;
+		goto unlock;
 	}
 	if ((agp_fe.backend_acquired != true) &&
 	    (cmd != AGPIOC_ACQUIRE32)) {
 		ret_val = -EBUSY;
-		goto ioctl_out;
+		goto unlock;
 	}
 	if (cmd != AGPIOC_ACQUIRE32) {
 		if (!(test_bit(AGP_FF_IS_CONTROLLER, &curr_priv->access_flags))) {
 			ret_val = -EPERM;
-			goto ioctl_out;
+			goto unlock;
 		}
 		/* Use the original pid of the controller,
 		 * in case it's threaded */
 
 		if (agp_fe.current_controller->pid != curr_priv->my_pid) {
 			ret_val = -EBUSY;
-			goto ioctl_out;
+			goto unlock;
 		}
 	}
 
@@ -274,8 +274,7 @@ long compat_agp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case AGPIOC_CHIPSET_FLUSH32:
 		break;
 	}
-
-ioctl_out:
+ unlock:
 	DBG("ioctl returns %d\n", ret_val);
 	mutex_unlock(&(agp_fe.agp_mutex));
 	return ret_val;
-- 
2.10.0

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

#1482774 — [PATCH 7/7] AGPGART-UniNorth: Delete an unnecessary check in uninorth_create_gatt_table()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 22:15:56 +0200

The check for the local variable "table" is unnecessary at the end
of this function because the corresponding source code place should only
be reached with a non-zero pointer for it after a failed call of the
function "kmalloc_array".
Thus remove it.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/agp/uninorth-agp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/agp/uninorth-agp.c b/drivers/char/agp/uninorth-agp.c
index 8b1eb72..657cf17 100644
--- a/drivers/char/agp/uninorth-agp.c
+++ b/drivers/char/agp/uninorth-agp.c
@@ -438,8 +438,7 @@ static int uninorth_create_gatt_table(struct agp_bridge_data *bridge)
 	return 0;
  free_page_array:
 	kfree(uninorth_priv.pages_arr);
-	if (table)
-		free_pages((unsigned long)table, page_order);
+	free_pages((unsigned long)table, page_order);
 	return -ENOMEM;
 }
 
-- 
2.10.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web