From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CFF49C433E0 for ; Fri, 8 Jan 2021 16:53:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8C54D239FD for ; Fri, 8 Jan 2021 16:53:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728211AbhAHQxA (ORCPT ); Fri, 8 Jan 2021 11:53:00 -0500 Received: from foss.arm.com ([217.140.110.172]:54126 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727530AbhAHQxA (ORCPT ); Fri, 8 Jan 2021 11:53:00 -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 9941A11FB; Fri, 8 Jan 2021 08:52:14 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CB9713F70D; Fri, 8 Jan 2021 08:52:12 -0800 (PST) Date: Fri, 8 Jan 2021 16:52:10 +0000 From: Cristian Marussi To: Thara Gopinath Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, lukasz.luba@arm.com, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, f.fainelli@gmail.com, etienne.carriere@linaro.org, vincent.guittot@linaro.org, souvik.chakravarty@arm.com Subject: Re: [PATCH v4 37/37] firmware: arm_scmi: add dynamic scmi devices creation Message-ID: <20210108165210.GF9138@e120937-lin> References: <20210106201610.26538-1-cristian.marussi@arm.com> <20210106201610.26538-38-cristian.marussi@arm.com> <50434a02-0fe0-50f3-1529-51ab8a0cc1f3@linaro.org> <20210108144257.GD9138@e120937-lin> <9d08fb9f-44e0-7f1d-9568-ebb499c91434@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9d08fb9f-44e0-7f1d-9568-ebb499c91434@linaro.org> User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 08, 2021 at 11:32:17AM -0500, Thara Gopinath wrote: > > > On 1/8/21 9:42 AM, Cristian Marussi wrote: > > On Thu, Jan 07, 2021 at 09:28:07AM -0500, Thara Gopinath wrote: > > > Hi Christian, > > > > > > On 1/6/21 3:16 PM, Cristian Marussi wrote: > > > > Having added the support for SCMI protocols as modules in order to let > > > > vendors extend the SCMI core with their own additions it seems odd to > > > > then force SCMI drivers built on top to use a static device table to > > > > declare their devices since this way any new SCMI drivers addition > > > > would need the core SCMI device table to be updated too. > > > > > > > > Remove the static core device table and let SCMI drivers to simply declare > > > > which device/protocol pair they need at initialization time: the core will > > > > then take care to generate such devices dynamically during platform > > > > initialization or at module loading time, as long as the requested > > > > underlying protocol is defined in the DT. > > > > > > > > Signed-off-by: Cristian Marussi > > > > --- > > > > > > [snip] > > > > > > > -static inline void > > > > -scmi_create_protocol_devices(struct device_node *np, struct scmi_info *info, > > > > - int prot_id) > > > > + for (; rdev; rdev = rdev->next) > > > > + scmi_create_protocol_device(np, info, prot_id, > > > > + rdev->id_table->name); > > > > +} > > > > + > > > > +/** > > > > + * scmi_request_protocol_device - Helper to request a device > > > > + * > > > > + * @id_table: A protocol/name pair descriptor for the device to be created. > > > > + * > > > > + * This helper let an SCMI driver request specific devices identified by the > > > > + * @id_table to be created for each active SCMI instance. > > > > + * > > > > + * The requested device name MUST NOT be already existent for any protocol; > > > > + * at first the freshly requested @id_table is annotated in the IDR table > > > > + * @scmi_requested_devices, then a matching device is created for each already > > > > + * active SCMI instance. (if any) > > > > + * > > > > + * This way the requested device is created straight-away for all the already > > > > + * initialized(probed) SCMI instances (handles) but it remains instead pending > > > > + * for creation if the requesting SCMI driver is loaded before some instance > > > > + * and related transports was available: when such late SCMI instance is probed > > > > + * it will take care to scan the list of pending requested devices and create > > > > + * those on its own (see @scmi_create_protocol_devices and its enclosing loop) > > > > + * > > > > + * Return: 0 on Success > > > > + */ > > > > +int scmi_request_protocol_device(const struct scmi_device_id *id_table) > > > > { > > > > - int loop, cnt; > > > > + int ret = 0; > > > > + unsigned int id = 0; > > > > + struct scmi_requested_dev *rdev, *proto_rdev = NULL; > > > > + struct scmi_info *info; > > > > - for (loop = 0; loop < ARRAY_SIZE(devnames); loop++) { > > > > - if (devnames[loop].protocol_id != prot_id) > > > > - continue; > > > > + pr_debug("Requesting SCMI device (%s) for protocol %x\n", > > > > + id_table->name, id_table->protocol_id); > > > > - for (cnt = 0; cnt < ARRAY_SIZE(devnames[loop].names); cnt++) { > > > > - const char *name = devnames[loop].names[cnt]; > > > > + /* > > > > + * Search for the matching protocol rdev list and then search > > > > + * of any existent equally named device...fails if any duplicate found. > > > > + */ > > > > + mutex_lock(&scmi_requested_devices_mutex); > > > > + idr_for_each_entry(&scmi_requested_devices, rdev, id) { > > > > + if (rdev->id_table->protocol_id == id_table->protocol_id) > > > > + proto_rdev = rdev; > > > > + for (; rdev; rdev = rdev->next) { > > > > + if (!strcmp(rdev->id_table->name, id_table->name)) { > > > > + pr_err("Ignoring duplicate request [%d] %s\n", > > > > + rdev->id_table->protocol_id, > > > > + rdev->id_table->name); > > > > + ret = -EINVAL; > > > > + goto out; > > > > + } > > > Shouldn't there be proto_rdev = rdev here as well ? > > > > > > > No, because each IDR entry points to one or more linked rdev descriptors > > for the same protocol: while scanning each list in the IDR table I'm > > searching for the proto_rdev representing the head of that protocol list > > (if any already exist) and also scan all the lists fully to check for > > duplicates, in such a case we give up. > > The IDR map containing list resembles a lot a Linux hash implementation > > but I decided not to use it because it seemed cumbersome to use an > > hash given most of the time each IDR entry will contain just one single > > element and this lookup happens really very infrequently (just at driver > > loading time) > > I agree that using hash might be overkill here. > I still think you need proto_rdev = rdev so that proto_rdev points to the > last element and not the head. Else later on, below when you do > proto_rdev->next = rdev; > you will lose devices. > > Basically in the current implementation if there are more than two devices > for a protocol, you will end up losing devices since you are adding the new > device as the second device always. > Ah right I missed that sorry...now I'm definitely convinced to use klist heads in the IDRs and avoid all of this :D > I think like you mentioned this should be a klist instead of a custom linked > list. And idr can keep track of head of each list. > > > > > > > + } > > > > + } > > > > + > > > > + /* > > > > + * No duplicate found for requested id_table, so let's create a new > > > > + * requested device entry for this new valid request. > > > > + */ > > > > + rdev = kzalloc(sizeof(*rdev), GFP_KERNEL); > > > > + if (!rdev) { > > > > + ret = -ENOMEM; > > > > + goto out; > > > > + } > > > > + rdev->id_table = id_table; > > > > + > > > > + /* > > > > + * Append the new requested device table descriptor to the head of the > > > > + * related protocol chain, eventually creating such chain if not already > > > > + * there. > > > > + */ > > > > + if (!proto_rdev) { > > > > + ret = idr_alloc(&scmi_requested_devices, (void *)rdev, > > > > + rdev->id_table->protocol_id, > > > > + rdev->id_table->protocol_id + 1, GFP_KERNEL); > > > > + if (ret != rdev->id_table->protocol_id) { > > > > + pr_err("Failed to save SCMI device - ret:%d\n", ret); > > > > + kfree(rdev); > > > > + ret = -EINVAL; > > > > + goto out; > > > > + } > > > > + ret = 0; > > > > + } else { > > > > + proto_rdev->next = rdev; > > > > + } > > > > + > > > > + /* > > > > + * Now effectively create and initialize the requested device for every > > > > + * already initialized SCMI instance which has registered the requested > > > > + * protocol as a valid active one: i.e. defined in DT and supported by > > > > + * current platform FW. > > > > + */ > > > > + mutex_lock(&scmi_list_mutex); > > > > + list_for_each_entry(info, &scmi_list, node) { > > > > + struct device_node *child; > > > > + > > > > + child = idr_find(&info->active_protocols, > > > > + id_table->protocol_id); > > > > + if (child) { > > > > + struct scmi_device *sdev; > > > > + > > > > + sdev = scmi_get_protocol_device(child, info, > > > > + id_table->protocol_id, > > > > + id_table->name); > > > > + /* Set handle if not already set (device existed) */ > > > > + if (sdev && !sdev->handle) > > > > + sdev->handle = scmi_handle_get_from_info(info); > > > > + } else { > > > > + dev_err(info->dev, > > > > + "Failed. SCMI protocol %d not active.\n", > > > > + id_table->protocol_id); > > > > + } > > > > + } > > > > + mutex_unlock(&scmi_list_mutex); > > > > + > > > > +out: > > > > + mutex_unlock(&scmi_requested_devices_mutex); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +/** > > > > + * scmi_unrequest_protocol_device - Helper to unrequest a device > > > > + * > > > > + * @id_table: A protocol/name pair descriptor for the device to be unrequested. > > > > + * > > > > + * An helper to let an SCMI driver release its request about devices; note that > > > > + * devices are created and initialized once the first SCMI driver request them > > > > + * but they destroyed only on SCMI core unloading/unbinding. > > > > + * > > > > + * The current SCMI transport layer uses such devices as internal references and > > > > + * as such they could be shared as same transport between multiple drivers so > > > > + * that cannot be safely destroyed till the whole SCMI stack is removed. > > > > + * (unless adding further burden of refcounting.) > > > > + */ > > > > +void scmi_unrequest_protocol_device(const struct scmi_device_id *id_table) > > > > +{ > > > > + struct scmi_requested_dev *victim, *prev, *head; > > > > + > > > > + pr_debug("Unrequesting SCMI device (%s) for protocol %x\n", > > > > + id_table->name, id_table->protocol_id); > > > > - if (name) > > > > - scmi_create_protocol_device(np, info, prot_id, > > > > - name); > > > > + head = idr_find(&scmi_requested_devices, id_table->protocol_id); > > > > + if (!head) > > > > + return; > > > > + > > > > + /* > > > > + * Scan the protocol list of requested device name searching > > > > + * for the victim. > > > > + */ > > > > + victim = head; > > > > + for (prev = victim; victim; prev = victim, victim = victim->next) > > > > > > The initial assignment for the for loop is wrong. With this when you break > > > prev will be equal to victim. You want prev to be the one pointing to the > > > victim. Or am I missing something? > > > > > > > Yes prev is the one preceding the victim, if any, but if it was the head > > I'll remove the head and not use at all the prev really. > > I think is right as it is, it is the naming that is misleading, because > > yes in the initial assignment prev = victim BUT victim = head, so if I bail > > out immediately I'm really removing the head. > > It would be clearer like > > > > prev = victim = head; > > for (; victim; prev = victim, victim = victim->next) > > ... > > > > But it's better that I review this whole loop in deep to simplify it; I > > avoided using klist because seemed easier enough to handle a singly > > linked list which most of the time is one element deep, buut maybe I > > should just stick with well known and proven kists. > > Yes you are right. No bug here. Like I mentioned above, klists are something > to consider here. > Thanks, I'll switch to klist. Cristian > > > > Thanks > > > > Cristian > > > > > > > > -- > > > Warm Regards > > > Thara > > -- > Warm Regards > Thara