On 2018/4/14 1:21, Mike Christie wrote:
On 04/12/2018 10:08 PM, Xiubo Li wrote:
+
+ if (val != 1) {
+ pr_err("Invalid block value %d\n", val);
I think you wanted
"Invalid reset value %d\n"
Yeah, just copied it from other place.
+ return -EINVAL;
+ }
+
+ spin_unlock(&udev->nl_cmd_lock);
Need spin_lock() instead of unlock.
I think before calling the code below you need to check if a nl command
is even waiting. If you just run this with no nl commands waiting then
next time we send a command nl_cmd->complete will be true and
tcmu_wait_genl_cmd_reply's wait_event call will return right away.
Will fix this.
+ nl_cmd->complete = true;
+ nl_cmd->status = -EINTR;
+ wake_up(&udev->complete_wq);
Instead of the code above, I think you need to take some of the guts out
of tcmu_genl_cmd_done to handle removal which is an odd cases due to the
refcoutning/locking and configfs use.
For non removal commands we need to do a target_undepend_item. For
remove, we don't want to since we came through configfs and teardown
already started.
So instead of the above lines take:
if (nl_cmd->cmd != completed_cmd) {
printk(KERN_ERR "Mismatched commands (Expecting reply
for %d. Current %d).\n",
completed_cmd, nl_cmd->cmd);
ret = -EINVAL;
} else {
nl_cmd->status = rc;
}
// Note: I changed this from the is_removed
if (completed_cmd != TCMU_CMD_REMOVED_DEVICE)
target_undepend_item(&dev->dev_group.cg_item);
if (!ret) {
nl_cmd->complete = true;
wake_up(&udev->complete_wq);
}
Sorry, this part I couldn't totally understand.
Do you mean take the code above you pasted by introducing
target_undepend_item() for none removal paths just like in
tcmu_genl_cmd_done() does?
While in tcmu_genl_cmd_done(), the target_depend_item() is called by
target_find_device() and then it should do target_undepend_item() after
that, but in
reset_netlink_store(), should it need target_undepend_item() ? If so
they won't be in pairs.
Yes, you are right. I thought we took a count when we sent the nl cmd.
I think we should only take this part:
if (nl_cmd->cmd != completed_cmd) {
printk(KERN_ERR "Mismatched commands (Expecting reply
for %d. Current %d).\n",
completed_cmd, nl_cmd->cmd);
ret = -EINVAL;
} else {
nl_cmd->status = rc;
}
from the tcmu_genl_cmd_done(), right ?
We want the complete and wake up part too right?
Yeah, I will fix them all you mentioned above.
Thanks,
BRs
nl_cmd->complete = true;
wake_up(&udev->complete_wq);
Thanks,
BRs
from tcmu_genl_cmd_done and make a function that takes the status code
and completed_cmd. You would just then do
__tcmu_genl_cmd_done(&udev->curr_nl_cmd.cmd, -EINTR);
+ spin_unlock(&udev->nl_cmd_lock);
+
+ return count;
+}
+CONFIGFS_ATTR_WO(tcmu_, reset_netlink);
+
static ssize_t tcmu_reset_ring_store(struct config_item *item,
const char *page,
size_t count)
{
@@ -2363,6 +2400,7 @@ static ssize_t tcmu_reset_ring_store(struct
config_item *item, const char *page,
static struct configfs_attribute *tcmu_action_attrs[] = {
&tcmu_attr_block_dev,
&tcmu_attr_reset_ring,
+ &tcmu_attr_reset_netlink,
NULL,
};
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
