Messages in this thread | | | Subject | Re: [PATCH v4 37/37] firmware: arm_scmi: add dynamic scmi devices creation | From | Thara Gopinath <> | Date | Fri, 8 Jan 2021 11:32:17 -0500 |
| |
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
| |