diff mbox

[16/22] FSP/LEDS: Move checkpoint status variable to led_set_cmd structure

Message ID 20150205084125.12859.86781.stgit@localhost.localdomain
State Changes Requested
Headers show

Commit Message

Vasant Hegde Feb. 5, 2015, 8:41 a.m. UTC 
"fsp_led_data" structure contains ckpt_status variable which keeps
current LED state before updating and if LED update fails then we
use this to revert the LED state.

We have introduced new strucutre (led_set_cmd) to queue up LED update
requests. It make sense to move checkpoint status variable to this
new structure.

Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 hw/fsp/fsp-leds.c |   42 +++++++++++++++++++++++++-----------------
 hw/fsp/fsp-leds.h |    2 +-
 2 files changed, 26 insertions(+), 18 deletions(-)

Comments

Stewart Smith Feb. 17, 2015, 8:43 p.m. UTC | #1 
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
> "fsp_led_data" structure contains ckpt_status variable which keeps
> current LED state before updating and if LED update fails then we
> use this to revert the LED state.
>
> We have introduced new strucutre (led_set_cmd) to queue up LED update
> requests. It make sense to move checkpoint status variable to this
> new structure.
>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> ---
>  hw/fsp/fsp-leds.c |   42 +++++++++++++++++++++++++-----------------
>  hw/fsp/fsp-leds.h |    2 +-
>  2 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/hw/fsp/fsp-leds.c b/hw/fsp/fsp-leds.c
> index 16c9b68..335a967 100644
> --- a/hw/fsp/fsp-leds.c
> +++ b/hw/fsp/fsp-leds.c
> @@ -244,14 +244,21 @@ enclosure:
>  	encl_led->excl_bit = encl_cec_led->excl_bit;
>  }
>  
> +/* Free SPCN command */
> +static inline void free_spcn_cmd(struct led_set_cmd *spcn_cmd)
> +{
> +	lock(&spcn_cmd_lock);
> +	free(spcn_cmd);
> +	unlock(&spcn_cmd_lock);
> +}

I'm struggling to work out why this needs a lock (or a separate function
that just wraps free()). What am I missing?
Vasant Hegde March 4, 2015, 11:41 a.m. UTC | #2 
On 02/18/2015 02:13 AM, Stewart Smith wrote:
> Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
>> "fsp_led_data" structure contains ckpt_status variable which keeps
>> current LED state before updating and if LED update fails then we
>> use this to revert the LED state.
>>

.../...

>>  
>> +/* Free SPCN command */
>> +static inline void free_spcn_cmd(struct led_set_cmd *spcn_cmd)
>> +{
>> +	lock(&spcn_cmd_lock);
>> +	free(spcn_cmd);
>> +	unlock(&spcn_cmd_lock);
>> +}
> 
> I'm struggling to work out why this needs a lock (or a separate function
> that just wraps free()). What am I missing?
> 

Stewart,

You are right.. This is redundant (also this function).. Fixed in v2.

-Vasant
diff mbox

Patch

diff --git a/hw/fsp/fsp-leds.c b/hw/fsp/fsp-leds.c
index 16c9b68..335a967 100644
--- a/hw/fsp/fsp-leds.c
+++ b/hw/fsp/fsp-leds.c
@@ -244,14 +244,21 @@  enclosure:
 	encl_led->excl_bit = encl_cec_led->excl_bit;
 }
 
+/* Free SPCN command */
+static inline void free_spcn_cmd(struct led_set_cmd *spcn_cmd)
+{
+	lock(&spcn_cmd_lock);
+	free(spcn_cmd);
+	unlock(&spcn_cmd_lock);
+}
+
 static void fsp_spcn_set_led_completion(struct fsp_msg *msg)
 {
-	u16 ckpt_status;
-	char loc_code[LOC_CODE_SIZE + 1];
-	struct fsp_msg *resp = msg->resp;
 	struct fsp_msg *smsg = NULL;
+	struct fsp_msg *resp = msg->resp;
 	u32 cmd = FSP_RSP_SET_LED_STATE;
 	u8 status = resp->word1 & 0xff00;
+	struct led_set_cmd *spcn_cmd = (struct led_set_cmd *)msg->user_data;
 
 	/*
 	 * LED state update request came as part of FSP async message
@@ -266,16 +273,8 @@  static void fsp_spcn_set_led_completion(struct fsp_msg *msg)
 			status);
 		cmd |= FSP_STATUS_GENERIC_ERROR;
 
-		/* Identify the failed command */
-		memset(loc_code, 0, sizeof(loc_code));
-		strncpy(loc_code,
-			((struct fsp_led_data *)(msg->user_data))->loc_code,
-			LOC_CODE_SIZE);
-		ckpt_status = ((struct fsp_led_data *)(msg->user_data))
-			->ckpt_status;
-
 		/* Rollback the changes */
-		update_led_list(loc_code, ckpt_status);
+		update_led_list(spcn_cmd->loc_code, spcn_cmd->ckpt_status);
 	}
 
 	smsg = fsp_mkmsg(cmd, 0);
@@ -288,6 +287,8 @@  static void fsp_spcn_set_led_completion(struct fsp_msg *msg)
 		}
 	}
 
+	free_spcn_cmd(spcn_cmd);
+
 	/* free msg */
 	fsp_freemsg(msg);
 
@@ -349,6 +350,7 @@  static int fsp_msg_set_led_state(struct led_set_cmd *spcn_cmd)
 					"|FSP_STATUS_INVALID_LC\n");
 			}
 		}
+		free_spcn_cmd(spcn_cmd);
 		return rc;
 	}
 
@@ -356,7 +358,7 @@  static int fsp_msg_set_led_state(struct led_set_cmd *spcn_cmd)
 	 * Checkpoint the status here, will use it if the SPCN
 	 * command eventually fails.
 	 */
-	led->ckpt_status = led->status;
+	spcn_cmd->ckpt_status = led->status;
 	sled.state = led->status;
 
 	/* Update the exclussive LED bits  */
@@ -400,20 +402,26 @@  static int fsp_msg_set_led_state(struct led_set_cmd *spcn_cmd)
 
 	msg = fsp_mkmsg(FSP_CMD_SPCN_PASSTHRU, 4,
 			SPCN_ADDR_MODE_CEC_NODE, cmd_hdr, 0, PSI_DMA_LED_BUF);
-	if (!msg)
+	if (!msg) {
+		free_spcn_cmd(spcn_cmd);
 		return rc;
+	}
+
 	/*
 	 * Update the local lists based on the attempted SPCN command to
 	 * set/reset an individual led (CEC or ENCL).
 	 */
 	lock(&led_lock);
 	update_led_list(spcn_cmd->loc_code, sled.state);
-	msg->user_data = led;
+	msg->user_data = spcn_cmd;
 	unlock(&led_lock);
 
 	rc = fsp_queue_msg(msg, fsp_spcn_set_led_completion);
-	if (rc != OPAL_SUCCESS)
+	if (rc != OPAL_SUCCESS) {
 		fsp_freemsg(msg);
+		free_spcn_cmd(spcn_cmd);
+	}
+
 	return rc;
 }
 
@@ -449,7 +457,7 @@  static int process_led_state_change(void)
 		log_simple_error(&e_info(OPAL_RC_LED_STATE),
 				 PREFIX "Set led state failed at LC=%s\n",
 				 spcn_cmd->loc_code);
-	free(spcn_cmd);
+
 	return rc;
 }
 
diff --git a/hw/fsp/fsp-leds.h b/hw/fsp/fsp-leds.h
index a09a27e..de750ad 100644
--- a/hw/fsp/fsp-leds.h
+++ b/hw/fsp/fsp-leds.h
@@ -54,7 +54,6 @@  struct fsp_led_data {
         char			loc_code[LOC_CODE_SIZE];
 	u16			parms;			/* Parameters */
 	u16			status;			/* Status */
-	u16			ckpt_status;		/* Checkpointed status */
 	u16			excl_bit;		/* Exclussive LED bit */
 	struct list_node	link;
 };
@@ -113,6 +112,7 @@  struct led_set_cmd {
 	char	loc_code[LOC_CODE_SIZE];
 	u8	command;
 	u8	state;
+	u16	ckpt_status;		/* Checkpointed status */
 	struct	list_node link;
 };