| Message ID | 20141021141647.GF15405@oracle.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
Hi Sowmini, On Wed, Oct 22, 2014 at 1:16 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > For NAPIfied drivers , there is no need to > synchronize by doing irqsave/restore on vio.lock in the I/O > path. > > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> > --- > arch/sparc/kernel/viohs.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c > index 7ef081a..d731586 100644 > --- a/arch/sparc/kernel/viohs.c > +++ b/arch/sparc/kernel/viohs.c > @@ -747,10 +747,11 @@ EXPORT_SYMBOL(vio_ldc_free); > > void vio_port_up(struct vio_driver_state *vio) > { > - unsigned long flags; > + unsigned long flags = 0; Is gcc not smart enough to know that this variable isn't used before it's set? (I assume it isn't used elsewhere in this function) > int err, state; > > - spin_lock_irqsave(&vio->lock, flags); > + if (!in_softirq()) > + spin_lock_irqsave(&vio->lock, flags); > > state = ldc_state(vio->lp); > > @@ -777,7 +778,8 @@ void vio_port_up(struct vio_driver_state *vio) > mod_timer(&vio->timer, expires); > } > > - spin_unlock_irqrestore(&vio->lock, flags); > + if (!in_softirq()) > + spin_unlock_irqrestore(&vio->lock, flags); > } > EXPORT_SYMBOL(vio_port_up); Thanks,
On (10/22/14 09:35), Julian Calaby wrote: > > void vio_port_up(struct vio_driver_state *vio) > > { > > - unsigned long flags; > > + unsigned long flags = 0; > > Is gcc not smart enough to know that this variable isn't used before > it's set? (I assume it isn't used elsewhere in this function) No, it's not used elsewhere in the function, and yes, I too was surprised by the build warning, which is why I initialized it as above. --Sowmini -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sowmini, On Wed, Oct 22, 2014 at 9:39 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (10/22/14 09:35), Julian Calaby wrote: >> > void vio_port_up(struct vio_driver_state *vio) >> > { >> > - unsigned long flags; >> > + unsigned long flags = 0; >> >> Is gcc not smart enough to know that this variable isn't used before >> it's set? (I assume it isn't used elsewhere in this function) > > No, it's not used elsewhere in the function, and yes, I too was > surprised by the build warning, which is why I initialized it > as above. Ok, fair enough then. Thanks,
On 10/21/2014 05:35 PM, Julian Calaby wrote: > Hi Sowmini, > > On Wed, Oct 22, 2014 at 1:16 AM, Sowmini Varadhan > <sowmini.varadhan@oracle.com> wrote: >> For NAPIfied drivers , there is no need to >> synchronize by doing irqsave/restore on vio.lock in the I/O >> path. >> >> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> >> --- >> arch/sparc/kernel/viohs.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c >> index 7ef081a..d731586 100644 >> --- a/arch/sparc/kernel/viohs.c >> +++ b/arch/sparc/kernel/viohs.c >> @@ -747,10 +747,11 @@ EXPORT_SYMBOL(vio_ldc_free); >> >> void vio_port_up(struct vio_driver_state *vio) >> { >> - unsigned long flags; >> + unsigned long flags = 0; > > Is gcc not smart enough to know that this variable isn't used before > it's set? (I assume it isn't used elsewhere in this function) It probably assumes in_softirq() might evaluate differently in the each case. > >> int err, state; >> >> - spin_lock_irqsave(&vio->lock, flags); >> + if (!in_softirq()) >> + spin_lock_irqsave(&vio->lock, flags); >> >> state = ldc_state(vio->lp); >> >> @@ -777,7 +778,8 @@ void vio_port_up(struct vio_driver_state *vio) >> mod_timer(&vio->timer, expires); >> } >> >> - spin_unlock_irqrestore(&vio->lock, flags); >> + if (!in_softirq()) >> + spin_unlock_irqrestore(&vio->lock, flags); >> } >> EXPORT_SYMBOL(vio_port_up); > > Thanks, > -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On (10/21/14 18:56), Dave Kleikamp wrote: > > > > Is gcc not smart enough to know that this variable isn't used before > > it's set? (I assume it isn't used elsewhere in this function) > > It probably assumes in_softirq() might evaluate differently in the each > case. yes, that's what I suspected too. I suppose it is possible from the compiler's point of view that something in between might change the result of in_softirq() so that we may be using an uninit variable in the second call. anyway, the warning was annoying, and would only numb the user into ignoring other real issues, so I figured I might as well silence the warning. --Sowmini -- To unsubscribe from this list: send the line "unsubscribe sparclinux" 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/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c index 7ef081a..d731586 100644 --- a/arch/sparc/kernel/viohs.c +++ b/arch/sparc/kernel/viohs.c @@ -747,10 +747,11 @@ EXPORT_SYMBOL(vio_ldc_free); void vio_port_up(struct vio_driver_state *vio) { - unsigned long flags; + unsigned long flags = 0; int err, state; - spin_lock_irqsave(&vio->lock, flags); + if (!in_softirq()) + spin_lock_irqsave(&vio->lock, flags); state = ldc_state(vio->lp); @@ -777,7 +778,8 @@ void vio_port_up(struct vio_driver_state *vio) mod_timer(&vio->timer, expires); } - spin_unlock_irqrestore(&vio->lock, flags); + if (!in_softirq()) + spin_unlock_irqrestore(&vio->lock, flags); } EXPORT_SYMBOL(vio_port_up);
For NAPIfied drivers , there is no need to synchronize by doing irqsave/restore on vio.lock in the I/O path. Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> --- arch/sparc/kernel/viohs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)