[PATCHv2 0/4] staging: tidspbridge - misc fixes

[PATCHv2 0/4] staging: tidspbridge - misc fixes

[Fernando Guzman Lugo]

* V2
 - rebase to the latest
 - remove patches no needed at this moment

This set of patches fix some issues found in lastest tree.

Fernando Guzman Lugo (4):
  staging: tidspbridge - use GTP7 for DSP stack dump
  staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow
  staging: tidspbridge - remove disabling twl when printing DSP stack
  staging: tidspbridge - change mmufault tasklet to a workqueue

 drivers/staging/tidspbridge/core/_deh.h            |    2 +-
 drivers/staging/tidspbridge/core/dsp-clock.c       |   17 ++++---
 drivers/staging/tidspbridge/core/ue_deh.c          |   55 +++++++++++--------
 .../staging/tidspbridge/include/dspbridge/clk.h    |    4 +-
 4 files changed, 46 insertions(+), 32 deletions(-)

-- 
1.7.3.2

[PATCHv2 1/4] staging: tidspbridge - use GTP7 for DSP stack dump

[Fernando Guzman Lugo]

DSP stack dump is changed to GTP7 due to GPT8 is used
by DSP side apps

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 drivers/staging/tidspbridge/core/ue_deh.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/ue_deh.c b/drivers/staging/tidspbridge/core/ue_deh.c
index 3430418..1e30ce8 100644
--- a/drivers/staging/tidspbridge/core/ue_deh.c
+++ b/drivers/staging/tidspbridge/core/ue_deh.c
@@ -193,15 +193,15 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
 			HW_PAGE_SIZE4KB, 1,
 			&map_attrs, HW_SET, HW_SET);
 
-	dsp_clk_enable(DSP_CLK_GPT8);
+	dsp_clk_enable(DSP_CLK_GPT7);
 
-	dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe);
+	dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe);
 
 	/* Clear MMU interrupt */
 	hw_mmu_event_ack(resources->dw_dmmu_base,
 			HW_MMU_TRANSLATION_FAULT);
 	dump_dsp_stack(dev_context);
-	dsp_clk_disable(DSP_CLK_GPT8);
+	dsp_clk_disable(DSP_CLK_GPT7);
 
 	hw_mmu_disable(resources->dw_dmmu_base);
 	free_page((unsigned long)dummy_va_addr);
-- 
1.7.3.2

[PATCHv2 2/4] staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow

[Fernando Guzman Lugo]

timeout was not being initialized correctly and should
use time_is-before_jiffies, also make it a parameter.

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 drivers/staging/tidspbridge/core/dsp-clock.c       |   17 ++++++++++-------
 drivers/staging/tidspbridge/core/ue_deh.c          |    5 +++++
 .../staging/tidspbridge/include/dspbridge/clk.h    |    4 +++-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index 46d17c7..db02e12 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -198,17 +198,19 @@ static void mcbsp_clk_prepare(bool flag, u8 id)
  * dsp_gpt_wait_overflow - set gpt overflow and wait for fixed timeout
  * @clk_id:      GP Timer clock id.
  * @load:        Overflow value.
+ * @timeout:     Timeout.
  *
  * Sets an overflow interrupt for the desired GPT waiting for a timeout
- * of 5 msecs for the interrupt to occur.
+ * of @timeout msecs for the interrupt to occur.
  */
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+			unsigned long timeout)
+
 {
 	struct omap_dm_timer *gpt = timer[clk_id - 1];
-	unsigned long timeout;
 
 	if (!gpt)
-		return;
+		return -EINVAL;
 
 	/* Enable overflow interrupt */
 	omap_dm_timer_set_int_enable(gpt, OMAP_TIMER_INT_OVERFLOW);
@@ -222,14 +224,15 @@ void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
 	/* Wait 80us for timer to overflow */
 	udelay(80);
 
-	timeout = msecs_to_jiffies(5);
+	timeout = jiffies + msecs_to_jiffies(timeout);
 	/* Check interrupt status and wait for interrupt */
 	while (!(omap_dm_timer_read_status(gpt) & OMAP_TIMER_INT_OVERFLOW)) {
-		if (time_is_after_jiffies(timeout)) {
+		if (time_is_before_jiffies(timeout)) {
 			pr_err("%s: GPTimer interrupt failed\n", __func__);
-			break;
+			return -ETIME;
 		}
 	}
+	return 0;
 }
 
 /*
diff --git a/drivers/staging/tidspbridge/core/ue_deh.c b/drivers/staging/tidspbridge/core/ue_deh.c
index 1e30ce8..0537edf 100644
--- a/drivers/staging/tidspbridge/core/ue_deh.c
+++ b/drivers/staging/tidspbridge/core/ue_deh.c
@@ -195,6 +195,11 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
 
 	dsp_clk_enable(DSP_CLK_GPT7);
 
+	if (dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe, 10)) {
+		pr_err("%s: error sending interrupt to DSP\n", __func__);
+		return;
+	}
+
 	dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe);
 
 	/* Clear MMU interrupt */
diff --git a/drivers/staging/tidspbridge/include/dspbridge/clk.h b/drivers/staging/tidspbridge/include/dspbridge/clk.h
index b239503..6fe1ff2 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/clk.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/clk.h
@@ -62,7 +62,9 @@ extern void dsp_clk_exit(void);
  */
 extern void dsp_clk_init(void);
 
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load);
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+			unsigned long timeout);
+
 
 /*
  *  ======== dsp_clk_enable ========
-- 
1.7.3.2

[PATCHv2 3/4] staging: tidspbridge - remove disabling twl when printing DSP stack

[Fernando Guzman Lugo]

Now the SHM segments are not in lock tlbs, instead
they are in translation tables. So we cannot disable
twl in order to get the DSP stack dump. Also add a check
for __get_free_page allocation.

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 drivers/staging/tidspbridge/core/ue_deh.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/ue_deh.c b/drivers/staging/tidspbridge/core/ue_deh.c
index 0537edf..183900e 100644
--- a/drivers/staging/tidspbridge/core/ue_deh.c
+++ b/drivers/staging/tidspbridge/core/ue_deh.c
@@ -180,13 +180,8 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
 	resources = dev_context->resources;
 	dummy_va_addr = (void*)__get_free_page(GFP_ATOMIC);
 
-	/*
-	 * Before acking the MMU fault, let's make sure MMU can only
-	 * access entry #0. Then add a new entry so that the DSP OS
-	 * can continue in order to dump the stack.
-	 */
-	hw_mmu_twl_disable(resources->dw_dmmu_base);
-	hw_mmu_tlb_flush_all(resources->dw_dmmu_base);
+	if (!dummy_va_addr)
+		return;
 
 	hw_mmu_tlb_add(resources->dw_dmmu_base,
 			virt_to_phys(dummy_va_addr), fault_addr,
-- 
1.7.3.2

[PATCHv2 4/4] staging: tidspbridge - change mmufault tasklet to a workqueue

[Fernando Guzman Lugo]

we don't need to manage the mmufault inside a tasklet
it is safer using a workqueue.

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 drivers/staging/tidspbridge/core/_deh.h   |    2 +-
 drivers/staging/tidspbridge/core/ue_deh.c |   39 +++++++++++++++++-----------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/_deh.h b/drivers/staging/tidspbridge/core/_deh.h
index 16723cd..d4aa561 100644
--- a/drivers/staging/tidspbridge/core/_deh.h
+++ b/drivers/staging/tidspbridge/core/_deh.h
@@ -29,7 +29,7 @@ struct deh_mgr {
 	struct ntfy_object *ntfy_obj;	/* NTFY object */
 
 	/* MMU Fault DPC */
-	struct tasklet_struct dpc_tasklet;
+	struct work_struct work;
 };
 
 #endif /* _DEH_ */
diff --git a/drivers/staging/tidspbridge/core/ue_deh.c b/drivers/staging/tidspbridge/core/ue_deh.c
index 183900e..bef1018 100644
--- a/drivers/staging/tidspbridge/core/ue_deh.c
+++ b/drivers/staging/tidspbridge/core/ue_deh.c
@@ -32,10 +32,11 @@
 #include <dspbridge/wdt.h>
 
 static u32 fault_addr;
+static struct workqueue_struct *deh_wq;
 
-static void mmu_fault_dpc(unsigned long data)
+static void mmu_fault_dpc(struct work_struct *work)
 {
-	struct deh_mgr *deh = (void *)data;
+	struct deh_mgr *deh = container_of(work, struct deh_mgr, work);
 
 	if (!deh)
 		return;
@@ -69,7 +70,7 @@ static irqreturn_t mmu_fault_isr(int irq, void *data)
 		 * necessary to check if DSP MMU fault is intended for
 		 * Bridge.
 		 */
-		tasklet_schedule(&deh->dpc_tasklet);
+		queue_work(deh_wq, &deh->work);
 
 		/* Disable the MMU events, else once we clear it will
 		 * start to raise INTs again */
@@ -105,12 +106,19 @@ int bridge_deh_create(struct deh_mgr **ret_deh,
 	deh->ntfy_obj = kmalloc(sizeof(struct ntfy_object), GFP_KERNEL);
 	if (!deh->ntfy_obj) {
 		status = -ENOMEM;
-		goto err;
+		goto err1;
 	}
 	ntfy_init(deh->ntfy_obj);
 
+
 	/* Create a MMUfault DPC */
-	tasklet_init(&deh->dpc_tasklet, mmu_fault_dpc, (u32) deh);
+	deh_wq = create_workqueue("dspbridge-mmu_wq");
+	if (!deh_wq) {
+		status = -ENOMEM;
+		goto err2;
+	}
+
+	INIT_WORK(&deh->work, mmu_fault_dpc);
 
 	/* Fill in context structure */
 	deh->hbridge_context = hbridge_context;
@@ -118,14 +126,17 @@ int bridge_deh_create(struct deh_mgr **ret_deh,
 	/* Install ISR function for DSP MMU fault */
 	status = request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
 			"DspBridge\tiommu fault", deh);
-	if (status < 0)
-		goto err;
-
-	*ret_deh = deh;
-	return 0;
+	if (!status) {
+		*ret_deh = deh;
+		return 0;
+	}
 
+	destroy_workqueue(deh_wq);
+err2:
+	kfree(deh->ntfy_obj);
+err1:
+	kfree(deh);
 err:
-	bridge_deh_destroy(deh);
 	*ret_deh = NULL;
 	return status;
 }
@@ -144,7 +155,7 @@ int bridge_deh_destroy(struct deh_mgr *deh)
 	free_irq(INT_DSP_MMU_IRQ, deh);
 
 	/* Free DPC object */
-	tasklet_kill(&deh->dpc_tasklet);
+	destroy_workqueue(deh_wq);
 
 	/* Deallocate the DEH manager object */
 	kfree(deh);
@@ -178,7 +189,7 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
 	void *dummy_va_addr;
 
 	resources = dev_context->resources;
-	dummy_va_addr = (void*)__get_free_page(GFP_ATOMIC);
+	dummy_va_addr = (void *)__get_free_page(GFP_KERNEL);
 
 	if (!dummy_va_addr)
 		return;
@@ -195,8 +206,6 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
 		return;
 	}
 
-	dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe);
-
 	/* Clear MMU interrupt */
 	hw_mmu_event_ack(resources->dw_dmmu_base,
 			HW_MMU_TRANSLATION_FAULT);
-- 
1.7.3.2

Re: [PATCHv2 0/4] staging: tidspbridge - misc fixes

[Nishanth Menon]

Fernando Guzman Lugo wrote, on 12/09/2010 09:55 PM:
> * V2
>   - rebase to the latest
>   - remove patches no needed at this moment
>
> This set of patches fix some issues found in lastest tree.
>
> Fernando Guzman Lugo (4):
>    staging: tidspbridge - use GTP7 for DSP stack dump
>    staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow
>    staging: tidspbridge - remove disabling twl when printing DSP stack
>    staging: tidspbridge - change mmufault tasklet to a workqueue
>
>   drivers/staging/tidspbridge/core/_deh.h            |    2 +-
>   drivers/staging/tidspbridge/core/dsp-clock.c       |   17 ++++---
>   drivers/staging/tidspbridge/core/ue_deh.c          |   55 +++++++++++--------
>   .../staging/tidspbridge/include/dspbridge/clk.h    |    4 +-
>   4 files changed, 46 insertions(+), 32 deletions(-)
>
Suggest looping in Linux-omap as well.

-- 
Regards,
Nishanth Menon

Re: [PATCHv2 3/4] staging: tidspbridge - remove disabling twl when printing DSP stack

[Felipe Contreras]

On Fri, Dec 10, 2010 at 5:55 AM, Fernando Guzman Lugo
<fernando.lugo@ti.com> wrote:
> Now the SHM segments are not in lock tlbs, instead
> they are in translation tables. So we cannot disable
> twl in order to get the DSP stack dump. Also add a check
> for __get_free_page allocation.

Why?

This means there's a possibility that there will be lingering entries
in the translation tables, and that the DSP side would be able to
corrupt kernel memory.

-- 
Felipe Contreras

Re: [PATCHv2 1/4] staging: tidspbridge - use GTP7 for DSP stack dump

[Felipe Contreras]

On Fri, Dec 10, 2010 at 5:55 AM, Fernando Guzman Lugo
<fernando.lugo@ti.com> wrote:
> DSP stack dump is changed to GTP7 due to GPT8 is used
> by DSP side apps

Does this require a special baseimage firmware? Or all firmwares can use GPT7?

-- 
Felipe Contreras

Re: [PATCHv2 0/4] staging: tidspbridge - misc fixes

[Guzman Lugo, Fernando]

On Fri, Dec 10, 2010 at 5:34 AM, Nishanth Menon <nm@ti.com> wrote:
> Fernando Guzman Lugo wrote, on 12/09/2010 09:55 PM:
>>
>> * V2
>>  - rebase to the latest
>>  - remove patches no needed at this moment
>>
>> This set of patches fix some issues found in lastest tree.
>>
>> Fernando Guzman Lugo (4):
>>   staging: tidspbridge - use GTP7 for DSP stack dump
>>   staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow
>>   staging: tidspbridge - remove disabling twl when printing DSP stack
>>   staging: tidspbridge - change mmufault tasklet to a workqueue
>>
>>  drivers/staging/tidspbridge/core/_deh.h            |    2 +-
>>  drivers/staging/tidspbridge/core/dsp-clock.c       |   17 ++++---
>>  drivers/staging/tidspbridge/core/ue_deh.c          |   55
>> +++++++++++--------
>>  .../staging/tidspbridge/include/dspbridge/clk.h    |    4 +-
>>  4 files changed, 46 insertions(+), 32 deletions(-)
>>
> Suggest looping in Linux-omap as well.

Yes, it was weird linux-omap was not showed by get_maintainer.pl script :(.

Regards,
Fernando.

>
> --
> Regards,
> Nishanth Menon
>

Re: [PATCHv2 3/4] staging: tidspbridge - remove disabling twl when printing DSP stack

[Felipe Contreras]

On Sat, Dec 11, 2010 at 9:25 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Fri, Dec 10, 2010 at 9:43 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Fri, Dec 10, 2010 at 5:55 AM, Fernando Guzman Lugo
>> <fernando.lugo@ti.com> wrote:
>>> Now the SHM segments are not in lock tlbs, instead
>>> they are in translation tables. So we cannot disable
>>> twl in order to get the DSP stack dump. Also add a check
>>> for __get_free_page allocation.
>>
>> Why?
>
> Dsp trace dump is not working if twl is disabled.

DSP trace dump is not working anyway; it never has.

Maybe in some internal or custom tree, but not on upstream, and not
with public firmware.

>> This means there's a possibility that there will be lingering entries
>> in the translation tables, and that the DSP side would be able to
>> corrupt kernel memory.
>
> The thing is that not all shared memory is mapped into tlbs, there is
> still some area which is mapped into twl, I could have added that to
> tlbs too, but with iommu migrations all the entries are in twl, so
> that would work after that patches, so there is no point doing that.
>
> The kernel was fixed with the change of use __get_free_page instead of
> kmalloc for the dummy page. With the change it could corrupt userspace
> pathces but just in the wierd case where you have enable DSP dump
> option, and that the fault have happend in an atomic context in the
> DSP side, thats is very rare case and it is more useful having this
> feature working. A final solution to that can be exporting a new API
> in iommu module to clean up all the twl entries, after that we just
> insert the dummy page and algo the area where the DSP dumps the trace.
> I can make the changes after iommu patches are merged.

Instead of __get_free_page we could use no page at all and pass only '0', no?

And yeah, the final solution would be to clean all the twl entries and
extra tlb's through some iommu API. In the meantime there's no point
in trying to fix a bug that can't be solved upstream anyway and adding
the potential to corrupt user-space memory while doing so.

Cheers.

-- 
Felipe Contreras