On Sun, Apr 26, 2020 at 01:50:05PM +0300, Max Gurtovoy wrote:
>>>> 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;
I don't think we should allow users to enable invalid configurations
and silently clear flags, but reject flags as soon as we can - write
to the attribute where possible, else during enable.
> 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" : "");
Ok.
