| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZqO54KACyHUUYEXj@pluto>
Date: Fri, 26 Jul 2024 15:59:44 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Etienne CARRIERE <etienne.carriere@...com>
Cc: Cristian Marussi <cristian.marussi@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
"arm-scmi@...r.kernel.org" <arm-scmi@...r.kernel.org>,
"sudeep.holla@....com" <sudeep.holla@....com>,
"james.quinlan@...adcom.com" <james.quinlan@...adcom.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
"peng.fan@....nxp.com" <peng.fan@....nxp.com>,
"michal.simek@....com" <michal.simek@....com>,
"quic_sibis@...cinc.com" <quic_sibis@...cinc.com>,
"quic_nkela@...cinc.com" <quic_nkela@...cinc.com>,
"ptosi@...gle.com" <ptosi@...gle.com>,
"dan.carpenter@...aro.org" <dan.carpenter@...aro.org>,
"souvik.chakravarty@....com" <souvik.chakravarty@....com>
Subject: Re: [PATCH v2 3/8] firmware: arm_scmi: Add support for standalone
transport drivers
On Tue, Jul 23, 2024 at 01:39:41PM +0000, Etienne CARRIERE wrote:
> Hi Cristian,
>
> Few nitpicking comments.
>
> On Wednesday, July 10, 2024, Cristian Marussi wrote:
> > Extend the core SCMI stack with structures and methods to allow for
> > transports to be split out as standalone drivers, while still supporting
> > old style transports, defined as built into the SCMI core stack.
> >
> > No functional change.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> > ---
> > NOTE: old style transport support will be removed later in this series.
> >
> > v1 --> v2
> > - fixed comit message
> > ---
> > drivers/firmware/arm_scmi/common.h | 84 ++++++++++++++++++++++++++++++
> > drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++-
> > drivers/firmware/arm_scmi/msg.c | 5 ++
> > drivers/firmware/arm_scmi/shmem.c | 5 ++
> > 4 files changed, 136 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 8e5751aaa600..4af06810eb39 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations {
> > bool tx, struct resource *res);
> > };
> >
> > +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void);
> > +
> > /* declarations for message passing transports */
> > struct scmi_msg_payld;
> >
> > @@ -376,6 +378,88 @@ struct scmi_message_operations {
> > size_t max_len, struct scmi_xfer *xfer);
> > };
> >
> > +const struct scmi_message_operations *scmi_message_operations_get(void);
> > +
> > +/**
> > + * struct scmi_transport_core_operations - Transpoert core operations
> > + *
> > + * @bad_message_trace: An helper to report a malformed/unexpected message
> > + * @rx_callback: Callback to report received messages
> > + * @shmem: Datagram operations for shared memory based transports
> > + * @msg: Datagram operations for message based transports
> > + */
> > +struct scmi_transport_core_operations {
> > + void (*bad_message_trace)(struct scmi_chan_info *cinfo,
> > + u32 msg_hdr, enum scmi_bad_msg err);
> > + void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr,
> > + void *priv);
> > + const struct scmi_shared_mem_operations *shmem;
> > + const struct scmi_message_operations *msg;
> > +};
> > +
> > +/**
> > + * struct scmi_transport - A structure representing a configured transport
> > + *
> > + * @supplier: Device representimng the transport and acting as a supplier for
>
> typo: s/representimng/representing/
>
Fixed in V3. (...still to be posted)
> > + * the core SCMI stack
> > + * @desc: Transport descriptor
> > + * @core_ops: A pointer to a pointer used by the core SCMI stack to make the
> > + * core transport operations accessible to the transports.
> > + */
> > +struct scmi_transport {
> > + struct device *supplier;
> > + const struct scmi_desc *desc;
> > + struct scmi_transport_core_operations **core_ops;
> > +};
> > +
> > +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, __core_ptr)\
> > +static int __trans##_probe(struct platform_device *pdev) \
> > +{ \
> > + struct scmi_transport *scmi_trans; \
> > + struct platform_device *scmi_pdev; \
> > + struct device *dev = &pdev->dev; \
> > + \
> > + scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), GFP_KERNEL);\
> > + if (!scmi_trans) \
> > + return -ENOMEM; \
> > + \
> > + scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), GFP_KERNEL); \
> > + if (!scmi_pdev) \
> > + return -ENOMEM; \
> > + \
> > + scmi_trans->supplier = dev; \
> > + scmi_trans->desc = &__trans##_desc; \
>
> It's a bit weird the scmi_desc shall be specifically labeled __trans##_desc
> in the transport driver source file while match table and transport core
> operations instances references are passed as arguments. I think it's
> worth having the scmi_desc label also passed as an argument to
> DEFINE_SCMI_TRANSPORT_DRIVER() macro.
Yes, I agree, I was unsure about this so I have reworked all of these in
V3 to pass as explicit parameter the driver name and desc name.
>
> > + scmi_trans->core_ops = __core_ptr; \
> > + \
> > + scmi_pdev->name = "arm-scmi"; \
> > + scmi_pdev->id = PLATFORM_DEVID_AUTO; \
> > + scmi_pdev->dev.platform_data = scmi_trans; \
> > + \
> > + device_set_of_node_from_dev(&scmi_pdev->dev, dev); \
> > + \
> > + dev_set_drvdata(dev, scmi_pdev); \
> > + \
> > + return platform_device_register(scmi_pdev); \
> > +} \
> > + \
> > +static void __trans##_remove(struct platform_device *pdev) \
> > +{ \
> > + struct platform_device *scmi_pdev; \
> > + \
> > + scmi_pdev = dev_get_drvdata(&pdev->dev); \
> > + \
> > + platform_device_unregister(scmi_pdev); \
> > +} \
> > + \
> > +static struct platform_driver __trans##_driver = { \
>
> Same here. I think __trans##_driver label should be also explicitly
> passed as an argument to DEFINE_SCMI_TRANSPORT_DRIVER().
>
Fixed in V3.
Thanks,
Cristian
Powered by blists - more mailing lists