On 4/24/2020 10:14 AM, Christoph Hellwig wrote:
On Thu, Apr 23, 2020 at 03:39:38PM +0300, Max Gurtovoy wrote:
+ if (ctrl->subsys->pi_support && ctrl->port->pi_enable) {
+ if (ctrl->port->pi_capable) {
+ ctrl->pi_support = true;
+ pr_info("controller %d T10-PI enabled\n", ctrl->cntlid);
+ } else {
+ ctrl->pi_support = false;
+ pr_warn("T10-PI is not supported on controller %d\n",
+ ctrl->cntlid);
+ }
I think the printks are a little verbose. Also why can we set
ctrl->port->pi_enable if ctrl->port->pi_capable is false? Shoudn't
we reject that earlier? In that case this could simply become:
ctrl->pi_support = ctrl->subsys->pi_support && ctrl->port->pi_enable;
for that we'll need to check pi_capable during add_port process and disable
pi_enable bit if user set it.
Which seems pretty sensible. In fact I think the configfs write for
pi enable should fail if we don't have the capability.
The port is not enabled so it's not possible currently.
But we can disable it afterwards (in nvmet_enable_port) :
+ /* If the transport didn't set pi_capable, then disable it. */
+ if (!port->pi_capable)
+ port->pi_enable = false;
User should set it before enable the port (this will always succeed).
I'll make this change as well.
re the verbosity, sometimes I get many requests from users to get
indication for some features.
We can remove this as well if needed.
I'd rather keep debug prints silent. You could add a verbose mode
in nvmetcli that prints all the settings applied if that helps these
users.
how about:
- pr_info("creating controller %d for subsystem %s for NQN %s.\n",
- ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);
+ pr_info("creating controller %d for subsystem %s for NQN %s%s.\n",
+ ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn,
+ ctrl->pi_support ? " T10-PI is enabled" : "");
