Re: svn commit: r250609 - head/sys/dev/ath
On Wed, May 22, 2019 at 10:34:35AM -0600, Ian Lepore wrote:
> On Wed, 2019-05-22 at 16:20 +, Alexey Dokuchaev wrote:
> > On Mon, May 13, 2013 at 07:03:13PM +, Adrian Chadd wrote:
> > > New Revision: 250609
> > > URL: http://svnweb.freebsd.org/changeset/base/250609
> > >
> > > Log:
> > > Since the node state is 100% back under the TX lock, just kill
> > > the use
> > > of atomics.
> > >
> > > Modified:
> > > head/sys/dev/ath/if_ath.c
> > >
> > > @@ -6140,13 +6133,13 @@ ath_tx_update_tim(struct ath_softc *sc,
> > > /*
> > >* Don't bother grabbing the lock unless the queue is
> > > empty.
> > >*/
> > > - if (atomic_load_acq_int(&an->an_swq_depth) != 0)
> > > + if (&an->an_swq_depth != 0)
> > > return;
> > >
> > > if (an->an_is_powersave &&
> > > an->an_stack_psq == 0 &&
> > > an->an_tim_set == 1 &&
> > > - atomic_load_acq_int(&an->an_swq_depth) == 0) {
> > > + an->an_swq_depth == 0) {
> >
> > PVS Studio complains here: warning: V560 A part of conditional
> > expression
> > is always true: an->an_swq_depth == 0. Which probably makes sense
> > since
> > you return earlier if it's != 0.
>
> You're replying to a six year old commit?
Yeah, anything wrong with it?
> It doesn't check earlier whether that value is 0, it checks whether the
> address of that value is 0. That's probably a bug.
No, the & is a vestige from the atomic_* conversion, it is not present in
current code. The check is still there, however.
./danfe
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r250609 - head/sys/dev/ath
On Wed, 2019-05-22 at 16:20 +, Alexey Dokuchaev wrote:
> On Mon, May 13, 2013 at 07:03:13PM +, Adrian Chadd wrote:
> > New Revision: 250609
> > URL: http://svnweb.freebsd.org/changeset/base/250609
> >
> > Log:
> > Since the node state is 100% back under the TX lock, just kill
> > the use
> > of atomics.
> >
> > Modified:
> > head/sys/dev/ath/if_ath.c
> >
> > @@ -6140,13 +6133,13 @@ ath_tx_update_tim(struct ath_softc *sc,
> > /*
> > * Don't bother grabbing the lock unless the queue is
> > empty.
> > */
> > - if (atomic_load_acq_int(&an->an_swq_depth) != 0)
> > + if (&an->an_swq_depth != 0)
> > return;
> >
> > if (an->an_is_powersave &&
> > an->an_stack_psq == 0 &&
> > an->an_tim_set == 1 &&
> > - atomic_load_acq_int(&an->an_swq_depth) == 0) {
> > + an->an_swq_depth == 0) {
>
> PVS Studio complains here: warning: V560 A part of conditional
> expression
> is always true: an->an_swq_depth == 0. Which probably makes sense
> since
> you return earlier if it's != 0.
>
> ./danfe
>
You're replying to a six year old commit?
It doesn't check earlier whether that value is 0, it checks whether the
address of that value is 0. That's probably a bug.
-- Ian
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r250609 - head/sys/dev/ath
On Mon, May 13, 2013 at 07:03:13PM +, Adrian Chadd wrote:
> New Revision: 250609
> URL: http://svnweb.freebsd.org/changeset/base/250609
>
> Log:
> Since the node state is 100% back under the TX lock, just kill the use
> of atomics.
>
> Modified:
> head/sys/dev/ath/if_ath.c
>
> @@ -6140,13 +6133,13 @@ ath_tx_update_tim(struct ath_softc *sc,
> /*
>* Don't bother grabbing the lock unless the queue is empty.
>*/
> - if (atomic_load_acq_int(&an->an_swq_depth) != 0)
> + if (&an->an_swq_depth != 0)
> return;
>
> if (an->an_is_powersave &&
> an->an_stack_psq == 0 &&
> an->an_tim_set == 1 &&
> - atomic_load_acq_int(&an->an_swq_depth) == 0) {
> + an->an_swq_depth == 0) {
PVS Studio complains here: warning: V560 A part of conditional expression
is always true: an->an_swq_depth == 0. Which probably makes sense since
you return earlier if it's != 0.
./danfe
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r250609 - head/sys/dev/ath
Author: adrian
Date: Mon May 13 19:03:12 2013
New Revision: 250609
URL: http://svnweb.freebsd.org/changeset/base/250609
Log:
Since the node state is 100% back under the TX lock, just kill the use
of atomics.
I'll re-think this nonsense later.
Modified:
head/sys/dev/ath/if_ath.c
head/sys/dev/ath/if_athvar.h
Modified: head/sys/dev/ath/if_ath.c
==
--- head/sys/dev/ath/if_ath.c Mon May 13 19:02:22 2013(r250608)
+++ head/sys/dev/ath/if_ath.c Mon May 13 19:03:12 2013(r250609)
@@ -6028,7 +6028,7 @@ ath_node_set_tim(struct ieee80211_node *
an->an_tim_set = 1;
ATH_TX_UNLOCK(sc);
changed = avp->av_set_tim(ni, enable);
- } else if (atomic_load_acq_int(&an->an_swq_depth) == 0) {
+ } else if (an->an_swq_depth == 0) {
/* disable */
DPRINTF(sc, ATH_DEBUG_NODE_PWRSAVE,
"%s: an=%p, enable=%d, an_swq_depth == 0, disabling\n",
@@ -6120,16 +6120,9 @@ ath_tx_update_tim(struct ath_softc *sc,
ATH_TX_LOCK_ASSERT(sc);
if (enable) {
- /*
-* Don't bother grabbing the lock unless the queue is not
-* empty.
-*/
- if (atomic_load_acq_int(&an->an_swq_depth) == 0)
- return;
-
if (an->an_is_powersave &&
an->an_tim_set == 0 &&
- atomic_load_acq_int(&an->an_swq_depth) != 0) {
+ an->an_swq_depth != 0) {
DPRINTF(sc, ATH_DEBUG_NODE_PWRSAVE,
"%s: an=%p, swq_depth>0, tim_set=0, set!\n",
__func__, an);
@@ -6140,13 +6133,13 @@ ath_tx_update_tim(struct ath_softc *sc,
/*
* Don't bother grabbing the lock unless the queue is empty.
*/
- if (atomic_load_acq_int(&an->an_swq_depth) != 0)
+ if (&an->an_swq_depth != 0)
return;
if (an->an_is_powersave &&
an->an_stack_psq == 0 &&
an->an_tim_set == 1 &&
- atomic_load_acq_int(&an->an_swq_depth) == 0) {
+ an->an_swq_depth == 0) {
DPRINTF(sc, ATH_DEBUG_NODE_PWRSAVE,
"%s: an=%p, swq_depth=0, tim_set=1, psq_set=0,"
" clear!\n",
Modified: head/sys/dev/ath/if_athvar.h
==
--- head/sys/dev/ath/if_athvar.hMon May 13 19:02:22 2013
(r250608)
+++ head/sys/dev/ath/if_athvar.hMon May 13 19:03:12 2013
(r250609)
@@ -415,17 +415,17 @@ struct ath_txq {
#define ATH_TID_INSERT_HEAD(_tq, _elm, _field) do { \
TAILQ_INSERT_HEAD(&(_tq)->tid_q, (_elm), _field); \
(_tq)->axq_depth++; \
- atomic_add_rel_32( &((_tq)->an)->an_swq_depth, 1); \
+ (_tq)->an->an_swq_depth++; \
} while (0)
#define ATH_TID_INSERT_TAIL(_tq, _elm, _field) do { \
TAILQ_INSERT_TAIL(&(_tq)->tid_q, (_elm), _field); \
(_tq)->axq_depth++; \
- atomic_add_rel_32( &((_tq)->an)->an_swq_depth, 1); \
+ (_tq)->an->an_swq_depth++; \
} while (0)
#define ATH_TID_REMOVE(_tq, _elm, _field) do { \
TAILQ_REMOVE(&(_tq)->tid_q, _elm, _field); \
(_tq)->axq_depth--; \
- atomic_subtract_rel_32( &((_tq)->an)->an_swq_depth, 1); \
+ (_tq)->an->an_swq_depth--; \
} while (0)
#defineATH_TID_FIRST(_tq) TAILQ_FIRST(&(_tq)->tid_q)
#defineATH_TID_LAST(_tq, _field) TAILQ_LAST(&(_tq)->tid_q,
_field)
@@ -436,17 +436,17 @@ struct ath_txq {
#define ATH_TID_FILT_INSERT_HEAD(_tq, _elm, _field) do { \
TAILQ_INSERT_HEAD(&(_tq)->filtq.tid_q, (_elm), _field); \
(_tq)->axq_depth++; \
- atomic_add_rel_32( &((_tq)->an)->an_swq_depth, 1); \
+ (_tq)->an->an_swq_depth++; \
} while (0)
#define ATH_TID_FILT_INSERT_TAIL(_tq, _elm, _field) do { \
TAILQ_INSERT_TAIL(&(_tq)->filtq.tid_q, (_elm), _field); \
(_tq)->axq_depth++; \
- atomic_add_rel_32( &((_tq)->an)->an_swq_depth, 1); \
+ (_tq)->an->an_swq_depth++; \
} while (0)
#define ATH_TID_FILT_REMOVE(_tq, _elm, _field) do { \
TAILQ_REMOVE(&(_tq)->filtq.tid_q, _elm, _field); \
(_tq)->axq_depth--; \
- atomic_subtract_rel_32( &((_tq)->an)->an_swq_depth, 1); \
+ (_tq)->an->an_swq_depth--; \
} while (0)
#defineATH_TID_FILT_FIRST(_tq) TAILQ_FIRST(&(_tq)->filtq.tid_q)
#defineATH_TID_FILT_LAST(_tq, _field)
TAILQ_LAST(&(_tq)->filtq.tid_q,_field)
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
