* [PATCH] firmware: arm_scmi: check return value of idr_find
@ 2019-03-09 4:02 Kangjie Lu
2019-03-23 2:18 ` Kangjie Lu
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Kangjie Lu @ 2019-03-09 4:02 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Sudeep Holla, linux-arm-kernel, linux-kernel
idr_find may return NULL, so check its return value and return an
error code.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/firmware/arm_scmi/driver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 8f952f2f1a29..35faa203d549 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -709,6 +709,8 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
if (scmi_mailbox_check(np)) {
cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
+ if (!cinfo)
+ return -EINVAL;
goto idr_alloc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware: arm_scmi: check return value of idr_find
2019-03-09 4:02 [PATCH] firmware: arm_scmi: check return value of idr_find Kangjie Lu
@ 2019-03-23 2:18 ` Kangjie Lu
2019-03-25 11:54 ` Steven Price
2019-04-10 16:25 ` Sudeep Holla
2 siblings, 0 replies; 7+ messages in thread
From: Kangjie Lu @ 2019-03-23 2:18 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Sudeep Holla, linux-arm-kernel, linux-kernel
> On Mar 8, 2019, at 10:02 PM, Kangjie Lu <kjlu@umn.edu> wrote:
>
> idr_find may return NULL, so check its return value and return an
> error code.
Can someone review this patch? Thanks.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
> drivers/firmware/arm_scmi/driver.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 8f952f2f1a29..35faa203d549 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -709,6 +709,8 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
>
> if (scmi_mailbox_check(np)) {
> cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
> + if (!cinfo)
> + return -EINVAL;
> goto idr_alloc;
> }
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware: arm_scmi: check return value of idr_find
2019-03-09 4:02 [PATCH] firmware: arm_scmi: check return value of idr_find Kangjie Lu
2019-03-23 2:18 ` Kangjie Lu
@ 2019-03-25 11:54 ` Steven Price
2019-03-25 20:48 ` Kangjie Lu
2019-04-10 16:25 ` Sudeep Holla
2 siblings, 1 reply; 7+ messages in thread
From: Steven Price @ 2019-03-25 11:54 UTC (permalink / raw)
To: Kangjie Lu; +Cc: Sudeep Holla, linux-kernel, linux-arm-kernel, pakki001
On 09/03/2019 04:02, Kangjie Lu wrote:
> idr_find may return NULL, so check its return value and return an
> error code.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
> drivers/firmware/arm_scmi/driver.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 8f952f2f1a29..35faa203d549 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -709,6 +709,8 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
>
> if (scmi_mailbox_check(np)) {
> cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
> + if (!cinfo)
> + return -EINVAL;
> goto idr_alloc;
> }
>
>
I don't believe this situation can ever actually occur. The
SCMI_PROTOCOL_BASE id is reserved just after the IDR is created in
scmi_probe() in the first call to scmi_mbox_chan_setup(). That first
call can't enter the above if() block because the scmi_mailbox_check()
call has already been checked in scmi_probe(). It is only when
processing the children in subsequent calls to scmi_mbox_chan_setup()
that the check is on another node and so might fail. But by this time
the SCMI_PROTOCOL_BASE id must exist.
So there shouldn't be any way of reaching the situation where the IDR
doesn't contain SCMI_PROTOCOL_BASE and idr_find() returns NULL.
Having said that it did take me a while to understand the code well
enough to reason that, and the above return should be handled
appropriately, so you can add my Reviewed-by. But it would be worth
expanding the commit message pointing out that there isn't actually a
bug (yet).
Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] firmware: arm_scmi: check return value of idr_find
2019-03-25 11:54 ` Steven Price
@ 2019-03-25 20:48 ` Kangjie Lu
2019-03-26 11:34 ` Steven Price
2019-04-10 16:31 ` Sudeep Holla
0 siblings, 2 replies; 7+ messages in thread
From: Kangjie Lu @ 2019-03-25 20:48 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Sudeep Holla, linux-arm-kernel, linux-kernel
Thanks for Steven Price's review of this patch. In the current code,
idr_find won't return NULL because the SCMI_PROTOCOL_BASE id must
exist. However, it might return NULL in the future code if the check
is on another node while processing the children in subsequent calls
to scmi_mbox_chan_setup().
Therefore, the patch conservatively checks the return value and
returns -EINVAL when it indeed failed.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Reviewed-by: Steven Price <steven.price@arm.com>
---
drivers/firmware/arm_scmi/driver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 8f952f2f1a29..35faa203d549 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -709,6 +709,8 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
if (scmi_mailbox_check(np)) {
cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
+ if (!cinfo)
+ return -EINVAL;
goto idr_alloc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware: arm_scmi: check return value of idr_find
2019-03-25 20:48 ` Kangjie Lu
@ 2019-03-26 11:34 ` Steven Price
2019-04-10 16:31 ` Sudeep Holla
1 sibling, 0 replies; 7+ messages in thread
From: Steven Price @ 2019-03-26 11:34 UTC (permalink / raw)
To: Kangjie Lu; +Cc: Sudeep Holla, linux-kernel, linux-arm-kernel, pakki001
On 25/03/2019 20:48, Kangjie Lu wrote:
> Thanks for Steven Price's review of this patch. In the current code,
There's no need to include a "thanks" message in the commit message -
the "Reviewed-by" tag is sufficient. Please also remember to include an
appropriate version tag in the subject - this should be "v2".
Thanks,
Steve
> idr_find won't return NULL because the SCMI_PROTOCOL_BASE id must
> exist. However, it might return NULL in the future code if the check
> is on another node while processing the children in subsequent calls
> to scmi_mbox_chan_setup().
> Therefore, the patch conservatively checks the return value and
> returns -EINVAL when it indeed failed.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 8f952f2f1a29..35faa203d549 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -709,6 +709,8 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
>
> if (scmi_mailbox_check(np)) {
> cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
> + if (!cinfo)
> + return -EINVAL;
> goto idr_alloc;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware: arm_scmi: check return value of idr_find
2019-03-09 4:02 [PATCH] firmware: arm_scmi: check return value of idr_find Kangjie Lu
2019-03-23 2:18 ` Kangjie Lu
2019-03-25 11:54 ` Steven Price
@ 2019-04-10 16:25 ` Sudeep Holla
2 siblings, 0 replies; 7+ messages in thread
From: Sudeep Holla @ 2019-04-10 16:25 UTC (permalink / raw)
To: Kangjie Lu; +Cc: pakki001, linux-arm-kernel, linux-kernel, Sudeep Holla
On Fri, Mar 08, 2019 at 10:02:39PM -0600, Kangjie Lu wrote:
> idr_find may return NULL, so check its return value and return an
> error code.
>
I can't imagine a scenario leading to this. Steven has already explained
why, so I won't repeat the same here. Did you hit this issue ?
If so I would like to know details, so that we can bail out much earlier
where the failure occurs than proceed until this point.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] firmware: arm_scmi: check return value of idr_find
2019-03-25 20:48 ` Kangjie Lu
2019-03-26 11:34 ` Steven Price
@ 2019-04-10 16:31 ` Sudeep Holla
1 sibling, 0 replies; 7+ messages in thread
From: Sudeep Holla @ 2019-04-10 16:31 UTC (permalink / raw)
To: Kangjie Lu
Cc: pakki001, linux-arm-kernel, linux-kernel, Sudeep Holla, Steven Price
On Mon, Mar 25, 2019 at 03:48:22PM -0500, Kangjie Lu wrote:
You should mark this v2 id you changed any code or commit message or
added any tags. Just FYI for future.
> Thanks for Steven Price's review of this patch. In the current code,
> idr_find won't return NULL because the SCMI_PROTOCOL_BASE id must
> exist. However, it might return NULL in the future code if the check
> is on another node while processing the children in subsequent calls
> to scmi_mbox_chan_setup().
I don't understand the reference here to future code here. If you have
out of tree code that results in such a scenario, please share or
provide the details on that.
As I mentioned in previous mail, I would bail out much earlier if
SCMI_PROTOCOL_BASE is not allocated IDR and not reach to this point.
We continue even if scmi_create_protocol_device fails for few child
devices.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-10 16:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-09 4:02 [PATCH] firmware: arm_scmi: check return value of idr_find Kangjie Lu
2019-03-23 2:18 ` Kangjie Lu
2019-03-25 11:54 ` Steven Price
2019-03-25 20:48 ` Kangjie Lu
2019-03-26 11:34 ` Steven Price
2019-04-10 16:31 ` Sudeep Holla
2019-04-10 16:25 ` Sudeep Holla
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).