Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   Fan Ni <fan.ni@samsung.com>
To:     Ira Weiny <ira.weiny@intel.com>
CC:     Fan Ni <fan.ni@gmx.us>, Dan Williams <dan.j.williams@intel.com>,
        "Navneet Singh" <navneet.singh@intel.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        "Dave Jiang" <dave.jiang@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 03/18] cxl/mem: Read Dynamic capacity
 configuration from the device
Thread-Topic: [PATCH RFC v2 03/18] cxl/mem: Read Dynamic capacity
        configuration from the device
Thread-Index: AQHZ2jkK/pP/klWxW0KG5oR5AqJrLLAD1zsAgA435ICABjMaAA==
Date:   Tue, 12 Sep 2023 21:32:42 +0000
Message-ID: <20230912213242.GD2859961@sjcvldevvm72>
In-Reply-To: <64fba59f73319_1e8e78294e2@iweiny-mobl.notmuch>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <E5E5A3975330C243BEF1F709F73946DF@ssi.samsung.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
CMS-TYPE: 301P
References: <20230604-dcd-type2-upstream-v2-0-f740c47e7916@intel.com>
        <20230604-dcd-type2-upstream-v2-3-f740c47e7916@intel.com>
        <ZO+4Qql0rOslp0bl@debian>
        <CGME20230908225229uscas1p2f1013a8dd9977718cbd19e6433adef57@uscas1p2.samsung.com>
        <64fba59f73319_1e8e78294e2@iweiny-mobl.notmuch>
Precedence: bulk

On Fri, Sep 08, 2023 at 03:52:15PM -0700, Ira Weiny wrote:

> Fan Ni wrote:
> > On Mon, Aug 28, 2023 at 10:20:54PM -0700, ira.weiny@intel.com wrote:
> > > From: Navneet Singh <navneet.singh@intel.com>
> > >
>=20
> [snip]
>=20
> > >
> > > +static int cxl_dc_save_region_info(struct cxl_memdev_state *mds, int=
 index,
> > > +				   struct cxl_dc_region_config *region_config)
> > > +{
> > > +	struct cxl_dc_region_info *dcr =3D &mds->dc_region[index];
> > > +	struct device *dev =3D mds->cxlds.dev;
> > > +
> > > +	dcr->base =3D le64_to_cpu(region_config->region_base);
> > > +	dcr->decode_len =3D le64_to_cpu(region_config->region_decode_length=
);
> > > +	dcr->decode_len *=3D CXL_CAPACITY_MULTIPLIER;
> > > +	dcr->len =3D le64_to_cpu(region_config->region_length);
> > > +	dcr->blk_size =3D le64_to_cpu(region_config->region_block_size);
> > > +	dcr->dsmad_handle =3D le32_to_cpu(region_config->region_dsmad_handl=
e);
> > > +	dcr->flags =3D region_config->flags;
> > > +	snprintf(dcr->name, CXL_DC_REGION_STRLEN, "dc%d", index);
> > > +
> > > +	/* Check regions are in increasing DPA order */
> > > +	if (index > 0) {
> > > +		struct cxl_dc_region_info *prev_dcr =3D &mds->dc_region[index - 1]=
;
> > > +
> > > +		if ((prev_dcr->base + prev_dcr->decode_len) > dcr->base) {
> > > +			dev_err(dev,
> > > +				"DPA ordering violation for DC region %d and %d\n",
> > > +				index - 1, index);
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > > +
> > > +	/* Check the region is 256 MB aligned */
> > > +	if (!IS_ALIGNED(dcr->base, SZ_256M)) {
> > > +		dev_err(dev, "DC region %d not aligned to 256MB: %#llx\n",
> > > +			index, dcr->base);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Check Region base and length are aligned to block size */
> > > +	if (!IS_ALIGNED(dcr->base, dcr->blk_size) ||
> > > +	    !IS_ALIGNED(dcr->len, dcr->blk_size)) {
> > > +		dev_err(dev, "DC region %d not aligned to %#llx\n", index,
> > > +			dcr->blk_size);
> > > +		return -EINVAL;
> > > +	}
> >=20
> > Based on on cxl 3.0 spec: Table 8-126, we may need some extra checks
> > here:
> > 1. region len <=3D decode_len
> > 2. region block size should be power of 2 and a multiple of 40H.
>=20
> Thanks for pointing these additional checks out!  I've added these.
>=20
> >=20
> > Also, if region len or block size is 0, it mentions that DC will not be
> > available, we may also need to handle that.
>=20
> I've just added checks for 0 in region length, length and block size.
>=20
> I don't think we need to handle this in any special way.  Any of these
> checks will fail the device probe.  From my interpretation of the spec
> reading these values as 0 would indicate an invalid device configuration.
>=20
> That said I think the spec is a bit vague here.  On the one hand the
> number of DC regions should reflect the number of valid regions.
>=20
> Table 8-125 'Number of Available Regions':
> 	"This is the number of valid region configurations returned in
> 	this payload."
>=20
> But it also says:
> 	"Each region may be unconfigured or configured with a different
> 	block size and capacity."
>=20
> I don't believe that a 0 in the Region Decode Length, Region Length, or
> Region Block Size is going to happen with the code structured the way it
> is.  I believe these values are used if the host specifically requests th=
e
> configuration of a region not indicated by 'Number of Available Regions'
> through the Starting Region Index in Table 8-163.  This code does not do
> that.
>=20
> Would you agree with this?

Agreed.

Fan
>=20
> Thanks again,
> Ira=