Messages in this thread | | | Date | Thu, 22 Feb 2018 16:58:05 +0000 | From | Lina Iyer <> | Subject | Re: [PATCH v2 07/10] drivers: qcom: rpmh: cache sleep/wake state requests |
| |
On Wed, Feb 21 2018 at 22:07 +0000, Evan Green wrote: >Hi Lina, > >On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer <ilina@codeaurora.org> wrote: [...] >> +static struct cache_req *cache_rpm_request(struct rpmh_client *rc, >> + enum rpmh_state state, >> + struct tcs_cmd *cmd) >> +{ >> + struct cache_req *req; >> + struct rpmh_ctrlr *rpm = rc->ctrlr; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&rpm->lock, flags); >> + req = __find_req(rc, cmd->addr); >> + if (req) >> + goto existing; >> + >> + req = kzalloc(sizeof(*req), GFP_ATOMIC); >> + if (!req) { >> + req = ERR_PTR(-ENOMEM); >> + goto unlock; >> + } >> + >> + req->addr = cmd->addr; >> + req->sleep_val = req->wake_val = UINT_MAX; > >So UINT_MAX is really never a valid value to write? Maybe it would be >good to at least print some sort of complaint if somebody sends down a >request with this value. Otherwise the request is silently ignored and >would be quite challenging to track down. > Yes, UINT_MAX is a invalid value. I have not encountered anything in the spec that points to hold UINT_MAX as a valid value. It would also be quite inefficient to validate each input. Drivers generally know what they are doing. These are data sent to the hardware and there is a general understanding that incorrect data may fail silently.
>> +/** >> + * rpmh_flush: Flushes the buffered active and sleep sets to TCS >> + * >> + * @rc: The RPMh handle got from rpmh_get_dev_channel >> + * >> + * This function is generally called from the sleep code from the last CPU >> + * that is powering down the entire system. >> + * >> + * Returns -EBUSY if the controller is busy, probably waiting on a response >> + * to a RPMH request sent earlier. >> + */ >> +int rpmh_flush(struct rpmh_client *rc) >> +{ >> + struct cache_req *p; >> + struct rpmh_ctrlr *rpm = rc->ctrlr; >> + int ret; >> + unsigned long flags; >> + >> + if (IS_ERR_OR_NULL(rc)) >> + return -EINVAL; >> + >> + spin_lock_irqsave(&rpm->lock, flags); >> + if (!rpm->dirty) { >> + pr_debug("Skipping flush, TCS has latest data.\n"); >> + spin_unlock_irqrestore(&rpm->lock, flags); >> + return 0; >> + } >> + spin_unlock_irqrestore(&rpm->lock, flags); >> + >> + /* >> + * Nobody else should be calling this function other than system PM,, >> + * hence we can run without locks. >> + */ >> + list_for_each_entry(p, &rc->ctrlr->cache, list) { >> + if (!is_req_valid(p)) { >> + pr_debug("%s: skipping RPMH req: a:0x%x s:0x%x w:0x%x", >> + __func__, p->addr, p->sleep_val, p->wake_val); >> + continue; >> + } >> + ret = send_single(rc, RPMH_SLEEP_STATE, p->addr, p->sleep_val); >> + if (ret) >> + return ret; >> + ret = send_single(rc, RPMH_WAKE_ONLY_STATE, p->addr, >> + p->wake_val); >> + if (ret) >> + return ret; >> + } >> + >> + spin_lock_irqsave(&rpm->lock, flags); >> + rpm->dirty = false; >> + spin_unlock_irqrestore(&rpm->lock, flags); >> + > >I've got some questions on the locking in this function. > >I understand that the lock protects the list, and I'm surmising that >you don't want to hold the lock across send_single (even though >there's another lock in there that's held for most of that time, so I >think you could). I'm still a newbie to Linux in general, so I'll pose >this as a question: is it generally okay in Linux to traverse across a >list that may have items concurrently added to it? You're never >removing items from this list, so I think there are no actual bugs, >but it does seem like it relies on the implementation details of the >list. And if you ever did remove items from the list, this would bite >you. > It is generally not advisable to traverse a list if the list is being modified externally. The reason why it is okay here, is the context of this function call. The only caller of this function will be a system PM driver. As noted in the comments, this is called when the last CPU is powering down and in such a context, there are no other active users of this library.
>Also, why do you need to acquire the lock just to set dirty to false? >Right now it looks like there's a race where someone could add an >element to this list just after you've terminated this loop (but >before you have the lock), but then the dirty = false here clobbers >their dirty = true, and the item is never sent during future flushes. > >I think it would be safer and faster to set dirty = false before >iterating through the list (either within the lock or outside of it >given that this is the only place that reads or clears dirty). That >way if new elements sneak in you know that they will either be flushed >already or dirty will be true for them on a subsequent flush. > ->dirty will be accessed as a u32 and so does not require a lock per-se. The lock here guarantees a certain system state when the flag is changed. While in this function it is not needed since, there will be no active threads changing the system state. I could possibly remove the lock in this function.
>> + return 0; >> +} >> +EXPORT_SYMBOL(rpmh_flush); >> + >> +/** >> + * rpmh_invalidate: Invalidate all sleep and active sets >> + * sets. >> + * >> + * @rc: The RPMh handle got from rpmh_get_dev_channel >> + * >> + * Invalidate the sleep and active values in the TCS blocks. >> + */ >> +int rpmh_invalidate(struct rpmh_client *rc) >> +{ >> + struct rpmh_ctrlr *rpm = rc->ctrlr; >> + int ret; >> + unsigned long flags; >> + >> + if (IS_ERR_OR_NULL(rc)) >> + return -EINVAL; >> + >> + spin_lock_irqsave(&rpm->lock, flags); >> + rpm->dirty = true; >> + spin_unlock_irqrestore(&rpm->lock, flags); > >I don't think the lock acquire/release provides anything here, can't >you just set dirty = true? > It helps synchronize to the state of the controller.
>So rpmh_invalidate clears any pending requests in the hardware, but >all the cached address/data pairs are all still in the cache, right? >As soon as someone else adds a new request and sets dirty to true, all >of these old ones get resent as well at flush, right? Is that the >desired behavior? Does anyone ever need to remove an address/data pair >from the RPMh's to-do list? > Drivers overwrite their requests and I have not had a requirement to remove their previous votes. Removing from the controller's registers is good enough to not send the data. If a driver calls invalidate, it is generally followed by a new sleep/wake request.
Thanks, Lina >> + >> + do { >> + ret = rpmh_rsc_invalidate(rc->ctrlr->drv); >> + } while (ret == -EAGAIN); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(rpmh_invalidate); >> +
| |