Received: by 2002:a05:6a10:2785:0:0:0:0 with SMTP id ia5csp438986pxb; Fri, 8 Jan 2021 08:44:53 -0800 (PST) X-Google-Smtp-Source: ABdhPJzbGUTYFVHio8rrInb96QU0zPLEDpOZ/aOQW+l43IE37BYuWtw7cfymUbb3sMornKMETyuR X-Received: by 2002:a17:906:4d52:: with SMTP id b18mr2997808ejv.405.1610124293396; Fri, 08 Jan 2021 08:44:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1610124293; cv=none; d=google.com; s=arc-20160816; b=xX0373Q3MXTyeyY17wgltFRf0UI5uRcfdVw7qkNf1U+EUtovFytVZnlZbzil4sQ0r3 UO0ZpjtQuYZcnO3ij1lzPhTega3IVd5zjiw2Lxzh2xEkEOmjjxZTDEUPnw2oG6Yo1ygO WUoAfxmBEVkAv2XrwYLMqJlPz5Z1NyXPQ9rLPcWxgGWIWgtlpPieYFMF2LkxpTSGP734 uX3doXyzRGZqWos5gXC8dp2ZwKGQEp8SxLE04tMXjxZdnvubZ1U6J5PM+Fx66+IsrBgM XU/8TjLiT7S6g3P9JP/TNxS8bT3dv3QI2bWFpeo5fXz7eDwT2FRES2OjuQr1mr2+Gc/y VdoQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=WyFep3pc3f98+/YLLrSu9SHxT4dHAakQfcWyXAvkVd0=; b=rDitIgKQGb/bJ3dsLVnWjw8Dv9CeGtSI9ymaH0gUf5csM4xf+EyZ35p1JpQ42VMPkX okASJjMJwpivM+92Q2f071FT+RzlqvJNOxTIpijKQZhV6r+blcm/YOz4svBERd/9fu08 hoAXrtzGyPg8jXPg1Ujjbw0T9JTKcY/keLdkDwa6USnQmKirIu7xv6QG/+LvJkFE229M T8YOqYPF1jZIFGdkTHXZb/68sNBUyd/3PFgO2kvgePO+hvklg+d9kNoVHx38MNTDutqQ k+qF2eF3cBr9iY2kuqtlpFt9FjE4ZoWzFRSFIDFOseCXcueuVR2hEzwaM/pw6W79WuEe gDCg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id n6si3837553edr.485.2021.01.08.08.44.27; Fri, 08 Jan 2021 08:44:53 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728204AbhAHQnF (ORCPT + 99 others); Fri, 8 Jan 2021 11:43:05 -0500 Received: from foss.arm.com ([217.140.110.172]:54038 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728169AbhAHQnE (ORCPT ); Fri, 8 Jan 2021 11:43:04 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A5A4D11FB; Fri, 8 Jan 2021 08:42:18 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CD34C3F70D; Fri, 8 Jan 2021 08:42:16 -0800 (PST) Date: Fri, 8 Jan 2021 16:42:14 +0000 From: Cristian Marussi To: Etienne Carriere Cc: Thara Gopinath , linux-kernel@vger.kernel.org, "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" , Sudeep Holla , lukasz.luba@arm.com, Jim Quinlan , Jonathan.Cameron@huawei.com, f.fainelli@gmail.com, Vincent Guittot , Souvik Chakravarty Subject: Re: [PATCH v4 03/37] firmware: arm_scmi: introduce devres get/put protocols operations Message-ID: <20210108164214.GE9138@e120937-lin> References: <20210106201610.26538-1-cristian.marussi@arm.com> <20210106201610.26538-4-cristian.marussi@arm.com> <20210108122439.GC9138@e120937-lin> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Etienne, thanks for the review. On Fri, Jan 08, 2021 at 02:42:07PM +0100, Etienne Carriere wrote: > On Fri, 8 Jan 2021 at 13:24, Cristian Marussi wrote: > > > > 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 > > > > --- > > > > 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 > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -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 > > > > Hello all, > > Sorry for not giving much feedback on your series. Big changes but I agree with. Sorry did not mean to pressure reviewing :D > BTW Cristian i've successfully tested your series, FWIW. Great thanks. > > I'm not sure to understand what you mean by hiding the protocol to the client. > Each client obviously knows the protocol it's using (it's stated in > struct scmi_device_id). > Whatever, I think here get_ops()/put_ops() simply don't need the > protocol_id argument. > scmi_device instance is already specific to a protocol (field > protocol_id in struct scmi_device). > This is an interesting point indeed, something that I wanted to clarify/ask here on the list: beside each scmi_dev being tied to a protocol, as you said, for matching purposes and (possibly) ad-hoc transport initialization, even in the current(old) handle->perf_ops style implementation any SCMI driver was reallly able to access any protocol it wished (handle->power), so I kept it this way also in this new interface, that's the reason why have to specify which protocol you want in get_ops/get_protocol. So even if CPUFREQ driver is mainly a PERF driver and tied to it in the device table, it could anyway access and use also any other std or custom protocol available: not sure if this make sense but I stuck with the original behavior. It's only in the new protocol implementation layer like perf.c that, using protocol handles as interface, the implementation is NOT allowed to craft any message other than the one related to its own protocol N. If this behaviour (multiple protocols available to the same SCMI driver) is not advisable/desirable would have an impact (as you pointed out) on the interface that I expose to the SCMI driver, so better to have this fixed now before I bother 5/6 maintainer to change their drivers and then again in a few weeks :D Thanks Cristian > Best regards, > Etienne > > > Thanks > > > > Cristian > > > > > > > > > + > > > > const struct scmi_notify_ops *notify_ops; > > > > /* for protocol internal use */ > > > > void *perf_priv; > > > > > > > > > > -- > > > Warm Regards > > > Thara