Re: [PATCH v2 2/8] firmware: arm_scmi: introduce protocol handles
From: Thara Gopinath
Date: Fri Nov 06 2020 - 11:26:49 EST
On 11/4/20 12:44 PM, Cristian Marussi wrote:
Hi
On Wed, Nov 04, 2020 at 11:16:18AM -0500, Thara Gopinath wrote:
Hi Cristian,
On 10/28/20 4:29 PM, Cristian Marussi wrote:
Add basic protocol handles definitions and helpers support.
All protocols initialization code and SCMI drivers probing is still
performed using the handle based interface.
Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
---
drivers/firmware/arm_scmi/common.h | 61 ++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/driver.c | 64 ++++++++++++++++++++++++++++++
2 files changed, 125 insertions(+)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index b08a8ddbc22a..f0678be02a09 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -151,6 +151,67 @@ int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id,
size_t tx_size, size_t rx_size, struct scmi_xfer **p);
void scmi_reset_rx_to_maxsz(const struct scmi_handle *handle,
struct scmi_xfer *xfer);
+
+struct scmi_xfer_ops;
+
+/**
+ * struct scmi_protocol_handle - Reference to an initialized protocol instance
+ *
+ * @dev: A reference to the associated SCMI instance device (handle->dev).
+ * @xops: A reference to a struct holding refs to the core xfer operations that
+ * can be used by the protocol implementation to generate SCMI messages.
+ * @set_priv: A method to set protocol private data for this instance.
+ * @get_priv: A method to get protocol private data previously set.
+ *
+ * This structure represents a protocol initialized against specific SCMI
+ * instance and it will be used as follows:
+ * - as a parameter fed from the core to the protocol initialization code so
+ * that it can access the core xfer operations to build and generate SCMI
+ * messages exclusively for the specific underlying protocol instance.
+ * - as an opaque handle fed by an SCMI driver user when it tries to access
+ * this protocol through its own protocol operations.
+ * In this case this handle will be returned as an opaque object together
+ * with the related protocol operations when the SCMI driver tries to access
+ * the protocol.
+ */
+struct scmi_protocol_handle {
+ struct device *dev;
+ const struct scmi_xfer_ops *xops;
+ int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv);
+ void *(*get_priv)(const struct scmi_protocol_handle *ph);
+};
So scmi_xfer_ops are the ops that actually talks with the scmi firmware on
the other end , right ? IIUC, these ops are the same for all the protocols
of a scmi instance. Imho, this struct is not the right place for these ops
to reside.You are inadvertently exposing scmi internal details to the client
drivers. There is no reason why this should be part of scmi_handle. The
protocols can extract it from the handle during protocol_reigster, right?
So, now to the second part, why do you need a scmi_protocol_handle? Again
IIUC, if you have set_priv and get_priv hooks and get_ops and put_ops hooks,
there is nothing that scmi_protocol_handle is providing extra, right? As
mentioned in the comments for last patch any reason all of this cannot be
rolled into scmi_protocol?
The basic idea for protocol_hande existence is that the protocol code
should be able to access the core xfer ops (without EXPORTing all
scmi_xfer ops) but protoX should NOT be allowed to mistakenly or
maliciously build and send protoY messages: since the protocol_handle
for protoX is embedded in a specific protocol_instance in this way you
can call from your protocol code something like:
ph->xops->xfer_get_init(ph, ...)
I am still confused by this one... scmi_protocol_instance has a pointer
to scmi_handle. So why not handle->xops->xfer_get_init(pi, ....). Here
also protoX will not be allowed to send protoY messages, right? And then
again set_priv and get_priv can be moved to scmi_protocol_instance right ?
--
Warm Regards
Thara