Re: [PATCH 3/7] cxl/mbox: Add sanitation handling machinery

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

 
> >  
> >> +		queue_delayed_work(system_wq,
> >> +				   &cxlds->sec.sanitize_dwork, tmo * HZ);
> >> +	}
> >> +	mutex_unlock(&cxlds->mbox_mutex);
> >> +}
> >> +
> >>  /**
> >>   * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
> >>   * @cxlds: The device state to communicate with.
> >> @@ -173,6 +210,16 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >>		return -EBUSY;
> >>	}
> >>
> >> +	/*
> >> +	 * With sanitize polling, hardware might be done and the poller still
> >> +	 * not be in sync. Ensure no new command comes in until so. Keep the
> >> +	 * hardware semantics and only allow device health status.
> >> +	 */
> >> +	if (unlikely(cxlds->sec.sanitize_tmo > 0)) {
> >> +		if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO)  
> >
> >Doesn't this let the value of mbox_cmd->opcode change to HEALTH_INFO so that
> >when we get here again we could carry on without other commands though still not in
> >sync (if things are very weird).  
> 
> I don't quite follow, mbox_cmd is local to each caller. Below I touch on this.

Indeed. Comment was result of a misread.

> >
> >That's confusing.  I'd add a separate structure element for it instead with
> >appropriate naming.  
> 
> Agreed, can be nicer. Another alternative is doing away with it altogether and only
> allow sanitation if interrupts are supported/enabled. Considering the potential runtimes,
> it's not a crazy ask to the hw to at least give some notification mechanism instead
> of having sw trying to stay up to date.

Whilst I fear someone will build it, we can be mean to them and make them add the support ;)
I don't mind either way.

Jonathan

> 
> Thanks,
> Davidlohr