Re: [PATCH v2 1/5] scsi: target: core: Add RTPI field to target port

From: Christoph Hellwig <hch@infradead.org>
To: Dmitry Bogdanov <d.bogdanov@yadro.com>
Cc: Martin Petersen <martin.petersen@oracle.com>,
	target-devel@vger.kernel.org,
	Mike Christie <michael.christie@oracle.com>,
	linux-scsi@vger.kernel.org, linux@yadro.com
Subject: Re: [PATCH v2 1/5] scsi: target: core: Add RTPI field to target port
Date: Mon, 17 Oct 2022 00:38:50 -0700	[thread overview]
Message-ID: <Y00Giu7q4M/jb6mG@infradead.org> (raw)
In-Reply-To: <20221006105057.30184-2-d.bogdanov@yadro.com>

> +DEFINE_XARRAY_ALLOC(tpg_xa);

I think this wants to be marked static.

> +static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)

Can you use target_ instead of the weird historic core_ prefixes for
everything here?

> +int core_tpg_enable(struct se_portal_group *se_tpg, bool enable)
> +{
> +	int ret;
> +
> +	if (enable) {
> +		ret = core_tpg_register_rtpi(se_tpg);
> +		if (ret)
> +			return ret;
> +	} else {
> +		core_tpg_deregister_rtpi(se_tpg);
> +	}
> +	ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, enable);
> +	if (ret) {
> +		core_tpg_deregister_rtpi(se_tpg);
> +		return ret;
> +	}
> +
> +	se_tpg->enabled = enable;

This bool enable logic is a bit weird and splitting the enable and
disable case would seem more sensible to me, but maybe there is
something later on that makes it more relevant.