Re: [PATCH] cpm_uart: Fix dpram allocation and non-console uarts

[Nishanth Aravamudan]

On 21.08.2005 [18:12:35 -0300], Marcelo Tosatti wrote:
> Hi,
> 
> On Wed, Aug 17, 2005 at 10:42:36PM -0700, Nish Aravamudan wrote:
> > On 8/8/05, Kumar Gala <galak@xxxxxxxxxxxxx> wrote:
> > > (A believe Marcelo would like to see this in 2.6.13, but I'll let him
> > > fight over that ;)
> > > 
> > > * Makes dpram allocations work
> > > * Makes non-console UART work on both 8xx and 82xx
> > > * Fixed whitespace in files that were touched
> > > 
> > > Signed-off-by: Vitaly Bordug <vbordug@xxxxxxxxxxxxx>
> > > Signed-off-by: Pantelis Antoniou <panto@xxxxxxxxxxx>
> > > Signed-off-by: Kumar Gala <kumar.gala@xxxxxxxxxxxxx>
> > > 
> > > ---
> > > commit 1de80554bcae877dce3b6d878053eb092ef65c72
> > > tree aba124824607fea1070e86501ddccc9decce362d
> > > parent ad81111fd554c9d3c14c0a50885e076af2f9ac9b
> > > author Kumar K. Gala <kumar.gala@xxxxxxxxxxxxx> Mon, 08 Aug 2005 22:35:39 -0500
> > > committer Kumar K. Gala <kumar.gala@xxxxxxxxxxxxx> Mon, 08 Aug 2005 22:35:39 -0500
> > 
> > <snip>
> > 
> > > diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
> > > --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> > > +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> > 
> > <snip>
> > 
> > > @@ -376,9 +396,19 @@ static int cpm_uart_startup(struct uart_
> > >                 pinfo->sccp->scc_sccm |= UART_SCCM_RX;
> > >         }
> > > 
> > > +       if (!(pinfo->flags & FLAG_CONSOLE))
> > > +               cpm_line_cr_cmd(line,CPM_CR_INIT_TRX);
> > >         return 0;
> > >  }
> > > 
> > > +inline void cpm_uart_wait_until_send(struct uart_cpm_port *pinfo)
> > > +{
> > > +       unsigned long target_jiffies = jiffies + pinfo->wait_closing;
> > > +
> > > +       while (!time_after(jiffies, target_jiffies))
> > > +               schedule();
> > > +}
> > 
> > Not sure about that call here. Does the state need to be set so that
> > you won't be run again immediately? In any case, I think direct
> > schedule() callers are discouraged? Do you want to call a yield() or
> > schedule_timeout({0,1}) instead maybe?
> 
> Yep, schedule_timeout(pinfo->wait_closing) looks much better.
> 
> > >  /*
> > >   * Shutdown the uart
> > >   */
> > > @@ -394,6 +424,12 @@ static void cpm_uart_shutdown(struct uar
> > > 
> > >         /* If the port is not the console, disable Rx and Tx. */
> > >         if (!(pinfo->flags & FLAG_CONSOLE)) {
> > > +               /* Wait for all the BDs marked sent */
> > > +               while(!cpm_uart_tx_empty(port))
> > > +                       schedule_timeout(2);
> > 
> > <snip>
> > 
> > I think you are using 2 jiffies to guarantee that at least one jiffy
> > elapses, which is fine. But, if you do not set the state beforehand,
> > schedule_timeout() returns immediately, so you have a busy-wait here.
> 
> Right, what about the following untested patch.
> 
> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
> --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> @@ -403,10 +403,9 @@ static int cpm_uart_startup(struct uart_
>  
>  inline void cpm_uart_wait_until_send(struct uart_cpm_port *pinfo)
>  {
> -	unsigned long target_jiffies = jiffies + pinfo->wait_closing;
> -
> -	while (!time_after(jiffies, target_jiffies))
> -   		schedule();
> +	set_current_state(TASK_UNINTERRUPTIBLE);
> +	schedule_timeout(pinfo->wait_closing);
> +	set_current_state(TASK_RUNNING);
>  }

Both changes look correct/better. Except you shouldn't need to set the
state back to TASK_RUNNING in either case, as schedule_timeout()
guarantees the task will return in that state.

Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/