Hi,
On 8/23/22 22:19, Vadim Pasternak wrote:
> Fix 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'.
>
> Fixes: 62f9529b8d5c87b ("platform/mellanox: mlxreg-lc: Add initial support for Nvidia line card devices")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> ---
> drivers/platform/mellanox/mlxreg-lc.c | 37 ++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/mellanox/mlxreg-lc.c b/drivers/platform/mellanox/mlxreg-lc.c
> index 9a1bfcd24317..e578c7bc060b 100644
> --- a/drivers/platform/mellanox/mlxreg-lc.c
> +++ b/drivers/platform/mellanox/mlxreg-lc.c
> @@ -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);
So here you are unlocking before return.
> 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;
Yet here you use a goto (better IMHO).
> }
> /* 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.
> 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;
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
>
> /* 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
