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;
Subject: Re: [PATCH v4 37/37] firmware: arm_scmi: add dynamic scmi devices
 creation
To:     Cristian Marussi <cristian.marussi@arm.com>
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
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>
From:   Thara Gopinath <thara.gopinath@linaro.org>
Message-ID: <9d08fb9f-44e0-7f1d-9568-ebb499c91434@linaro.org>
Date:   Fri, 8 Jan 2021 11:32:17 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210108144257.GD9138@e120937-lin>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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 <cristian.marussi@arm.com>
>>> ---
>> 	
>> [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.

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
> 
> Cristian
> 
>>
>> -- 
>> Warm Regards
>> Thara

-- 
Warm Regards
Thara