On Wed, 9 Feb 2022 08:26:43 -0800
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> On Wed, Feb 9, 2022 at 2:13 AM Jonathan Cameron
> <Jonathan.Cameron@xxxxxxxxxx> wrote:
> [..]
...
>
> >
> > >
> > > > +}
> > > > +
> > > > +static int pci_doe_send_req(struct pci_doe *doe, struct pci_doe_exchange *ex)
> > >
> > > The relationship between tasks, requests, responses, and exchanges is
> > > not immediately clear to me. For example, can this helper be renamed
> > > in terms of its relationship to a task? A theory of operation document
> > > would help, but it seems there is also room for the implementation to
> > > be more self documenting.
> >
> > Not totally sure what such naming would be.
> >
> > A task is the management wrapper around an exchange which is a request
> > + response pair. In the sense you queue a task which will carry out
> > and exchange by sending a request and receiving a response.
> >
> > Could rename this pci_doe_start_exchange() but that then obscures
> > that we mean send the request to the hardware and removes the resemblance
> > to what I recall the specification uses.
>
> I'm not a big fan of copying spec names *if* Linux has a more
> idiomatic name for the concept. I am mainly reviewing this from the
> perspective that 'struct bio' and 'struct request' naming /
> organization is idiomatic for Linux driver transaction flows. Up to
> this point in the review I was mapping tasks to bios and exchanges to
> requests but then the usage of "req" in this function name threw off
> my ontology. At a minimum a decoder ring style comment, like your
> reply, about the relationship between these terms would help avoid
> this exercise again.
OK. So up to Ira, but my suggestion is go with a comment unless
someone comes up with clearer naming.
Mind you, if we are now exposing the doe_exchange to callers anyway,
we could just squash the structure into the doe_task one and drop
the separation.
Intent before was doe_exchange was all the stuff related to the protocol
(so buffers etc0 whereas task was about the implementation but
if we expose struct doe_task anyway that separation becomes a bit pointless.
>
> > > > + case DOE_WAIT_ABORT:
> > > > + case DOE_WAIT_ABORT_ON_ERR:
> > > > + pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> > > > +
> > > > + if (!FIELD_GET(PCI_DOE_STATUS_ERROR, val) &&
> > > > + !FIELD_GET(PCI_DOE_STATUS_BUSY, val)) {
> > > > + /* Back to normal state - carry on */
> > > > + mutex_lock(&doe->state_lock);
> > > > + doe->cur_task = NULL;
> > > > + mutex_unlock(&doe->state_lock);
> > > > + wake_up_interruptible(&doe->wq);
> > > > +
> > > > + /*
> > > > + * For deliberately triggered abort, someone is
> > > > + * waiting.
> > > > + */
> > > > + if (doe->state == DOE_WAIT_ABORT)
> > > > + complete(&doe->abort_c);
> > >
> > > Why is a completion and waitqueue needed? I.e. a waiter could simply
> > > look for an abort completion flag to be set instead.
> >
> > You mean use the main completion (the one for the non abort case)
> > and a flag?
> >
> > Or a wait_event() with appropriate check?
> >
> > Could do that but I'm not sure I understand why we care either way?
>
> Just reduction in machinery that needs to be maintained /
> comprehended. 2 wait primitives when one will do will always be a
> tempting cleanup target.
Ah. Fair enough - it looks like using the same completion won't
be a huge addition in complexity.
>
> [..]
> > > > +/**
> > > > + * pci_doe_exchange_sync() - Send a request, then wait for and receive a
> > > > + * response
> > > > + * @doe_dev: DOE mailbox state structure
> > > > + * @ex: Description of the buffers and Vendor ID + type used in this
> > > > + * request/response pair
> > > > + *
> > > > + * Excess data will be discarded.
> > > > + *
> > > > + * RETURNS: payload in bytes on success, < 0 on error
> > > > + */
> > > > +int pci_doe_exchange_sync(struct pci_doe_dev *doe_dev,
> > > > + struct pci_doe_exchange *ex)
> > > > +{
> > > > + struct pci_doe *doe = dev_get_drvdata(&doe_dev->adev.dev);
> > > > + struct pci_doe_task task;
> > > > + DECLARE_COMPLETION_ONSTACK(c);
> > > > +
> > > > + if (!doe)
> > > > + return -EAGAIN;
> > > > +
> > > > + /* DOE requests must be a whole number of DW */
> > > > + if (ex->request_pl_sz % sizeof(u32))
> > > > + return -EINVAL;
> > > > +
> > > > + task.ex = ex;
> > > > + task.cb = pci_doe_task_complete;
> > > > + task.private = &c;
> > > > +
> > > > +again:
> > > > + mutex_lock(&doe->state_lock);
> > > > + if (doe->cur_task) {
> > > > + mutex_unlock(&doe->state_lock);
> > > > + wait_event_interruptible(doe->wq, doe->cur_task == NULL);
> > > > + goto again;
> > > > + }
> > > > +
> > > > + if (doe->dead) {
> > > > + mutex_unlock(&doe->state_lock);
> > > > + return -EIO;
> > > > + }
> > > > + doe->cur_task = &task;
> > > > + schedule_delayed_work(&doe->statemachine, 0);
> > > > + mutex_unlock(&doe->state_lock);
> > > > +
> > > > + wait_for_completion(&c);
> > >
> > > I would expect that the caller of this routine would want to specify
> > > the task and end_task() callback and use that as the completion
> > > signal. It may also want "no wait" behavior where it is prepared for
> > > the DOE result to come back sometime later. With that change the
> > > exchange fields can move into the task directly.
> >
> > This is the simple synchronous wrapper around an async core.
> > If we want an async path at somepoint in the future where we have
> > someone using it then sure, we can have an async version that
> > takes the callback.
>
> It just seems an unnecessary hunk of code for the core to carry when
> it's trivial for a client of the core to do:
>
> task->private = &completion;
> task->end_task = complete_completion;
> submit_task()
> wait_for_completion(&completion);
OK, we can move this to the callers though function obviously will
also need renaming - I guess to pci_doe_exchange() and now need to take a
task rather than the exchange.
I personally slightly prefer the layered approach, but don't care that
strongly.
>
> > > > + return task.rv;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_doe_exchange_sync);
> > > > +
> > > > +/**
> > > > + * pci_doe_supports_prot() - Return if the DOE instance supports the given
> > > > + * protocol
> > > > + * @pdev: Device on which to find the DOE instance
> > > > + * @vid: Protocol Vendor ID
> > > > + * @type: protocol type
> > > > + *
> > > > + * This device can then be passed to pci_doe_exchange_sync() to execute a
> > > > + * mailbox exchange through that DOE mailbox.
> > > > + *
> > > > + * RETURNS: True if the DOE device supports the protocol specified
> > > > + */
> > > > +bool pci_doe_supports_prot(struct pci_doe_dev *doe_dev, u16 vid, u8 type)
> > > > +{
> > > > + struct pci_doe *doe = dev_get_drvdata(&doe_dev->adev.dev);
> > > > + int i;
> > > > +
> > > > + if (!doe)
> > > > + return false;
> > > > +
> > > > + for (i = 0; i < doe->num_prots; i++)
> > > > + if ((doe->prots[i].vid == vid) &&
> > > > + (doe->prots[i].type == type))
> > > > + return true;
> > > > +
> > > > + return false;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
> > > > +
> > > > +static int pci_doe_discovery(struct pci_doe *doe, u8 *index, u16 *vid,
> > > > + u8 *protocol)
> > > > +{
> > > > + u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
> > > > + *index);
> > > > + u32 response_pl;
> > > > + struct pci_doe_exchange ex = {
> > > > + .prot.vid = PCI_VENDOR_ID_PCI_SIG,
> > > > + .prot.type = PCI_DOE_PROTOCOL_DISCOVERY,
> > > > + .request_pl = &request_pl,
> > > > + .request_pl_sz = sizeof(request_pl),
> > > > + .response_pl = &response_pl,
> > > > + .response_pl_sz = sizeof(response_pl),
> > > > + };
> > > > + int ret;
> > > > +
> > > > + ret = pci_doe_exchange_sync(doe->doe_dev, &ex);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + if (ret != sizeof(response_pl))
> > > > + return -EIO;
> > > > +
> > > > + *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response_pl);
> > > > + *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL,
> > > > + response_pl);
> > > > + *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX,
> > > > + response_pl);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int pci_doe_cache_protocols(struct pci_doe *doe)
> > > > +{
> > > > + u8 index = 0;
> > > > + int num_prots;
> > > > + int rc;
> > > > +
> > > > + /* Discovery protocol must always be supported and must report itself */
> > > > + num_prots = 1;
> > > > + doe->prots = devm_kcalloc(&doe->doe_dev->adev.dev, num_prots,
> > > > + sizeof(*doe->prots), GFP_KERNEL);
> > > > + if (doe->prots == NULL)
> > > > + return -ENOMEM;
> > > > +
> > > > + do {
> > > > + struct pci_doe_protocol *prot;
> > > > +
> > > > + prot = &doe->prots[num_prots - 1];
> > > > + rc = pci_doe_discovery(doe, &index, &prot->vid, &prot->type);
> > > > + if (rc)
> > > > + return rc;
> > > > +
> > > > + if (index) {
> > > > + struct pci_doe_protocol *prot_new;
> > > > +
> > > > + num_prots++;
> > > > + prot_new = devm_krealloc(&doe->doe_dev->adev.dev,
> > > > + doe->prots,
> > > > + sizeof(*doe->prots) *
> > > > + num_prots,
> > > > + GFP_KERNEL);
> > > > + if (prot_new == NULL)
> > > > + return -ENOMEM;
> > > > + doe->prots = prot_new;
> > > > + }
> > > > + } while (index);
> > > > +
> > > > + doe->num_prots = num_prots;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int pci_doe_abort(struct pci_doe *doe)
> > > > +{
> > > > + reinit_completion(&doe->abort_c);
> > > > + mutex_lock(&doe->state_lock);
> > > > + doe->abort = true;
> > >
> > > Why not a flags field where atomic bitops can be used without need for a mutex.
> >
> > I'll go the other way, why bother with atomics when this isn't a high performance
> > path or something expected to happen often?
>
> It obfuscates what the lock is protecting if it's used for state
> management and atomic flag management, but I am not holding the pen
> here, so I can let this arbitrary trade-off go.
Sure, given Ira is now doing the leg work, up to Ira or other reviewers.
Jonathan
