[PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning
Joel Stanley
joel at jms.id.au
Wed Sep 28 15:25:10 AEST 2016
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> xchg() is used to set NCSI channel's state in order for consistent
> access to the state. xchg()'s return value should be used. Otherwise,
> one build warning will be raised (with -Wunused-value) as below
> message
> indicates. It is reported by ia64-linux-gcc (GCC) 4.9.0.
>
> net/ncsi/ncsi-manage.c: In function 'ncsi_channel_monitor':
> arch/ia64/include/uapi/asm/cmpxchg.h:56:2: warning: value computed
> is \
> not used [-Wunused-value]
> ((__typeof__(*(ptr))) __xchg((unsigned long) (x), (ptr),
> sizeof(*(ptr))))
> ^
> net/ncsi/ncsi-manage.c:202:3: note: in expansion of macro 'xchg'
> xchg(&nc->state, NCSI_CHANNEL_INACTIVE);
>
> This avoids the build warning by replacing xchg() with atomic_set()
> and atomic_xchg(). atomic_read() is used to retrieve the NCSI
> channel's
> state. No functional change introduced.
>
> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
> ---
> net/ncsi/internal.h | 2 +-
> net/ncsi/ncsi-aen.c | 17 ++++++++++-------
> net/ncsi/ncsi-manage.c | 36 ++++++++++++++++++++----------------
> net/ncsi/ncsi-rsp.c | 4 ++--
> 4 files changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 33738c0..549846b 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -175,7 +175,7 @@ struct ncsi_package;
>
> struct ncsi_channel {
> unsigned char id;
> - int state;
> + atomic_t state;
> #define NCSI_CHANNEL_INACTIVE 1
> #define NCSI_CHANNEL_ACTIVE 2
> #define NCSI_CHANNEL_INVISIBLE 3
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index d463468..77303da 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> +#include <linux/atomic.h>
> #include <linux/netdevice.h>
> #include <linux/skbuff.h>
>
> @@ -55,6 +56,7 @@ static int ncsi_aen_handler_lsc(struct
> ncsi_dev_priv *ndp,
> struct ncsi_channel_mode *ncm;
> unsigned long old_data;
> unsigned long flags;
> + int state;
>
> /* Find the NCSI channel */
> ncsi_find_package_and_channel(ndp, h->common.channel, NULL,
> &nc);
> @@ -70,12 +72,13 @@ static int ncsi_aen_handler_lsc(struct
> ncsi_dev_priv *ndp,
> if (!((old_data ^ ncm->data[2]) & 0x1) ||
> !list_empty(&nc->link))
> return 0;
> - if (!(nc->state == NCSI_CHANNEL_INACTIVE && (ncm->data[2] &
> 0x1)) &&
> - !(nc->state == NCSI_CHANNEL_ACTIVE && !(ncm->data[2] &
> 0x1)))
> +
> + state = atomic_read(&nc->state);
It's not clear that state needs to have atomic access. Can you explain
why you made the change?
Would leaving it as an int and using READ_ONCE() be sufficient?
> + if (!(state == NCSI_CHANNEL_INACTIVE && (ncm->data[2] &
> 0x1)) &&
> + !(state == NCSI_CHANNEL_ACTIVE && !(ncm->data[2] &
> 0x1)))
> return 0;
>
> - if (!(ndp->flags & NCSI_DEV_HWA) &&
> - nc->state == NCSI_CHANNEL_ACTIVE)
> + if (!(ndp->flags & NCSI_DEV_HWA) && state ==
> NCSI_CHANNEL_ACTIVE)
> ndp->flags |= NCSI_DEV_RESHUFFLE;
>
> ncsi_stop_channel_monitor(nc);
> @@ -98,12 +101,12 @@ static int ncsi_aen_handler_cr(struct
> ncsi_dev_priv *ndp,
> return -ENODEV;
>
> if (!list_empty(&nc->link) ||
> - nc->state != NCSI_CHANNEL_ACTIVE)
> + atomic_read(&nc->state) != NCSI_CHANNEL_ACTIVE)
> return 0;
>
> ncsi_stop_channel_monitor(nc);
> spin_lock_irqsave(&ndp->lock, flags);
> - xchg(&nc->state, NCSI_CHANNEL_INACTIVE);
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> @@ -128,7 +131,7 @@ static int ncsi_aen_handler_hncdsc(struct
> ncsi_dev_priv *ndp,
> hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> ncm->data[3] = ntohl(hncdsc->status);
> if (!list_empty(&nc->link) ||
> - nc->state != NCSI_CHANNEL_ACTIVE ||
> + atomic_read(&nc->state) != NCSI_CHANNEL_ACTIVE ||
> (ncm->data[3] & 0x1))
> return 0;
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index ef017b8..b325c1d 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> +#include <linux/atomic.h>
> #include <linux/netdevice.h>
> #include <linux/skbuff.h>
> #include <linux/netlink.h>
> @@ -143,7 +144,7 @@ static void ncsi_report_link(struct ncsi_dev_priv
> *ndp, bool force_down)
> NCSI_FOR_EACH_PACKAGE(ndp, np) {
> NCSI_FOR_EACH_CHANNEL(np, nc) {
> if (!list_empty(&nc->link) ||
> - nc->state != NCSI_CHANNEL_ACTIVE)
> + atomic_read(&nc->state) !=
> NCSI_CHANNEL_ACTIVE)
> continue;
>
> if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1)
> {
> @@ -166,7 +167,7 @@ static void ncsi_channel_monitor(unsigned long
> data)
> bool enabled;
> unsigned int timeout;
> unsigned long flags;
> - int ret;
> + int state, ret;
>
> spin_lock_irqsave(&nc->lock, flags);
> timeout = nc->timeout;
> @@ -175,8 +176,9 @@ static void ncsi_channel_monitor(unsigned long
> data)
>
> if (!enabled || !list_empty(&nc->link))
> return;
> - if (nc->state != NCSI_CHANNEL_INACTIVE &&
> - nc->state != NCSI_CHANNEL_ACTIVE)
> +
> + state = atomic_read(&nc->state);
> + if (state != NCSI_CHANNEL_INACTIVE && state !=
> NCSI_CHANNEL_ACTIVE)
> return;
>
> if (!(timeout % 2)) {
> @@ -195,11 +197,11 @@ static void ncsi_channel_monitor(unsigned long
> data)
>
> if (timeout + 1 >= 3) {
> if (!(ndp->flags & NCSI_DEV_HWA) &&
> - nc->state == NCSI_CHANNEL_ACTIVE)
> + state == NCSI_CHANNEL_ACTIVE)
> ncsi_report_link(ndp, true);
>
> spin_lock_irqsave(&ndp->lock, flags);
> - xchg(&nc->state, NCSI_CHANNEL_INACTIVE);
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
> ncsi_process_next_channel(ndp);
> @@ -266,7 +268,7 @@ struct ncsi_channel *ncsi_add_channel(struct
> ncsi_package *np, unsigned char id)
>
> nc->id = id;
> nc->package = np;
> - nc->state = NCSI_CHANNEL_INACTIVE;
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> nc->enabled = false;
> setup_timer(&nc->timer, ncsi_channel_monitor, (unsigned
> long)nc);
> spin_lock_init(&nc->lock);
> @@ -309,7 +311,7 @@ static void ncsi_remove_channel(struct
> ncsi_channel *nc)
> kfree(ncf);
> }
>
> - nc->state = NCSI_CHANNEL_INACTIVE;
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> spin_unlock_irqrestore(&nc->lock, flags);
> ncsi_stop_channel_monitor(nc);
>
> @@ -556,7 +558,7 @@ static void ncsi_suspend_channel(struct
> ncsi_dev_priv *ndp)
>
> break;
> case ncsi_dev_state_suspend_done:
> - xchg(&nc->state, NCSI_CHANNEL_INACTIVE);
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> ncsi_process_next_channel(ndp);
>
> break;
> @@ -676,9 +678,9 @@ static void ncsi_configure_channel(struct
> ncsi_dev_priv *ndp)
> break;
> case ncsi_dev_state_config_done:
> if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1)
> - xchg(&nc->state, NCSI_CHANNEL_ACTIVE);
> + atomic_set(&nc->state, NCSI_CHANNEL_ACTIVE);
> else
> - xchg(&nc->state, NCSI_CHANNEL_INACTIVE);
> + atomic_set(&nc->state,
> NCSI_CHANNEL_INACTIVE);
>
> ncsi_start_channel_monitor(nc);
> ncsi_process_next_channel(ndp);
> @@ -708,7 +710,7 @@ static int ncsi_choose_active_channel(struct
> ncsi_dev_priv *ndp)
> NCSI_FOR_EACH_PACKAGE(ndp, np) {
> NCSI_FOR_EACH_CHANNEL(np, nc) {
> if (!list_empty(&nc->link) ||
> - nc->state != NCSI_CHANNEL_INACTIVE)
> + atomic_read(&nc->state) !=
> NCSI_CHANNEL_INACTIVE)
> continue;
>
> if (!found)
> @@ -770,7 +772,8 @@ static int ncsi_enable_hwa(struct ncsi_dev_priv
> *ndp)
> spin_lock_irqsave(&ndp->lock, flags);
> NCSI_FOR_EACH_PACKAGE(ndp, np) {
> NCSI_FOR_EACH_CHANNEL(np, nc) {
> - WARN_ON_ONCE(nc->state !=
> NCSI_CHANNEL_INACTIVE ||
> + WARN_ON_ONCE(atomic_read(&nc->state) !=
> + NCSI_CHANNEL_INACTIVE ||
> !list_empty(&nc->link));
> ncsi_stop_channel_monitor(nc);
> list_add_tail_rcu(&nc->link, &ndp-
> >channel_queue);
> @@ -987,7 +990,7 @@ int ncsi_process_next_channel(struct
> ncsi_dev_priv *ndp)
> goto out;
> }
>
> - old_state = xchg(&nc->state, NCSI_CHANNEL_INVISIBLE);
> + old_state = atomic_xchg(&nc->state, NCSI_CHANNEL_INVISIBLE);
> list_del_init(&nc->link);
>
> spin_unlock_irqrestore(&ndp->lock, flags);
> @@ -1006,7 +1009,7 @@ int ncsi_process_next_channel(struct
> ncsi_dev_priv *ndp)
> break;
> default:
> netdev_err(ndp->ndev.dev, "Invalid state 0x%x on
> %d:%d\n",
> - nc->state, nc->package->id, nc->id);
> + old_state, nc->package->id, nc->id);
> ncsi_report_link(ndp, false);
> return -EINVAL;
> }
> @@ -1166,7 +1169,8 @@ int ncsi_start_dev(struct ncsi_dev *nd)
> /* Reset channel's state and start over */
> NCSI_FOR_EACH_PACKAGE(ndp, np) {
> NCSI_FOR_EACH_CHANNEL(np, nc) {
> - old_state = xchg(&nc->state,
> NCSI_CHANNEL_INACTIVE);
> + old_state = atomic_xchg(&nc->state,
> + NCSI_CHANNEL_INACTIV
> E);
> WARN_ON_ONCE(!list_empty(&nc->link) ||
> old_state ==
> NCSI_CHANNEL_INVISIBLE);
> }
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index af84389..54f7eed 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -123,7 +123,7 @@ static int ncsi_rsp_handler_dp(struct
> ncsi_request *nr)
> /* Change state of all channels attached to the package */
> NCSI_FOR_EACH_CHANNEL(np, nc) {
> spin_lock_irqsave(&nc->lock, flags);
> - nc->state = NCSI_CHANNEL_INACTIVE;
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> spin_unlock_irqrestore(&nc->lock, flags);
We don't need both the atomic_set and the spin locks. One or the other
should be enough.
> }
>
> @@ -195,7 +195,7 @@ static int ncsi_rsp_handler_rc(struct
> ncsi_request *nr)
>
> /* Update state for the specified channel */
> spin_lock_irqsave(&nc->lock, flags);
> - nc->state = NCSI_CHANNEL_INACTIVE;
> + atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> spin_unlock_irqrestore(&nc->lock, flags);
>
> return 0;
>
More information about the openbmc
mailing list