[PATCH] platform/mellanox: mlxreg-lc: Check before variable dereferenced

[Yu Sun]

there is a warning saying variable dereferenced before
check 'data->notifier' in line 828.
add "for(data->notifier)" before variable deferenced.

Signed-off-by: Yu Sun <u2021...@hust.edu.cn>
---
drivers/platform/mellanox/mlxreg-lc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/mellanox/mlxreg-lc.c b/drivers/platform/mellanox/mlxreg-lc.c
index 43d119e3a473..e92add40750b 100644
--- a/drivers/platform/mellanox/mlxreg-lc.c
+++ b/drivers/platform/mellanox/mlxreg-lc.c
@@ -824,9 +824,12 @@ static int mlxreg_lc_probe(struct platform_device *pdev)
return -ENOMEM;

mutex_init(&mlxreg_lc->lock);
- /* Set event notification callback. */
- data->notifier->user_handler = mlxreg_lc_event_handler;
- data->notifier->handle = mlxreg_lc;
+
+ if (data->notifier) {
+ /* Set event notification callback. */
+ data->notifier->user_handler = mlxreg_lc_event_handler;
+ data->notifier->handle = mlxreg_lc;
+ }

data->hpdev.adapter = i2c_get_adapter(data->hpdev.nr);
if (!data->hpdev.adapter) {
--
2.42.0

[ktest...@126.com]

Hi, Yu Sun
This email is automatically replied by KTestRobot(version 1.0). Please do not reply to this email.
If you have any questions or suggestions about KTestRobot, please contact Lishuchang <U2020...@hust.edu.cn>

--- Changed Paths ---
drivers/platform/mellanox/mlxreg-lc.c

--- Log Message ---

there is a warning saying variable dereferenced before
check 'data->notifier' in line 828.
add "for(data->notifier)" before variable deferenced.

--- Test Result ---
*** CheckPatch PASS ***
*** ApplyTolinux-next PASS ***
*** ApplyTomainline PASS ***
*** BuildCheck PASS ***
*** CheckCocci PASS ***
*** CheckCppcheck PASS ***

--
KTestRobot(version 1.0)

[Dongliang Mu]

On 11/16/23 17:42, Yu Sun wrote:
> there is a warning saying variable dereferenced before
> check 'data->notifier' in line 828.
> add "for(data->notifier)" before variable deferenced.
>
> Signed-off-by: Yu Sun <u2021...@hust.edu.cn>

+cc Dan

This patch looks fine. However, can you help take a look, Dan?

[Dan Carpenter]

On Sat, Nov 18, 2023 at 11:57:42AM +0800, Dongliang Mu wrote:
>
> On 11/16/23 17:42, Yu Sun wrote:
> > there is a warning saying variable dereferenced before
> > check 'data->notifier' in line 828.
> > add "for(data->notifier)" before variable deferenced.
> >
> > Signed-off-by: Yu Sun <u2021...@hust.edu.cn>
>
> +cc Dan
>
> This patch looks fine. However, can you help take a look, Dan?
>

Here is my review process for this patch:
1) This is not a diff I can apply so it's a little complicated to
review inline.
2) The NULL checks are in the error handling path but you're changing
the success path. Normally the success path has been tested so
it suggests that the NULL check is not necessary.
3) I wanted to look at the Fixes tag to see where the warning was
introduced but there was no Fixes tag. We need one.
4) The warning was introduced by commit b4b830a34d804
("platform/mellanox: mlxreg-lc: Fix error flow and extend verbosity")
Honestly, I'm surprised that commit was merged because it does a
bunch of unrelated things.

However, the commit message and comments explain that the NULL check
is only required because of hotplug events. So the NULL check is
only necessary after we enable hotplug. But I don't know when that
is or if the patch is correct.

A hotplug event would be unlikely this early in probe so that
explains why no one has seen this bug during testing.
5) Normally kbuild or I report these kinds of Smatch warnings so did I
do that? Let me look on lore.kernel.org.
There was a report but no response.
https://lore.kernel.org/all/b6a28173-636b-a51f...@redhat.com/

I think the NULL check is *probably* required. If not, then it's at
least harmless. It's slightly awkward that there isn't a way to set up
a notifier except in probe... But it needs a Fixes tag.

regards,
dan carpenter