Re: [PATCH 6/9] DSPBRIDGE: Remove DPC, create, destroy and schedule wrappers

[Nishanth Menon <nm@ti.com>]

Ramirez Luna, Omar had written, on 11/23/2009 06:50 PM, the following:
> Remove DPC wrappers and replace with direct Linux calls.
> 
> These changes apply to IO DPC and MMUfault DPC
> 
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
> ---
>  arch/arm/plat-omap/include/dspbridge/dpc.h |   90 +++--------------
>  drivers/dsp/bridge/services/dpc.c          |  144 +---------------------------
>  drivers/dsp/bridge/wmd/io_sm.c             |   73 +++++++++++++-
>  drivers/dsp/bridge/wmd/mmu_fault.c         |   24 ++++-
>  drivers/dsp/bridge/wmd/ue_deh.c            |   31 ++++++-
>  5 files changed, 132 insertions(+), 230 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/dspbridge/dpc.h b/arch/arm/plat-omap/include/dspbridge/dpc.h
> index 870a1b2..0c60342 100644
> --- a/arch/arm/plat-omap/include/dspbridge/dpc.h
> +++ b/arch/arm/plat-omap/include/dspbridge/dpc.h
> @@ -19,8 +19,6 @@
>  #ifndef DPC_
>  #define DPC_
> 
> -       struct DPC_OBJECT;
> -
>  /*
>   *  ======== DPC_PROC ========
>   *  Purpose:
> @@ -40,64 +38,22 @@
>   */
>         typedef void(*DPC_PROC) (void *pRefData);
> 
> -/*
> - *  ======== DPC_Cancel ========
> - *  Purpose:
> - *      Cancel a DPC previously scheduled by DPC_Schedule.
> - *  Parameters:
> - *      hDPC:           A DPC object handle created in DPC_Create().
> - *  Returns:
> - *      DSP_SOK:        Scheduled DPC, if any, is cancelled.
> - *      DSP_SFALSE:     No DPC is currently scheduled for execution.
> - *      DSP_EHANDLE:    Invalid hDPC.
> - *  Requires:
> - *  Ensures:
> - *      If the DPC has already executed, is executing, or was not yet
> - *      scheduled, this function will have no effect.
> - */
> -       extern DSP_STATUS DPC_Cancel(IN struct DPC_OBJECT *hDPC);
> +/* The DPC object, passed to our priority event callback routine: */
You may want to follow linux coding style. and not loose info from the 
comments.

Documentation/kernel-doc-nano-HOWTO.txt
[...]
Example kernel-doc function comment:

/**
  * foobar() - short function description of foobar
  * @arg1:       Describe the first argument to foobar.
  * @arg2:       Describe the second argument to foobar.
  *              One can provide multiple line descriptions
  *              for arguments.
  *
  * A longer description, with more discussion of the function foobar()
  * that might be useful to those using or modifying it.  Begins with
  * empty comment line, and may include additional embedded empty
  * comment lines.
  *
  * The longer description can have multiple paragraphs.
  */
[...]
Example kernel-doc data structure comment.

**
* struct blah - the basic blah structure
* @mem1:       describe the first member of struct blah
* @mem2:       describe the second member of struct blah,
*              perhaps with more lines and words.
*
* Longer description of this structure.
*/


> +struct DPC_OBJECT {
> +       u32 dwSignature;        /* Used for object validation.   */
> +       void *pRefData;         /* Argument for client's DPC.    */
> +       DPC_PROC pfnDPC;        /* Client's DPC.                 */
> +       u32 numRequested;       /* Number of requested DPC's.      */
> +       u32 numScheduled;       /* Number of executed DPC's.      */
> +       struct tasklet_struct dpc_tasklet;
[...]


> diff --git a/drivers/dsp/bridge/wmd/io_sm.c b/drivers/dsp/bridge/wmd/io_sm.c
> index 96a5aa6..60dbc62 100644
> --- a/drivers/dsp/bridge/wmd/io_sm.c
> +++ b/drivers/dsp/bridge/wmd/io_sm.c
> @@ -251,7 +251,26 @@ DSP_STATUS WMD_IO_Create(OUT struct IO_MGR **phIOMgr,
> 
>         if (devType == DSP_UNIT) {
>                 /* Create a DPC object: */
> -               status = DPC_Create(&pIOMgr->hDPC, IO_DPC, (void *)pIOMgr);
> +               MEM_AllocObject(pIOMgr->hDPC, struct DPC_OBJECT,
> +                               IO_MGRSIGNATURE);
> +               if (pIOMgr->hDPC) {
> +                       tasklet_init(&pIOMgr->hDPC->dpc_tasklet,
> +                               DPC_DeferredProcedure, (u32)pIOMgr->hDPC);
> +                       /* Fill out our DPC Object: */
> +                       pIOMgr->hDPC->pRefData = (void *)pIOMgr;
> +                       pIOMgr->hDPC->pfnDPC = IO_DPC;
> +                       pIOMgr->hDPC->numRequested = 0;
> +                       pIOMgr->hDPC->numScheduled = 0;
> +#ifdef DEBUG
> +                       pIOMgr->hDPC->numRequestedMax = 0;
> +                       pIOMgr->hDPC->cEntryCount = 0;
> +#endif

Do you want to do a memset with 0 instead and fill up the deltas?

> +                       spin_lock_init(&pIOMgr->hDPC->dpc_lock);
> +               } else {
> +                       DBG_Trace(GT_6CLASS, "IO DPC Create: DSP_EMEMORY\n");
> +                       status = DSP_EMEMORY;
> +               }
> +
>                 if (DSP_SUCCEEDED(status))
>                         status = DEV_GetDevNode(hDevObject, &hDevNode);
> 
> @@ -312,8 +331,13 @@ DSP_STATUS WMD_IO_Destroy(struct IO_MGR *hIOMgr)
>                 destroy_workqueue(bridge_workqueue);
>                 /* Linux function to uninstall ISR */
>                 free_irq(INT_MAIL_MPU_IRQ, (void *)hIOMgr);
> -               if (hIOMgr->hDPC)
> -                       (void)DPC_Destroy(hIOMgr->hDPC);
> +
> +               /* Free DPC object */
> +               tasklet_kill(&hIOMgr->hDPC->dpc_tasklet);
> +               MEM_FreeObject(hIOMgr->hDPC);
> +               hIOMgr->hDPC = NULL;
> +               DBG_Trace(GT_2CLASS, "DPC_Destroy: SUCCESS\n");
> +
>  #ifndef DSP_TRACEBUF_DISABLED
>                 if (hIOMgr->pMsg)
>                         MEM_Free(hIOMgr->pMsg);
> @@ -1009,6 +1033,8 @@ irqreturn_t IO_ISR(int irq, IN void *pRefData)
>  {
>         struct IO_MGR *hIOMgr = (struct IO_MGR *)pRefData;
>         bool fSchedDPC;
> +       unsigned long flags;
> +
>         if (irq != INT_MAIL_MPU_IRQ ||
>            !MEM_IsValidHandle(hIOMgr, IO_MGRSIGNATURE))
>                 return IRQ_NONE;
> @@ -1030,8 +1056,26 @@ irqreturn_t IO_ISR(int irq, IN void *pRefData)
>                         DBG_Trace(DBG_LEVEL6, "*** DSP RESET ***\n");
>                         hIOMgr->wIntrVal = 0;
>                 } else if (fSchedDPC) {
> -                       /* PROC-COPY defer i/o  */
> -                       DPC_Schedule(hIOMgr->hDPC);
> +                       /*
> +                        * PROC-COPY defer i/o.
> +                        * Increment count of DPC's pending.
> +                        */
> +                       spin_lock_irqsave(&hIOMgr->hDPC->dpc_lock, flags);
> +                       hIOMgr->hDPC->numRequested++;
> +                       spin_unlock_irqrestore(&hIOMgr->hDPC->dpc_lock, flags);
> +
> +                       /* Schedule DPC */
> +                       tasklet_schedule(&hIOMgr->hDPC->dpc_tasklet);
> +#ifdef DEBUG
> +                       if (hIOMgr->hDPC->numRequested >
> +                          hIOMgr->hDPC->numScheduled +
> +                          hIOMgr->hDPC->numRequestedMax) {
> +                               hIOMgr->hDPC->numRequestedMax =
> +                                       hIOMgr->hDPC->numRequested -
> +                                       hIOMgr->hDPC->numScheduled;
a) is this safe?
b) {} for a one liner ? mebbe the level of indentation is too much?

> +                       }
> +#endif
[...]

> --- a/drivers/dsp/bridge/wmd/ue_deh.c
> +++ b/drivers/dsp/bridge/wmd/ue_deh.c
> @@ -91,8 +91,27 @@ DSP_STATUS WMD_DEH_Create(OUT struct DEH_MGR **phDehMgr,
>                         status = NTFY_Create(&pDehMgr->hNtfy);
> 
>                 /* Create a DPC object. */
> -               status = DPC_Create(&pDehMgr->hMmuFaultDpc, MMU_FaultDpc,
> -                                  (void *)pDehMgr);
> +               MEM_AllocObject(pDehMgr->hMmuFaultDpc, struct DPC_OBJECT,
> +                               SIGNATURE);
> +               if (pDehMgr->hMmuFaultDpc) {
> +                       tasklet_init(&pDehMgr->hMmuFaultDpc->dpc_tasklet,
> +                               DPC_DeferredProcedure,
> +                               (u32)pDehMgr->hMmuFaultDpc);
> +                       /* Fill out DPC Object */
> +                       pDehMgr->hMmuFaultDpc->pRefData = (void *)pDehMgr;
> +                       pDehMgr->hMmuFaultDpc->pfnDPC = MMU_FaultDpc;
> +                       pDehMgr->hMmuFaultDpc->numRequested = 0;
> +                       pDehMgr->hMmuFaultDpc->numScheduled = 0;
> +#ifdef DEBUG
> +                       pDehMgr->hMmuFaultDpc->numRequestedMax = 0;
> +                       pDehMgr->hMmuFaultDpc->cEntryCount = 0;
> +#endif
> +                       spin_lock_init(&pDehMgr->hMmuFaultDpc->dpc_lock);
> +               } else {
> +                       DBG_Trace(GT_6CLASS, "DEH DPC Create: DSP_EMEMORY\n");
> +                       status = DSP_EMEMORY;
I think you give up at this point right? why continue the code? wont it 
be easier if your code did:
if (!pDehMgr->hMmuFaultDpc) {
	DBG_Trace(GT_6CLASS, "DEH DPC Create: DSP_EMEMORY\n");
	return DSP_EMEMORY;
}
  do rest of code..

> +               }
> +
>                 if (DSP_SUCCEEDED(status))
>                         status = DEV_GetDevNode(hDevObject, &hDevNode);
> 
> @@ -144,7 +163,13 @@ DSP_STATUS WMD_DEH_Destroy(struct DEH_MGR *hDehMgr)
>                         (void)NTFY_Delete(pDehMgr->hNtfy);
>                 /* Disable DSP MMU fault */
>                 free_irq(INT_DSP_MMU_IRQ, pDehMgr);
> -               (void)DPC_Destroy(pDehMgr->hMmuFaultDpc);
> +
> +               /* Free DPC object */
> +               tasklet_kill(&pDehMgr->hMmuFaultDpc->dpc_tasklet);
> +               MEM_FreeObject(pDehMgr->hMmuFaultDpc);
> +               pDehMgr->hMmuFaultDpc = NULL;
> +               DBG_Trace(GT_2CLASS, "DPC_Destroy: SUCCESS\n");
> +
>                 /* Deallocate the DEH manager object */
>                 MEM_FreeObject(pDehMgr);
>         }
> --

-- 
Regards,
Nishanth Menon