On Tue, Mar 23, 2021 at 11:25 AM Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> 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.
