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
| ||
|
Date: Fri, 8 Jan 2021 12:24:39 +0000 From: Cristian Marussi <cristian.marussi@....com> To: Thara Gopinath <thara.gopinath@...aro.org> Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, sudeep.holla@....com, lukasz.luba@....com, james.quinlan@...adcom.com, Jonathan.Cameron@...wei.com, f.fainelli@...il.com, etienne.carriere@...aro.org, vincent.guittot@...aro.org, souvik.chakravarty@....com Subject: Re: [PATCH v4 03/37] firmware: arm_scmi: introduce devres get/put protocols operations On Thu, Jan 07, 2021 at 09:28:37AM -0500, Thara Gopinath wrote: > > > On 1/6/21 3:15 PM, Cristian Marussi wrote: > > Expose to the SCMI drivers a new devres managed common protocols API based > > on generic get/put methods and protocol handles. > > > > All drivers still keep using the old API, no functional change. > > > > Signed-off-by: Cristian Marussi <cristian.marussi@....com> > > --- > > drivers/firmware/arm_scmi/driver.c | 92 ++++++++++++++++++++++++++++++ > > include/linux/scmi_protocol.h | 11 ++++ > > 2 files changed, 103 insertions(+) > > > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > > index 10fe9aacae1b..fbc3ba1b69f6 100644 > > --- a/drivers/firmware/arm_scmi/driver.c > > +++ b/drivers/firmware/arm_scmi/driver.c > > @@ -15,6 +15,7 @@ > > */ > > #include <linux/bitmap.h> > > +#include <linux/device.h> > > #include <linux/export.h> > > #include <linux/idr.h> > > #include <linux/io.h> > > @@ -732,6 +733,95 @@ scmi_is_protocol_implemented(const struct scmi_handle *handle, u8 prot_id) > > return false; > > } > > +struct scmi_protocol_devres { > > + struct scmi_handle *handle; > > + u8 protocol_id; > > +}; > > + > > +static void scmi_devm_release_protocol(struct device *dev, void *res) > > +{ > > + struct scmi_protocol_devres *dres = res; > > + > > + scmi_release_protocol(dres->handle, dres->protocol_id); > > +} > > + > > +/** > > + * scmi_devm_get_protocol_ops - Devres managed get protocol operations > > + * @sdev: A reference to an scmi_device whose embedded struct device is to > > + * be used for devres accounting. > > + * @protocol_id: The protocol being requested. > > + * @ph: A pointer reference used to pass back the associated protocol handle. > > + * > > + * Get hold of a protocol accounting for its usage, eventually triggering its > > + * initialization, and returning the protocol specific operations and related > > + * protocol handle which will be used as first argument in most of the > > + * protocols operations methods. > > + * Being a devres based managed method, protocol hold will be automatically > > + * released, and possibly de-initialized on last user, once the SCMI driver > > + * owning the scmi_device is unbound from it. > > + * > > + * Return: A reference to the requested protocol operations or error. > > + * Must be checked for errors by caller. > > + */ > > +static const void __must_check * > > +scmi_devm_get_protocol_ops(struct scmi_device *sdev, u8 protocol_id, > > + struct scmi_protocol_handle **ph) > > +{ > > + struct scmi_protocol_instance *pi; > > + struct scmi_protocol_devres *dres; > > + struct scmi_handle *handle = sdev->handle; > > + > > + if (!ph) > > + return ERR_PTR(-EINVAL); > > + > > + dres = devres_alloc(scmi_devm_release_protocol, > > + sizeof(*dres), GFP_KERNEL); > > + if (!dres) > > + return ERR_PTR(-ENOMEM); > > + > > + pi = scmi_get_protocol_instance(handle, protocol_id); > > + if (IS_ERR(pi)) { > > + devres_free(dres); > > + return pi; > > + } > > + > > + dres->handle = handle; > > + dres->protocol_id = protocol_id; > > + devres_add(&sdev->dev, dres); > > + > > + *ph = &pi->ph; > > + > > + return pi->proto->ops; > > +} > > + > > +static int scmi_devm_protocol_match(struct device *dev, void *res, void *data) > > +{ > > + struct scmi_protocol_devres *dres = res; > > + > > + if (WARN_ON(!dres || !data)) > > + return 0; > > + > > + return dres->protocol_id == *((u8 *)data); > > +} > > + > > +/** > > + * scmi_devm_put_protocol_ops - Devres managed put protocol operations > > + * @sdev: A reference to an scmi_device whose embedded struct device is to > > + * be used for devres accounting. > > + * @protocol_id: The protocol being requested. > > + * > > + * Explicitly release a protocol hold previously obtained calling the above > > + * @scmi_devm_get_protocol_ops. > > + */ > > +static void scmi_devm_put_protocol_ops(struct scmi_device *sdev, u8 protocol_id) > > +{ > > + int ret; > > + > > + ret = devres_release(&sdev->dev, scmi_devm_release_protocol, > > + scmi_devm_protocol_match, &protocol_id); > > + WARN_ON(ret); > > +} > > + > > /** > > * scmi_handle_get() - Get the SCMI handle for a device > > * > > @@ -986,6 +1076,8 @@ static int scmi_probe(struct platform_device *pdev) > > handle = &info->handle; > > handle->dev = info->dev; > > handle->version = &info->version; > > + handle->devm_get_ops = scmi_devm_get_protocol_ops; > > + handle->devm_put_ops = scmi_devm_put_protocol_ops; > > ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE); > > if (ret) > > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > > index 757a826e3cef..2fd2fffb4024 100644 > > --- a/include/linux/scmi_protocol.h > > +++ b/include/linux/scmi_protocol.h > > @@ -57,6 +57,8 @@ struct scmi_clock_info { > > }; > > struct scmi_handle; > > +struct scmi_device; > > +struct scmi_protocol_handle; > > /** > > * struct scmi_clk_ops - represents the various operations provided > > @@ -593,6 +595,9 @@ struct scmi_notify_ops { > > * @sensor_ops: pointer to set of sensor protocol operations > > * @reset_ops: pointer to set of reset protocol operations > > * @voltage_ops: pointer to set of voltage protocol operations > > + * @devm_get_ops: devres managed method to acquire a protocol and get specific > > + * operations and a dedicated protocol handler > > + * @devm_put_ops: devres managed method to release a protocol > > * @notify_ops: pointer to set of notifications related operations > > * @perf_priv: pointer to private data structure specific to performance > > * protocol(for internal use only) > > @@ -618,6 +623,12 @@ struct scmi_handle { > > const struct scmi_sensor_ops *sensor_ops; > > const struct scmi_reset_ops *reset_ops; > > const struct scmi_voltage_ops *voltage_ops; > > + > > + const void __must_check * > > + (*devm_get_ops)(struct scmi_device *sdev, u8 proto, > > + struct scmi_protocol_handle **ph); > > + void (*devm_put_ops)(struct scmi_device *sdev, u8 proto); > > These names are misleading. The devm_get_ops does two things. One populate > the scmi_protocol_handle, second return the protocol ops. Either split this > into two separate functions or rename it into something like > devm_get_protocol (or something better). Similar comment for devm_put_ops as > there is no releasing of ops happening here per say. Yes I agree, now that you really get ops and a hold on the protocol in fact it'd be better _get_protocol/_put_protocol or similar; I'd prefer not to split retrieving the ops from the protocol_handle since they need each other to work. I'll fix in V5. > Also I am still not convinced that protocol_instance should be hidden from > the client drivers. But if everyone else is aligned towards this approach, I > am fine. Do you mean passing the protocol_handle around instead of the handle, so that protocols impementation are restricted to their own protocol number and SCMI drivers cannot access anything at the protocol layer ? It seems reasonable to me but I'm happy to discuss your concerns, also because up until now you are my main and only feedback about this :D Thanks Cristian > > > + > > const struct scmi_notify_ops *notify_ops; > > /* for protocol internal use */ > > void *perf_priv; > > > > -- > Warm Regards > Thara
Powered by blists - more mailing lists