Messages in this thread | | | Subject | Re: [PATCH v2 2/8] firmware: arm_scmi: introduce protocol handles | From | Thara Gopinath <> | Date | Fri, 6 Nov 2020 11:26:44 -0500 |
| |
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@arm.com> >>> --- >>> 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
| |