On Mon, Feb 03, 2025 at 11:33:54AM +0000, Jonathan Cameron wrote:
>
> > >
> > > > + int dpa_range_count = san_info->dpa_range_count;
> > > > + int rc = 0;
> > > > +
> > > > + for (int i = 0; i < dpa_range_count; i++) {
> > > > + rc = sanitize_range(ct3d,
> > > > san_info->dpa_range_list[i].starting_dpa,
> > > > + san_info->dpa_range_list[i].length,
> > > > san_info->fill_value);
> > > > + if (rc) {
> > > > + goto exit;
> > > > + }
> > > > + }
> > > > + cxl_discard_all_event_records(&ct3d->cxl_dstate);
> > >
> > > Add a comment on why we are deleting event records when sanitizing a small
> > > part of memory?
> > >
> >
> > See response below for disabling the media state. Same section referenced
> > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome
> > depends on how we interpret "follow the method described in 8.2.10.9.5.1".
> >
>
> This also sounds like reading too much into that comment.
>
Agreed, Vinayak let's drop the discard of all event records
from this patch.
> > > > + }
> > > > +
> > > > +start_bg:
> > > > + /* EBUSY other bg cmds as of now */
> > > > + cci->bg.runtime = secs * 1000UL;
> > > > + *len_out = 0;
> > > > + /* sanitize when done */
> > > > + cxl_dev_disable_media(&ct3d->cxl_dstate);
> > > Why? This is santizing part of the device. As I undestand it the
> > > main aim is to offload cleanup when the device is in use. Definitely
> > > don't want to disable media. If I'm missing a reason please give
> > > a spec reference.
> >
> > Table 8-164, sanitize description mentions to follow method
> > in 8.2.10.9.5.1, which does call out placing device in disabled
> > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1
> > or the method devices uses to sanitize internally.
>
> I think it is meant to just be the method of sanitizing.
>
Ok, let's use this interpretation. Vinayak, can you remove this as well
and then we put a comment in the patch that media op sanitize is targeted
so no need to disable media or clear event logs.
> >
> > I would imagine since sanitize is destructive we would not want to return
> > any data from device ranges impacted by sanitize. I believe a simple
> > way to achieve this is to disable entire device.
>
> Hmm. That rather destroys the main use case I'm aware of for this
> (unlike the general santize commands from earlier CXL versions)/
> Superficially sounds like we need a spec clarification as
> clearly not super clear!
>
For this series, let's drive the work with the use case you have in
mind. We will start a thread with the consortium, but I don't think
that should delay this work.
> >
>
> Jonathan
>
>
