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 <cristian.maru...@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.
>
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
