linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] firmware: tegra: add checks for BPMP error return code
@ 2017-09-07  9:31 Timo Alho
  2017-09-07  9:31 ` [PATCH 1/4] firmware: tegra: propagate error code to caller Timo Alho
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Timo Alho @ 2017-09-07  9:31 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-tegra, linux-kernel, Timo Alho

Hi Thierry,

There is a bug in the BPMP driver as error code in response message is
not being checked.

Patch 1 adds the error code as part of tegra_bpmp_message struct and
propagates that code to callers

Patches 2 through 4 add code to client drivers to check the error code
appropriately

BR,
Timo

Timo Alho (4):
  firmware: tegra: propagate error code to caller
  clk: tegra: check BPMP response return code
  reset: tegra: check BPMP response return code
  soc/tegra: bpmp: check BPMP response return code

 drivers/clk/tegra/clk-bpmp.c       | 15 ++++++++++-----
 drivers/firmware/tegra/bpmp.c      | 22 ++++++++++++++++------
 drivers/reset/tegra/reset-bpmp.c   |  9 ++++++++-
 drivers/soc/tegra/powergate-bpmp.c | 15 +++++++++++++--
 include/soc/tegra/bpmp.h           |  1 +
 5 files changed, 48 insertions(+), 14 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/4] firmware: tegra: propagate error code to caller
  2017-09-07  9:31 [PATCH 0/4] firmware: tegra: add checks for BPMP error return code Timo Alho
@ 2017-09-07  9:31 ` Timo Alho
  2017-09-21 11:19   ` Jon Hunter
  2017-10-17 10:41   ` Thierry Reding
  2017-09-07  9:31 ` [PATCH 2/4] clk: tegra: check BPMP response return code Timo Alho
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 20+ messages in thread
From: Timo Alho @ 2017-09-07  9:31 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-tegra, linux-kernel, Timo Alho

Response messages from Tegra BPMP firmware contain an error return
code as the first word of payload. The error code is used to indicate
incorrectly formatted request message or use of non-existing resource
(clk, reset, powergate) identifier. Current implementation of
tegra_bpmp_transfer() ignores this code and does not pass it to
caller. Fix this by adding an extra struct member to
tegra_bpmp_message and populate that with return code.

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/firmware/tegra/bpmp.c | 22 ++++++++++++++++------
 include/soc/tegra/bpmp.h      |  1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index 73ca55b..33683b5 100644
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -194,16 +194,24 @@ static int tegra_bpmp_wait_master_free(struct tegra_bpmp_channel *channel)
 }
 
 static ssize_t __tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
-					 void *data, size_t size)
+					 void *data, size_t size, int *ret)
 {
+	int err;
+
 	if (data && size > 0)
 		memcpy(data, channel->ib->data, size);
 
-	return tegra_ivc_read_advance(channel->ivc);
+	err = tegra_ivc_read_advance(channel->ivc);
+	if (err < 0)
+		return err;
+
+	*ret = channel->ib->code;
+
+	return 0;
 }
 
 static ssize_t tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
-				       void *data, size_t size)
+				       void *data, size_t size, int *ret)
 {
 	struct tegra_bpmp *bpmp = channel->bpmp;
 	unsigned long flags;
@@ -217,7 +225,7 @@ static ssize_t tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
 	}
 
 	spin_lock_irqsave(&bpmp->lock, flags);
-	err = __tegra_bpmp_channel_read(channel, data, size);
+	err = __tegra_bpmp_channel_read(channel, data, size, ret);
 	clear_bit(index, bpmp->threaded.allocated);
 	spin_unlock_irqrestore(&bpmp->lock, flags);
 
@@ -337,7 +345,8 @@ int tegra_bpmp_transfer_atomic(struct tegra_bpmp *bpmp,
 	if (err < 0)
 		return err;
 
-	return __tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size);
+	return __tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size,
+					 &msg->rx.ret);
 }
 EXPORT_SYMBOL_GPL(tegra_bpmp_transfer_atomic);
 
@@ -371,7 +380,8 @@ int tegra_bpmp_transfer(struct tegra_bpmp *bpmp,
 	if (err == 0)
 		return -ETIMEDOUT;
 
-	return tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size);
+	return tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size,
+				       &msg->rx.ret);
 }
 EXPORT_SYMBOL_GPL(tegra_bpmp_transfer);
 
diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h
index 9ba6522..57519f4 100644
--- a/include/soc/tegra/bpmp.h
+++ b/include/soc/tegra/bpmp.h
@@ -110,6 +110,7 @@ struct tegra_bpmp_message {
 	struct {
 		void *data;
 		size_t size;
+		int ret;
 	} rx;
 };
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-09-07  9:31 [PATCH 0/4] firmware: tegra: add checks for BPMP error return code Timo Alho
  2017-09-07  9:31 ` [PATCH 1/4] firmware: tegra: propagate error code to caller Timo Alho
@ 2017-09-07  9:31 ` Timo Alho
  2017-09-21 11:21   ` Jon Hunter
  2017-10-17 10:37   ` Thierry Reding
  2017-09-07  9:31 ` [PATCH 3/4] reset: " Timo Alho
  2017-09-07  9:31 ` [PATCH 4/4] soc/tegra: bpmp: " Timo Alho
  3 siblings, 2 replies; 20+ messages in thread
From: Timo Alho @ 2017-09-07  9:31 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-tegra, linux-kernel, Timo Alho

Check return code in BPMP response message(s). The typical error case
is when clock operation is attempted with invalid clock identifier.

Also remove error print from call to clk_get_info() as the
implementation loops through range of all possible identifier, but the
operation is expected error out when the clock id is unused.

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
index 638ace6..a896692 100644
--- a/drivers/clk/tegra/clk-bpmp.c
+++ b/drivers/clk/tegra/clk-bpmp.c
@@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
 	struct {
 		void *data;
 		size_t size;
+		int ret;
 	} rx;
 };
 
@@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
 	struct mrq_clk_request request;
 	struct tegra_bpmp_message msg;
 	void *req = &request;
+	int err;
 
 	memset(&request, 0, sizeof(request));
 	request.cmd_and_id = (clk->cmd << 24) | clk->id;
@@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
 	msg.rx.data = clk->rx.data;
 	msg.rx.size = clk->rx.size;
 
-	return tegra_bpmp_transfer(bpmp, &msg);
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return err;
+	else if (msg.rx.ret < 0)
+		return -EINVAL;
+
+	return 0;
 }
 
 static int tegra_bpmp_clk_prepare(struct clk_hw *hw)
@@ -414,11 +422,8 @@ static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
 		struct tegra_bpmp_clk_info *info = &clocks[count];
 
 		err = tegra_bpmp_clk_get_info(bpmp, id, info);
-		if (err < 0) {
-			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
-				id, err);
+		if (err < 0)
 			continue;
-		}
 
 		if (info->num_parents >= U8_MAX) {
 			dev_err(bpmp->dev,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/4] reset: tegra: check BPMP response return code
  2017-09-07  9:31 [PATCH 0/4] firmware: tegra: add checks for BPMP error return code Timo Alho
  2017-09-07  9:31 ` [PATCH 1/4] firmware: tegra: propagate error code to caller Timo Alho
  2017-09-07  9:31 ` [PATCH 2/4] clk: tegra: check BPMP response return code Timo Alho
@ 2017-09-07  9:31 ` Timo Alho
  2017-10-02 20:24   ` Jon Hunter
  2017-10-17 10:40   ` Thierry Reding
  2017-09-07  9:31 ` [PATCH 4/4] soc/tegra: bpmp: " Timo Alho
  3 siblings, 2 replies; 20+ messages in thread
From: Timo Alho @ 2017-09-07  9:31 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-tegra, linux-kernel, Timo Alho

Add checks for return code in BPMP response message.

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/reset/tegra/reset-bpmp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/tegra/reset-bpmp.c b/drivers/reset/tegra/reset-bpmp.c
index 5daf2ee..fac2db6 100644
--- a/drivers/reset/tegra/reset-bpmp.c
+++ b/drivers/reset/tegra/reset-bpmp.c
@@ -23,6 +23,7 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
 	struct tegra_bpmp *bpmp = to_tegra_bpmp(rstc);
 	struct mrq_reset_request request;
 	struct tegra_bpmp_message msg;
+	int err;
 
 	memset(&request, 0, sizeof(request));
 	request.cmd = command;
@@ -33,7 +34,13 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
 	msg.tx.data = &request;
 	msg.tx.size = sizeof(request);
 
-	return tegra_bpmp_transfer(bpmp, &msg);
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return err;
+	else if (msg.rx.ret < 0)
+		return -EINVAL;
+
+	return 0;
 }
 
 static int tegra_bpmp_reset_module(struct reset_controller_dev *rstc,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/4] soc/tegra: bpmp: check BPMP response return code
  2017-09-07  9:31 [PATCH 0/4] firmware: tegra: add checks for BPMP error return code Timo Alho
                   ` (2 preceding siblings ...)
  2017-09-07  9:31 ` [PATCH 3/4] reset: " Timo Alho
@ 2017-09-07  9:31 ` Timo Alho
  2017-10-02 20:26   ` Jon Hunter
  2017-10-17 10:41   ` Thierry Reding
  3 siblings, 2 replies; 20+ messages in thread
From: Timo Alho @ 2017-09-07  9:31 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-tegra, linux-kernel, Timo Alho

Add checks for return code in BPMP response message.

Signed-off-by: Timo Alho <talho@nvidia.com>
---
 drivers/soc/tegra/powergate-bpmp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/tegra/powergate-bpmp.c b/drivers/soc/tegra/powergate-bpmp.c
index 8fc3560..82c7e27 100644
--- a/drivers/soc/tegra/powergate-bpmp.c
+++ b/drivers/soc/tegra/powergate-bpmp.c
@@ -42,6 +42,7 @@ static int tegra_bpmp_powergate_set_state(struct tegra_bpmp *bpmp,
 {
 	struct mrq_pg_request request;
 	struct tegra_bpmp_message msg;
+	int err;
 
 	memset(&request, 0, sizeof(request));
 	request.cmd = CMD_PG_SET_STATE;
@@ -53,7 +54,13 @@ static int tegra_bpmp_powergate_set_state(struct tegra_bpmp *bpmp,
 	msg.tx.data = &request;
 	msg.tx.size = sizeof(request);
 
-	return tegra_bpmp_transfer(bpmp, &msg);
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return err;
+	else if (msg.rx.ret < 0)
+		return -EINVAL;
+
+	return 0;
 }
 
 static int tegra_bpmp_powergate_get_state(struct tegra_bpmp *bpmp,
@@ -80,6 +87,8 @@ static int tegra_bpmp_powergate_get_state(struct tegra_bpmp *bpmp,
 	err = tegra_bpmp_transfer(bpmp, &msg);
 	if (err < 0)
 		return PG_STATE_OFF;
+	else if (msg.rx.ret < 0)
+		return -EINVAL;
 
 	return response.get_state.state;
 }
@@ -106,6 +115,8 @@ static int tegra_bpmp_powergate_get_max_id(struct tegra_bpmp *bpmp)
 	err = tegra_bpmp_transfer(bpmp, &msg);
 	if (err < 0)
 		return err;
+	else if (msg.rx.ret < 0)
+		return -EINVAL;
 
 	return response.get_max_id.max_id;
 }
@@ -132,7 +143,7 @@ static char *tegra_bpmp_powergate_get_name(struct tegra_bpmp *bpmp,
 	msg.rx.size = sizeof(response);
 
 	err = tegra_bpmp_transfer(bpmp, &msg);
-	if (err < 0)
+	if (err < 0 || msg.rx.ret < 0)
 		return NULL;
 
 	return kstrdup(response.get_name.name, GFP_KERNEL);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/4] firmware: tegra: propagate error code to caller
  2017-09-07  9:31 ` [PATCH 1/4] firmware: tegra: propagate error code to caller Timo Alho
@ 2017-09-21 11:19   ` Jon Hunter
  2017-10-17 10:41   ` Thierry Reding
  1 sibling, 0 replies; 20+ messages in thread
From: Jon Hunter @ 2017-09-21 11:19 UTC (permalink / raw)
  To: Timo Alho, thierry.reding; +Cc: linux-tegra, linux-kernel


On 07/09/17 10:31, Timo Alho wrote:
> Response messages from Tegra BPMP firmware contain an error return
> code as the first word of payload. The error code is used to indicate
> incorrectly formatted request message or use of non-existing resource
> (clk, reset, powergate) identifier. Current implementation of
> tegra_bpmp_transfer() ignores this code and does not pass it to
> caller. Fix this by adding an extra struct member to
> tegra_bpmp_message and populate that with return code.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 22 ++++++++++++++++------
>  include/soc/tegra/bpmp.h      |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index 73ca55b..33683b5 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -194,16 +194,24 @@ static int tegra_bpmp_wait_master_free(struct tegra_bpmp_channel *channel)
>  }
>  
>  static ssize_t __tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
> -					 void *data, size_t size)
> +					 void *data, size_t size, int *ret)
>  {
> +	int err;
> +
>  	if (data && size > 0)
>  		memcpy(data, channel->ib->data, size);
>  
> -	return tegra_ivc_read_advance(channel->ivc);
> +	err = tegra_ivc_read_advance(channel->ivc);
> +	if (err < 0)
> +		return err;
> +
> +	*ret = channel->ib->code;
> +
> +	return 0;
>  }
>  
>  static ssize_t tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
> -				       void *data, size_t size)
> +				       void *data, size_t size, int *ret)
>  {
>  	struct tegra_bpmp *bpmp = channel->bpmp;
>  	unsigned long flags;
> @@ -217,7 +225,7 @@ static ssize_t tegra_bpmp_channel_read(struct tegra_bpmp_channel *channel,
>  	}
>  
>  	spin_lock_irqsave(&bpmp->lock, flags);
> -	err = __tegra_bpmp_channel_read(channel, data, size);
> +	err = __tegra_bpmp_channel_read(channel, data, size, ret);
>  	clear_bit(index, bpmp->threaded.allocated);
>  	spin_unlock_irqrestore(&bpmp->lock, flags);
>  
> @@ -337,7 +345,8 @@ int tegra_bpmp_transfer_atomic(struct tegra_bpmp *bpmp,
>  	if (err < 0)
>  		return err;
>  
> -	return __tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size);
> +	return __tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size,
> +					 &msg->rx.ret);
>  }
>  EXPORT_SYMBOL_GPL(tegra_bpmp_transfer_atomic);
>  
> @@ -371,7 +380,8 @@ int tegra_bpmp_transfer(struct tegra_bpmp *bpmp,
>  	if (err == 0)
>  		return -ETIMEDOUT;
>  
> -	return tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size);
> +	return tegra_bpmp_channel_read(channel, msg->rx.data, msg->rx.size,
> +				       &msg->rx.ret);
>  }
>  EXPORT_SYMBOL_GPL(tegra_bpmp_transfer);
>  
> diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h
> index 9ba6522..57519f4 100644
> --- a/include/soc/tegra/bpmp.h
> +++ b/include/soc/tegra/bpmp.h
> @@ -110,6 +110,7 @@ struct tegra_bpmp_message {
>  	struct {
>  		void *data;
>  		size_t size;
> +		int ret;
>  	} rx;
>  };

Looks good to me ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon


-- 
nvpublic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-09-07  9:31 ` [PATCH 2/4] clk: tegra: check BPMP response return code Timo Alho
@ 2017-09-21 11:21   ` Jon Hunter
  2017-09-29 13:46     ` Timo Alho
  2017-10-17 10:37   ` Thierry Reding
  1 sibling, 1 reply; 20+ messages in thread
From: Jon Hunter @ 2017-09-21 11:21 UTC (permalink / raw)
  To: Timo Alho, thierry.reding; +Cc: linux-tegra, linux-kernel



On 07/09/17 10:31, Timo Alho wrote:
> Check return code in BPMP response message(s). The typical error case
> is when clock operation is attempted with invalid clock identifier.
> 
> Also remove error print from call to clk_get_info() as the
> implementation loops through range of all possible identifier, but the
> operation is expected error out when the clock id is unused.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> index 638ace6..a896692 100644
> --- a/drivers/clk/tegra/clk-bpmp.c
> +++ b/drivers/clk/tegra/clk-bpmp.c
> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>  	struct {
>  		void *data;
>  		size_t size;
> +		int ret;
>  	} rx;
>  };
>  
> @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	struct mrq_clk_request request;
>  	struct tegra_bpmp_message msg;
>  	void *req = &request;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd_and_id = (clk->cmd << 24) | clk->id;
> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	msg.rx.data = clk->rx.data;
>  	msg.rx.size = clk->rx.size;
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;

I assume that the error codes returned do not correlated to the Linux
error codes here. Is that correct? If not we could just return the
actual error code. Otherwise would it be useful to print a message with
the bpmp error code for debug?

> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_clk_prepare(struct clk_hw *hw)
> @@ -414,11 +422,8 @@ static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
>  		struct tegra_bpmp_clk_info *info = &clocks[count];
>  
>  		err = tegra_bpmp_clk_get_info(bpmp, id, info);
> -		if (err < 0) {
> -			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
> -				id, err);
> +		if (err < 0)
>  			continue;
> -		}
>  
>  		if (info->num_parents >= U8_MAX) {
>  			dev_err(bpmp->dev,
> 

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-09-21 11:21   ` Jon Hunter
@ 2017-09-29 13:46     ` Timo Alho
  2017-09-29 14:53       ` Jon Hunter
  0 siblings, 1 reply; 20+ messages in thread
From: Timo Alho @ 2017-09-29 13:46 UTC (permalink / raw)
  To: Jonathan Hunter, thierry.reding; +Cc: linux-tegra, linux-kernel

Hi Jon,

On 21.09.2017 14:21, Jonathan Hunter wrote:
> 
> 
> On 07/09/17 10:31, Timo Alho wrote:
>> Check return code in BPMP response message(s). The typical error case
>> is when clock operation is attempted with invalid clock identifier.
>>
>> Also remove error print from call to clk_get_info() as the
>> implementation loops through range of all possible identifier, but the
>> operation is expected error out when the clock id is unused.
>>
>> Signed-off-by: Timo Alho <talho@nvidia.com>
>> ---
>>   drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
>> index 638ace6..a896692 100644
>> --- a/drivers/clk/tegra/clk-bpmp.c
>> +++ b/drivers/clk/tegra/clk-bpmp.c
>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>   	struct {
>>   		void *data;
>>   		size_t size;
>> +		int ret;
>>   	} rx;
>>   };
>>   
>> @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>>   	struct mrq_clk_request request;
>>   	struct tegra_bpmp_message msg;
>>   	void *req = &request;
>> +	int err;
>>   
>>   	memset(&request, 0, sizeof(request));
>>   	request.cmd_and_id = (clk->cmd << 24) | clk->id;
>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>>   	msg.rx.data = clk->rx.data;
>>   	msg.rx.size = clk->rx.size;
>>   
>> -	return tegra_bpmp_transfer(bpmp, &msg);
>> +	err = tegra_bpmp_transfer(bpmp, &msg);
>> +	if (err < 0)
>> +		return err;
>> +	else if (msg.rx.ret < 0)
>> +		return -EINVAL;
> 
> I assume that the error codes returned do not correlated to the Linux
> error codes here. Is that correct? If not we could just return the
> actual error code. Otherwise would it be useful to print a message with
> the bpmp error code for debug?

The error codes are not 1:1 match with Linux. Unfortunately, printing 
message for debug is not either viable as during clock probing we are 
expecting many of the calls to return -BPMP_EINVAL to indicate that 
particular clock ID is unused.

-Timo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-09-29 13:46     ` Timo Alho
@ 2017-09-29 14:53       ` Jon Hunter
  2017-10-02  8:43         ` Timo Alho
  0 siblings, 1 reply; 20+ messages in thread
From: Jon Hunter @ 2017-09-29 14:53 UTC (permalink / raw)
  To: Timo Alho, thierry.reding; +Cc: linux-tegra, linux-kernel


On 29/09/17 14:46, Timo Alho wrote:
> Hi Jon,
> 
> On 21.09.2017 14:21, Jonathan Hunter wrote:
>>
>>
>> On 07/09/17 10:31, Timo Alho wrote:
>>> Check return code in BPMP response message(s). The typical error case
>>> is when clock operation is attempted with invalid clock identifier.
>>>
>>> Also remove error print from call to clk_get_info() as the
>>> implementation loops through range of all possible identifier, but the
>>> operation is expected error out when the clock id is unused.
>>>
>>> Signed-off-by: Timo Alho <talho@nvidia.com>
>>> ---
>>>   drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
>>> index 638ace6..a896692 100644
>>> --- a/drivers/clk/tegra/clk-bpmp.c
>>> +++ b/drivers/clk/tegra/clk-bpmp.c
>>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>>       struct {
>>>           void *data;
>>>           size_t size;
>>> +        int ret;
>>>       } rx;
>>>   };
>>>   @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct
>>> tegra_bpmp *bpmp,
>>>       struct mrq_clk_request request;
>>>       struct tegra_bpmp_message msg;
>>>       void *req = &request;
>>> +    int err;
>>>         memset(&request, 0, sizeof(request));
>>>       request.cmd_and_id = (clk->cmd << 24) | clk->id;
>>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct
>>> tegra_bpmp *bpmp,
>>>       msg.rx.data = clk->rx.data;
>>>       msg.rx.size = clk->rx.size;
>>>   -    return tegra_bpmp_transfer(bpmp, &msg);
>>> +    err = tegra_bpmp_transfer(bpmp, &msg);
>>> +    if (err < 0)
>>> +        return err;
>>> +    else if (msg.rx.ret < 0)
>>> +        return -EINVAL;
>>
>> I assume that the error codes returned do not correlated to the Linux
>> error codes here. Is that correct? If not we could just return the
>> actual error code. Otherwise would it be useful to print a message with
>> the bpmp error code for debug?
> 
> The error codes are not 1:1 match with Linux. Unfortunately, printing
> message for debug is not either viable as during clock probing we are
> expecting many of the calls to return -BPMP_EINVAL to indicate that
> particular clock ID is unused.

OK. Could it return other errors other than BPMP_EINVAL? I am just
wondering if we need to differentiate between unused and an actual
error? Maybe that is not possible here?

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-09-29 14:53       ` Jon Hunter
@ 2017-10-02  8:43         ` Timo Alho
  2017-10-02 20:23           ` Jon Hunter
  0 siblings, 1 reply; 20+ messages in thread
From: Timo Alho @ 2017-10-02  8:43 UTC (permalink / raw)
  To: Jonathan Hunter, thierry.reding; +Cc: linux-tegra, linux-kernel



On 29.09.2017 17:53, Jonathan Hunter wrote:
> 
> On 29/09/17 14:46, Timo Alho wrote:
>> Hi Jon,
>>
>> On 21.09.2017 14:21, Jonathan Hunter wrote:
>>>
>>>
>>> On 07/09/17 10:31, Timo Alho wrote:
>>>> Check return code in BPMP response message(s). The typical error case
>>>> is when clock operation is attempted with invalid clock identifier.
>>>>
>>>> Also remove error print from call to clk_get_info() as the
>>>> implementation loops through range of all possible identifier, but the
>>>> operation is expected error out when the clock id is unused.
>>>>
>>>> Signed-off-by: Timo Alho <talho@nvidia.com>
>>>> ---
>>>>    drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
>>>> index 638ace6..a896692 100644
>>>> --- a/drivers/clk/tegra/clk-bpmp.c
>>>> +++ b/drivers/clk/tegra/clk-bpmp.c
>>>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>>>        struct {
>>>>            void *data;
>>>>            size_t size;
>>>> +        int ret;
>>>>        } rx;
>>>>    };
>>>>    @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct
>>>> tegra_bpmp *bpmp,
>>>>        struct mrq_clk_request request;
>>>>        struct tegra_bpmp_message msg;
>>>>        void *req = &request;
>>>> +    int err;
>>>>          memset(&request, 0, sizeof(request));
>>>>        request.cmd_and_id = (clk->cmd << 24) | clk->id;
>>>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct
>>>> tegra_bpmp *bpmp,
>>>>        msg.rx.data = clk->rx.data;
>>>>        msg.rx.size = clk->rx.size;
>>>>    -    return tegra_bpmp_transfer(bpmp, &msg);
>>>> +    err = tegra_bpmp_transfer(bpmp, &msg);
>>>> +    if (err < 0)
>>>> +        return err;
>>>> +    else if (msg.rx.ret < 0)
>>>> +        return -EINVAL;
>>>
>>> I assume that the error codes returned do not correlated to the Linux
>>> error codes here. Is that correct? If not we could just return the
>>> actual error code. Otherwise would it be useful to print a message with
>>> the bpmp error code for debug?
>>
>> The error codes are not 1:1 match with Linux. Unfortunately, printing
>> message for debug is not either viable as during clock probing we are
>> expecting many of the calls to return -BPMP_EINVAL to indicate that
>> particular clock ID is unused.
> 
> OK. Could it return other errors other than BPMP_EINVAL? I am just
> wondering if we need to differentiate between unused and an actual
> error? Maybe that is not possible here?

Other error codes are possible (though they are not explicitly 
documented in abi header). It's not easy to differentiate the error code 
at this level: -BPMP_EINVAL is "expected" condition with 
CMD_CLK_GET_ALL_INFO, whereas -BPMP_EINVAL is true error on other commands.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-10-02  8:43         ` Timo Alho
@ 2017-10-02 20:23           ` Jon Hunter
  0 siblings, 0 replies; 20+ messages in thread
From: Jon Hunter @ 2017-10-02 20:23 UTC (permalink / raw)
  To: Timo Alho, thierry.reding; +Cc: linux-tegra, linux-kernel


On 02/10/17 09:43, Timo Alho wrote:
> 
> 
> On 29.09.2017 17:53, Jonathan Hunter wrote:
>>
>> On 29/09/17 14:46, Timo Alho wrote:
>>> Hi Jon,
>>>
>>> On 21.09.2017 14:21, Jonathan Hunter wrote:
>>>>
>>>>
>>>> On 07/09/17 10:31, Timo Alho wrote:
>>>>> Check return code in BPMP response message(s). The typical error case
>>>>> is when clock operation is attempted with invalid clock identifier.
>>>>>
>>>>> Also remove error print from call to clk_get_info() as the
>>>>> implementation loops through range of all possible identifier, but the
>>>>> operation is expected error out when the clock id is unused.
>>>>>
>>>>> Signed-off-by: Timo Alho <talho@nvidia.com>
>>>>> ---
>>>>>    drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-bpmp.c
>>>>> b/drivers/clk/tegra/clk-bpmp.c
>>>>> index 638ace6..a896692 100644
>>>>> --- a/drivers/clk/tegra/clk-bpmp.c
>>>>> +++ b/drivers/clk/tegra/clk-bpmp.c
>>>>> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>>>>>        struct {
>>>>>            void *data;
>>>>>            size_t size;
>>>>> +        int ret;
>>>>>        } rx;
>>>>>    };
>>>>>    @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct
>>>>> tegra_bpmp *bpmp,
>>>>>        struct mrq_clk_request request;
>>>>>        struct tegra_bpmp_message msg;
>>>>>        void *req = &request;
>>>>> +    int err;
>>>>>          memset(&request, 0, sizeof(request));
>>>>>        request.cmd_and_id = (clk->cmd << 24) | clk->id;
>>>>> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct
>>>>> tegra_bpmp *bpmp,
>>>>>        msg.rx.data = clk->rx.data;
>>>>>        msg.rx.size = clk->rx.size;
>>>>>    -    return tegra_bpmp_transfer(bpmp, &msg);
>>>>> +    err = tegra_bpmp_transfer(bpmp, &msg);
>>>>> +    if (err < 0)
>>>>> +        return err;
>>>>> +    else if (msg.rx.ret < 0)
>>>>> +        return -EINVAL;
>>>>
>>>> I assume that the error codes returned do not correlated to the Linux
>>>> error codes here. Is that correct? If not we could just return the
>>>> actual error code. Otherwise would it be useful to print a message with
>>>> the bpmp error code for debug?
>>>
>>> The error codes are not 1:1 match with Linux. Unfortunately, printing
>>> message for debug is not either viable as during clock probing we are
>>> expecting many of the calls to return -BPMP_EINVAL to indicate that
>>> particular clock ID is unused.
>>
>> OK. Could it return other errors other than BPMP_EINVAL? I am just
>> wondering if we need to differentiate between unused and an actual
>> error? Maybe that is not possible here?
> 
> Other error codes are possible (though they are not explicitly
> documented in abi header). It's not easy to differentiate the error code
> at this level: -BPMP_EINVAL is "expected" condition with
> CMD_CLK_GET_ALL_INFO, whereas -BPMP_EINVAL is true error on other commands.

OK, thanks for clarifying.

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] reset: tegra: check BPMP response return code
  2017-09-07  9:31 ` [PATCH 3/4] reset: " Timo Alho
@ 2017-10-02 20:24   ` Jon Hunter
  2017-10-17 10:40   ` Thierry Reding
  1 sibling, 0 replies; 20+ messages in thread
From: Jon Hunter @ 2017-10-02 20:24 UTC (permalink / raw)
  To: Timo Alho, thierry.reding; +Cc: linux-tegra, linux-kernel


On 07/09/17 10:31, Timo Alho wrote:
> Add checks for return code in BPMP response message.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/reset/tegra/reset-bpmp.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/tegra/reset-bpmp.c b/drivers/reset/tegra/reset-bpmp.c
> index 5daf2ee..fac2db6 100644
> --- a/drivers/reset/tegra/reset-bpmp.c
> +++ b/drivers/reset/tegra/reset-bpmp.c
> @@ -23,6 +23,7 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
>  	struct tegra_bpmp *bpmp = to_tegra_bpmp(rstc);
>  	struct mrq_reset_request request;
>  	struct tegra_bpmp_message msg;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd = command;
> @@ -33,7 +34,13 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
>  	msg.tx.data = &request;
>  	msg.tx.size = sizeof(request);
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_reset_module(struct reset_controller_dev *rstc,

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] soc/tegra: bpmp: check BPMP response return code
  2017-09-07  9:31 ` [PATCH 4/4] soc/tegra: bpmp: " Timo Alho
@ 2017-10-02 20:26   ` Jon Hunter
  2017-10-17 10:41   ` Thierry Reding
  1 sibling, 0 replies; 20+ messages in thread
From: Jon Hunter @ 2017-10-02 20:26 UTC (permalink / raw)
  To: Timo Alho, thierry.reding; +Cc: linux-tegra, linux-kernel


On 07/09/17 10:31, Timo Alho wrote:
> Add checks for return code in BPMP response message.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/soc/tegra/powergate-bpmp.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/tegra/powergate-bpmp.c b/drivers/soc/tegra/powergate-bpmp.c
> index 8fc3560..82c7e27 100644
> --- a/drivers/soc/tegra/powergate-bpmp.c
> +++ b/drivers/soc/tegra/powergate-bpmp.c
> @@ -42,6 +42,7 @@ static int tegra_bpmp_powergate_set_state(struct tegra_bpmp *bpmp,
>  {
>  	struct mrq_pg_request request;
>  	struct tegra_bpmp_message msg;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd = CMD_PG_SET_STATE;
> @@ -53,7 +54,13 @@ static int tegra_bpmp_powergate_set_state(struct tegra_bpmp *bpmp,
>  	msg.tx.data = &request;
>  	msg.tx.size = sizeof(request);
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_powergate_get_state(struct tegra_bpmp *bpmp,
> @@ -80,6 +87,8 @@ static int tegra_bpmp_powergate_get_state(struct tegra_bpmp *bpmp,
>  	err = tegra_bpmp_transfer(bpmp, &msg);
>  	if (err < 0)
>  		return PG_STATE_OFF;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
>  
>  	return response.get_state.state;
>  }
> @@ -106,6 +115,8 @@ static int tegra_bpmp_powergate_get_max_id(struct tegra_bpmp *bpmp)
>  	err = tegra_bpmp_transfer(bpmp, &msg);
>  	if (err < 0)
>  		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
>  
>  	return response.get_max_id.max_id;
>  }
> @@ -132,7 +143,7 @@ static char *tegra_bpmp_powergate_get_name(struct tegra_bpmp *bpmp,
>  	msg.rx.size = sizeof(response);
>  
>  	err = tegra_bpmp_transfer(bpmp, &msg);
> -	if (err < 0)
> +	if (err < 0 || msg.rx.ret < 0)
>  		return NULL;
>  
>  	return kstrdup(response.get_name.name, GFP_KERNEL);

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-09-07  9:31 ` [PATCH 2/4] clk: tegra: check BPMP response return code Timo Alho
  2017-09-21 11:21   ` Jon Hunter
@ 2017-10-17 10:37   ` Thierry Reding
  2017-10-21 14:10     ` Stephen Boyd
  1 sibling, 1 reply; 20+ messages in thread
From: Thierry Reding @ 2017-10-17 10:37 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Timo Alho, linux-clk, linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

On Thu, Sep 07, 2017 at 12:31:02PM +0300, Timo Alho wrote:
> Check return code in BPMP response message(s). The typical error case
> is when clock operation is attempted with invalid clock identifier.
> 
> Also remove error print from call to clk_get_info() as the
> implementation loops through range of all possible identifier, but the
> operation is expected error out when the clock id is unused.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Hi Mike, Stephen,

do you mind if I pick this up into the Tegra tree? It has a build
dependency on patch 1/4.

I only now realized that you guys weren't on Cc, hence quoting in
full.

Thierry

> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> index 638ace6..a896692 100644
> --- a/drivers/clk/tegra/clk-bpmp.c
> +++ b/drivers/clk/tegra/clk-bpmp.c
> @@ -55,6 +55,7 @@ struct tegra_bpmp_clk_message {
>  	struct {
>  		void *data;
>  		size_t size;
> +		int ret;
>  	} rx;
>  };
>  
> @@ -64,6 +65,7 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	struct mrq_clk_request request;
>  	struct tegra_bpmp_message msg;
>  	void *req = &request;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd_and_id = (clk->cmd << 24) | clk->id;
> @@ -84,7 +86,13 @@ static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
>  	msg.rx.data = clk->rx.data;
>  	msg.rx.size = clk->rx.size;
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_clk_prepare(struct clk_hw *hw)
> @@ -414,11 +422,8 @@ static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
>  		struct tegra_bpmp_clk_info *info = &clocks[count];
>  
>  		err = tegra_bpmp_clk_get_info(bpmp, id, info);
> -		if (err < 0) {
> -			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
> -				id, err);
> +		if (err < 0)
>  			continue;
> -		}
>  
>  		if (info->num_parents >= U8_MAX) {
>  			dev_err(bpmp->dev,
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] reset: tegra: check BPMP response return code
  2017-09-07  9:31 ` [PATCH 3/4] reset: " Timo Alho
  2017-10-02 20:24   ` Jon Hunter
@ 2017-10-17 10:40   ` Thierry Reding
  2017-10-17 10:52     ` Philipp Zabel
  1 sibling, 1 reply; 20+ messages in thread
From: Thierry Reding @ 2017-10-17 10:40 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: Timo Alho, linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

On Thu, Sep 07, 2017 at 12:31:03PM +0300, Timo Alho wrote:
> Add checks for return code in BPMP response message.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/reset/tegra/reset-bpmp.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Hi Philipp,

Would you provide an Acked-by on this so that I can take it into the
Tegra tree? There's a build dependency on patch 1/4 in the series.

Quoting in full since you were not previously on Cc, unfortunately.

Timo, please remember to always Cc the relevant maintainers.

Thierry

> diff --git a/drivers/reset/tegra/reset-bpmp.c b/drivers/reset/tegra/reset-bpmp.c
> index 5daf2ee..fac2db6 100644
> --- a/drivers/reset/tegra/reset-bpmp.c
> +++ b/drivers/reset/tegra/reset-bpmp.c
> @@ -23,6 +23,7 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
>  	struct tegra_bpmp *bpmp = to_tegra_bpmp(rstc);
>  	struct mrq_reset_request request;
>  	struct tegra_bpmp_message msg;
> +	int err;
>  
>  	memset(&request, 0, sizeof(request));
>  	request.cmd = command;
> @@ -33,7 +34,13 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
>  	msg.tx.data = &request;
>  	msg.tx.size = sizeof(request);
>  
> -	return tegra_bpmp_transfer(bpmp, &msg);
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +	else if (msg.rx.ret < 0)
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int tegra_bpmp_reset_module(struct reset_controller_dev *rstc,
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] soc/tegra: bpmp: check BPMP response return code
  2017-09-07  9:31 ` [PATCH 4/4] soc/tegra: bpmp: " Timo Alho
  2017-10-02 20:26   ` Jon Hunter
@ 2017-10-17 10:41   ` Thierry Reding
  1 sibling, 0 replies; 20+ messages in thread
From: Thierry Reding @ 2017-10-17 10:41 UTC (permalink / raw)
  To: Timo Alho; +Cc: linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

On Thu, Sep 07, 2017 at 12:31:04PM +0300, Timo Alho wrote:
> Add checks for return code in BPMP response message.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/soc/tegra/powergate-bpmp.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/4] firmware: tegra: propagate error code to caller
  2017-09-07  9:31 ` [PATCH 1/4] firmware: tegra: propagate error code to caller Timo Alho
  2017-09-21 11:19   ` Jon Hunter
@ 2017-10-17 10:41   ` Thierry Reding
  1 sibling, 0 replies; 20+ messages in thread
From: Thierry Reding @ 2017-10-17 10:41 UTC (permalink / raw)
  To: Timo Alho; +Cc: linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Thu, Sep 07, 2017 at 12:31:01PM +0300, Timo Alho wrote:
> Response messages from Tegra BPMP firmware contain an error return
> code as the first word of payload. The error code is used to indicate
> incorrectly formatted request message or use of non-existing resource
> (clk, reset, powergate) identifier. Current implementation of
> tegra_bpmp_transfer() ignores this code and does not pass it to
> caller. Fix this by adding an extra struct member to
> tegra_bpmp_message and populate that with return code.
> 
> Signed-off-by: Timo Alho <talho@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 22 ++++++++++++++++------
>  include/soc/tegra/bpmp.h      |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] reset: tegra: check BPMP response return code
  2017-10-17 10:40   ` Thierry Reding
@ 2017-10-17 10:52     ` Philipp Zabel
  2017-10-17 11:49       ` Thierry Reding
  0 siblings, 1 reply; 20+ messages in thread
From: Philipp Zabel @ 2017-10-17 10:52 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Timo Alho, linux-tegra, linux-kernel

Hi Thierry,

On Tue, 2017-10-17 at 12:40 +0200, Thierry Reding wrote:
> On Thu, Sep 07, 2017 at 12:31:03PM +0300, Timo Alho wrote:
> > Add checks for return code in BPMP response message.
> > 
> > Signed-off-by: Timo Alho <talho@nvidia.com>
> > ---
> >  drivers/reset/tegra/reset-bpmp.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> Hi Philipp,
> 
> Would you provide an Acked-by on this so that I can take it into the
> Tegra tree? There's a build dependency on patch 1/4 in the series.

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

to take this via the Tegra tree.

> Quoting in full since you were not previously on Cc, unfortunately.
> 
> Timo, please remember to always Cc the relevant maintainers.
> 
> Thierry
> 
> > diff --git a/drivers/reset/tegra/reset-bpmp.c b/drivers/reset/tegra/reset-bpmp.c
> > index 5daf2ee..fac2db6 100644
> > --- a/drivers/reset/tegra/reset-bpmp.c
> > +++ b/drivers/reset/tegra/reset-bpmp.c
> > @@ -23,6 +23,7 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
> >  	struct tegra_bpmp *bpmp = to_tegra_bpmp(rstc);
> >  	struct mrq_reset_request request;
> >  	struct tegra_bpmp_message msg;
> > +	int err;
> >  
> >  	memset(&request, 0, sizeof(request));
> >  	request.cmd = command;
> > @@ -33,7 +34,13 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
> >  	msg.tx.data = &request;
> >  	msg.tx.size = sizeof(request);
> >  
> > -	return tegra_bpmp_transfer(bpmp, &msg);
> > +	err = tegra_bpmp_transfer(bpmp, &msg);
> > +	if (err < 0)
> > +		return err;
> > +	else if (msg.rx.ret < 0)
> > +		return -EINVAL;

I don't really understand why you complicate the call sites like this
instead of just letting tegra_bmp_transfer return -EINVAL if msg.rx.ret
< 0, but I haven't seen the other patches.

regards
Philipp

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] reset: tegra: check BPMP response return code
  2017-10-17 10:52     ` Philipp Zabel
@ 2017-10-17 11:49       ` Thierry Reding
  0 siblings, 0 replies; 20+ messages in thread
From: Thierry Reding @ 2017-10-17 11:49 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: Timo Alho, linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

On Tue, Oct 17, 2017 at 12:52:17PM +0200, Philipp Zabel wrote:
> Hi Thierry,
> 
> On Tue, 2017-10-17 at 12:40 +0200, Thierry Reding wrote:
> > On Thu, Sep 07, 2017 at 12:31:03PM +0300, Timo Alho wrote:
> > > Add checks for return code in BPMP response message.
> > > 
> > > Signed-off-by: Timo Alho <talho@nvidia.com>
> > > ---
> > >  drivers/reset/tegra/reset-bpmp.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > Hi Philipp,
> > 
> > Would you provide an Acked-by on this so that I can take it into the
> > Tegra tree? There's a build dependency on patch 1/4 in the series.
> 
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> to take this via the Tegra tree.

Thanks!

> > Quoting in full since you were not previously on Cc, unfortunately.
> > 
> > Timo, please remember to always Cc the relevant maintainers.
> > 
> > Thierry
> > 
> > > diff --git a/drivers/reset/tegra/reset-bpmp.c b/drivers/reset/tegra/reset-bpmp.c
> > > index 5daf2ee..fac2db6 100644
> > > --- a/drivers/reset/tegra/reset-bpmp.c
> > > +++ b/drivers/reset/tegra/reset-bpmp.c
> > > @@ -23,6 +23,7 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
> > >  	struct tegra_bpmp *bpmp = to_tegra_bpmp(rstc);
> > >  	struct mrq_reset_request request;
> > >  	struct tegra_bpmp_message msg;
> > > +	int err;
> > >  
> > >  	memset(&request, 0, sizeof(request));
> > >  	request.cmd = command;
> > > @@ -33,7 +34,13 @@ static int tegra_bpmp_reset_common(struct reset_controller_dev *rstc,
> > >  	msg.tx.data = &request;
> > >  	msg.tx.size = sizeof(request);
> > >  
> > > -	return tegra_bpmp_transfer(bpmp, &msg);
> > > +	err = tegra_bpmp_transfer(bpmp, &msg);
> > > +	if (err < 0)
> > > +		return err;
> > > +	else if (msg.rx.ret < 0)
> > > +		return -EINVAL;
> 
> I don't really understand why you complicate the call sites like this
> instead of just letting tegra_bmp_transfer return -EINVAL if msg.rx.ret
> < 0, but I haven't seen the other patches.

Timo replied to a similar question from Jon here:

	https://patchwork.ozlabs.org/patch/810934/

Essentially this boils down to that there are cases where we may want to
know the exact error because it is relevant. In this case, we don't, but
BPMP provides a number of other services where the errors may be useful.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] clk: tegra: check BPMP response return code
  2017-10-17 10:37   ` Thierry Reding
@ 2017-10-21 14:10     ` Stephen Boyd
  0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2017-10-21 14:10 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Michael Turquette, Timo Alho, linux-clk, linux-tegra, linux-kernel

On 10/17, Thierry Reding wrote:
> On Thu, Sep 07, 2017 at 12:31:02PM +0300, Timo Alho wrote:
> > Check return code in BPMP response message(s). The typical error case
> > is when clock operation is attempted with invalid clock identifier.
> > 
> > Also remove error print from call to clk_get_info() as the
> > implementation loops through range of all possible identifier, but the
> > operation is expected error out when the clock id is unused.
> > 
> > Signed-off-by: Timo Alho <talho@nvidia.com>
> > ---
> >  drivers/clk/tegra/clk-bpmp.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Hi Mike, Stephen,
> 
> do you mind if I pick this up into the Tegra tree? It has a build
> dependency on patch 1/4.
> 
> I only now realized that you guys weren't on Cc, hence quoting in
> full.
> 

No worries

Acked-by: Stephen Boyd <sboyd@codeaurora.org>


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2017-10-21 14:10 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-07  9:31 [PATCH 0/4] firmware: tegra: add checks for BPMP error return code Timo Alho
2017-09-07  9:31 ` [PATCH 1/4] firmware: tegra: propagate error code to caller Timo Alho
2017-09-21 11:19   ` Jon Hunter
2017-10-17 10:41   ` Thierry Reding
2017-09-07  9:31 ` [PATCH 2/4] clk: tegra: check BPMP response return code Timo Alho
2017-09-21 11:21   ` Jon Hunter
2017-09-29 13:46     ` Timo Alho
2017-09-29 14:53       ` Jon Hunter
2017-10-02  8:43         ` Timo Alho
2017-10-02 20:23           ` Jon Hunter
2017-10-17 10:37   ` Thierry Reding
2017-10-21 14:10     ` Stephen Boyd
2017-09-07  9:31 ` [PATCH 3/4] reset: " Timo Alho
2017-10-02 20:24   ` Jon Hunter
2017-10-17 10:40   ` Thierry Reding
2017-10-17 10:52     ` Philipp Zabel
2017-10-17 11:49       ` Thierry Reding
2017-09-07  9:31 ` [PATCH 4/4] soc/tegra: bpmp: " Timo Alho
2017-10-02 20:26   ` Jon Hunter
2017-10-17 10:41   ` Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).