Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

[Dan Williams <dan.j.williams@intel.com>]

On Tue, Mar 23, 2021 at 11:25 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
[..]
> > I was thinking something like 'struct pcie_doe_object' pointers rather
> > than u32 arrays.
>
> Possibly... I'm not convinced that it doesn't end up being
> struct pcie_doe_object doe_obj;
> DOE_OBJ_INIT(&doe_obj, vid, type, request_pl, request_pl_sz,
>              response_pl, response_pl_sz)
>
> ret = pcie_doe_exchange(doe, &doe_obj);
>
> If that's the pattern we see, why split it?
> We might well have a struct pcie_doe_object internally but it doesn't
> seem like a sensible external interface to me simply because we'd
> just be filling it in and immediately passing it to a 'send' function.

I don't think there are going to be so many DOE users that would
justify DOE_OBJ_INIT(). I am thinking of a model where the payload
construction is open coded and typesafe similar to how
intel_bus_fwa_activate() [1] builds its mailbox payload.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/nfit/intel.c#n546


> > > The disadvantage is that at least some of the specs just have the
> > > header as their first few DW.  So there isn't a clear distinction
> > > between header and payload. May lead to people getting offsets wrong
> > > in a way they wouldn't do if driver was responsible for building the
> > > whole message.
> >
> > Aren't they more likely to get offsets wrong with u32 arrays rather
> > than data structures?
>
> I'm not sure what level you mean this at.  The CDAT patch review you
> followed this with suggested just breaking out vid and type which is
> fine because those are always packed the same and we can do appropriate
> special handling.
>
> If you meant the whole object as packed structure, then it is a whole
> different matter.
>
> Easy one to point is that u64 values are going to end up with their
> top and bottom halves swapped.  Things get even messier if we break
> up below the u32 level.
>
> We can do this at a higher level by having wrappers that deal with
> each protocol and do a serialize / deserialize for the protocol.
> I'm not sure if that will make sense or not yet though.

Again, I think the protocols to support are limited (CDAT and IDE/SPDM
are the only ones on the horizon), so not much value to having a rich
set of wrappers and macros to obfuscate payload generation.

>
> One thing I don't understand is why you proposed a delayed work queue
> above?  I'm not seeing that model in the libsas SMP for example.  As far
> as I can tell that just processes work items asap.
>
> Can you point to a more specific example if you thinks that we should
> use one?

For polling on a timeout a delayed workqueue can poll at an interval
without need for any explicit sleep calls. There are several examples
of queue_delayed_work() in drivers/ being used to advance a state
machine after a protocol relative timeout.