On 3/16/22 17:01, Guixin Liu wrote:
> If dev is not blocked when resetting ring, then there could be new
> commands coming in after resetting ring, this will make cmd ring broken,
> because tcmu can not find tcmu_cmd when tcmu-runner handled these
> newcome commands.
>
> Signed-off-by: Guixin Liu <kanie@xxxxxxxxxxxxxxxxx>
> ---
> drivers/target/target_core_user.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 7b2a89a..548ad94 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -2333,7 +2333,7 @@ static void tcmu_block_dev(struct tcmu_dev *udev)
> mutex_unlock(&udev->cmdr_lock);
> }
>
> -static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
> +static int tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
> {
> struct tcmu_mailbox *mb;
> struct tcmu_cmd *cmd;
> @@ -2341,6 +2341,12 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
>
> mutex_lock(&udev->cmdr_lock);
>
> + if (!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags)) {
> + pr_err("The dev should be blocked before resetting ring.\n");
This looks like a bug... So I think this should at least be a WARN_ON(). E.g.
if (WARN_ON(!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags))) {
mutex_unlock(&udev->cmdr_lock);
return -EINVAL;
}
But why is the ring reset even allowed without the device being blocked in
the first place ? What is the root cause ?
> + mutex_unlock(&udev->cmdr_lock);
> + return -EINVAL;
> + }
> +
> xa_for_each(&udev->commands, i, cmd) {
> pr_debug("removing cmd %u on dev %s from ring %s\n",
> cmd->cmd_id, udev->name,
> @@ -2396,6 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
> run_qfull_queue(udev, false);
>
> mutex_unlock(&udev->cmdr_lock);
> + return 0;
> }
>
> enum {
> @@ -2995,7 +3002,10 @@ static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page,
> return -EINVAL;
> }
>
> - tcmu_reset_ring(udev, val);
> + ret = tcmu_reset_ring(udev, val);
> + if (ret < 0)
> + return ret;
> +
> return count;
> }
> CONFIGFS_ATTR_WO(tcmu_, reset_ring);
--
Damien Le Moal
Western Digital Research
