Received: by 2002:a05:6a10:2785:0:0:0:0 with SMTP id ia5csp354860pxb; Fri, 8 Jan 2021 06:46:56 -0800 (PST) X-Google-Smtp-Source: ABdhPJwmqcEuAKl11RMsf/PN74/Q8ZxH/IwJlJSwSHK5F4q5IzN4EQAxL0Ws1hiZ8FLxONGtNufb X-Received: by 2002:a17:906:653:: with SMTP id t19mr2902281ejb.44.1610117216601; Fri, 08 Jan 2021 06:46:56 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1610117216; cv=none; d=google.com; s=arc-20160816; b=nMv35RKQP5lYIiI0n4r4zAYSvypGmRIBBJ3IGCSxIFWZPI8INMVru+0yDuYyOEjO4m lGk7XavsPPAOfvrGVmPbo24iFR7h7t+FdHfME9YytH05oHmH8Ehm46JsqrZD1hmgk0E5 YKOlcvIcB2pMJcsV2Xa/pdZCaPFc36QcWUfxSs2GZkbHQ6SqI8/zDICaY7WaPk84sB86 3hSQkszLaisAnmf8PYyQe0g3dBPq6bsh8h4Mf+NZB7wI9bFyfYdLV9tEWXc1kGB3j1kK dUPsM2o7g52L+D7wOG2VWRtH5fb8jHwk1TxCmVV+E7PMzkqcket7F0WvfPTyvpxTLDc+ htiA== 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=754X3lbC1zwKLp8l+hHtMbXLHkdmMXZUkxs8ADxrP00=; b=PV5D7Gg9EFFNb5moN7Kz4SrCVJYEuBYemCKNccC0jo+574QLUK15geMR/e2RL5iRWN PFJUp5Rlstfk0f0GCP4awLg3taaVv7KXL54NQ8koRkjm5kWXiEKFjt72YHYoR+q216rp 7vdwSvfMYrl/+A8HCCIMzHdzrWO68hiaNUB6aQVVYrfcO9swrFq4HpmT7WrL89wi5+9S LkyRmOzwiOxHLO5n1NSNaJ2xCplKXcCYLLnVWsCzROF0bGoaptE8K2tNB6/ytC4X4fof 8lOkhP5NzuTUZGlnHQd5u9spRPmWdD81INvBiMmuCRBkpboTdWsNV+ivqBgZ2jghnCXA Ec5A== 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 lz20si3515932ejb.451.2021.01.08.06.46.32; Fri, 08 Jan 2021 06:46:56 -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 S1726733AbhAHOns (ORCPT + 99 others); Fri, 8 Jan 2021 09:43:48 -0500 Received: from foss.arm.com ([217.140.110.172]:52244 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726474AbhAHOnr (ORCPT ); Fri, 8 Jan 2021 09:43:47 -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 8C877ED1; Fri, 8 Jan 2021 06:43:01 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BD6123F70D; Fri, 8 Jan 2021 06:42:59 -0800 (PST) Date: Fri, 8 Jan 2021 14:42:57 +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: <20210108144257.GD9138@e120937-lin> References: <20210106201610.26538-1-cristian.marussi@arm.com> <20210106201610.26538-38-cristian.marussi@arm.com> <50434a02-0fe0-50f3-1529-51ab8a0cc1f3@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50434a02-0fe0-50f3-1529-51ab8a0cc1f3@linaro.org> User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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) > > + } > > + } > > + > > + /* > > + * 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. Thanks Cristian > > -- > Warm Regards > Thara