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;
Date:   Wed, 15 Jun 2022 08:01:50 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Alison Schofield <alison.schofield@intel.com>
Cc:     "Williams, Dan J" <dan.j.williams@intel.com>,
        "Verma, Vishal L" <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] cxl/mbox: Add GET_POISON_LIST mailbox command support
Message-ID: <Yqn0XqfPd4q89pL7@iweiny-server>
References: <cover.1655250669.git.alison.schofield@intel.com>
 <382a9c35ef43e89db85670637d88371f9197b7a2.1655250669.git.alison.schofield@intel.com>
 <YqlQgbDUTJ6k9UBY@iweiny-desk3>
 <20220615050752.GA1521419@alison-desk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220615050752.GA1521419@alison-desk>
Precedence: bulk

On Tue, Jun 14, 2022 at 10:07:52PM -0700, Alison Schofield wrote:
> On Tue, Jun 14, 2022 at 08:22:41PM -0700, Ira Weiny wrote:
> 

[snip]

> > > +
> > > +	do {
> > > +		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_POISON, &pi,
> > > +				       sizeof(pi), po, cxlds->payload_size);
> > > +		if (rc)
> > > +			goto out;
> > > +
> > > +		if (po->flags & CXL_POISON_FLAG_OVERFLOW) {
> > > +			time64_t o_time = le64_to_cpu(po->overflow_timestamp);
> > > +
> > > +			dev_err(dev, "Poison list overflow at %ptTs UTC\n",
> > > +				&o_time);
> > > +			rc = -ENXIO;
> > > +			goto out;
> > 
> > I guess the idea is that this return will trigger something else will clear the list,
> > rebuild the list, and perform a scan media request?
> > 
> Per CXL Spec 8.2.9.5.4.1: The poison list may be incomplete when the list
> has overflowed. User can perform a Scan Media to try to clear and rebuild
> the list, with no guarantee that the overflow will not recur.
> 
> So yes to what you are saying. This return value should indicate to
> user space that a Scan Media should be issued. Issuing the Scan Media
> to the device does lead the device to rebuild it's list, as you say.
> Also, when we get the Scan Media results, the device is able to report
> partial results and tell the host to collect the error records, and
> then restart the scan, get results again, and on and on until the scan
> is complete.
> 
> Perhaps a clarification - there is not a logical pairing of Scan Media
> followed by Get Poison List.  Scan Media followed by Get Scan Media
> Results is the logical pairing. Get Poison List is getting a snapshot
> of the poison list at a point in time. The device adds DPAs to the list
> when the device detects poison, some devices run their own backround
> scans and add to the poison list, and then there are the user initiated
> actions (Scan Media and Poison Inject) that can affect the list.
> 
> > I'm just wondering if this loop should continue to clear the list and then let
> > something else do the scan media request?
> 
> It's not like the _MORE status where the device is telling the host to
> come back and gather more. I think the action of failing, and letting
> user initiated a Scan Media is correct course here.

Fair enough.  But I guess I'm still confused by the spec.  The way I read it
yesterday (and I could be wrong) was that the OS was supposed to read the
entries to clear the list?  Is that not true?

I the device will clear the list internally when Scan Media is run?

At this point I'm just trying to understand not necessarily objecting to the
patch.

Ira

> 
> So, this response got kind of long winded. As you can see, especially
> if one looks in the spec as I know you are, there are additional
> commands that need to be implemented to complete the ARS feature set.
> And, of course, we'll offer user space tooling (NDCTL and libcxl).
>