Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 25 Aug 2021 11:04:24 +0200
From:   "hch@lst.de" <hch@lst.de>
To:     Sagi Grimberg <sagi@grimberg.me>
Cc:     sasaki tatsuya <tatsuya6.sasaki@kioxia.com>,
        "kbusch@kernel.org" <kbusch@kernel.org>,
        "axboe@fb.com" <axboe@fb.com>, "hch@lst.de" <hch@lst.de>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] nvme: update keep alive interval when kato is
 modified
Message-ID: <20210825090424.GA468@lst.de>
References: <526a1a756d6c4643b15b1b305cc32817@kioxia.com> <05033836-83b9-c060-0348-774a02b60d01@grimberg.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <05033836-83b9-c060-0348-774a02b60d01@grimberg.me>
User-Agent: Mutt/1.5.17 (2007-11-01)
Precedence: bulk

On Tue, Aug 24, 2021 at 01:41:44PM -0700, Sagi Grimberg wrote:
>> +{
>> +	/*
>> +	 * Keep alive commands interval on the host should be updated
>> +	 * when KATO is modified by Set Features commands.
>> +	 */
>> +	if (cmd->opcode == nvme_admin_set_features &&
>> +	    (cmd->cdw10 & 0xFF) == NVME_FEAT_KATO) {
>> +		/* ms -> s */
>
> no need for this comment.
>
>> +		unsigned int new_kato = DIV_ROUND_UP(cmd->cdw11, 1000);
>> +
>> +		nvme_update_keep_alive(ctrl, new_kato);
>
> I think you can now inline nvme_update_keep_alive here, no need to keep
> it in a function.

Actually, іf thinking ahead I think one helper per fixup would be really
useful to keep the code organized.  But the DIV_ROUND_UP should move into
nvme_update_keep_alive to keep it self-contained.
We can then restructure the caller a bit to make it easier to expand:

	switch (cmd->opcode) {
	case nvme_admin_set_features:
		switch (cmd->cdw10 & 0xff) {
		case NVME_FEAT_KATO:
			nvme_update_keep_alive(ctrl, cmd);
			break;
		}
	}