* [PATCH v4.7 2/7] net/ncsi: Introduce NCSI_RESERVED_CHANNEL
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
@ 2016-09-28 2:48 ` Gavin Shan
2016-09-28 5:30 ` Joel Stanley
2016-09-28 2:48 ` [PATCH v4.7 3/7] net/ncsi: Don't probe on the reserved channel ID (0x1f) Gavin Shan
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 2:48 UTC (permalink / raw)
To: openbmc; +Cc: joel, Gavin Shan
This defines NCSI_RESERVED_CHANNEL as the reserved NCSI channel
ID (0x1f). No logical changes introduced.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
net/ncsi/internal.h | 1 +
net/ncsi/ncsi-manage.c | 14 +++++++-------
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 549846b..0a5c580 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -170,6 +170,7 @@ struct ncsi_package;
#define NCSI_PACKAGE_SHIFT 5
#define NCSI_PACKAGE_INDEX(c) (((c) >> NCSI_PACKAGE_SHIFT) & 0x7)
+#define NCSI_RESERVED_CHANNEL 0x1f
#define NCSI_CHANNEL_INDEX(c) ((c) & ((1 << NCSI_PACKAGE_SHIFT) - 1))
#define NCSI_TO_CHANNEL(p, c) (((p) << NCSI_PACKAGE_SHIFT) | (c))
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index b325c1d..b03d1df 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -529,7 +529,7 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
nca.package = np->id;
if (nd->state == ncsi_dev_state_suspend_select) {
nca.type = NCSI_PKT_CMD_SP;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
if (ndp->flags & NCSI_DEV_HWA)
nca.bytes[0] = 0;
else
@@ -546,7 +546,7 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
nd->state = ncsi_dev_state_suspend_deselect;
} else if (nd->state == ncsi_dev_state_suspend_deselect) {
nca.type = NCSI_PKT_CMD_DP;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
nd->state = ncsi_dev_state_suspend_done;
}
@@ -592,7 +592,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
else
nca.bytes[0] = 1;
nca.package = np->id;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
ret = ncsi_xmit_cmd(&nca);
if (ret)
goto error;
@@ -810,7 +810,7 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
/* Deselect all possible packages */
nca.type = NCSI_PKT_CMD_DP;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
for (index = 0; index < 8; index++) {
nca.package = index;
ret = ncsi_xmit_cmd(&nca);
@@ -826,7 +826,7 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
/* Select all possible packages */
nca.type = NCSI_PKT_CMD_SP;
nca.bytes[0] = 1;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
for (index = 0; index < 8; index++) {
nca.package = index;
ret = ncsi_xmit_cmd(&nca);
@@ -879,7 +879,7 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
nca.type = NCSI_PKT_CMD_SP;
nca.bytes[0] = 1;
nca.package = ndp->active_package->id;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
ret = ncsi_xmit_cmd(&nca);
if (ret)
goto error;
@@ -936,7 +936,7 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
/* Deselect the active package */
nca.type = NCSI_PKT_CMD_DP;
nca.package = ndp->active_package->id;
- nca.channel = 0x1f;
+ nca.channel = NCSI_RESERVED_CHANNEL;
ret = ncsi_xmit_cmd(&nca);
if (ret)
goto error;
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 2/7] net/ncsi: Introduce NCSI_RESERVED_CHANNEL
2016-09-28 2:48 ` [PATCH v4.7 2/7] net/ncsi: Introduce NCSI_RESERVED_CHANNEL Gavin Shan
@ 2016-09-28 5:30 ` Joel Stanley
0 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:30 UTC (permalink / raw)
To: Gavin Shan, openbmc
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> This defines NCSI_RESERVED_CHANNEL as the reserved NCSI channel
> ID (0x1f). No logical changes introduced.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> net/ncsi/internal.h | 1 +
> net/ncsi/ncsi-manage.c | 14 +++++++-------
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 549846b..0a5c580 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -170,6 +170,7 @@ struct ncsi_package;
>
> #define NCSI_PACKAGE_SHIFT 5
> #define NCSI_PACKAGE_INDEX(c) (((c) >> NCSI_PACKAGE_SHIFT) &
> 0x7)
> +#define NCSI_RESERVED_CHANNEL 0x1f
> #define NCSI_CHANNEL_INDEX(c) ((c) & ((1 <<
> NCSI_PACKAGE_SHIFT) - 1))
> #define NCSI_TO_CHANNEL(p, c) (((p) << NCSI_PACKAGE_SHIFT) |
> (c))
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index b325c1d..b03d1df 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -529,7 +529,7 @@ static void ncsi_suspend_channel(struct
> ncsi_dev_priv *ndp)
> nca.package = np->id;
> if (nd->state == ncsi_dev_state_suspend_select) {
> nca.type = NCSI_PKT_CMD_SP;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> if (ndp->flags & NCSI_DEV_HWA)
> nca.bytes[0] = 0;
> else
> @@ -546,7 +546,7 @@ static void ncsi_suspend_channel(struct
> ncsi_dev_priv *ndp)
> nd->state = ncsi_dev_state_suspend_deselect;
> } else if (nd->state ==
> ncsi_dev_state_suspend_deselect) {
> nca.type = NCSI_PKT_CMD_DP;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> nd->state = ncsi_dev_state_suspend_done;
> }
>
> @@ -592,7 +592,7 @@ static void ncsi_configure_channel(struct
> ncsi_dev_priv *ndp)
> else
> nca.bytes[0] = 1;
> nca.package = np->id;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> ret = ncsi_xmit_cmd(&nca);
> if (ret)
> goto error;
> @@ -810,7 +810,7 @@ static void ncsi_probe_channel(struct
> ncsi_dev_priv *ndp)
>
> /* Deselect all possible packages */
> nca.type = NCSI_PKT_CMD_DP;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> for (index = 0; index < 8; index++) {
> nca.package = index;
> ret = ncsi_xmit_cmd(&nca);
> @@ -826,7 +826,7 @@ static void ncsi_probe_channel(struct
> ncsi_dev_priv *ndp)
> /* Select all possible packages */
> nca.type = NCSI_PKT_CMD_SP;
> nca.bytes[0] = 1;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> for (index = 0; index < 8; index++) {
> nca.package = index;
> ret = ncsi_xmit_cmd(&nca);
> @@ -879,7 +879,7 @@ static void ncsi_probe_channel(struct
> ncsi_dev_priv *ndp)
> nca.type = NCSI_PKT_CMD_SP;
> nca.bytes[0] = 1;
> nca.package = ndp->active_package->id;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> ret = ncsi_xmit_cmd(&nca);
> if (ret)
> goto error;
> @@ -936,7 +936,7 @@ static void ncsi_probe_channel(struct
> ncsi_dev_priv *ndp)
> /* Deselect the active package */
> nca.type = NCSI_PKT_CMD_DP;
> nca.package = ndp->active_package->id;
> - nca.channel = 0x1f;
> + nca.channel = NCSI_RESERVED_CHANNEL;
> ret = ncsi_xmit_cmd(&nca);
> if (ret)
> goto error;
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4.7 3/7] net/ncsi: Don't probe on the reserved channel ID (0x1f)
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
2016-09-28 2:48 ` [PATCH v4.7 2/7] net/ncsi: Introduce NCSI_RESERVED_CHANNEL Gavin Shan
@ 2016-09-28 2:48 ` Gavin Shan
2016-09-28 5:32 ` Joel Stanley
2016-09-28 2:48 ` [PATCH v4.7 4/7] net/ncsi: Rework request index allocation Gavin Shan
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 2:48 UTC (permalink / raw)
To: openbmc; +Cc: joel, Gavin Shan
We needn't send CIS (Clear Initial State) command to the NCSI
reserved channel (0x1f) in the enumeration. We shouldn't receive
a valid response from CIS on NCSI channel 0x1f.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
net/ncsi/ncsi-manage.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index b03d1df..8011d51 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -887,12 +887,12 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
nd->state = ncsi_dev_state_probe_cis;
break;
case ncsi_dev_state_probe_cis:
- ndp->pending_req_num = 32;
+ ndp->pending_req_num = NCSI_RESERVED_CHANNEL;
/* Clear initial state */
nca.type = NCSI_PKT_CMD_CIS;
nca.package = ndp->active_package->id;
- for (index = 0; index < 0x20; index++) {
+ for (index = 0; index < NCSI_RESERVED_CHANNEL; index++) {
nca.channel = index;
ret = ncsi_xmit_cmd(&nca);
if (ret)
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 3/7] net/ncsi: Don't probe on the reserved channel ID (0x1f)
2016-09-28 2:48 ` [PATCH v4.7 3/7] net/ncsi: Don't probe on the reserved channel ID (0x1f) Gavin Shan
@ 2016-09-28 5:32 ` Joel Stanley
0 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:32 UTC (permalink / raw)
To: Gavin Shan, openbmc
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> We needn't send CIS (Clear Initial State) command to the NCSI
> reserved channel (0x1f) in the enumeration. We shouldn't receive
> a valid response from CIS on NCSI channel 0x1f.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> net/ncsi/ncsi-manage.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index b03d1df..8011d51 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -887,12 +887,12 @@ static void ncsi_probe_channel(struct
> ncsi_dev_priv *ndp)
> nd->state = ncsi_dev_state_probe_cis;
> break;
> case ncsi_dev_state_probe_cis:
> - ndp->pending_req_num = 32;
> + ndp->pending_req_num = NCSI_RESERVED_CHANNEL;
>
> /* Clear initial state */
> nca.type = NCSI_PKT_CMD_CIS;
> nca.package = ndp->active_package->id;
> - for (index = 0; index < 0x20; index++) {
> + for (index = 0; index < NCSI_RESERVED_CHANNEL;
> index++) {
> nca.channel = index;
> ret = ncsi_xmit_cmd(&nca);
> if (ret)
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4.7 4/7] net/ncsi: Rework request index allocation
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
2016-09-28 2:48 ` [PATCH v4.7 2/7] net/ncsi: Introduce NCSI_RESERVED_CHANNEL Gavin Shan
2016-09-28 2:48 ` [PATCH v4.7 3/7] net/ncsi: Don't probe on the reserved channel ID (0x1f) Gavin Shan
@ 2016-09-28 2:48 ` Gavin Shan
2016-09-28 5:34 ` Joel Stanley
2016-09-28 2:48 ` [PATCH v4.7 5/7] net/ncsi: Allow to extend NCSI request properties Gavin Shan
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 2:48 UTC (permalink / raw)
To: openbmc; +Cc: joel, Gavin Shan
This reworks request index allocation for: (A) @ndp->request_id should
represent last allocated request index instead of accumulated request
indexes; (B) The request index (1) is reserved according to section
6.3.1.1 in v1.1.0 NCSI spec.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
net/ncsi/internal.h | 1 +
net/ncsi/ncsi-manage.c | 17 +++++++++--------
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 0a5c580..821f1d3 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -259,6 +259,7 @@ struct ncsi_dev_priv {
struct list_head packages; /* List of packages */
struct ncsi_request requests[256]; /* Request table */
unsigned int request_id; /* Last used request ID */
+#define NCSI_REQ_START_IDX 1
unsigned int pending_req_num; /* Number of pending requests */
struct ncsi_package *active_package; /* Currently handled package */
struct ncsi_channel *active_channel; /* Currently handled channel */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 8011d51..282be06 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -415,30 +415,31 @@ struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp, bool driven)
/* Check if there is one available request until the ceiling */
spin_lock_irqsave(&ndp->lock, flags);
- for (i = ndp->request_id; !nr && i < limit; i++) {
+ for (i = ndp->request_id; i < limit; i++) {
if (ndp->requests[i].used)
continue;
nr = &ndp->requests[i];
nr->used = true;
nr->driven = driven;
- if (++ndp->request_id >= limit)
- ndp->request_id = 0;
+ ndp->request_id = i + 1;
+ goto found;
}
/* Fail back to check from the starting cursor */
- for (i = 0; !nr && i < ndp->request_id; i++) {
+ for (i = NCSI_REQ_START_IDX; i < ndp->request_id; i++) {
if (ndp->requests[i].used)
continue;
nr = &ndp->requests[i];
nr->used = true;
nr->driven = driven;
- if (++ndp->request_id >= limit)
- ndp->request_id = 0;
+ ndp->request_id = i + 1;
+ goto found;
}
- spin_unlock_irqrestore(&ndp->lock, flags);
+found:
+ spin_unlock_irqrestore(&ndp->lock, flags);
return nr;
}
@@ -1121,7 +1122,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
/* Initialize private NCSI device */
spin_lock_init(&ndp->lock);
INIT_LIST_HEAD(&ndp->packages);
- ndp->request_id = 0;
+ ndp->request_id = NCSI_REQ_START_IDX;
for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
ndp->requests[i].id = i;
ndp->requests[i].ndp = ndp;
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 4/7] net/ncsi: Rework request index allocation
2016-09-28 2:48 ` [PATCH v4.7 4/7] net/ncsi: Rework request index allocation Gavin Shan
@ 2016-09-28 5:34 ` Joel Stanley
0 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:34 UTC (permalink / raw)
To: Gavin Shan, openbmc
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> This reworks request index allocation for: (A) @ndp->request_id
> should
> represent last allocated request index instead of accumulated request
> indexes; (B) The request index (1) is reserved according to section
> 6.3.1.1 in v1.1.0 NCSI spec.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
The commit message could be clearer. Code makes sense though.
Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> net/ncsi/internal.h | 1 +
> net/ncsi/ncsi-manage.c | 17 +++++++++--------
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 0a5c580..821f1d3 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -259,6 +259,7 @@ struct ncsi_dev_priv {
> struct list_head packages; /* List of
> packages */
> struct ncsi_request requests[256]; /* Request
> table */
> unsigned int request_id; /* Last used request
> ID */
> +#define NCSI_REQ_START_IDX 1
> unsigned int pending_req_num; /* Number of pending
> requests */
> struct ncsi_package *active_package; /* Currently handled
> package */
> struct ncsi_channel *active_channel; /* Currently handled
> channel */
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 8011d51..282be06 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -415,30 +415,31 @@ struct ncsi_request *ncsi_alloc_request(struct
> ncsi_dev_priv *ndp, bool driven)
>
> /* Check if there is one available request until the ceiling
> */
> spin_lock_irqsave(&ndp->lock, flags);
> - for (i = ndp->request_id; !nr && i < limit; i++) {
> + for (i = ndp->request_id; i < limit; i++) {
> if (ndp->requests[i].used)
> continue;
>
> nr = &ndp->requests[i];
> nr->used = true;
> nr->driven = driven;
> - if (++ndp->request_id >= limit)
> - ndp->request_id = 0;
> + ndp->request_id = i + 1;
> + goto found;
> }
>
> /* Fail back to check from the starting cursor */
> - for (i = 0; !nr && i < ndp->request_id; i++) {
> + for (i = NCSI_REQ_START_IDX; i < ndp->request_id; i++) {
> if (ndp->requests[i].used)
> continue;
>
> nr = &ndp->requests[i];
> nr->used = true;
> nr->driven = driven;
> - if (++ndp->request_id >= limit)
> - ndp->request_id = 0;
> + ndp->request_id = i + 1;
> + goto found;
> }
> - spin_unlock_irqrestore(&ndp->lock, flags);
>
> +found:
> + spin_unlock_irqrestore(&ndp->lock, flags);
> return nr;
> }
>
> @@ -1121,7 +1122,7 @@ struct ncsi_dev *ncsi_register_dev(struct
> net_device *dev,
> /* Initialize private NCSI device */
> spin_lock_init(&ndp->lock);
> INIT_LIST_HEAD(&ndp->packages);
> - ndp->request_id = 0;
> + ndp->request_id = NCSI_REQ_START_IDX;
> for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
> ndp->requests[i].id = i;
> ndp->requests[i].ndp = ndp;
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4.7 5/7] net/ncsi: Allow to extend NCSI request properties
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
` (2 preceding siblings ...)
2016-09-28 2:48 ` [PATCH v4.7 4/7] net/ncsi: Rework request index allocation Gavin Shan
@ 2016-09-28 2:48 ` Gavin Shan
2016-09-28 5:40 ` Joel Stanley
2016-09-28 2:48 ` [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring Gavin Shan
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 2:48 UTC (permalink / raw)
To: openbmc; +Cc: joel, Gavin Shan
There is only one NCSI request property for now: the response for
the sent command need drive the workqueue or not. So we had one
field (@driven) for the purpose. We lost the flexibility to extend
NCSI request properties.
This replaces @driven with @flags and @req_flags in NCSI request
and NCSI command argument struct. Each bit of the newly introduced
field can be used for one property. No functional changes introduced.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
net/ncsi/internal.h | 8 +++++---
net/ncsi/ncsi-cmd.c | 2 +-
net/ncsi/ncsi-manage.c | 19 ++++++++++---------
net/ncsi/ncsi-rsp.c | 2 +-
4 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 821f1d3..a2e3af9 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -207,7 +207,8 @@ struct ncsi_package {
struct ncsi_request {
unsigned char id; /* Request ID - 0 to 255 */
bool used; /* Request that has been assigned */
- bool driven; /* Drive state machine */
+ unsigned int flags; /* NCSI request property */
+#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
struct ncsi_dev_priv *ndp; /* Associated NCSI device */
struct sk_buff *cmd; /* Associated NCSI command packet */
struct sk_buff *rsp; /* Associated NCSI response packet */
@@ -276,7 +277,7 @@ struct ncsi_cmd_arg {
unsigned char package; /* Destination package ID */
unsigned char channel; /* Detination channel ID or 0x1f */
unsigned short payload; /* Command packet payload length */
- bool driven; /* Drive the state machine? */
+ unsigned int req_flags; /* NCSI request properties */
union {
unsigned char bytes[16]; /* Command packet specific data */
unsigned short words[8];
@@ -315,7 +316,8 @@ void ncsi_find_package_and_channel(struct ncsi_dev_priv *ndp,
unsigned char id,
struct ncsi_package **np,
struct ncsi_channel **nc);
-struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp, bool driven);
+struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp,
+ unsigned int req_flags);
void ncsi_free_request(struct ncsi_request *nr);
struct ncsi_dev *ncsi_find_dev(struct net_device *dev);
int ncsi_process_next_channel(struct ncsi_dev_priv *ndp);
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 21057a8..db7083b 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -272,7 +272,7 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
struct sk_buff *skb;
struct ncsi_request *nr;
- nr = ncsi_alloc_request(ndp, nca->driven);
+ nr = ncsi_alloc_request(ndp, nca->req_flags);
if (!nr)
return NULL;
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 282be06..3019432 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -186,7 +186,7 @@ static void ncsi_channel_monitor(unsigned long data)
nca.package = np->id;
nca.channel = nc->id;
nca.type = NCSI_PKT_CMD_GLS;
- nca.driven = false;
+ nca.req_flags = 0;
ret = ncsi_xmit_cmd(&nca);
if (ret) {
netdev_err(ndp->ndev.dev, "Error %d sending GLS\n",
@@ -407,7 +407,8 @@ void ncsi_find_package_and_channel(struct ncsi_dev_priv *ndp,
* be same. Otherwise, the bogus response might be replied. So
* the available IDs are allocated in round-robin fashion.
*/
-struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp, bool driven)
+struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp,
+ unsigned int req_flags)
{
struct ncsi_request *nr = NULL;
int i, limit = ARRAY_SIZE(ndp->requests);
@@ -421,7 +422,7 @@ struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp, bool driven)
nr = &ndp->requests[i];
nr->used = true;
- nr->driven = driven;
+ nr->flags = req_flags;
ndp->request_id = i + 1;
goto found;
}
@@ -433,7 +434,7 @@ struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp, bool driven)
nr = &ndp->requests[i];
nr->used = true;
- nr->driven = driven;
+ nr->flags = req_flags;
ndp->request_id = i + 1;
goto found;
}
@@ -461,7 +462,7 @@ void ncsi_free_request(struct ncsi_request *nr)
nr->cmd = NULL;
nr->rsp = NULL;
nr->used = false;
- driven = nr->driven;
+ driven = !!(nr->flags & NCSI_REQ_FLAG_EVENT_DRIVEN);
spin_unlock_irqrestore(&ndp->lock, flags);
if (driven && cmd && --ndp->pending_req_num == 0)
@@ -514,7 +515,7 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
int ret;
nca.ndp = ndp;
- nca.driven = true;
+ nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
switch (nd->state) {
case ncsi_dev_state_suspend:
nd->state = ncsi_dev_state_suspend_select;
@@ -580,7 +581,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
int ret;
nca.ndp = ndp;
- nca.driven = true;
+ nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
switch (nd->state) {
case ncsi_dev_state_config:
case ncsi_dev_state_config_sp:
@@ -801,7 +802,7 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
int ret;
nca.ndp = ndp;
- nca.driven = true;
+ nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
switch (nd->state) {
case ncsi_dev_state_probe:
nd->state = ncsi_dev_state_probe_deselect;
@@ -1074,7 +1075,7 @@ static int ncsi_inet6addr_event(struct notifier_block *this,
return NOTIFY_OK;
nca.ndp = ndp;
- nca.driven = false;
+ nca.req_flags = 0;
nca.package = np->id;
nca.channel = nc->id;
nca.dwords[0] = nc->caps[NCSI_CAP_MC].cap;
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 54f7eed..1429e8b 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -317,7 +317,7 @@ static int ncsi_rsp_handler_gls(struct ncsi_request *nr)
ncm->data[3] = ntohl(rsp->other);
ncm->data[4] = ntohl(rsp->oem_status);
- if (nr->driven)
+ if (nr->flags & NCSI_REQ_FLAG_EVENT_DRIVEN)
return 0;
/* Reset the channel monitor if it has been enabled */
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 5/7] net/ncsi: Allow to extend NCSI request properties
2016-09-28 2:48 ` [PATCH v4.7 5/7] net/ncsi: Allow to extend NCSI request properties Gavin Shan
@ 2016-09-28 5:40 ` Joel Stanley
0 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:40 UTC (permalink / raw)
To: Gavin Shan, openbmc
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> There is only one NCSI request property for now: the response for
> the sent command need drive the workqueue or not. So we had one
> field (@driven) for the purpose. We lost the flexibility to extend
> NCSI request properties.
>
> This replaces @driven with @flags and @req_flags in NCSI request
> and NCSI command argument struct. Each bit of the newly introduced
> field can be used for one property. No functional changes introduced.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> net/ncsi/internal.h | 8 +++++---
> net/ncsi/ncsi-cmd.c | 2 +-
> net/ncsi/ncsi-manage.c | 19 ++++++++++---------
> net/ncsi/ncsi-rsp.c | 2 +-
> 4 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 821f1d3..a2e3af9 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -207,7 +207,8 @@ struct ncsi_package {
> struct ncsi_request {
> unsigned char id; /* Request ID - 0 to
> 255 */
> bool used; /* Request that has been
> assigned */
> - bool driven; /* Drive state
> machine */
> + unsigned int flags; /* NCSI request
> property */
> +#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> struct ncsi_dev_priv *ndp; /* Associated NCSI
> device */
> struct sk_buff *cmd; /* Associated NCSI command
> packet */
> struct sk_buff *rsp; /* Associated NCSI response
> packet */
> @@ -276,7 +277,7 @@ struct ncsi_cmd_arg {
> unsigned char package; /* Destination package
> ID */
> unsigned char channel; /* Detination channel ID
> or 0x1f */
> unsigned short payload; /* Command packet payload
> length */
> - bool driven; /* Drive the state
> machine? */
> + unsigned int req_flags; /* NCSI request
> properties */
> union {
> unsigned char bytes[16]; /* Command packet specific
> data */
> unsigned short words[8];
> @@ -315,7 +316,8 @@ void ncsi_find_package_and_channel(struct
> ncsi_dev_priv *ndp,
> unsigned char id,
> struct ncsi_package **np,
> struct ncsi_channel **nc);
> -struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp,
> bool driven);
> +struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp,
> + unsigned int req_flags);
> void ncsi_free_request(struct ncsi_request *nr);
> struct ncsi_dev *ncsi_find_dev(struct net_device *dev);
> int ncsi_process_next_channel(struct ncsi_dev_priv *ndp);
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 21057a8..db7083b 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -272,7 +272,7 @@ static struct ncsi_request
> *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
> struct sk_buff *skb;
> struct ncsi_request *nr;
>
> - nr = ncsi_alloc_request(ndp, nca->driven);
> + nr = ncsi_alloc_request(ndp, nca->req_flags);
> if (!nr)
> return NULL;
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 282be06..3019432 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -186,7 +186,7 @@ static void ncsi_channel_monitor(unsigned long
> data)
> nca.package = np->id;
> nca.channel = nc->id;
> nca.type = NCSI_PKT_CMD_GLS;
> - nca.driven = false;
> + nca.req_flags = 0;
> ret = ncsi_xmit_cmd(&nca);
> if (ret) {
> netdev_err(ndp->ndev.dev, "Error %d sending
> GLS\n",
> @@ -407,7 +407,8 @@ void ncsi_find_package_and_channel(struct
> ncsi_dev_priv *ndp,
> * be same. Otherwise, the bogus response might be replied. So
> * the available IDs are allocated in round-robin fashion.
> */
> -struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp,
> bool driven)
> +struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp,
> + unsigned int req_flags)
> {
> struct ncsi_request *nr = NULL;
> int i, limit = ARRAY_SIZE(ndp->requests);
> @@ -421,7 +422,7 @@ struct ncsi_request *ncsi_alloc_request(struct
> ncsi_dev_priv *ndp, bool driven)
>
> nr = &ndp->requests[i];
> nr->used = true;
> - nr->driven = driven;
> + nr->flags = req_flags;
> ndp->request_id = i + 1;
> goto found;
> }
> @@ -433,7 +434,7 @@ struct ncsi_request *ncsi_alloc_request(struct
> ncsi_dev_priv *ndp, bool driven)
>
> nr = &ndp->requests[i];
> nr->used = true;
> - nr->driven = driven;
> + nr->flags = req_flags;
> ndp->request_id = i + 1;
> goto found;
> }
> @@ -461,7 +462,7 @@ void ncsi_free_request(struct ncsi_request *nr)
> nr->cmd = NULL;
> nr->rsp = NULL;
> nr->used = false;
> - driven = nr->driven;
> + driven = !!(nr->flags & NCSI_REQ_FLAG_EVENT_DRIVEN);
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> if (driven && cmd && --ndp->pending_req_num == 0)
> @@ -514,7 +515,7 @@ static void ncsi_suspend_channel(struct
> ncsi_dev_priv *ndp)
> int ret;
>
> nca.ndp = ndp;
> - nca.driven = true;
> + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> case ncsi_dev_state_suspend:
> nd->state = ncsi_dev_state_suspend_select;
> @@ -580,7 +581,7 @@ static void ncsi_configure_channel(struct
> ncsi_dev_priv *ndp)
> int ret;
>
> nca.ndp = ndp;
> - nca.driven = true;
> + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> case ncsi_dev_state_config:
> case ncsi_dev_state_config_sp:
> @@ -801,7 +802,7 @@ static void ncsi_probe_channel(struct
> ncsi_dev_priv *ndp)
> int ret;
>
> nca.ndp = ndp;
> - nca.driven = true;
> + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> case ncsi_dev_state_probe:
> nd->state = ncsi_dev_state_probe_deselect;
> @@ -1074,7 +1075,7 @@ static int ncsi_inet6addr_event(struct
> notifier_block *this,
> return NOTIFY_OK;
>
> nca.ndp = ndp;
> - nca.driven = false;
> + nca.req_flags = 0;
> nca.package = np->id;
> nca.channel = nc->id;
> nca.dwords[0] = nc->caps[NCSI_CAP_MC].cap;
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 54f7eed..1429e8b 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -317,7 +317,7 @@ static int ncsi_rsp_handler_gls(struct
> ncsi_request *nr)
> ncm->data[3] = ntohl(rsp->other);
> ncm->data[4] = ntohl(rsp->oem_status);
>
> - if (nr->driven)
> + if (nr->flags & NCSI_REQ_FLAG_EVENT_DRIVEN)
> return 0;
>
> /* Reset the channel monitor if it has been enabled */
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
` (3 preceding siblings ...)
2016-09-28 2:48 ` [PATCH v4.7 5/7] net/ncsi: Allow to extend NCSI request properties Gavin Shan
@ 2016-09-28 2:48 ` Gavin Shan
2016-09-28 5:50 ` Joel Stanley
2016-09-28 2:48 ` [PATCH v4.7 7/7] net/ncsi: Introduce ncsi_stop_dev() Gavin Shan
2016-09-28 5:25 ` [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Joel Stanley
6 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 2:48 UTC (permalink / raw)
To: openbmc; +Cc: joel, Gavin Shan
The original NCSI channel monitoring was implemented based on a
backoff algorithm: the GLS response should be received in the
specified interval. Otherwise, the channel is regarded as dead
and failover should be taken if current channel is an active one.
There are several problems in the implementation: (A) On BCM5718,
we found when the IID (Instance ID) in the GLS command packet
changes from 255 to 1, the response corresponding to IID#1 never
comes in. It means we cannot make the unfair judgement that the
channel is dead when one response is missed. (B) The code's
readability should be improved. (C) We should do failover when
current channel is active one and the channel monitoring should
be marked as disabled before doing failover.
This reworks the channel monitoring to address all above issues.
The fields for channel monitoring is put into separate struct
and the state of channel monitoring is predefined. The channel
is regarded alive if the network controller responses to one of
two GLS commands or both of them in 5 seconds.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
net/ncsi/internal.h | 12 +++++++++---
net/ncsi/ncsi-manage.c | 46 ++++++++++++++++++++++++++--------------------
net/ncsi/ncsi-rsp.c | 2 +-
3 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index a2e3af9..d9b3705 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -187,9 +187,15 @@ struct ncsi_channel {
struct ncsi_channel_mode modes[NCSI_MODE_MAX];
struct ncsi_channel_filter *filters[NCSI_FILTER_MAX];
struct ncsi_channel_stats stats;
- struct timer_list timer; /* Link monitor timer */
- bool enabled; /* Timer is enabled */
- unsigned int timeout; /* Times of timeout */
+ struct {
+ struct timer_list timer; /* Link monitor timer */
+ bool enabled; /* Timer is enabled */
+ unsigned int state; /* Link monitor state */
+#define NCSI_CHANNEL_MONITOR_START 0
+#define NCSI_CHANNEL_MONITOR_RETRY 1
+#define NCSI_CHANNEL_MONITOR_WAIT 2
+#define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
+ } monitor;
struct list_head node;
struct list_head link;
};
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 3019432..5813ebe 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -165,13 +165,13 @@ static void ncsi_channel_monitor(unsigned long data)
struct ncsi_dev_priv *ndp = np->ndp;
struct ncsi_cmd_arg nca;
bool enabled;
- unsigned int timeout;
+ unsigned int monitor_state;
unsigned long flags;
int state, ret;
spin_lock_irqsave(&nc->lock, flags);
- timeout = nc->timeout;
- enabled = nc->enabled;
+ monitor_state = nc->monitor.state;
+ enabled = nc->monitor.enabled;
spin_unlock_irqrestore(&nc->lock, flags);
if (!enabled || !list_empty(&nc->link))
@@ -181,7 +181,9 @@ static void ncsi_channel_monitor(unsigned long data)
if (state != NCSI_CHANNEL_INACTIVE && state != NCSI_CHANNEL_ACTIVE)
return;
- if (!(timeout % 2)) {
+ switch (monitor_state) {
+ case NCSI_CHANNEL_MONITOR_START:
+ case NCSI_CHANNEL_MONITOR_RETRY:
nca.ndp = ndp;
nca.package = np->id;
nca.channel = nc->id;
@@ -193,15 +195,19 @@ static void ncsi_channel_monitor(unsigned long data)
ret);
return;
}
- }
- if (timeout + 1 >= 3) {
+ break;
+ case NCSI_CHANNEL_MONITOR_WAIT ... NCSI_CHANNEL_MONITOR_WAIT_MAX:
+ break;
+ default:
if (!(ndp->flags & NCSI_DEV_HWA) &&
- state == NCSI_CHANNEL_ACTIVE)
+ state == NCSI_CHANNEL_ACTIVE) {
ncsi_report_link(ndp, true);
+ ndp->flags |= NCSI_DEV_RESHUFFLE;
+ }
spin_lock_irqsave(&ndp->lock, flags);
- atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
+ atomic_set(&nc->state, NCSI_CHANNEL_ACTIVE);
list_add_tail_rcu(&nc->link, &ndp->channel_queue);
spin_unlock_irqrestore(&ndp->lock, flags);
ncsi_process_next_channel(ndp);
@@ -209,10 +215,9 @@ static void ncsi_channel_monitor(unsigned long data)
}
spin_lock_irqsave(&nc->lock, flags);
- nc->timeout = timeout + 1;
- nc->enabled = true;
+ nc->monitor.state++;
spin_unlock_irqrestore(&nc->lock, flags);
- mod_timer(&nc->timer, jiffies + HZ * (1 << (nc->timeout / 2)));
+ mod_timer(&nc->monitor.timer, jiffies + HZ);
}
void ncsi_start_channel_monitor(struct ncsi_channel *nc)
@@ -220,12 +225,12 @@ void ncsi_start_channel_monitor(struct ncsi_channel *nc)
unsigned long flags;
spin_lock_irqsave(&nc->lock, flags);
- WARN_ON_ONCE(nc->enabled);
- nc->timeout = 0;
- nc->enabled = true;
+ WARN_ON_ONCE(nc->monitor.enabled);
+ nc->monitor.enabled = true;
+ nc->monitor.state = NCSI_CHANNEL_MONITOR_START;
spin_unlock_irqrestore(&nc->lock, flags);
- mod_timer(&nc->timer, jiffies + HZ * (1 << (nc->timeout / 2)));
+ mod_timer(&nc->monitor.timer, jiffies + HZ);
}
void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
@@ -233,14 +238,14 @@ void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
unsigned long flags;
spin_lock_irqsave(&nc->lock, flags);
- if (!nc->enabled) {
+ if (!nc->monitor.enabled) {
spin_unlock_irqrestore(&nc->lock, flags);
return;
}
- nc->enabled = false;
+ nc->monitor.enabled = false;
spin_unlock_irqrestore(&nc->lock, flags);
- del_timer_sync(&nc->timer);
+ del_timer_sync(&nc->monitor.timer);
}
struct ncsi_channel *ncsi_find_channel(struct ncsi_package *np,
@@ -269,8 +274,9 @@ struct ncsi_channel *ncsi_add_channel(struct ncsi_package *np, unsigned char id)
nc->id = id;
nc->package = np;
atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
- nc->enabled = false;
- setup_timer(&nc->timer, ncsi_channel_monitor, (unsigned long)nc);
+ nc->monitor.enabled = false;
+ setup_timer(&nc->monitor.timer,
+ ncsi_channel_monitor, (unsigned long)nc);
spin_lock_init(&nc->lock);
INIT_LIST_HEAD(&nc->link);
for (index = 0; index < NCSI_CAP_MAX; index++)
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 1429e8b..eec88c7 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -322,7 +322,7 @@ static int ncsi_rsp_handler_gls(struct ncsi_request *nr)
/* Reset the channel monitor if it has been enabled */
spin_lock_irqsave(&nc->lock, flags);
- nc->timeout = 0;
+ nc->monitor.state = NCSI_CHANNEL_MONITOR_START;
spin_unlock_irqrestore(&nc->lock, flags);
return 0;
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring
2016-09-28 2:48 ` [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring Gavin Shan
@ 2016-09-28 5:50 ` Joel Stanley
2016-09-28 7:18 ` Gavin Shan
0 siblings, 1 reply; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:50 UTC (permalink / raw)
To: Gavin Shan, openbmc
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> The original NCSI channel monitoring was implemented based on a
> backoff algorithm: the GLS response should be received in the
> specified interval. Otherwise, the channel is regarded as dead
> and failover should be taken if current channel is an active one.
> There are several problems in the implementation: (A) On BCM5718,
> we found when the IID (Instance ID) in the GLS command packet
> changes from 255 to 1, the response corresponding to IID#1 never
> comes in. It means we cannot make the unfair judgement that the
> channel is dead when one response is missed. (B) The code's
> readability should be improved. (C) We should do failover when
> current channel is active one and the channel monitoring should
> be marked as disabled before doing failover.
>
> This reworks the channel monitoring to address all above issues.
> The fields for channel monitoring is put into separate struct
> and the state of channel monitoring is predefined. The channel
> is regarded alive if the network controller responses to one of
> two GLS commands or both of them in 5 seconds.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
> net/ncsi/internal.h | 12 +++++++++---
> net/ncsi/ncsi-manage.c | 46 ++++++++++++++++++++++++++------------
> --------
> net/ncsi/ncsi-rsp.c | 2 +-
> 3 files changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index a2e3af9..d9b3705 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -187,9 +187,15 @@ struct ncsi_channel {
> struct ncsi_channel_mode modes[NCSI_MODE_MAX];
> struct ncsi_channel_filter *filters[NCSI_FILTER_MAX];
> struct ncsi_channel_stats stats;
> - struct timer_list timer; /* Link monitor
> timer */
> - bool enabled; /* Timer is
> enabled */
> - unsigned int timeout; /* Times of
> timeout */
> + struct {
> + struct timer_list timer; /* Link monitor
> timer */
> + bool enabled; /* Timer is
> enabled */
> + unsigned int state; /* Link monitor
> state */
These comments don't add much value.
> +#define NCSI_CHANNEL_MONITOR_START 0
> +#define NCSI_CHANNEL_MONITOR_RETRY 1
> +#define NCSI_CHANNEL_MONITOR_WAIT 2
> +#define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
> + } monitor;
> struct list_head node;
> struct list_head link;
> };
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 3019432..5813ebe 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -165,13 +165,13 @@ static void ncsi_channel_monitor(unsigned long
> data)
> struct ncsi_dev_priv *ndp = np->ndp;
> struct ncsi_cmd_arg nca;
> bool enabled;
> - unsigned int timeout;
> + unsigned int monitor_state;
> unsigned long flags;
> int state, ret;
>
> spin_lock_irqsave(&nc->lock, flags);
> - timeout = nc->timeout;
> - enabled = nc->enabled;
> + monitor_state = nc->monitor.state;
> + enabled = nc->monitor.enabled;
> spin_unlock_irqrestore(&nc->lock, flags);
>
> if (!enabled || !list_empty(&nc->link))
> @@ -181,7 +181,9 @@ static void ncsi_channel_monitor(unsigned long
> data)
> if (state != NCSI_CHANNEL_INACTIVE && state !=
> NCSI_CHANNEL_ACTIVE)
> return;
>
> - if (!(timeout % 2)) {
> + switch (monitor_state) {
> + case NCSI_CHANNEL_MONITOR_START:
> + case NCSI_CHANNEL_MONITOR_RETRY:
> nca.ndp = ndp;
> nca.package = np->id;
> nca.channel = nc->id;
> @@ -193,15 +195,19 @@ static void ncsi_channel_monitor(unsigned long
> data)
> ret);
> return;
> }
> - }
>
> - if (timeout + 1 >= 3) {
> + break;
> + case NCSI_CHANNEL_MONITOR_WAIT ...
> NCSI_CHANNEL_MONITOR_WAIT_MAX:
It's not clear why you have the MAX_WAIT state if it's doing the same
thing as WAIT.
> + break;
> + default:
> if (!(ndp->flags & NCSI_DEV_HWA) &&
> - state == NCSI_CHANNEL_ACTIVE)
> + state == NCSI_CHANNEL_ACTIVE) {
> ncsi_report_link(ndp, true);
> + ndp->flags |= NCSI_DEV_RESHUFFLE;
> + }
>
> spin_lock_irqsave(&ndp->lock, flags);
> - atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> + atomic_set(&nc->state, NCSI_CHANNEL_ACTIVE);
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
> ncsi_process_next_channel(ndp);
> @@ -209,10 +215,9 @@ static void ncsi_channel_monitor(unsigned long
> data)
> }
>
> spin_lock_irqsave(&nc->lock, flags);
> - nc->timeout = timeout + 1;
> - nc->enabled = true;
> + nc->monitor.state++;
> spin_unlock_irqrestore(&nc->lock, flags);
> - mod_timer(&nc->timer, jiffies + HZ * (1 << (nc->timeout /
> 2)));
> + mod_timer(&nc->monitor.timer, jiffies + HZ);
> }
>
> void ncsi_start_channel_monitor(struct ncsi_channel *nc)
> @@ -220,12 +225,12 @@ void ncsi_start_channel_monitor(struct
> ncsi_channel *nc)
> unsigned long flags;
>
> spin_lock_irqsave(&nc->lock, flags);
> - WARN_ON_ONCE(nc->enabled);
> - nc->timeout = 0;
> - nc->enabled = true;
> + WARN_ON_ONCE(nc->monitor.enabled);
> + nc->monitor.enabled = true;
> + nc->monitor.state = NCSI_CHANNEL_MONITOR_START;
> spin_unlock_irqrestore(&nc->lock, flags);
>
> - mod_timer(&nc->timer, jiffies + HZ * (1 << (nc->timeout /
> 2)));
> + mod_timer(&nc->monitor.timer, jiffies + HZ);
> }
>
> void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
> @@ -233,14 +238,14 @@ void ncsi_stop_channel_monitor(struct
> ncsi_channel *nc)
> unsigned long flags;
>
> spin_lock_irqsave(&nc->lock, flags);
> - if (!nc->enabled) {
> + if (!nc->monitor.enabled) {
> spin_unlock_irqrestore(&nc->lock, flags);
> return;
> }
> - nc->enabled = false;
> + nc->monitor.enabled = false;
> spin_unlock_irqrestore(&nc->lock, flags);
>
> - del_timer_sync(&nc->timer);
> + del_timer_sync(&nc->monitor.timer);
> }
>
> struct ncsi_channel *ncsi_find_channel(struct ncsi_package *np,
> @@ -269,8 +274,9 @@ struct ncsi_channel *ncsi_add_channel(struct
> ncsi_package *np, unsigned char id)
> nc->id = id;
> nc->package = np;
> atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
> - nc->enabled = false;
> - setup_timer(&nc->timer, ncsi_channel_monitor, (unsigned
> long)nc);
> + nc->monitor.enabled = false;
> + setup_timer(&nc->monitor.timer,
> + ncsi_channel_monitor, (unsigned long)nc);
> spin_lock_init(&nc->lock);
> INIT_LIST_HEAD(&nc->link);
> for (index = 0; index < NCSI_CAP_MAX; index++)
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 1429e8b..eec88c7 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -322,7 +322,7 @@ static int ncsi_rsp_handler_gls(struct
> ncsi_request *nr)
>
> /* Reset the channel monitor if it has been enabled */
> spin_lock_irqsave(&nc->lock, flags);
> - nc->timeout = 0;
> + nc->monitor.state = NCSI_CHANNEL_MONITOR_START;
> spin_unlock_irqrestore(&nc->lock, flags);
>
> return 0;
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring
2016-09-28 5:50 ` Joel Stanley
@ 2016-09-28 7:18 ` Gavin Shan
2016-09-29 2:14 ` Joel Stanley
0 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 7:18 UTC (permalink / raw)
To: Joel Stanley; +Cc: Gavin Shan, openbmc
On Wed, Sep 28, 2016 at 03:20:32PM +0930, Joel Stanley wrote:
>On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
>> The original NCSI channel monitoring was implemented based on a
>> backoff algorithm: the GLS response should be received in the
>> specified interval. Otherwise, the channel is regarded as dead
>> and failover should be taken if current channel is an active one.
>> There are several problems in the implementation: (A) On BCM5718,
>> we found when the IID (Instance ID) in the GLS command packet
>> changes from 255 to 1, the response corresponding to IID#1 never
>> comes in. It means we cannot make the unfair judgement that the
>> channel is dead when one response is missed. (B) The code's
>> readability should be improved. (C) We should do failover when
>> current channel is active one and the channel monitoring should
>> be marked as disabled before doing failover.
>>
>> This reworks the channel monitoring to address all above issues.
>> The fields for channel monitoring is put into separate struct
>> and the state of channel monitoring is predefined. The channel
>> is regarded alive if the network controller responses to one of
>> two GLS commands or both of them in 5 seconds.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>> net/ncsi/internal.h | 12 +++++++++---
>> net/ncsi/ncsi-manage.c | 46 ++++++++++++++++++++++++++------------
>> --------
>> net/ncsi/ncsi-rsp.c | 2 +-
>> 3 files changed, 36 insertions(+), 24 deletions(-)
>>
>> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
>> index a2e3af9..d9b3705 100644
>> --- a/net/ncsi/internal.h
>> +++ b/net/ncsi/internal.h
>> @@ -187,9 +187,15 @@ struct ncsi_channel {
>> struct ncsi_channel_mode modes[NCSI_MODE_MAX];
>> struct ncsi_channel_filter *filters[NCSI_FILTER_MAX];
>> struct ncsi_channel_stats stats;
>> - struct timer_list timer; /* Link monitor
>> timer */
>> - bool enabled; /* Timer is
>> enabled */
>> - unsigned int timeout; /* Times of
>> timeout */
>> + struct {
>> + struct timer_list timer; /* Link monitor
>> timer */
>> + bool enabled; /* Timer is
>> enabled */
>> + unsigned int state; /* Link monitor
>> state */
>
>These comments don't add much value.
>
Joel, thanks for your comments. I agree and will drop those comments
in next revision.
>> +#define NCSI_CHANNEL_MONITOR_START 0
>> +#define NCSI_CHANNEL_MONITOR_RETRY 1
>> +#define NCSI_CHANNEL_MONITOR_WAIT 2
>> +#define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
>> + } monitor;
>> struct list_head node;
>> struct list_head link;
>> };
>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>> index 3019432..5813ebe 100644
>> --- a/net/ncsi/ncsi-manage.c
>> +++ b/net/ncsi/ncsi-manage.c
>> @@ -165,13 +165,13 @@ static void ncsi_channel_monitor(unsigned long
>> data)
>> struct ncsi_dev_priv *ndp = np->ndp;
>> struct ncsi_cmd_arg nca;
>> bool enabled;
>> - unsigned int timeout;
>> + unsigned int monitor_state;
>> unsigned long flags;
>> int state, ret;
>>
>> spin_lock_irqsave(&nc->lock, flags);
>> - timeout = nc->timeout;
>> - enabled = nc->enabled;
>> + monitor_state = nc->monitor.state;
>> + enabled = nc->monitor.enabled;
>> spin_unlock_irqrestore(&nc->lock, flags);
>>
>> if (!enabled || !list_empty(&nc->link))
>> @@ -181,7 +181,9 @@ static void ncsi_channel_monitor(unsigned long
>> data)
>> if (state != NCSI_CHANNEL_INACTIVE && state !=
>> NCSI_CHANNEL_ACTIVE)
>> return;
>>
>> - if (!(timeout % 2)) {
>> + switch (monitor_state) {
>> + case NCSI_CHANNEL_MONITOR_START:
>> + case NCSI_CHANNEL_MONITOR_RETRY:
>> nca.ndp = ndp;
>> nca.package = np->id;
>> nca.channel = nc->id;
>> @@ -193,15 +195,19 @@ static void ncsi_channel_monitor(unsigned long
>> data)
>> ret);
>> return;
>> }
>> - }
>>
>> - if (timeout + 1 >= 3) {
>> + break;
>> + case NCSI_CHANNEL_MONITOR_WAIT ...
>> NCSI_CHANNEL_MONITOR_WAIT_MAX:
>
>It's not clear why you have the MAX_WAIT state if it's doing the same
>thing as WAIT.
>
@nc->monitor.state tracks two types of information: (A) the current state
of the channel monitor; (B) the time since last GLS reponse was received
successfully. The general idea behind is: we have two GLS commands sent
and at least one response should be received in 5 seconds, which is
represented by NCSI_CHANNEL_MONITOR_WAIT_MAX.
>> + break;
>> + default:
>> if (!(ndp->flags & NCSI_DEV_HWA) &&
>> - state == NCSI_CHANNEL_ACTIVE)
>> + state == NCSI_CHANNEL_ACTIVE) {
>> ncsi_report_link(ndp, true);
>> + ndp->flags |= NCSI_DEV_RESHUFFLE;
>> + }
>>
>> spin_lock_irqsave(&ndp->lock, flags);
>> - atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
>> + atomic_set(&nc->state, NCSI_CHANNEL_ACTIVE);
>> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
>> spin_unlock_irqrestore(&ndp->lock, flags);
>> ncsi_process_next_channel(ndp);
>> @@ -209,10 +215,9 @@ static void ncsi_channel_monitor(unsigned long
>> data)
>> }
>>
>> spin_lock_irqsave(&nc->lock, flags);
>> - nc->timeout = timeout + 1;
>> - nc->enabled = true;
>> + nc->monitor.state++;
>> spin_unlock_irqrestore(&nc->lock, flags);
>> - mod_timer(&nc->timer, jiffies + HZ * (1 << (nc->timeout /
>> 2)));
>> + mod_timer(&nc->monitor.timer, jiffies + HZ);
>> }
>>
>> void ncsi_start_channel_monitor(struct ncsi_channel *nc)
>> @@ -220,12 +225,12 @@ void ncsi_start_channel_monitor(struct
>> ncsi_channel *nc)
>> unsigned long flags;
>>
>> spin_lock_irqsave(&nc->lock, flags);
>> - WARN_ON_ONCE(nc->enabled);
>> - nc->timeout = 0;
>> - nc->enabled = true;
>> + WARN_ON_ONCE(nc->monitor.enabled);
>> + nc->monitor.enabled = true;
>> + nc->monitor.state = NCSI_CHANNEL_MONITOR_START;
>> spin_unlock_irqrestore(&nc->lock, flags);
>>
>> - mod_timer(&nc->timer, jiffies + HZ * (1 << (nc->timeout /
>> 2)));
>> + mod_timer(&nc->monitor.timer, jiffies + HZ);
>> }
>>
>> void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
>> @@ -233,14 +238,14 @@ void ncsi_stop_channel_monitor(struct
>> ncsi_channel *nc)
>> unsigned long flags;
>>
>> spin_lock_irqsave(&nc->lock, flags);
>> - if (!nc->enabled) {
>> + if (!nc->monitor.enabled) {
>> spin_unlock_irqrestore(&nc->lock, flags);
>> return;
>> }
>> - nc->enabled = false;
>> + nc->monitor.enabled = false;
>> spin_unlock_irqrestore(&nc->lock, flags);
>>
>> - del_timer_sync(&nc->timer);
>> + del_timer_sync(&nc->monitor.timer);
>> }
>>
>> struct ncsi_channel *ncsi_find_channel(struct ncsi_package *np,
>> @@ -269,8 +274,9 @@ struct ncsi_channel *ncsi_add_channel(struct
>> ncsi_package *np, unsigned char id)
>> nc->id = id;
>> nc->package = np;
>> atomic_set(&nc->state, NCSI_CHANNEL_INACTIVE);
>> - nc->enabled = false;
>> - setup_timer(&nc->timer, ncsi_channel_monitor, (unsigned
>> long)nc);
>> + nc->monitor.enabled = false;
>> + setup_timer(&nc->monitor.timer,
>> + ncsi_channel_monitor, (unsigned long)nc);
>> spin_lock_init(&nc->lock);
>> INIT_LIST_HEAD(&nc->link);
>> for (index = 0; index < NCSI_CAP_MAX; index++)
>> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
>> index 1429e8b..eec88c7 100644
>> --- a/net/ncsi/ncsi-rsp.c
>> +++ b/net/ncsi/ncsi-rsp.c
>> @@ -322,7 +322,7 @@ static int ncsi_rsp_handler_gls(struct
>> ncsi_request *nr)
>>
>> /* Reset the channel monitor if it has been enabled */
>> spin_lock_irqsave(&nc->lock, flags);
>> - nc->timeout = 0;
>> + nc->monitor.state = NCSI_CHANNEL_MONITOR_START;
>> spin_unlock_irqrestore(&nc->lock, flags);
>>
>> return 0;
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring
2016-09-28 7:18 ` Gavin Shan
@ 2016-09-29 2:14 ` Joel Stanley
2016-09-29 2:42 ` Gavin Shan
0 siblings, 1 reply; 20+ messages in thread
From: Joel Stanley @ 2016-09-29 2:14 UTC (permalink / raw)
To: Gavin Shan; +Cc: OpenBMC Maillist
On Wed, Sep 28, 2016 at 4:48 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> On Wed, Sep 28, 2016 at 03:20:32PM +0930, Joel Stanley wrote:
>>> +#define NCSI_CHANNEL_MONITOR_START 0
>>> +#define NCSI_CHANNEL_MONITOR_RETRY 1
>>> +#define NCSI_CHANNEL_MONITOR_WAIT 2
>>> +#define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
>>> + } monitor;
>>> struct list_head node;
>>> struct list_head link;
>>> };
>>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>>> index 3019432..5813ebe 100644
>>> --- a/net/ncsi/ncsi-manage.c
>>> +++ b/net/ncsi/ncsi-manage.c
>>> @@ -165,13 +165,13 @@ static void ncsi_channel_monitor(unsigned long
>>> data)
>>> struct ncsi_dev_priv *ndp = np->ndp;
>>> struct ncsi_cmd_arg nca;
>>> bool enabled;
>>> - unsigned int timeout;
>>> + unsigned int monitor_state;
>>> unsigned long flags;
>>> int state, ret;
>>>
>>> spin_lock_irqsave(&nc->lock, flags);
>>> - timeout = nc->timeout;
>>> - enabled = nc->enabled;
>>> + monitor_state = nc->monitor.state;
>>> + enabled = nc->monitor.enabled;
>>> spin_unlock_irqrestore(&nc->lock, flags);
>>>
>>> if (!enabled || !list_empty(&nc->link))
>>> @@ -181,7 +181,9 @@ static void ncsi_channel_monitor(unsigned long
>>> data)
>>> if (state != NCSI_CHANNEL_INACTIVE && state !=
>>> NCSI_CHANNEL_ACTIVE)
>>> return;
>>>
>>> - if (!(timeout % 2)) {
>>> + switch (monitor_state) {
>>> + case NCSI_CHANNEL_MONITOR_START:
>>> + case NCSI_CHANNEL_MONITOR_RETRY:
>>> nca.ndp = ndp;
>>> nca.package = np->id;
>>> nca.channel = nc->id;
>>> @@ -193,15 +195,19 @@ static void ncsi_channel_monitor(unsigned long
>>> data)
>>> ret);
>>> return;
>>> }
>>> - }
>>>
>>> - if (timeout + 1 >= 3) {
>>> + break;
>>> + case NCSI_CHANNEL_MONITOR_WAIT ...
>>> NCSI_CHANNEL_MONITOR_WAIT_MAX:
>>
>>It's not clear why you have the MAX_WAIT state if it's doing the same
>>thing as WAIT.
>>
>
> @nc->monitor.state tracks two types of information: (A) the current state
> of the channel monitor; (B) the time since last GLS reponse was received
> successfully. The general idea behind is: we have two GLS commands sent
> and at least one response should be received in 5 seconds, which is
> represented by NCSI_CHANNEL_MONITOR_WAIT_MAX.
I follow that. But the code path for MAX_WAIT appears to be there same
as WAIT. There's nowhere that we do if (nc->monitor.state ==
NCSI_CHANNEL_MONITOR_WAIT_MAX) explode();
Is that intentional? If so, why do we need the two separate states?
Cheers,
Joel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring
2016-09-29 2:14 ` Joel Stanley
@ 2016-09-29 2:42 ` Gavin Shan
2016-09-29 3:11 ` Joel Stanley
0 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-29 2:42 UTC (permalink / raw)
To: Joel Stanley; +Cc: Gavin Shan, OpenBMC Maillist
On Thu, Sep 29, 2016 at 11:44:05AM +0930, Joel Stanley wrote:
>On Wed, Sep 28, 2016 at 4:48 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> On Wed, Sep 28, 2016 at 03:20:32PM +0930, Joel Stanley wrote:
>>>> +#define NCSI_CHANNEL_MONITOR_START 0
>>>> +#define NCSI_CHANNEL_MONITOR_RETRY 1
>>>> +#define NCSI_CHANNEL_MONITOR_WAIT 2
>>>> +#define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
>>>> + } monitor;
>>>> struct list_head node;
>>>> struct list_head link;
>>>> };
>>>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>>>> index 3019432..5813ebe 100644
>>>> --- a/net/ncsi/ncsi-manage.c
>>>> +++ b/net/ncsi/ncsi-manage.c
>>>> @@ -165,13 +165,13 @@ static void ncsi_channel_monitor(unsigned long
>>>> data)
>>>> struct ncsi_dev_priv *ndp = np->ndp;
>>>> struct ncsi_cmd_arg nca;
>>>> bool enabled;
>>>> - unsigned int timeout;
>>>> + unsigned int monitor_state;
>>>> unsigned long flags;
>>>> int state, ret;
>>>>
>>>> spin_lock_irqsave(&nc->lock, flags);
>>>> - timeout = nc->timeout;
>>>> - enabled = nc->enabled;
>>>> + monitor_state = nc->monitor.state;
>>>> + enabled = nc->monitor.enabled;
>>>> spin_unlock_irqrestore(&nc->lock, flags);
>>>>
>>>> if (!enabled || !list_empty(&nc->link))
>>>> @@ -181,7 +181,9 @@ static void ncsi_channel_monitor(unsigned long
>>>> data)
>>>> if (state != NCSI_CHANNEL_INACTIVE && state !=
>>>> NCSI_CHANNEL_ACTIVE)
>>>> return;
>>>>
>>>> - if (!(timeout % 2)) {
>>>> + switch (monitor_state) {
>>>> + case NCSI_CHANNEL_MONITOR_START:
>>>> + case NCSI_CHANNEL_MONITOR_RETRY:
>>>> nca.ndp = ndp;
>>>> nca.package = np->id;
>>>> nca.channel = nc->id;
>>>> @@ -193,15 +195,19 @@ static void ncsi_channel_monitor(unsigned long
>>>> data)
>>>> ret);
>>>> return;
>>>> }
>>>> - }
>>>>
>>>> - if (timeout + 1 >= 3) {
>>>> + break;
>>>> + case NCSI_CHANNEL_MONITOR_WAIT ...
>>>> NCSI_CHANNEL_MONITOR_WAIT_MAX:
>>>
>>>It's not clear why you have the MAX_WAIT state if it's doing the same
>>>thing as WAIT.
>>>
>>
>> @nc->monitor.state tracks two types of information: (A) the current state
>> of the channel monitor; (B) the time since last GLS reponse was received
>> successfully. The general idea behind is: we have two GLS commands sent
>> and at least one response should be received in 5 seconds, which is
>> represented by NCSI_CHANNEL_MONITOR_WAIT_MAX.
>
>I follow that. But the code path for MAX_WAIT appears to be there same
>as WAIT. There's nowhere that we do if (nc->monitor.state ==
>NCSI_CHANNEL_MONITOR_WAIT_MAX) explode();
>
>Is that intentional? If so, why do we need the two separate states?
>
Yes, there are 4 waiting states (2/3/4/5) as below.
#define NCSI_CHANNEL_MONITOR_WAIT 2
#define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
I want to use "switch...case" in the code as it's a state machine.
Also, I would like to have two macros to specify the range of
waiting state, which makes the code clearer. Yes, it's intentional.
Instead, we can use "if...else if...else". It's not good enough
to have this in the state machine driven code, but it's a personal
preference. In this case, we need less defined states:
#define NCSI_CHANNEL_MONITOR_RETRY 1
#define NCSI_CHANNEL_MONITOR_TIMEOUT 6
if (monitor.state <= NCSI_CHANNEL_MONITOR_RETRY)
/* send GLS packet */
else if (monitor.state < NCSI_CHANNEL_MONITOR_TIMEOUT)
/* break */
else
/* failover */
monitor.state++;
rescheule_timeout();
Thanks,
Gavin
>Cheers,
>
>Joel
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring
2016-09-29 2:42 ` Gavin Shan
@ 2016-09-29 3:11 ` Joel Stanley
0 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2016-09-29 3:11 UTC (permalink / raw)
To: Gavin Shan; +Cc: OpenBMC Maillist
On Thu, Sep 29, 2016 at 12:12 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>>>>>
>>>>> - if (timeout + 1 >= 3) {
>>>>> + break;
>>>>> + case NCSI_CHANNEL_MONITOR_WAIT ...
>>>>> NCSI_CHANNEL_MONITOR_WAIT_MAX:
>>>>
>>>>It's not clear why you have the MAX_WAIT state if it's doing the same
>>>>thing as WAIT.
>>>>
>>>
>>> @nc->monitor.state tracks two types of information: (A) the current state
>>> of the channel monitor; (B) the time since last GLS reponse was received
>>> successfully. The general idea behind is: we have two GLS commands sent
>>> and at least one response should be received in 5 seconds, which is
>>> represented by NCSI_CHANNEL_MONITOR_WAIT_MAX.
>>
>>I follow that. But the code path for MAX_WAIT appears to be there same
>>as WAIT. There's nowhere that we do if (nc->monitor.state ==
>>NCSI_CHANNEL_MONITOR_WAIT_MAX) explode();
>>
>>Is that intentional? If so, why do we need the two separate states?
>>
>
> Yes, there are 4 waiting states (2/3/4/5) as below.
>
> #define NCSI_CHANNEL_MONITOR_WAIT 2
> #define NCSI_CHANNEL_MONITOR_WAIT_MAX 5
>
> I want to use "switch...case" in the code as it's a state machine.
> Also, I would like to have two macros to specify the range of
> waiting state, which makes the code clearer. Yes, it's intentional.
Okay. I guess my confusion was around never explicitly doing anything
in WAIT state. We say in retry state until we transition to MAX.
>
> Instead, we can use "if...else if...else". It's not good enough
> to have this in the state machine driven code, but it's a personal
> preference. In this case, we need less defined states:
>
> #define NCSI_CHANNEL_MONITOR_RETRY 1
> #define NCSI_CHANNEL_MONITOR_TIMEOUT 6
>
> if (monitor.state <= NCSI_CHANNEL_MONITOR_RETRY)
> /* send GLS packet */
> else if (monitor.state < NCSI_CHANNEL_MONITOR_TIMEOUT)
> /* break */
> else
> /* failover */
I think this is clearer. However, now that I understand what your
patch is doing, either approach is ok.
Cheers,
Joel
>
> monitor.state++;
> rescheule_timeout();
>
> Thanks,
> Gavin
>
>>Cheers,
>>
>>Joel
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4.7 7/7] net/ncsi: Introduce ncsi_stop_dev()
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
` (4 preceding siblings ...)
2016-09-28 2:48 ` [PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring Gavin Shan
@ 2016-09-28 2:48 ` Gavin Shan
2016-09-28 5:51 ` Joel Stanley
2016-09-28 5:25 ` [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Joel Stanley
6 siblings, 1 reply; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 2:48 UTC (permalink / raw)
To: openbmc; +Cc: joel, Gavin Shan
This introduces ncsi_stop_dev(), as counterpart to ncsi_start_dev(),
to stop the NCSI device so that it can be enabled in future. This
API should be called when the network device driver is going to
shutdown the device. There are 3 things done in the function: Stop
the channel monitoring; Reset channels to inactive state; Report
NCSI link down.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
drivers/net/ethernet/faraday/ftgmac100.c | 2 ++
include/net/ncsi.h | 1 +
net/ncsi/ncsi-manage.c | 37 +++++++++++++++++++++-----------
3 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 134b9c5..f40fa92 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1189,6 +1189,8 @@ static int ftgmac100_stop(struct net_device *netdev)
napi_disable(&priv->napi);
if (netdev->phydev)
phy_stop(netdev->phydev);
+ else if (priv->use_ncsi)
+ ncsi_stop_dev(priv->ndev);
ftgmac100_stop_hw(priv);
free_irq(priv->irq, netdev);
diff --git a/include/net/ncsi.h b/include/net/ncsi.h
index 1dbf42f..3d166f7 100644
--- a/include/net/ncsi.h
+++ b/include/net/ncsi.h
@@ -31,6 +31,7 @@ struct ncsi_dev {
struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
void (*notifier)(struct ncsi_dev *nd));
int ncsi_start_dev(struct ncsi_dev *nd);
+void ncsi_stop_dev(struct ncsi_dev *nd);
void ncsi_unregister_dev(struct ncsi_dev *nd);
#else /* !CONFIG_NET_NCSI */
static inline struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 5813ebe..73ab65f 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1160,9 +1160,7 @@ EXPORT_SYMBOL_GPL(ncsi_register_dev);
int ncsi_start_dev(struct ncsi_dev *nd)
{
struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
- struct ncsi_package *np;
- struct ncsi_channel *nc;
- int old_state, ret;
+ int ret;
if (nd->state != ncsi_dev_state_registered &&
nd->state != ncsi_dev_state_functional)
@@ -1174,16 +1172,6 @@ int ncsi_start_dev(struct ncsi_dev *nd)
return 0;
}
- /* Reset channel's state and start over */
- NCSI_FOR_EACH_PACKAGE(ndp, np) {
- NCSI_FOR_EACH_CHANNEL(np, nc) {
- old_state = atomic_xchg(&nc->state,
- NCSI_CHANNEL_INACTIVE);
- WARN_ON_ONCE(!list_empty(&nc->link) ||
- old_state == NCSI_CHANNEL_INVISIBLE);
- }
- }
-
if (ndp->flags & NCSI_DEV_HWA)
ret = ncsi_enable_hwa(ndp);
else
@@ -1193,6 +1181,29 @@ int ncsi_start_dev(struct ncsi_dev *nd)
}
EXPORT_SYMBOL_GPL(ncsi_start_dev);
+void ncsi_stop_dev(struct ncsi_dev *nd)
+{
+ struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
+ struct ncsi_package *np;
+ struct ncsi_channel *nc;
+ int old_state;
+
+ /* Stop the channel monitor and reset channel's state */
+ NCSI_FOR_EACH_PACKAGE(ndp, np) {
+ NCSI_FOR_EACH_CHANNEL(np, nc) {
+ ncsi_stop_channel_monitor(nc);
+
+ old_state = atomic_xchg(&nc->state,
+ NCSI_CHANNEL_INACTIVE);
+ WARN_ON_ONCE(!list_empty(&nc->link) ||
+ old_state == NCSI_CHANNEL_INVISIBLE);
+ }
+ }
+
+ ncsi_report_link(ndp, true);
+}
+EXPORT_SYMBOL_GPL(ncsi_stop_dev);
+
void ncsi_unregister_dev(struct ncsi_dev *nd)
{
struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 7/7] net/ncsi: Introduce ncsi_stop_dev()
2016-09-28 2:48 ` [PATCH v4.7 7/7] net/ncsi: Introduce ncsi_stop_dev() Gavin Shan
@ 2016-09-28 5:51 ` Joel Stanley
2016-09-28 7:18 ` Gavin Shan
0 siblings, 1 reply; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:51 UTC (permalink / raw)
To: Gavin Shan, openbmc
On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
> This introduces ncsi_stop_dev(), as counterpart to ncsi_start_dev(),
> to stop the NCSI device so that it can be enabled in future. This
> API should be called when the network device driver is going to
> shutdown the device. There are 3 things done in the function: Stop
> the channel monitoring; Reset channels to inactive state; Report
> NCSI link down.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Makes sense. Can you split out the change to the faraday driver?
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 2 ++
> include/net/ncsi.h | 1 +
> net/ncsi/ncsi-manage.c | 37 +++++++++++++++++++++-
> ----------
> 3 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 134b9c5..f40fa92 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1189,6 +1189,8 @@ static int ftgmac100_stop(struct net_device
> *netdev)
> napi_disable(&priv->napi);
> if (netdev->phydev)
> phy_stop(netdev->phydev);
> + else if (priv->use_ncsi)
> + ncsi_stop_dev(priv->ndev);
>
> ftgmac100_stop_hw(priv);
> free_irq(priv->irq, netdev);
> diff --git a/include/net/ncsi.h b/include/net/ncsi.h
> index 1dbf42f..3d166f7 100644
> --- a/include/net/ncsi.h
> +++ b/include/net/ncsi.h
> @@ -31,6 +31,7 @@ struct ncsi_dev {
> struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
> void (*notifier)(struct ncsi_dev
> *nd));
> int ncsi_start_dev(struct ncsi_dev *nd);
> +void ncsi_stop_dev(struct ncsi_dev *nd);
> void ncsi_unregister_dev(struct ncsi_dev *nd);
> #else /* !CONFIG_NET_NCSI */
> static inline struct ncsi_dev *ncsi_register_dev(struct net_device
> *dev,
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 5813ebe..73ab65f 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -1160,9 +1160,7 @@ EXPORT_SYMBOL_GPL(ncsi_register_dev);
> int ncsi_start_dev(struct ncsi_dev *nd)
> {
> struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
> - struct ncsi_package *np;
> - struct ncsi_channel *nc;
> - int old_state, ret;
> + int ret;
>
> if (nd->state != ncsi_dev_state_registered &&
> nd->state != ncsi_dev_state_functional)
> @@ -1174,16 +1172,6 @@ int ncsi_start_dev(struct ncsi_dev *nd)
> return 0;
> }
>
> - /* Reset channel's state and start over */
> - NCSI_FOR_EACH_PACKAGE(ndp, np) {
> - NCSI_FOR_EACH_CHANNEL(np, nc) {
> - old_state = atomic_xchg(&nc->state,
> - NCSI_CHANNEL_INACTIV
> E);
> - WARN_ON_ONCE(!list_empty(&nc->link) ||
> - old_state ==
> NCSI_CHANNEL_INVISIBLE);
> - }
> - }
> -
> if (ndp->flags & NCSI_DEV_HWA)
> ret = ncsi_enable_hwa(ndp);
> else
> @@ -1193,6 +1181,29 @@ int ncsi_start_dev(struct ncsi_dev *nd)
> }
> EXPORT_SYMBOL_GPL(ncsi_start_dev);
>
> +void ncsi_stop_dev(struct ncsi_dev *nd)
> +{
> + struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + int old_state;
> +
> + /* Stop the channel monitor and reset channel's state */
> + NCSI_FOR_EACH_PACKAGE(ndp, np) {
> + NCSI_FOR_EACH_CHANNEL(np, nc) {
> + ncsi_stop_channel_monitor(nc);
> +
> + old_state = atomic_xchg(&nc->state,
> + NCSI_CHANNEL_INACTIV
> E);
> + WARN_ON_ONCE(!list_empty(&nc->link) ||
> + old_state ==
> NCSI_CHANNEL_INVISIBLE);
> + }
> + }
> +
> + ncsi_report_link(ndp, true);
> +}
> +EXPORT_SYMBOL_GPL(ncsi_stop_dev);
> +
> void ncsi_unregister_dev(struct ncsi_dev *nd)
> {
> struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 7/7] net/ncsi: Introduce ncsi_stop_dev()
2016-09-28 5:51 ` Joel Stanley
@ 2016-09-28 7:18 ` Gavin Shan
0 siblings, 0 replies; 20+ messages in thread
From: Gavin Shan @ 2016-09-28 7:18 UTC (permalink / raw)
To: Joel Stanley; +Cc: Gavin Shan, openbmc
On Wed, Sep 28, 2016 at 03:21:20PM +0930, Joel Stanley wrote:
>On Wed, 2016-09-28 at 12:48 +1000, Gavin Shan wrote:
>> This introduces ncsi_stop_dev(), as counterpart to ncsi_start_dev(),
>> to stop the NCSI device so that it can be enabled in future. This
>> API should be called when the network device driver is going to
>> shutdown the device. There are 3 things done in the function: Stop
>> the channel monitoring; Reset channels to inactive state; Report
>> NCSI link down.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>Makes sense. Can you split out the change to the faraday driver?
>
Sure, will do in next revision.
>> ---
>> drivers/net/ethernet/faraday/ftgmac100.c | 2 ++
>> include/net/ncsi.h | 1 +
>> net/ncsi/ncsi-manage.c | 37 +++++++++++++++++++++-
>> ----------
>> 3 files changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
>> b/drivers/net/ethernet/faraday/ftgmac100.c
>> index 134b9c5..f40fa92 100644
>> --- a/drivers/net/ethernet/faraday/ftgmac100.c
>> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
>> @@ -1189,6 +1189,8 @@ static int ftgmac100_stop(struct net_device
>> *netdev)
>> napi_disable(&priv->napi);
>> if (netdev->phydev)
>> phy_stop(netdev->phydev);
>> + else if (priv->use_ncsi)
>> + ncsi_stop_dev(priv->ndev);
>>
>> ftgmac100_stop_hw(priv);
>> free_irq(priv->irq, netdev);
>> diff --git a/include/net/ncsi.h b/include/net/ncsi.h
>> index 1dbf42f..3d166f7 100644
>> --- a/include/net/ncsi.h
>> +++ b/include/net/ncsi.h
>> @@ -31,6 +31,7 @@ struct ncsi_dev {
>> struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
>> void (*notifier)(struct ncsi_dev
>> *nd));
>> int ncsi_start_dev(struct ncsi_dev *nd);
>> +void ncsi_stop_dev(struct ncsi_dev *nd);
>> void ncsi_unregister_dev(struct ncsi_dev *nd);
>> #else /* !CONFIG_NET_NCSI */
>> static inline struct ncsi_dev *ncsi_register_dev(struct net_device
>> *dev,
>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>> index 5813ebe..73ab65f 100644
>> --- a/net/ncsi/ncsi-manage.c
>> +++ b/net/ncsi/ncsi-manage.c
>> @@ -1160,9 +1160,7 @@ EXPORT_SYMBOL_GPL(ncsi_register_dev);
>> int ncsi_start_dev(struct ncsi_dev *nd)
>> {
>> struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
>> - struct ncsi_package *np;
>> - struct ncsi_channel *nc;
>> - int old_state, ret;
>> + int ret;
>>
>> if (nd->state != ncsi_dev_state_registered &&
>> nd->state != ncsi_dev_state_functional)
>> @@ -1174,16 +1172,6 @@ int ncsi_start_dev(struct ncsi_dev *nd)
>> return 0;
>> }
>>
>> - /* Reset channel's state and start over */
>> - NCSI_FOR_EACH_PACKAGE(ndp, np) {
>> - NCSI_FOR_EACH_CHANNEL(np, nc) {
>> - old_state = atomic_xchg(&nc->state,
>> - NCSI_CHANNEL_INACTIV
>> E);
>> - WARN_ON_ONCE(!list_empty(&nc->link) ||
>> - old_state ==
>> NCSI_CHANNEL_INVISIBLE);
>> - }
>> - }
>> -
>> if (ndp->flags & NCSI_DEV_HWA)
>> ret = ncsi_enable_hwa(ndp);
>> else
>> @@ -1193,6 +1181,29 @@ int ncsi_start_dev(struct ncsi_dev *nd)
>> }
>> EXPORT_SYMBOL_GPL(ncsi_start_dev);
>>
>> +void ncsi_stop_dev(struct ncsi_dev *nd)
>> +{
>> + struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
>> + struct ncsi_package *np;
>> + struct ncsi_channel *nc;
>> + int old_state;
>> +
>> + /* Stop the channel monitor and reset channel's state */
>> + NCSI_FOR_EACH_PACKAGE(ndp, np) {
>> + NCSI_FOR_EACH_CHANNEL(np, nc) {
>> + ncsi_stop_channel_monitor(nc);
>> +
>> + old_state = atomic_xchg(&nc->state,
>> + NCSI_CHANNEL_INACTIV
>> E);
>> + WARN_ON_ONCE(!list_empty(&nc->link) ||
>> + old_state ==
>> NCSI_CHANNEL_INVISIBLE);
>> + }
>> + }
>> +
>> + ncsi_report_link(ndp, true);
>> +}
>> +EXPORT_SYMBOL_GPL(ncsi_stop_dev);
>> +
>> void ncsi_unregister_dev(struct ncsi_dev *nd)
>> {
>> struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning
2016-09-28 2:48 [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Gavin Shan
` (5 preceding siblings ...)
2016-09-28 2:48 ` [PATCH v4.7 7/7] net/ncsi: Introduce ncsi_stop_dev() Gavin Shan
@ 2016-09-28 5:25 ` Joel Stanley
2016-09-29 0:27 ` Gavin Shan
6 siblings, 1 reply; 20+ messages in thread
From: Joel Stanley @ 2016-09-28 5:25 UTC (permalink / raw)
To: Gavin Shan, openbmc
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@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;
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning
2016-09-28 5:25 ` [PATCH v4.7 1/7] net/ncsi: Avoid unused-value warning Joel Stanley
@ 2016-09-29 0:27 ` Gavin Shan
0 siblings, 0 replies; 20+ messages in thread
From: Gavin Shan @ 2016-09-29 0:27 UTC (permalink / raw)
To: Joel Stanley; +Cc: Gavin Shan, openbmc
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@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;
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread