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
Add checks for return code in BPMP response message.
Signed-off-by: Timo Alho <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add checks for return code in BPMP response message.
Signed-off-by: Timo Alho <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Cheers
Jon
--
nvpublic
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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