Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Fan Ni <fan.ni@samsung.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     "alison.schofield@intel.com" <alison.schofield@intel.com>,
        "vishal.l.verma@intel.com" <vishal.l.verma@intel.com>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        "bwidawsk@kernel.org" <bwidawsk@kernel.org>,
        "Jonathan.Cameron@huawei.com" <Jonathan.Cameron@huawei.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        Adam Manzanares <a.manzanares@samsung.com>,
        "dave@stgolabs.net" <dave@stgolabs.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field
 check
Thread-Topic: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field
        check
Thread-Index: AQHZS8WnqoF9q+YY/UCqLavADTGrLa7qFEkAgAAP1ICAAAuggIAdaWIA
Date:   Wed, 22 Mar 2023 16:45:15 +0000
Message-ID: <20230322164505.GA1641600@bgt-140510-bm03>
In-Reply-To: <640276695c8e8_5b27929473@dwillia2-xfh.jf.intel.com.notmuch>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2F1143051CDEA048AF91AD1197433780@ssi.samsung.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
CMS-TYPE: 301P
References: <CGME20230228224029uscas1p1e2fb92a8a595f80fa2985b452899d785@uscas1p1.samsung.com>
        <20230228224014.1402545-1-fan.ni@samsung.com>
        <64025f6219d2d_71138294e5@dwillia2-xfh.jf.intel.com.notmuch>
        <20230303215446.GA1479551@bgt-140510-bm03>
        <640276695c8e8_5b27929473@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk

On Fri, Mar 03, 2023 at 02:36:25PM -0800, Dan Williams wrote:
> Fan Ni wrote:
> [..]
> > > I think a separate fix for that crash is needed, can you send the
> > > backtrace? I.e. I worry that crash can be triggered by other means.
> > Hi Dan,
> > See backtrace below.
>=20
> Thanks, I'll take a look.=20
>=20
> [..]
> > > > @@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *=
port, struct cxl_decoder *cxld,
> > > >  	base =3D ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(wh=
ich));
> > > >  	size =3D ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(wh=
ich));
> > > >  	committed =3D !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> > > > +	should_commit =3D !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
> > >=20
> > > This change looks like a good idea in general given the ambiguity of
> > > 'committed'. However just combine the two checks into the @committed
> > > variable with something like this:
> > >=20
> > > commit_mask =3D CXL_HDM_DECODER0_CTRL_COMMITTED|CXL_HDM_DECODER0_CTRL=
_COMMIT;
> > > committed =3D (ctrl & commit_mask) =3D=3D commit_mask;
>=20
> Did you also notice this ^^^ request for a fixed up version of the
> current patch?

Hi Dan,
Jonathan sent out a qemu patch to fix the committed field
reset as below, and the patch fixed the system crash discussed here.
https://lore.kernel.org/linux-cxl/20230322102731.4219-1-Jonathan.Cameron@hu=
awei.com/T/#me5283349b37d53abc93904a2428910a2f6a354f6

Do you think we need a separate fix at kernel side to fix the
possible system crash when cxl_dpa_release is called and dpa_res is
null? I have noticed at some location, dpa_res is checked before
calling cxl_dpa_release for example in function cxl_dpa_free, but no guard
from other callers. If it is needed, I have a simple fix and ready
to send out.

Fan=