| Message ID | 20210106201610.26538-38-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMI vendor protocols and modularization | expand |
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 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?
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) > > + } > > + } > > + > > + /* > > + * 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
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
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.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. > 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
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 88e5057f4e85..88149a46e6d9 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -51,6 +51,31 @@ static int scmi_dev_match(struct device *dev, struct device_driver *drv) return 0; } +static int scmi_match_by_id_table(struct device *dev, void *data) +{ + struct scmi_device *sdev = to_scmi_dev(dev); + struct scmi_device_id *id_table = data; + + return sdev->protocol_id == id_table->protocol_id && + !strcmp(sdev->name, id_table->name); +} + +struct scmi_device *scmi_find_child_dev(struct device *parent, + int prot_id, const char *name) +{ + struct scmi_device_id id_table; + struct device *dev; + + id_table.protocol_id = prot_id; + id_table.name = name; + + dev = device_find_child(parent, &id_table, scmi_match_by_id_table); + if (!dev) + return NULL; + + return to_scmi_dev(dev); +} + const struct scmi_protocol *scmi_get_protocol(int protocol_id) { const struct scmi_protocol *proto; @@ -114,6 +139,10 @@ int scmi_driver_register(struct scmi_driver *driver, struct module *owner, { int retval; + retval = scmi_request_protocol_device(driver->id_table); + if (retval) + return retval; + driver->driver.bus = &scmi_bus_type; driver->driver.name = driver->name; driver->driver.owner = owner; @@ -130,6 +159,7 @@ EXPORT_SYMBOL_GPL(scmi_driver_register); void scmi_driver_unregister(struct scmi_driver *driver) { driver_unregister(&driver->driver); + scmi_unrequest_protocol_device(driver->id_table); } EXPORT_SYMBOL_GPL(scmi_driver_unregister); diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 1e2046c61d43..9a0519db4865 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -307,6 +307,11 @@ struct scmi_transport_ops { bool (*poll_done)(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer); }; +int scmi_request_protocol_device(const struct scmi_device_id *id_table); +void scmi_unrequest_protocol_device(const struct scmi_device_id *id_table); +struct scmi_device *scmi_find_child_dev(struct device *parent, + int prot_id, const char *name); + /** * struct scmi_desc - Description of SoC integration * diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 4b68952e49db..94d30fa652d9 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -56,6 +56,14 @@ static DEFINE_MUTEX(scmi_list_mutex); /* Track the unique id for the transfers for debug & profiling purpose */ static atomic_t transfer_last_id; +static DEFINE_IDR(scmi_requested_devices); +static DEFINE_MUTEX(scmi_requested_devices_mutex); + +struct scmi_requested_dev { + const struct scmi_device_id *id_table; + struct scmi_requested_dev *next; +}; + /** * struct scmi_xfers_info - Structure to manage transfer information * @@ -113,6 +121,8 @@ struct scmi_protocol_instance { * @protocols_mtx: A mutex to protect protocols instances initialization. * @protocols_imp: List of protocols implemented, currently maximum of * MAX_PROTOCOLS_IMP elements allocated by the base protocol + * @active_protocols: IDR storing device_nodes for protocols actually defined + * in the DT and confirmed as implemented by fw. * @notify_priv: Pointer to private data structure specific to notifications. * @node: List head * @users: Number of users of this instance @@ -130,6 +140,7 @@ struct scmi_info { /* Ensure mutual exclusive access to protocols instance array */ struct mutex protocols_mtx; u8 *protocols_imp; + struct idr active_protocols; void *notify_priv; struct list_head node; int users; @@ -929,6 +940,13 @@ static void scmi_devm_put_protocol_ops(struct scmi_device *sdev, u8 protocol_id) WARN_ON(ret); } +static inline +struct scmi_handle *scmi_handle_get_from_info(struct scmi_info *info) +{ + info->users++; + return &info->handle; +} + /** * scmi_handle_get() - Get the SCMI handle for a device * @@ -950,8 +968,7 @@ struct scmi_handle *scmi_handle_get(struct device *dev) list_for_each(p, &scmi_list) { info = list_entry(p, struct scmi_info, node); if (dev->parent == info->dev) { - handle = &info->handle; - info->users++; + handle = scmi_handle_get_from_info(info); break; } } @@ -1094,62 +1111,254 @@ scmi_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id) return ret; } -static inline void -scmi_create_protocol_device(struct device_node *np, struct scmi_info *info, - int prot_id, const char *name) +/** + * scmi_get_protocol_device - Helper to get/create an SCMI device. + * + * @np: A device node representing a valid active protocols for the referred + * SCMI instance. + * @info: The referred SCMI instance for which we are getting/creating this + * device. + * @prot_id: The protocol ID. + * @name: The device name. + * + * Referring to the specific SCMI instance identified by @info, this helper + * takes care to return a properly initialized device matching the requested + * @proto_id and @name: if device was still not existent it is created as a + * child of the specified SCMI instance @info and its transport properly + * initialized as usual. + */ +static inline struct scmi_device * +scmi_get_protocol_device(struct device_node *np, struct scmi_info *info, + int prot_id, const char *name) { struct scmi_device *sdev; + /* Already created for this parent SCMI instance ? */ + sdev = scmi_find_child_dev(info->dev, prot_id, name); + if (sdev) + return sdev; + + pr_debug("Creating SCMI device (%s) for protocol %x\n", name, prot_id); + sdev = scmi_device_create(np, info->dev, prot_id, name); if (!sdev) { dev_err(info->dev, "failed to create %d protocol device\n", prot_id); - return; + return NULL; } if (scmi_txrx_setup(info, &sdev->dev, prot_id)) { dev_err(&sdev->dev, "failed to setup transport\n"); scmi_device_destroy(sdev); - return; + return NULL; } + return sdev; +} + +static inline void +scmi_create_protocol_device(struct device_node *np, struct scmi_info *info, + int prot_id, const char *name) +{ + struct scmi_device *sdev; + + sdev = scmi_get_protocol_device(np, info, prot_id, name); + if (!sdev) + return; + /* setup handle now as the transport is ready */ scmi_set_handle(sdev); } -#define MAX_SCMI_DEV_PER_PROTOCOL 2 -struct scmi_prot_devnames { - int protocol_id; - char *names[MAX_SCMI_DEV_PER_PROTOCOL]; -}; +/** + * scmi_create_protocol_devices - Create devices for all pending requests for + * this SCMI instance. + * + * @np: The device node describing the protocol + * @info: The SCMI instance descriptor + * @prot_id: The protocol ID + * + * All devices previously requested for this instance (if any) are found and + * created by scanning the proper @&scmi_requested_devices entry. + */ +static void scmi_create_protocol_devices(struct device_node *np, + struct scmi_info *info, int prot_id) +{ + struct scmi_requested_dev *rdev; -static struct scmi_prot_devnames devnames[] = { - { SCMI_PROTOCOL_POWER, { "genpd" },}, - { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, - { SCMI_PROTOCOL_PERF, { "cpufreq" },}, - { SCMI_PROTOCOL_CLOCK, { "clocks" },}, - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, - { SCMI_PROTOCOL_RESET, { "reset" },}, - { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, -}; + rdev = idr_find(&scmi_requested_devices, prot_id); + if (!rdev) + return; -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; + } + } + } + + /* + * 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) + if (!strcmp(victim->id_table->name, id_table->name)) + break; + + if (victim) { + if (victim == head) { + head = victim->next; + if (head) + idr_replace(&scmi_requested_devices, head, + victim->id_table->protocol_id); + else + idr_remove(&scmi_requested_devices, + victim->id_table->protocol_id); + } else { + prev->next = victim->next; } + kfree(victim); } } @@ -1175,6 +1384,7 @@ static int scmi_probe(struct platform_device *pdev) INIT_LIST_HEAD(&info->node); idr_init(&info->protocols); mutex_init(&info->protocols_mtx); + idr_init(&info->active_protocols); platform_set_drvdata(pdev, info); idr_init(&info->tx_idr); @@ -1229,6 +1439,19 @@ static int scmi_probe(struct platform_device *pdev) continue; } + /* + * Save this valid DT protocol descriptor amongst + * @active_protocols for this SCMI instance/ + */ + ret = idr_alloc(&info->active_protocols, child, + prot_id, prot_id + 1, GFP_KERNEL); + if (ret != prot_id) { + dev_err(dev, "SCMI protocol %d already activated. Skip\n", + prot_id); + continue; + } + + of_node_get(child); scmi_create_protocol_devices(child, info, prot_id); } @@ -1242,9 +1465,10 @@ void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id) static int scmi_remove(struct platform_device *pdev) { - int ret = 0; + int ret = 0, id; struct scmi_info *info = platform_get_drvdata(pdev); struct idr *idr = &info->tx_idr; + struct device_node *child; scmi_notification_exit(&info->handle); @@ -1262,6 +1486,10 @@ static int scmi_remove(struct platform_device *pdev) idr_destroy(&info->protocols); mutex_unlock(&info->protocols_mtx); + idr_for_each_entry(&info->active_protocols, child, id) + of_node_put(child); + idr_destroy(&info->active_protocols); + /* Safe to free channels since no more users */ ret = idr_for_each(idr, info->desc->ops->chan_free, idr); idr_destroy(&info->tx_idr);
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> --- v3 --> v4 - add a few comments --- drivers/firmware/arm_scmi/bus.c | 30 +++ drivers/firmware/arm_scmi/common.h | 5 + drivers/firmware/arm_scmi/driver.c | 296 +++++++++++++++++++++++++---- 3 files changed, 297 insertions(+), 34 deletions(-)