Fixes for issues with coverity, locking, redundant checks
Subsystems (2)
Vadim Pasternak
maintainercover Fixes for issues with coverity, locking, redundant checks
The patch set includes fixes for several issues:
Patch #1: fixes coverity issue.
Patch #2: fixes locking dependency issue.
Patches #3-#4: remove unnecessary code.
Vadim Pasternak (4):
platform/mellanox: mlxreg-lc: Fix coverity warning platform/mellanox: mlxreg-lc: Fix locking issue platform/mellanox: Remove unnecessary code platform/mellanox: Remove redundant 'NULL' check
drivers/platform/mellanox/mlxreg-lc.c | 53 ++++++++++++++++----------- 1 file changed, 31 insertions(+), 22 deletions(-)
Signature · 1 line
2.20.1
Vadim Pasternak
maintainerPATCH 1/4 platform/mellanox: mlxreg-lc: Fix coverity warning
Fix smatch warning:drivers/platform/mellanox/mlxreg-lc.c:866 mlxreg_lc_probe() warn: passing zero to 'PTR_ERR' by removing 'err = PTR_ERR(regmap)'.
--- drivers/platform/mellanox/mlxreg-lc.c | 1 - 1 file changed, 1 deletion(-)
drivers/platform/mellanox/mlxreg-lc.c +0 -1
@@ -863,7 +863,6 @@ static int mlxreg_lc_probe(struct platform_device *pdev)if (err) {dev_err (&pdev->dev,"Failed to sync regmap for client %s at bus %d at addr 0x%02x\n" , data->hpdev.brdinfo->type, data->hpdev.nr, data->hpdev.brdinfo->addr); - err =PTR_ERR (regmap); goto regcache_sync_fail; }
Signature · 1 line
2.20.1
Vadim Pasternak
maintainerPATCH 2/4 platform/mellanox: mlxreg-lc: Fix locking issue
In reply to Vadim PasternakFix locking issues:
- mlxreg_lc_state_update() takes a lock when set or clear "MLXREG_LC_POWERED".
- All the devices can be deleted before MLXREG_LC_POWERED flag is cleared.
To fix it:
- Add lock() / unlock() at the beginning / end of mlxreg_lc_event_handler() and remove locking from mlxreg_lc_power_on_off() and mlxreg_lc_enable_disable()
- Add locked version of mlxreg_lc_state_update() - mlxreg_lc_state_update_locked() for using outside mlxreg_lc_event_handler().
(2) Remove redundant NULL check for of if 'data->notifier'.
--- drivers/platform/mellanox/mlxreg-lc.c | 37 ++++++++++++++++++--------- 1 file changed, 25 insertions(+), 12 deletions(-)
drivers/platform/mellanox/mlxreg-lc.c +25 -12
@@ -460,8 +460,6 @@ static int mlxreg_lc_power_on_off(struct mlxreg_lc *mlxreg_lc, u8 action)u32 regval;int err; -mutex_lock (&mlxreg_lc->lock); - err =regmap_read (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_pwr, ®val);if (err) goto regmap_read_fail;
@@ -474,7 +472,6 @@ static int mlxreg_lc_power_on_off(struct mlxreg_lc *mlxreg_lc, u8 action) err =regmap_write (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_pwr, regval); regmap_read_fail: -mutex_unlock (&mlxreg_lc->lock);return err; }
@@ -491,8 +488,6 @@ static int mlxreg_lc_enable_disable(struct mlxreg_lc *mlxreg_lc, bool action)* line card which is already has been enabled. Disabling does not affect the disabled line * card. */ -mutex_lock (&mlxreg_lc->lock); - err =regmap_read (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_ena, ®val);if (err) goto regmap_read_fail;
@@ -505,7 +500,6 @@ static int mlxreg_lc_enable_disable(struct mlxreg_lc *mlxreg_lc, bool action) err =regmap_write (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_ena, regval); regmap_read_fail: -mutex_unlock (&mlxreg_lc->lock);return err; }
@@ -537,6 +531,15 @@ mlxreg_lc_sn4800_c16_config_init(struct mlxreg_lc *mlxreg_lc, void *regmap,static void mlxreg_lc_state_update (struct mlxreg_lc *mlxreg_lc,enum mlxreg_lc_state state,u8 action) +{ +if (action) + mlxreg_lc->state |= state; +else + mlxreg_lc->state &= ~state; +} + +static void +mlxreg_lc_state_update_locked (struct mlxreg_lc *mlxreg_lc,enum mlxreg_lc_state state,u8 action) {mutex_lock (&mlxreg_lc->lock);
@@ -560,8 +563,11 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,dev_info (mlxreg_lc->dev,"linecard#%d state %d event kind %d action %d\n" , mlxreg_lc->data->slot, mlxreg_lc->state, kind, action); -if (!(mlxreg_lc->state &MLXREG_LC_INITIALIZED )) +mutex_lock (&mlxreg_lc->lock); +if (!(mlxreg_lc->state &MLXREG_LC_INITIALIZED )) { +mutex_unlock (&mlxreg_lc->lock);return 0 ; + }switch (kind) {case MLXREG_HOTPLUG_LC_SYNCED :
@@ -574,7 +580,7 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,if (!(mlxreg_lc->state &MLXREG_LC_POWERED ) && action) { err =mlxreg_lc_power_on_off (mlxreg_lc,1 );if (err) -return err; + goto mlxreg_lc_power_on_off_fail; }/* In case line card is configured - enable it. */ if (mlxreg_lc->state &MLXREG_LC_CONFIGURED && action)
@@ -588,12 +594,13 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,/* In case line card is configured - enable it. */ if (mlxreg_lc->state &MLXREG_LC_CONFIGURED ) err =mlxreg_lc_enable_disable (mlxreg_lc,1 ); +mutex_unlock (&mlxreg_lc->lock);return err; } err =mlxreg_lc_create_static_devices (mlxreg_lc, mlxreg_lc->main_devs, mlxreg_lc->main_devs_num);if (err) -return err; + goto mlxreg_lc_create_static_devices_fail;/* In case line card is already in ready state - enable it. */ if (mlxreg_lc->state &MLXREG_LC_CONFIGURED )
@@ -620,6 +627,10 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,break ; } +mlxreg_lc_power_on_off_fail: +mlxreg_lc_create_static_devices_fail: +mutex_unlock (&mlxreg_lc->lock); +return err; }
@@ -665,7 +676,7 @@ static int mlxreg_lc_completion_notify(void *handle, struct i2c_adapter *parent,if (err) goto mlxreg_lc_create_static_devices_failed; -mlxreg_lc_state_update (mlxreg_lc,MLXREG_LC_POWERED ,1 ); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_POWERED ,1 ); }/* Verify if line card is synchronized. */
@@ -676,7 +687,7 @@ static int mlxreg_lc_completion_notify(void *handle, struct i2c_adapter *parent,/* Power on line card if necessary. */ if (regval & mlxreg_lc->data->mask) { mlxreg_lc->state |=MLXREG_LC_SYNCED ; -mlxreg_lc_state_update (mlxreg_lc,MLXREG_LC_SYNCED ,1 ); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_SYNCED ,1 );if (mlxreg_lc->state & ~MLXREG_LC_POWERED ) { err =mlxreg_lc_power_on_off (mlxreg_lc,1 );if (err)
@@ -684,7 +695,7 @@ static int mlxreg_lc_completion_notify(void *handle, struct i2c_adapter *parent, } } -mlxreg_lc_state_update (mlxreg_lc,MLXREG_LC_INITIALIZED ,1 ); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_INITIALIZED ,1 );return 0 ;
@@ -904,6 +915,8 @@ static int mlxreg_lc_remove(struct platform_device *pdev)struct mlxreg_core_data *data =dev_get_platdata (&pdev->dev);struct mlxreg_lc *mlxreg_lc =platform_get_drvdata (pdev); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_INITIALIZED ,0 ); +/* * Probing and removing are invoked by hotplug events raised upon line card insertion and * removing. If probing procedure fails all data is cleared. However, hotplug event still
Signature · 1 line
2.20.1
Re: [PATCH platform 2/4] platform/mellanox: mlxreg-lc: Fix locking issue
Hi,
Quoted 77 lines · Vadim Pasternak
Fix locking issues:
-mlxreg_lc_state_update()takes a lock when set or clear "MLXREG_LC_POWERED".
- All the devices can be deleted beforeMLXREG_LC_POWEREDflag is cleared.To fix it:
- Addlock()/unlock()at the beginning / end ofmlxreg_lc_event_handler()and remove locking frommlxreg_lc_power_on_off()andmlxreg_lc_enable_disable()
- Add locked version ofmlxreg_lc_state_update()-mlxreg_lc_state_update_locked()for using outsidemlxreg_lc_event_handler().(2) Remove redundant NULL check for of if '
data->notifier'.Fixes 62f9529b8d5c87b ("platform/mellanox: mlxreg-lc: Add initial support for Nvidia line card devices")Reported-by Dan Carpenter <[email protected]>Signed-off-by Vadim Pasternak <[email protected]>--- drivers/platform/mellanox/mlxreg-lc.c | 37 ++++++++++++++++++--------- 1 file changed, 25 insertions(+), 12 deletions(-)drivers/platform/mellanox/mlxreg-lc.c +9
-6@@ -460,8 +460,6 @@ static int mlxreg_lc_power_on_off(struct mlxreg_lc *mlxreg_lc, u8 action)u32 regval;int err; -mutex_lock (&mlxreg_lc->lock); - err =regmap_read (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_pwr, ®val);if (err) goto regmap_read_fail;@@ -474,7 +472,6 @@ static int mlxreg_lc_power_on_off(struct mlxreg_lc *mlxreg_lc, u8 action) err =regmap_write (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_pwr, regval); regmap_read_fail: -mutex_unlock (&mlxreg_lc->lock);return err; }@@ -491,8 +488,6 @@ static int mlxreg_lc_enable_disable(struct mlxreg_lc *mlxreg_lc, bool action)* line card which is already has been enabled. Disabling does not affect the disabled line * card. */ -mutex_lock (&mlxreg_lc->lock); - err =regmap_read (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_ena, ®val);if (err) goto regmap_read_fail;@@ -505,7 +500,6 @@ static int mlxreg_lc_enable_disable(struct mlxreg_lc *mlxreg_lc, bool action) err =regmap_write (mlxreg_lc->par_regmap, mlxreg_lc->data->reg_ena, regval); regmap_read_fail: -mutex_unlock (&mlxreg_lc->lock);return err; }@@ -537,6 +531,15 @@ mlxreg_lc_sn4800_c16_config_init(struct mlxreg_lc *mlxreg_lc, void *regmap,static void mlxreg_lc_state_update (struct mlxreg_lc *mlxreg_lc,enum mlxreg_lc_state state,u8 action) +{ +if (action) + mlxreg_lc->state |= state; +else + mlxreg_lc->state &= ~state; +} + +static void +mlxreg_lc_state_update_locked (struct mlxreg_lc *mlxreg_lc,enum mlxreg_lc_state state,u8 action) {mutex_lock (&mlxreg_lc->lock);
drivers/platform/mellanox/mlxreg-lc.c static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind, · line 568
Quoted 7 lines
drivers/platform/mellanox/mlxreg-lc.c +3
-1@@ -560,8 +563,11 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,dev_info (mlxreg_lc->dev,"linecard#%d state %d event kind %d action %d\n" , mlxreg_lc->data->slot, mlxreg_lc->state, kind, action); -if (!(mlxreg_lc->state &MLXREG_LC_INITIALIZED )) +mutex_lock (&mlxreg_lc->lock); +if (!(mlxreg_lc->state &MLXREG_LC_INITIALIZED )) { +mutex_unlock (&mlxreg_lc->lock);
So here you are unlocking before return.
drivers/platform/mellanox/mlxreg-lc.c static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind, · line 583
Quoted 11 lines
drivers/platform/mellanox/mlxreg-lc.c +2
-1return 0 ; + }switch (kind) {case MLXREG_HOTPLUG_LC_SYNCED :@@ -574,7 +580,7 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,if (!(mlxreg_lc->state &MLXREG_LC_POWERED ) && action) { err =mlxreg_lc_power_on_off (mlxreg_lc,1 );if (err) -return err; + goto mlxreg_lc_power_on_off_fail;
Yet here you use a goto (better IMHO).
drivers/platform/mellanox/mlxreg-lc.c static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind, · line 597
Quoted 8 lines
drivers/platform/mellanox/mlxreg-lc.c +1
-0}/* In case line card is configured - enable it. */ if (mlxreg_lc->state &MLXREG_LC_CONFIGURED && action)@@ -588,12 +594,13 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,/* In case line card is configured - enable it. */ if (mlxreg_lc->state &MLXREG_LC_CONFIGURED ) err =mlxreg_lc_enable_disable (mlxreg_lc,1 ); +mutex_unlock (&mlxreg_lc->lock);
and here is another unlocking before return.
drivers/platform/mellanox/mlxreg-lc.c static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind, · line 603
Quoted 7 lines
drivers/platform/mellanox/mlxreg-lc.c +1
-1return err; } err =mlxreg_lc_create_static_devices (mlxreg_lc, mlxreg_lc->main_devs, mlxreg_lc->main_devs_num);if (err) -return err; + goto mlxreg_lc_create_static_devices_fail;
and here is an other goto.
This is not very consistent. Can you please switch to goto-s everywhere? Preferable with just a simply single "out" label?
I always prefer the goto-s here so that the function has a single entry + exit point and we can easily see that the lock + unlock is balanced since there is only 1 of each.
Regards,
Hans
Quoted 50 lines
drivers/platform/mellanox/mlxreg-lc.c +9
-3/* In case line card is already in ready state - enable it. */ if (mlxreg_lc->state &MLXREG_LC_CONFIGURED )@@ -620,6 +627,10 @@ static int mlxreg_lc_event_handler(void *handle, enum mlxreg_hotplug_kind kind,break ; } +mlxreg_lc_power_on_off_fail: +mlxreg_lc_create_static_devices_fail: +mutex_unlock (&mlxreg_lc->lock); +return err; }@@ -665,7 +676,7 @@ static int mlxreg_lc_completion_notify(void *handle, struct i2c_adapter *parent,if (err) goto mlxreg_lc_create_static_devices_failed; -mlxreg_lc_state_update (mlxreg_lc,MLXREG_LC_POWERED ,1 ); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_POWERED ,1 ); }/* Verify if line card is synchronized. */ @@ -676,7 +687,7 @@ static int mlxreg_lc_completion_notify(void *handle, struct i2c_adapter *parent,/* Power on line card if necessary. */ if (regval & mlxreg_lc->data->mask) { mlxreg_lc->state |=MLXREG_LC_SYNCED ; -mlxreg_lc_state_update (mlxreg_lc,MLXREG_LC_SYNCED ,1 ); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_SYNCED ,1 );if (mlxreg_lc->state & ~MLXREG_LC_POWERED ) { err =mlxreg_lc_power_on_off (mlxreg_lc,1 );if (err)@@ -684,7 +695,7 @@ static int mlxreg_lc_completion_notify(void *handle, struct i2c_adapter *parent, } } -mlxreg_lc_state_update (mlxreg_lc,MLXREG_LC_INITIALIZED ,1 ); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_INITIALIZED ,1 );return 0 ;@@ -904,6 +915,8 @@ static int mlxreg_lc_remove(struct platform_device *pdev)struct mlxreg_core_data *data =dev_get_platdata (&pdev->dev);struct mlxreg_lc *mlxreg_lc =platform_get_drvdata (pdev); +mlxreg_lc_state_update_locked (mlxreg_lc,MLXREG_LC_INITIALIZED ,0 ); +/* * Probing and removing are invoked by hotplug events raised upon line card insertion and * removing. If probing procedure fails all data is cleared. However, hotplug event still
Vadim Pasternak
maintainerPATCH 3/4 platform/mellanox: Remove unnecessary code
In reply to Vadim PasternakRemove redundant 'NULL' check for of if 'data->notifier'.
Replace 'return err' by 'return 0' in mlxreg_lc_probe().
--- drivers/platform/mellanox/mlxreg-lc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/platform/mellanox/mlxreg-lc.c +4 -5
@@ -825,10 +825,9 @@ static int mlxreg_lc_probe(struct platform_device *pdev)mutex_init (&mlxreg_lc->lock);/* Set event notification callback. */ -if (data->notifier) { - data->notifier->user_handler = mlxreg_lc_event_handler; - data->notifier->handle = mlxreg_lc; - } + 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) { dev_err(&pdev->dev, "Failed to get adapter for bus %d\n",
@@ -888,7 +887,7 @@ static int mlxreg_lc_probe(struct platform_device *pdev)if (err) goto mlxreg_lc_config_init_fail; -return err; +return 0 ; mlxreg_lc_config_init_fail: regcache_sync_fail:
Signature · 1 line
2.20.1
Vadim Pasternak
maintainerPATCH 4/4 platform/mellanox: Remove redundant 'NULL' check
In reply to Vadim PasternakRemove 'NULL' check for 'data->hpdev.client' in error flow of mlxreg_lc_probe(). It cannot be 'NULL' at this point.
--- drivers/platform/mellanox/mlxreg-lc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/platform/mellanox/mlxreg-lc.c +2 -4
@@ -893,10 +893,8 @@ static int mlxreg_lc_probe(struct platform_device *pdev) regcache_sync_fail: regmap_write_fail: devm_regmap_init_i2c_fail: -if (data->hpdev.client) { -i2c_unregister_device (data->hpdev.client); - data->hpdev.client =NULL ; - } +i2c_unregister_device (data->hpdev.client); + data->hpdev.client =NULL ; i2c_new_device_fail:i2c_put_adapter (data->hpdev.adapter); data->hpdev.adapter =NULL ;
Signature · 1 line
2.20.1
Hi All,
Quoted 4 lines · Vadim Pasternak
The patch set includes fixes for several issues:
Patch #1: fixes coverity issue.
Patch #2: fixes locking dependency issue.
Patches #3-#4: remove unnecessary code.
Thank you for your patch-series, I've applied the series to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
I will also send this out as part of my next fixes pull-req for 6.0-rc#.
Note I do have one code-style remark to patch 2/4, since I want to add this to my fixes pull-req I've taken 2/4 as is. Please send a follow-up (incremental) patch addressing the code-style issue. See my reply to patch 2/4.
Regards,
Hans
Quoted 10 lines
Vadim Pasternak (4):
platform/mellanox: mlxreg-lc: Fix coverity warning platform/mellanox: mlxreg-lc: Fix locking issue platform/mellanox: Remove unnecessary code platform/mellanox: Remove redundant 'NULL' checkdrivers/platform/mellanox/mlxreg-lc.c | 53 ++++++++++++++++----------- 1 file changed, 31 insertions(+), 22 deletions(-)