[PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning
Gavin Shan
gwshan at linux.vnet.ibm.com
Thu Sep 29 10:27:01 AEST 2016
On Wed, Sep 28, 2016 at 02:55:10PM +0930, Joel Stanley wrote:
>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?
>
Yes, READ_ONCE() + WRITE_ONCE() should 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.
>
Right, I will use WRITE_ONCE() and remove the lock.
Thanks,
Gavin
>> }
>>
>> @@ -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