[PATCHv2 0/7] Miscellaneous fixes to host1x

[PATCHv2 0/7] Miscellaneous fixes to host1x

[Arto Merilainen]

This patch series fixes two issues in the host1x driver: First, the
command buffer validation routine had vulnerabilities that were not
detected in earlier testing. Second, the return codes of some
functions were misleading or completely missing. This caused the
driver to give wrong return codes also to the userspace.

The series is based on top of 3.10rc3. I have tested the patch series
on cardhu by running host1x and gr2d test cases (available at [0]).
I would appreciate any help in testing/reviewing these patches.

Changes from v1:
 * Rebased on top of 3.10rc3
 * Split firewall fixes to smaller patches
 * Reworked no-reloc case in firewall code. Fix in v1 was not
   sufficient in all cases
 * Dropped patch "Fix syncpoint wait return value" as it is not
   critical and discussion on it has not yet settled.
 * Fixed style and whitespace issues

[0] https://gitorious.org/linux-host1x/libdrm-host1x

Arto Merilainen (5):
  gpu: host1x: Check reloc table before usage
  gpu: host1x: Copy gathers before verification
  gpu: host1x: Fix memory access in syncpt request
  gpu: host1x: Fix client_managed type
  gpu: host1x: Rework CPU syncpoint increment

Terje Bergstrom (2):
  gpu: host1x: Check INCR opcode correctly
  gpu: host1x: Don't reset firewall between gathers

 drivers/gpu/host1x/dev.h          |   8 +--
 drivers/gpu/host1x/drm/drm.c      |   3 +-
 drivers/gpu/host1x/drm/gr2d.c     |   2 +-
 drivers/gpu/host1x/hw/cdma_hw.c   |   2 +-
 drivers/gpu/host1x/hw/syncpt_hw.c |  12 ++--
 drivers/gpu/host1x/job.c          | 135 +++++++++++++++++---------------------
 drivers/gpu/host1x/syncpt.c       |  26 +++-----
 drivers/gpu/host1x/syncpt.h       |  13 ++--
 8 files changed, 85 insertions(+), 116 deletions(-)

-- 
1.8.1.5

[PATCHv2 1/7] gpu: host1x: Check INCR opcode correctly

[Arto Merilainen]

From: Terje Bergstrom <tbergstrom@nvidia.com>

The firewall code used a wrong loop condition (pointer to a
structure) while checking INCR opcode. This patch fixes the code to
use correct loop condition (number of words remaining).

Signed-off-by: Terje Bergstrom <tbergstrom@nvidia.com>
Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
---
 drivers/gpu/host1x/job.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index f665d67..2974ac8 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -330,7 +330,7 @@ static int check_incr(struct host1x_firewall *fw)
 	u32 count = fw->count;
 	u32 reg = fw->reg;
 
-	while (fw) {
+	while (count) {
 		if (fw->words == 0)
 			return -EINVAL;
 
-- 
1.8.1.5

[PATCHv2 2/7] gpu: host1x: Check reloc table before usage

[Arto Merilainen]

The firewall assumed that the user space always delivers a relocation
table when it is accessing address registers. If userspace did not
deliver a relocation table and tried to access the address registers,
the code performed bad memory accesses.

This patch modifies the firewall to check correctly that the firewall
table is available before accessing it. In addition, check_reloc() is
converted to use boolean return value (true when the reloc is valid,
false when invalid).

Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
---
 drivers/gpu/host1x/job.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index 2974ac8..83804fd 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -268,15 +268,15 @@ static unsigned int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
 	return 0;
 }
 
-static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
+static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
 		       unsigned int offset)
 {
 	offset *= sizeof(u32);
 
 	if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
-		return -EINVAL;
+		return false;
 
-	return 0;
+	return true;
 }
 
 struct host1x_firewall {
@@ -307,10 +307,10 @@ static int check_mask(struct host1x_firewall *fw)
 
 		if (mask & 1) {
 			if (fw->job->is_addr_reg(fw->dev, fw->class, reg)) {
-				bool bad_reloc = check_reloc(fw->reloc,
-							     fw->cmdbuf_id,
-							     fw->offset);
-				if (!fw->num_relocs || bad_reloc)
+				if (!fw->num_relocs)
+					return -EINVAL;
+				if (!check_reloc(fw->reloc, fw->cmdbuf_id,
+						 fw->offset))
 					return -EINVAL;
 				fw->reloc++;
 				fw->num_relocs--;
@@ -335,9 +335,9 @@ static int check_incr(struct host1x_firewall *fw)
 			return -EINVAL;
 
 		if (fw->job->is_addr_reg(fw->dev, fw->class, reg)) {
-			bool bad_reloc = check_reloc(fw->reloc, fw->cmdbuf_id,
-						     fw->offset);
-			if (!fw->num_relocs || bad_reloc)
+			if (!fw->num_relocs)
+				return -EINVAL;
+			if (!check_reloc(fw->reloc, fw->cmdbuf_id, fw->offset))
 				return -EINVAL;
 			fw->reloc++;
 			fw->num_relocs--;
@@ -361,9 +361,9 @@ static int check_nonincr(struct host1x_firewall *fw)
 			return -EINVAL;
 
 		if (is_addr_reg) {
-			bool bad_reloc = check_reloc(fw->reloc, fw->cmdbuf_id,
-						     fw->offset);
-			if (!fw->num_relocs || bad_reloc)
+			if (!fw->num_relocs)
+				return -EINVAL;
+			if (!check_reloc(fw->reloc, fw->cmdbuf_id, fw->offset))
 				return -EINVAL;
 			fw->reloc++;
 			fw->num_relocs--;
-- 
1.8.1.5

[PATCHv2 3/7] gpu: host1x: Don't reset firewall between gathers

[Arto Merilainen]

From: Terje Bergstrom <tbergstrom@nvidia.com>

The firewall was reinitialised for each gather. Because the filter
was reinitialised, it did not track the class over gather boundaries.
This allowed the user application to set host1x class to one class
in one gather and use that class in another gather without firewall
having knowledge about that.

Signed-off-by: Terje Bergstrom <tbergstrom@nvidia.com>
Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
---
 drivers/gpu/host1x/job.c | 73 ++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index 83804fd..028d2d4 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -376,69 +376,60 @@ static int check_nonincr(struct host1x_firewall *fw)
 	return 0;
 }
 
-static int validate(struct host1x_job *job, struct device *dev,
-		    struct host1x_job_gather *g)
+static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
 {
 	u32 *cmdbuf_base;
 	int err = 0;
-	struct host1x_firewall fw;
-
-	fw.job = job;
-	fw.dev = dev;
-	fw.reloc = job->relocarray;
-	fw.num_relocs = job->num_relocs;
-	fw.cmdbuf_id = g->bo;
-
-	fw.offset = 0;
-	fw.class = 0;
 
-	if (!job->is_addr_reg)
+	if (!fw->job->is_addr_reg)
 		return 0;
 
 	cmdbuf_base = host1x_bo_mmap(g->bo);
 	if (!cmdbuf_base)
 		return -ENOMEM;
+	fw->words = g->words;
+	fw->cmdbuf_id = g->bo;
+	fw->offset = 0;
 
-	fw.words = g->words;
-	while (fw.words && !err) {
-		u32 word = cmdbuf_base[fw.offset];
+	while (fw->words && !err) {
+		u32 word = cmdbuf_base[fw->offset];
 		u32 opcode = (word & 0xf0000000) >> 28;
 
-		fw.mask = 0;
-		fw.reg = 0;
-		fw.count = 0;
-		fw.words--;
-		fw.offset++;
+		fw->mask = 0;
+		fw->reg = 0;
+		fw->count = 0;
+		fw->words--;
+		fw->offset++;
 
 		switch (opcode) {
 		case 0:
-			fw.class = word >> 6 & 0x3ff;
-			fw.mask = word & 0x3f;
-			fw.reg = word >> 16 & 0xfff;
-			err = check_mask(&fw);
+			fw->class = word >> 6 & 0x3ff;
+			fw->mask = word & 0x3f;
+			fw->reg = word >> 16 & 0xfff;
+			err = check_mask(fw);
 			if (err)
 				goto out;
 			break;
 		case 1:
-			fw.reg = word >> 16 & 0xfff;
-			fw.count = word & 0xffff;
-			err = check_incr(&fw);
+			fw->reg = word >> 16 & 0xfff;
+			fw->count = word & 0xffff;
+			err = check_incr(fw);
 			if (err)
 				goto out;
 			break;
 
 		case 2:
-			fw.reg = word >> 16 & 0xfff;
-			fw.count = word & 0xffff;
-			err = check_nonincr(&fw);
+			fw->reg = word >> 16 & 0xfff;
+			fw->count = word & 0xffff;
+			err = check_nonincr(fw);
 			if (err)
 				goto out;
 			break;
 
 		case 3:
-			fw.mask = word & 0xffff;
-			fw.reg = word >> 16 & 0xfff;
-			err = check_mask(&fw);
+			fw->mask = word & 0xffff;
+			fw->reg = word >> 16 & 0xfff;
+			err = check_mask(fw);
 			if (err)
 				goto out;
 			break;
@@ -453,12 +444,10 @@ static int validate(struct host1x_job *job, struct device *dev,
 	}
 
 	/* No relocs should remain at this point */
-	if (fw.num_relocs)
+	if (fw->num_relocs)
 		err = -EINVAL;
 
 out:
-	host1x_bo_munmap(g->bo, cmdbuf_base);
-
 	return err;
 }
 
@@ -508,8 +497,15 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 	int err;
 	unsigned int i, j;
 	struct host1x *host = dev_get_drvdata(dev->parent);
+	struct host1x_firewall fw;
 	DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
 
+	fw.job = job;
+	fw.dev = dev;
+	fw.reloc = job->relocarray;
+	fw.num_relocs = job->num_relocs;
+	fw.class = 0;
+
 	bitmap_zero(waitchk_mask, host1x_syncpt_nb_pts(host));
 	for (i = 0; i < job->num_waitchk; i++) {
 		u32 syncpt_id = job->waitchk[i].syncpt_id;
@@ -543,7 +539,7 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 		err = 0;
 
 		if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
-			err = validate(job, dev, g);
+			err = validate(&fw, g);
 
 		if (err)
 			dev_err(dev, "Job invalid (err=%d)\n", err);
@@ -553,7 +549,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 
 		if (!err)
 			err = do_waitchks(job, host, g->bo);
-
 		if (err)
 			break;
 	}
-- 
1.8.1.5

Re: [PATCHv2 3/7] gpu: host1x: Don't reset firewall between gathers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

* Arto Merilainen wrote:
[...]
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
[...]
> @@ -553,7 +549,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
>  
>  		if (!err)
>  			err = do_waitchks(job, host, g->bo);
> -
>  		if (err)
>  			break;
>  	}

This is another unnecessary whitespace change, but since it was the only
issue I saw with the series I fixed it up myself while applying.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 3/7] gpu: host1x: Don't reset firewall between gathers

[Arto Merilainen]

On 05/29/2013 02:21 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> * Arto Merilainen wrote:
> [...]
>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> [...]
>> @@ -553,7 +549,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
>>
>>   		if (!err)
>>   			err = do_waitchks(job, host, g->bo);
>> -
>>   		if (err)
>>   			break;
>>   	}
>
> This is another unnecessary whitespace change, but since it was the only
> issue I saw with the series I fixed it up myself while applying.
>

Thanks! I thought I got them all before posting but apparently not...

- Arto

[PATCHv2 4/7] gpu: host1x: Copy gathers before verification

[Arto Merilainen]

The firewall verified gather buffers before copying them. This
allowed a malicious application to rewrite the buffer content by
timing the rewrite carefully.

This patch makes the buffer validation occur after copying the
buffers.

Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
Signed-off-by: Terje Bergstrom <tbergstrom@nvidia.com>
---
 drivers/gpu/host1x/job.c | 50 +++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index 028d2d4..cc80766 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
 	void *cmdbuf_page_addr = NULL;
 
 	/* pin & patch the relocs for one gather */
-	while (i < job->num_relocs) {
+	for (i = 0; i < job->num_relocs; i++) {
 		struct host1x_reloc *reloc = &job->relocarray[i];
 		u32 reloc_addr = (job->reloc_addr_phys[i] +
 			reloc->target_offset) >> reloc->shift;
 		u32 *target;
 
 		/* skip all other gathers */
-		if (!(reloc->cmdbuf && cmdbuf == reloc->cmdbuf)) {
-			i++;
+		if (cmdbuf != reloc->cmdbuf)
 			continue;
-		}
 
 		if (last_page != reloc->cmdbuf_offset >> PAGE_SHIFT) {
 			if (cmdbuf_page_addr)
@@ -257,9 +255,6 @@ static unsigned int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
 
 		target = cmdbuf_page_addr + (reloc->cmdbuf_offset & ~PAGE_MASK);
 		*target = reloc_addr;
-
-		/* mark this gather as handled */
-		reloc->cmdbuf = 0;
 	}
 
 	if (cmdbuf_page_addr)
@@ -378,15 +373,13 @@ static int check_nonincr(struct host1x_firewall *fw)
 
 static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
 {
-	u32 *cmdbuf_base;
+	u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
+		(g->offset / sizeof(u32));
 	int err = 0;
 
 	if (!fw->job->is_addr_reg)
 		return 0;
 
-	cmdbuf_base = host1x_bo_mmap(g->bo);
-	if (!cmdbuf_base)
-		return -ENOMEM;
 	fw->words = g->words;
 	fw->cmdbuf_id = g->bo;
 	fw->offset = 0;
@@ -453,10 +446,17 @@ out:
 
 static inline int copy_gathers(struct host1x_job *job, struct device *dev)
 {
+	struct host1x_firewall fw;
 	size_t size = 0;
 	size_t offset = 0;
 	int i;
 
+	fw.job = job;
+	fw.dev = dev;
+	fw.reloc = job->relocarray;
+	fw.num_relocs = job->num_relocs;
+	fw.class = 0;
+
 	for (i = 0; i < job->num_gathers; i++) {
 		struct host1x_job_gather *g = &job->gathers[i];
 		size += g->words * sizeof(u32);
@@ -477,14 +477,19 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
 		struct host1x_job_gather *g = &job->gathers[i];
 		void *gather;
 
+		/* Copy the gather */
 		gather = host1x_bo_mmap(g->bo);
 		memcpy(job->gather_copy_mapped + offset, gather + g->offset,
 		       g->words * sizeof(u32));
 		host1x_bo_munmap(g->bo, gather);
 
+		/* Store the location in the buffer */
 		g->base = job->gather_copy;
 		g->offset = offset;
-		g->bo = NULL;
+
+		/* Validate the job */
+		if (validate(&fw, g))
+			return -EINVAL;
 
 		offset += g->words * sizeof(u32);
 	}
@@ -497,15 +502,8 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 	int err;
 	unsigned int i, j;
 	struct host1x *host = dev_get_drvdata(dev->parent);
-	struct host1x_firewall fw;
 	DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
 
-	fw.job = job;
-	fw.dev = dev;
-	fw.reloc = job->relocarray;
-	fw.num_relocs = job->num_relocs;
-	fw.class = 0;
-
 	bitmap_zero(waitchk_mask, host1x_syncpt_nb_pts(host));
 	for (i = 0; i < job->num_waitchk; i++) {
 		u32 syncpt_id = job->waitchk[i].syncpt_id;
@@ -536,19 +534,11 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 			if (job->gathers[j].bo == g->bo)
 				job->gathers[j].handled = true;
 
-		err = 0;
-
-		if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
-			err = validate(&fw, g);
-
+		err = do_relocs(job, g->bo);
 		if (err)
-			dev_err(dev, "Job invalid (err=%d)\n", err);
-
-		if (!err)
-			err = do_relocs(job, g->bo);
+			break;
 
-		if (!err)
-			err = do_waitchks(job, host, g->bo);
+		err = do_waitchks(job, host, g->bo);
 		if (err)
 			break;
 	}
-- 
1.8.1.5

[PATCHv2 5/7] gpu: host1x: Fix memory access in syncpt request

[Arto Merilainen]

This patch fixes a bad memory access in syncpoint request code. If
no syncpoints were available, the code accessed unreserved memory
area causing unexpected behaviour.

Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
---
 drivers/gpu/host1x/syncpt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 4b49345..2b03f1b 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -40,7 +40,8 @@ static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host,
 
 	for (i = 0; i < host->info->nb_pts && sp->name; i++, sp++)
 		;
-	if (sp->dev)
+
+	if (i >= host->info->nb_pts)
 		return NULL;
 
 	name = kasprintf(GFP_KERNEL, "%02d-%s", sp->id,
-- 
1.8.1.5

[PATCHv2 6/7] gpu: host1x: Fix client_managed type

[Arto Merilainen]

client_managed field in syncpoint structure was defined as an
integer. The field holds, however, only a boolean value. This patch
modifies the type to boolean.

Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
---
 drivers/gpu/host1x/drm/gr2d.c | 2 +-
 drivers/gpu/host1x/syncpt.c   | 8 ++++----
 drivers/gpu/host1x/syncpt.h   | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/host1x/drm/gr2d.c b/drivers/gpu/host1x/drm/gr2d.c
index 6a45ae0..6c3a27b 100644
--- a/drivers/gpu/host1x/drm/gr2d.c
+++ b/drivers/gpu/host1x/drm/gr2d.c
@@ -281,7 +281,7 @@ static int gr2d_probe(struct platform_device *pdev)
 	if (!gr2d->channel)
 		return -ENOMEM;
 
-	*syncpts = host1x_syncpt_request(dev, 0);
+	*syncpts = host1x_syncpt_request(dev, false);
 	if (!(*syncpts)) {
 		host1x_channel_free(gr2d->channel);
 		return -ENOMEM;
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 2b03f1b..27201b5 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -32,7 +32,7 @@
 
 static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host,
 						  struct device *dev,
-						  int client_managed)
+						  bool client_managed)
 {
 	int i;
 	struct host1x_syncpt *sp = host->syncpt;
@@ -332,7 +332,7 @@ int host1x_syncpt_init(struct host1x *host)
 	host1x_syncpt_restore(host);
 
 	/* Allocate sync point to use for clearing waits for expired fences */
-	host->nop_sp = _host1x_syncpt_alloc(host, NULL, 0);
+	host->nop_sp = _host1x_syncpt_alloc(host, NULL, false);
 	if (!host->nop_sp)
 		return -ENOMEM;
 
@@ -340,7 +340,7 @@ int host1x_syncpt_init(struct host1x *host)
 }
 
 struct host1x_syncpt *host1x_syncpt_request(struct device *dev,
-					    int client_managed)
+					    bool client_managed)
 {
 	struct host1x *host = dev_get_drvdata(dev->parent);
 	return _host1x_syncpt_alloc(host, dev, client_managed);
@@ -354,7 +354,7 @@ void host1x_syncpt_free(struct host1x_syncpt *sp)
 	kfree(sp->name);
 	sp->dev = NULL;
 	sp->name = NULL;
-	sp->client_managed = 0;
+	sp->client_managed = false;
 }
 
 void host1x_syncpt_deinit(struct host1x *host)
diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h
index c998061..d00e758 100644
--- a/drivers/gpu/host1x/syncpt.h
+++ b/drivers/gpu/host1x/syncpt.h
@@ -36,7 +36,7 @@ struct host1x_syncpt {
 	atomic_t max_val;
 	u32 base_val;
 	const char *name;
-	int client_managed;
+	bool client_managed;
 	struct host1x *host;
 	struct device *dev;
 
@@ -94,7 +94,7 @@ static inline bool host1x_syncpt_check_max(struct host1x_syncpt *sp, u32 real)
 }
 
 /* Return true if sync point is client managed. */
-static inline int host1x_syncpt_client_managed(struct host1x_syncpt *sp)
+static inline bool host1x_syncpt_client_managed(struct host1x_syncpt *sp)
 {
 	return sp->client_managed;
 }
@@ -157,7 +157,7 @@ u32 host1x_syncpt_id(struct host1x_syncpt *sp);
 
 /* Allocate a sync point for a device. */
 struct host1x_syncpt *host1x_syncpt_request(struct device *dev,
-		int client_managed);
+					    bool client_managed);
 
 /* Free a sync point. */
 void host1x_syncpt_free(struct host1x_syncpt *sp);
-- 
1.8.1.5

[PATCHv2 7/7] gpu: host1x: Rework CPU syncpoint increment

[Arto Merilainen]

This patch merges host1x_syncpt_cpu_incr to host1x_syncpt_incr() as
they are in practise doing the same thing. host1x_syncpt_incr() is
also modified to return error codes. User space interface is modified
accordingly to pass return values.

Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
---
 drivers/gpu/host1x/dev.h          |  8 ++++----
 drivers/gpu/host1x/drm/drm.c      |  3 +--
 drivers/gpu/host1x/hw/cdma_hw.c   |  2 +-
 drivers/gpu/host1x/hw/syncpt_hw.c | 12 +++++-------
 drivers/gpu/host1x/syncpt.c       | 15 ++-------------
 drivers/gpu/host1x/syncpt.h       |  7 ++-----
 6 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
index a1607d6..790ddf1 100644
--- a/drivers/gpu/host1x/dev.h
+++ b/drivers/gpu/host1x/dev.h
@@ -73,7 +73,7 @@ struct host1x_syncpt_ops {
 	void (*restore_wait_base)(struct host1x_syncpt *syncpt);
 	void (*load_wait_base)(struct host1x_syncpt *syncpt);
 	u32 (*load)(struct host1x_syncpt *syncpt);
-	void (*cpu_incr)(struct host1x_syncpt *syncpt);
+	int (*cpu_incr)(struct host1x_syncpt *syncpt);
 	int (*patch_wait)(struct host1x_syncpt *syncpt, void *patch_addr);
 };
 
@@ -157,10 +157,10 @@ static inline u32 host1x_hw_syncpt_load(struct host1x *host,
 	return host->syncpt_op->load(sp);
 }
 
-static inline void host1x_hw_syncpt_cpu_incr(struct host1x *host,
-					     struct host1x_syncpt *sp)
+static inline int host1x_hw_syncpt_cpu_incr(struct host1x *host,
+					    struct host1x_syncpt *sp)
 {
-	host->syncpt_op->cpu_incr(sp);
+	return host->syncpt_op->cpu_incr(sp);
 }
 
 static inline int host1x_hw_syncpt_patch_wait(struct host1x *host,
diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c
index 2b561c9..1dfd454 100644
--- a/drivers/gpu/host1x/drm/drm.c
+++ b/drivers/gpu/host1x/drm/drm.c
@@ -378,8 +378,7 @@ static int tegra_syncpt_incr(struct drm_device *drm, void *data,
 	if (!sp)
 		return -EINVAL;
 
-	host1x_syncpt_incr(sp);
-	return 0;
+	return host1x_syncpt_incr(sp);
 }
 
 static int tegra_syncpt_wait(struct drm_device *drm, void *data,
diff --git a/drivers/gpu/host1x/hw/cdma_hw.c b/drivers/gpu/host1x/hw/cdma_hw.c
index 590b69d..2ee4ad5 100644
--- a/drivers/gpu/host1x/hw/cdma_hw.c
+++ b/drivers/gpu/host1x/hw/cdma_hw.c
@@ -44,7 +44,7 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr,
 	u32 i;
 
 	for (i = 0; i < syncpt_incrs; i++)
-		host1x_syncpt_cpu_incr(cdma->timeout.syncpt);
+		host1x_syncpt_incr(cdma->timeout.syncpt);
 
 	/* after CPU incr, ensure shadow is up to date */
 	host1x_syncpt_load(cdma->timeout.syncpt);
diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c
index 6117499..0cf6095 100644
--- a/drivers/gpu/host1x/hw/syncpt_hw.c
+++ b/drivers/gpu/host1x/hw/syncpt_hw.c
@@ -77,21 +77,19 @@ static u32 syncpt_load(struct host1x_syncpt *sp)
  * Write a cpu syncpoint increment to the hardware, without touching
  * the cache.
  */
-static void syncpt_cpu_incr(struct host1x_syncpt *sp)
+static int syncpt_cpu_incr(struct host1x_syncpt *sp)
 {
 	struct host1x *host = sp->host;
 	u32 reg_offset = sp->id / 32;
 
 	if (!host1x_syncpt_client_managed(sp) &&
-	    host1x_syncpt_idle(sp)) {
-		dev_err(host->dev, "Trying to increment syncpoint id %d beyond max\n",
-			sp->id);
-		host1x_debug_dump(sp->host);
-		return;
-	}
+	    host1x_syncpt_idle(sp))
+		return -EINVAL;
 	host1x_sync_writel(host, BIT_MASK(sp->id),
 			   HOST1X_SYNC_SYNCPT_CPU_INCR(reg_offset));
 	wmb();
+
+	return 0;
 }
 
 /* remove a wait pointed to by patch_addr */
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 27201b5..409745b 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -129,22 +129,11 @@ u32 host1x_syncpt_load_wait_base(struct host1x_syncpt *sp)
 }
 
 /*
- * Write a cpu syncpoint increment to the hardware, without touching
- * the cache. Caller is responsible for host being powered.
- */
-void host1x_syncpt_cpu_incr(struct host1x_syncpt *sp)
-{
-	host1x_hw_syncpt_cpu_incr(sp->host, sp);
-}
-
-/*
  * Increment syncpoint value from cpu, updating cache
  */
-void host1x_syncpt_incr(struct host1x_syncpt *sp)
+int host1x_syncpt_incr(struct host1x_syncpt *sp)
 {
-	if (host1x_syncpt_client_managed(sp))
-		host1x_syncpt_incr_max(sp, 1);
-	host1x_syncpt_cpu_incr(sp);
+	return host1x_hw_syncpt_cpu_incr(sp->host, sp);
 }
 
 /*
diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h
index d00e758..267c0b9 100644
--- a/drivers/gpu/host1x/syncpt.h
+++ b/drivers/gpu/host1x/syncpt.h
@@ -115,9 +115,6 @@ static inline bool host1x_syncpt_idle(struct host1x_syncpt *sp)
 /* Return pointer to struct denoting sync point id. */
 struct host1x_syncpt *host1x_syncpt_get(struct host1x *host, u32 id);
 
-/* Request incrementing a sync point. */
-void host1x_syncpt_cpu_incr(struct host1x_syncpt *sp);
-
 /* Load current value from hardware to the shadow register. */
 u32 host1x_syncpt_load(struct host1x_syncpt *sp);
 
@@ -133,8 +130,8 @@ void host1x_syncpt_restore(struct host1x *host);
 /* Read current wait base value into shadow register and return it. */
 u32 host1x_syncpt_load_wait_base(struct host1x_syncpt *sp);
 
-/* Increment sync point and its max. */
-void host1x_syncpt_incr(struct host1x_syncpt *sp);
+/* Request incrementing a sync point. */
+int host1x_syncpt_incr(struct host1x_syncpt *sp);
 
 /* Indicate future operations by incrementing the sync point max. */
 u32 host1x_syncpt_incr_max(struct host1x_syncpt *sp, u32 incrs);
-- 
1.8.1.5

Re: [PATCHv2 0/7] Miscellaneous fixes to host1x

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

On Wed, May 29, 2013 at 01:26:01PM +0300, Arto Merilainen wrote:
> This patch series fixes two issues in the host1x driver: First, the
> command buffer validation routine had vulnerabilities that were not
> detected in earlier testing. Second, the return codes of some
> functions were misleading or completely missing. This caused the
> driver to give wrong return codes also to the userspace.
> 
> The series is based on top of 3.10rc3. I have tested the patch series
> on cardhu by running host1x and gr2d test cases (available at [0]).
> I would appreciate any help in testing/reviewing these patches.
> 
> Changes from v1:
>  * Rebased on top of 3.10rc3
>  * Split firewall fixes to smaller patches
>  * Reworked no-reloc case in firewall code. Fix in v1 was not
>    sufficient in all cases
>  * Dropped patch "Fix syncpoint wait return value" as it is not
>    critical and discussion on it has not yet settled.
>  * Fixed style and whitespace issues
> 
> [0] https://gitorious.org/linux-host1x/libdrm-host1x
> 
> Arto Merilainen (5):
>   gpu: host1x: Check reloc table before usage
>   gpu: host1x: Copy gathers before verification
>   gpu: host1x: Fix memory access in syncpt request
>   gpu: host1x: Fix client_managed type
>   gpu: host1x: Rework CPU syncpoint increment
> 
> Terje Bergstrom (2):
>   gpu: host1x: Check INCR opcode correctly
>   gpu: host1x: Don't reset firewall between gathers
> 
>  drivers/gpu/host1x/dev.h          |   8 +--
>  drivers/gpu/host1x/drm/drm.c      |   3 +-
>  drivers/gpu/host1x/drm/gr2d.c     |   2 +-
>  drivers/gpu/host1x/hw/cdma_hw.c   |   2 +-
>  drivers/gpu/host1x/hw/syncpt_hw.c |  12 ++--
>  drivers/gpu/host1x/job.c          | 135 +++++++++++++++++---------------------
>  drivers/gpu/host1x/syncpt.c       |  26 +++-----
>  drivers/gpu/host1x/syncpt.h       |  13 ++--
>  8 files changed, 85 insertions(+), 116 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 0/7] Miscellaneous fixes to host1x

[Terje Bergström]

On 29.05.2013 13:26, Arto Merilainen wrote:
> This patch series fixes two issues in the host1x driver: First, the
> command buffer validation routine had vulnerabilities that were not
> detected in earlier testing. Second, the return codes of some
> functions were misleading or completely missing. This caused the
> driver to give wrong return codes also to the userspace.
> 
> The series is based on top of 3.10rc3. I have tested the patch series
> on cardhu by running host1x and gr2d test cases (available at [0]).
> I would appreciate any help in testing/reviewing these patches.
> 
> Changes from v1:
>  * Rebased on top of 3.10rc3
>  * Split firewall fixes to smaller patches
>  * Reworked no-reloc case in firewall code. Fix in v1 was not
>    sufficient in all cases
>  * Dropped patch "Fix syncpoint wait return value" as it is not
>    critical and discussion on it has not yet settled.
>  * Fixed style and whitespace issues
> 
> [0] https://gitorious.org/linux-host1x/libdrm-host1x
> 
> Arto Merilainen (5):
>   gpu: host1x: Check reloc table before usage
>   gpu: host1x: Copy gathers before verification
>   gpu: host1x: Fix memory access in syncpt request
>   gpu: host1x: Fix client_managed type
>   gpu: host1x: Rework CPU syncpoint increment
> 
> Terje Bergstrom (2):
>   gpu: host1x: Check INCR opcode correctly
>   gpu: host1x: Don't reset firewall between gathers
> 
>  drivers/gpu/host1x/dev.h          |   8 +--
>  drivers/gpu/host1x/drm/drm.c      |   3 +-
>  drivers/gpu/host1x/drm/gr2d.c     |   2 +-
>  drivers/gpu/host1x/hw/cdma_hw.c   |   2 +-
>  drivers/gpu/host1x/hw/syncpt_hw.c |  12 ++--
>  drivers/gpu/host1x/job.c          | 135 +++++++++++++++++---------------------
>  drivers/gpu/host1x/syncpt.c       |  26 +++-----
>  drivers/gpu/host1x/syncpt.h       |  13 ++--
>  8 files changed, 85 insertions(+), 116 deletions(-)
> 

Arto's patches above,

Acked-By: Terje Bergstrom <tbergstrom@nvidia.com>

Terje