| Message ID | 1271632401-2472-1-git-send-email-afleming@freescale.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Apr 18, 2010 at 6:13 PM, Andy Fleming <afleming@freescale.com> wrote: > - while (!(gfar_read(®s->ievent) & > - (IEVENT_GRSC | IEVENT_GTSC))) > + while ((gfar_read(®s->ievent) & > + (IEVENT_GRSC | IEVENT_GTSC)) != > + (IEVENT_GRSC | IEVENT_GTSC)) > cpu_relax(); How about using spin_event_timeout()? It streamlines this process and includes a timeout. The U-Boot version of this code doesn't have a timeout either, but spin_event_timeout() is not available in U-Boot.
On Apr 19, 2010, at 4:08 PM, Timur Tabi wrote: > On Sun, Apr 18, 2010 at 6:13 PM, Andy Fleming <afleming@freescale.com> wrote: > >> - while (!(gfar_read(®s->ievent) & >> - (IEVENT_GRSC | IEVENT_GTSC))) >> + while ((gfar_read(®s->ievent) & >> + (IEVENT_GRSC | IEVENT_GTSC)) != >> + (IEVENT_GRSC | IEVENT_GTSC)) >> cpu_relax(); > > How about using spin_event_timeout()? It streamlines this process and > includes a timeout. > > The U-Boot version of this code doesn't have a timeout either, but > spin_event_timeout() is not available in U-Boot. spin_event_timeout doesn't make sense for this. The patch is fine. - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Apr 18, 2010, at 6:13 PM, Andy Fleming wrote: > When gracefully stopping the controller, the driver was continuing if > *either* RX or TX had stopped. We need to wait for both, or the > controller could get into an invalid state. > > Signed-off-by: Andy Fleming <afleming@freescale.com> > --- > drivers/net/gianfar.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) Acked-by: Kumar Gala <galak@kernel.crashing.org> (please pick this up for 2.6.34, fixes an annoying bug). - k > > diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c > index 032073d..6038397 100644 > --- a/drivers/net/gianfar.c > +++ b/drivers/net/gianfar.c > @@ -1571,8 +1571,9 @@ static void gfar_halt_nodisable(struct net_device *dev) > tempval |= (DMACTRL_GRS | DMACTRL_GTS); > gfar_write(®s->dmactrl, tempval); > > - while (!(gfar_read(®s->ievent) & > - (IEVENT_GRSC | IEVENT_GTSC))) > + while ((gfar_read(®s->ievent) & > + (IEVENT_GRSC | IEVENT_GTSC)) != > + (IEVENT_GRSC | IEVENT_GTSC)) > cpu_relax(); > } > } > -- > 1.6.5.2.g6ff9a > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kumar Gala <galak@kernel.crashing.org> Date: Mon, 19 Apr 2010 23:44:49 -0500 > > On Apr 18, 2010, at 6:13 PM, Andy Fleming wrote: > >> When gracefully stopping the controller, the driver was continuing if >> *either* RX or TX had stopped. We need to wait for both, or the >> controller could get into an invalid state. >> >> Signed-off-by: Andy Fleming <afleming@freescale.com> >> --- >> drivers/net/gianfar.c | 5 +++-- >> 1 files changed, 3 insertions(+), 2 deletions(-) > > Acked-by: Kumar Gala <galak@kernel.crashing.org> > > (please pick this up for 2.6.34, fixes an annoying bug). I will do this tomorrow, thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
> spin_event_timeout doesn't make sense for this. The patch is fine.
Can you please elaborate on that? I don't understand why you think
that. spin_event_timeout() takes an expression and a timeout, and
loops over the expression calling cpu_relax(), just like this loop
does.
On Tue, Apr 20, 2010 at 10:01 AM, Timur Tabi <timur.tabi@gmail.com> wrote: > On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote: > >> spin_event_timeout doesn't make sense for this. The patch is fine. > > Can you please elaborate on that? I don't understand why you think > that. spin_event_timeout() takes an expression and a timeout, and > loops over the expression calling cpu_relax(), just like this loop > does. I haven't tested it, but I think this should work: spin_event_timeout((gfar_read(®s->ievent) & (IEVENT_GRSC | IEVENT_GTSC)) == (IEVENT_GRSC | IEVENT_GTSC), -1, 0); Ideally, Andy should use a timeout value other than -1, and then test the result like this: u32 ret; ret = spin_event_timeout((gfar_read(®s->ievent) & (IEVENT_GRSC | IEVENT_GTSC)) == (IEVENT_GRSC | IEVENT_GTSC), 1000, 0); if (!ret) /* timeout has occurred */
From: Timur Tabi <timur.tabi@gmail.com> Date: Tue, 20 Apr 2010 10:01:48 -0500 > On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote: > >> spin_event_timeout doesn't make sense for this. The patch is fine. > > Can you please elaborate on that? I don't understand why you think > that. spin_event_timeout() takes an expression and a timeout, and > loops over the expression calling cpu_relax(), just like this loop > does. Indeed it does, Kumar this request seems reasonable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Apr 20, 2010, at 8:06 PM, David Miller wrote: > From: Timur Tabi <timur.tabi@gmail.com> > Date: Tue, 20 Apr 2010 10:01:48 -0500 > >> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote: >> >>> spin_event_timeout doesn't make sense for this. The patch is fine. >> >> Can you please elaborate on that? I don't understand why you think >> that. spin_event_timeout() takes an expression and a timeout, and >> loops over the expression calling cpu_relax(), just like this loop >> does. > > Indeed it does, Kumar this request seems reasonable. Are we saying that cpu_relax() is useless and should be removed if we are spinning on a HW register? Its fatally buggy HW if the bits never clear or get set in the few conditions that cpu_relax() are being used. - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kumar Gala <galak@kernel.crashing.org> Date: Tue, 20 Apr 2010 23:22:19 -0500 > > On Apr 20, 2010, at 8:06 PM, David Miller wrote: > >> From: Timur Tabi <timur.tabi@gmail.com> >> Date: Tue, 20 Apr 2010 10:01:48 -0500 >> >>> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote: >>> >>>> spin_event_timeout doesn't make sense for this. The patch is fine. >>> >>> Can you please elaborate on that? I don't understand why you think >>> that. spin_event_timeout() takes an expression and a timeout, and >>> loops over the expression calling cpu_relax(), just like this loop >>> does. >> >> Indeed it does, Kumar this request seems reasonable. > > Are we saying that cpu_relax() is useless and should be removed if we are spinning on a HW register? Kumar, take a deep breath and a step back. spin_event_timeout() does the cpu_relax() too, that's what Timur is trying to tell you. The code will be basically identical as far as I can tell. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Apr 21, 2010, at 12:36 AM, David Miller wrote: > From: Kumar Gala <galak@kernel.crashing.org> > Date: Tue, 20 Apr 2010 23:22:19 -0500 > >> >> On Apr 20, 2010, at 8:06 PM, David Miller wrote: >> >>> From: Timur Tabi <timur.tabi@gmail.com> >>> Date: Tue, 20 Apr 2010 10:01:48 -0500 >>> >>>> On Mon, Apr 19, 2010 at 11:43 PM, Kumar Gala <galak@kernel.crashing.org> wrote: >>>> >>>>> spin_event_timeout doesn't make sense for this. The patch is fine. >>>> >>>> Can you please elaborate on that? I don't understand why you think >>>> that. spin_event_timeout() takes an expression and a timeout, and >>>> loops over the expression calling cpu_relax(), just like this loop >>>> does. >>> >>> Indeed it does, Kumar this request seems reasonable. >> >> Are we saying that cpu_relax() is useless and should be removed if we are spinning on a HW register? > > Kumar, take a deep breath and a step back. > > spin_event_timeout() does the cpu_relax() too, that's what Timur is > trying to tell you. > > The code will be basically identical as far as I can tell. I understand, its more a sense that we are saying we want to time out for what I consider a catastrophic HW failure. - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 21, 2010 at 7:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
> I understand, its more a sense that we are saying we want to time out for what I consider a catastrophic HW failure.
And how else will you detect and recover from such a failure without a
timeout? And are you absolutely certain that there will never be a
programming failure that will cause this loop to spin forever?
If you're really opposed to a timeout, you can still use
spin_event_timeout() by just setting the timeout to -1 and adding a
comment explaining why.
On Apr 21, 2010, at 9:33 AM, Timur Tabi wrote: > On Wed, Apr 21, 2010 at 7:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote: > >> I understand, its more a sense that we are saying we want to time out for what I consider a catastrophic HW failure. > > And how else will you detect and recover from such a failure without a > timeout? And are you absolutely certain that there will never be a > programming failure that will cause this loop to spin forever? > > If you're really opposed to a timeout, you can still use > spin_event_timeout() by just setting the timeout to -1 and adding a > comment explaining why. I'm not opposed, I'm just asking if we are saying we shouldn't be using cpu_relax() for spinning on HW status registers ever. If we are suggesting that cpu_relax() shouldn't be used in these scenarios going forward I'm ok w/the change you suggest and starting to convert other cpu_relax() calls to use spin_event_timeout() - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kumar Gala <galak@kernel.crashing.org> Date: Wed, 21 Apr 2010 14:13:00 -0500 > I'm not opposed, I'm just asking if we are saying we shouldn't be using cpu_relax() for spinning on HW status registers ever. > > If we are suggesting that cpu_relax() shouldn't be used in these scenarios going forward I'm ok w/the change you suggest and starting to convert other cpu_relax() calls to use spin_event_timeout() Kumar this isn't an either-or thing. In both cases we're using cpu_relax(). But by using spin_event_timeout() you're getting both the cpu_relax() and a break-out in case the hardware gets wedged for some reason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c index 032073d..6038397 100644 --- a/drivers/net/gianfar.c +++ b/drivers/net/gianfar.c @@ -1571,8 +1571,9 @@ static void gfar_halt_nodisable(struct net_device *dev) tempval |= (DMACTRL_GRS | DMACTRL_GTS); gfar_write(®s->dmactrl, tempval); - while (!(gfar_read(®s->ievent) & - (IEVENT_GRSC | IEVENT_GTSC))) + while ((gfar_read(®s->ievent) & + (IEVENT_GRSC | IEVENT_GTSC)) != + (IEVENT_GRSC | IEVENT_GTSC)) cpu_relax(); } }
When gracefully stopping the controller, the driver was continuing if *either* RX or TX had stopped. We need to wait for both, or the controller could get into an invalid state. Signed-off-by: Andy Fleming <afleming@freescale.com> --- drivers/net/gianfar.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)