lkml.org 
[lkml]   [2021]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 37/37] firmware: arm_scmi: add dynamic scmi devices creation
From
Date


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

\
 
 \ /
  Last update: 2021-01-08 17:35    [W:0.060 / U:2.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site