Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type
flat view
From: Arnd Bergmann <arnd@arndb.de>
Date: 2020-01-10 11:16:17
Also in:
lkml
On Fri, Jan 10, 2020 at 10:43 AM Viresh Kumar [off-list ref] wrote:
The SCMI specification is fairly independent of the transport protocol, which can be a simple mailbox (already implemented) or anything else. The current Linux implementation however is very much dependent of the mailbox transport layer. This patch makes the SCMI core code (driver.c) independent of the mailbox transport layer and moves all mailbox related code to a new file: mailbox.c. We can now implement more transport protocols to transport SCMI messages, some of the transport protocols getting discussed currently are SMC/HVC, SPCI (built on top of SMC/HVC), OPTEE based mailbox (similar to SPCI), and vitio based transport as alternative to mailbox. The transport protocols just need to provide struct scmi_desc, which also implements the struct scmi_transport_ops. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V2: - Dropped __iomem from payload data.
Simply dropping the __iomem isn't much better, now you get other type mismatches.
quoted hunk ↗ jump to hunk
- Moved transport ops to scmi_desc, and that has a per transport instance now which is differentiated using the compatible string. - Converted IS_ERR_OR_NULL to IS_ERR.
These look good to me.
quoted hunk ↗ jump to hunk
+ * @payload: Transmit/Receive payload area + * @dev: Reference to device in the SCMI hierarchy corresponding to this + * channel + * @handle: Pointer to SCMI entity handle + * @transport_info: Transport layer related information + */ +struct scmi_chan_info { + void *payload; + struct device *dev; + struct scmi_handle *handle; + void *transport_info; +};
Maybe you can wrap the scmi_chan_info inside of another structure that contains the payload pointer, and use container_of to convert between them? It's not obvious which parts of the structure should be shared and which are transport specific.
quoted hunk ↗ jump to hunk
-static void scmi_rx_callback(struct mbox_client *cl, void *m) +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t) { u8 msg_type; u32 msg_hdr; u16 xfer_id; struct scmi_xfer *xfer; - struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl); struct device *dev = cinfo->dev; struct scmi_info *info = handle_to_scmi_info(cinfo->handle); struct scmi_xfers_info *minfo = &info->tx_minfo; - struct scmi_shared_mem __iomem *mem = cinfo->payload; + struct scmi_shared_mem *mem = cinfo->payload; msg_hdr = ioread32(&mem->msg_header);
This is where it goes wrong: you cannot pass a kernel pointer
without __iomem into ioread32(). Building the driver with sparse
(using "make C=1") should show you this and possibly other
related conversion bugs.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel