Re: [PATCH v2 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Evan Green]

Hi Lina,

On Thu, Feb 22, 2018 at 9:04 AM, Lina Iyer  wrote:
> On Wed, Feb 21 2018 at 23:25 +, Evan Green wrote:
>>
>> Hello Lina,
>>
>> On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer  wrote:
>>>
>>> Platform drivers need make a lot of resource state requests at the same
>>> time, say, at the start or end of an usecase. It can be quite
>>> inefficient to send each request separately. Instead they can give the
>>> RPMH library a batch of requests to be sent and wait on the whole
>>> transaction to be complete.
>>>
>>> rpmh_write_batch() is a blocking call that can be used to send multiple
>>> RPMH command sets. Each RPMH command set is set asynchronously and the
>>> API blocks until all the command sets are complete and receive their
>>> tx_done callbacks.
>>>
>>> Signed-off-by: Lina Iyer 
>>> ---
>>>  drivers/soc/qcom/rpmh.c | 150
>>> 
>>>  include/soc/qcom/rpmh.h |   8 +++
>>>  2 files changed, 158 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>>> index dff4c46be3af..6f60bb9a4dfa 100644
>>> --- a/drivers/soc/qcom/rpmh.c
>>> +++ b/drivers/soc/qcom/rpmh.c
>>
>> [...]
>>>
>>> @@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
>>> }
>>> spin_unlock_irqrestore(>lock, flags);
>>>
>>> +   /* First flush the cached batch requests */
>>> +   ret = flush_batch(rc);
>>> +   if (ret)
>>> +   return ret;
>>> +
>>> /*
>>>  * Nobody else should be calling this function other than system
>>> PM,,
>>>  * hence we can run without locks.
>
> This comment and the comment in the header of this function.
>
>>> @@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
>>> if (IS_ERR_OR_NULL(rc))
>>> return -EINVAL;
>>>
>>> +   invalidate_batch(rc);
>>> +
>>
>>
>> Similarly to my comments in patch 7, aren't there races here with
>> adding new elements? After flush_batch, but before invalidate_batch,
>> somebody could call cache_batch, which would add new things on the end
>> of the array. These new items would be clobbered by invalidate_batch,
>> without having been flushed first.
>>
> Flush is a system PM function and is not called by drivers and is not
> expected to run conncurrently with other threads using the RPMH library.

My comment above was a little off because I was reading those two
hunks (flush_batch and invalidate_batch) as being in the same
function. They're not.

I'm okay with the locking here, though you could remove the locking
from flush_batch, since that's only run in single-threaded PM
contexts.

>
> Thanks,
> Lina
>

Re: [PATCH v2 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Evan Green]

Hi Lina,

On Thu, Feb 22, 2018 at 9:04 AM, Lina Iyer  wrote:
> On Wed, Feb 21 2018 at 23:25 +, Evan Green wrote:
>>
>> Hello Lina,
>>
>> On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer  wrote:
>>>
>>> Platform drivers need make a lot of resource state requests at the same
>>> time, say, at the start or end of an usecase. It can be quite
>>> inefficient to send each request separately. Instead they can give the
>>> RPMH library a batch of requests to be sent and wait on the whole
>>> transaction to be complete.
>>>
>>> rpmh_write_batch() is a blocking call that can be used to send multiple
>>> RPMH command sets. Each RPMH command set is set asynchronously and the
>>> API blocks until all the command sets are complete and receive their
>>> tx_done callbacks.
>>>
>>> Signed-off-by: Lina Iyer 
>>> ---
>>>  drivers/soc/qcom/rpmh.c | 150
>>> 
>>>  include/soc/qcom/rpmh.h |   8 +++
>>>  2 files changed, 158 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>>> index dff4c46be3af..6f60bb9a4dfa 100644
>>> --- a/drivers/soc/qcom/rpmh.c
>>> +++ b/drivers/soc/qcom/rpmh.c
>>
>> [...]
>>>
>>> @@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
>>> }
>>> spin_unlock_irqrestore(>lock, flags);
>>>
>>> +   /* First flush the cached batch requests */
>>> +   ret = flush_batch(rc);
>>> +   if (ret)
>>> +   return ret;
>>> +
>>> /*
>>>  * Nobody else should be calling this function other than system
>>> PM,,
>>>  * hence we can run without locks.
>
> This comment and the comment in the header of this function.
>
>>> @@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
>>> if (IS_ERR_OR_NULL(rc))
>>> return -EINVAL;
>>>
>>> +   invalidate_batch(rc);
>>> +
>>
>>
>> Similarly to my comments in patch 7, aren't there races here with
>> adding new elements? After flush_batch, but before invalidate_batch,
>> somebody could call cache_batch, which would add new things on the end
>> of the array. These new items would be clobbered by invalidate_batch,
>> without having been flushed first.
>>
> Flush is a system PM function and is not called by drivers and is not
> expected to run conncurrently with other threads using the RPMH library.

My comment above was a little off because I was reading those two
hunks (flush_batch and invalidate_batch) as being in the same
function. They're not.

I'm okay with the locking here, though you could remove the locking
from flush_batch, since that's only run in single-threaded PM
contexts.

>
> Thanks,
> Lina
>

Re: [PATCH v2 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Lina Iyer]

On Wed, Feb 21 2018 at 23:25 +, Evan Green wrote:

Hello Lina,

On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer  wrote:

Platform drivers need make a lot of resource state requests at the same
time, say, at the start or end of an usecase. It can be quite
inefficient to send each request separately. Instead they can give the
RPMH library a batch of requests to be sent and wait on the whole
transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple
RPMH command sets. Each RPMH command set is set asynchronously and the
API blocks until all the command sets are complete and receive their
tx_done callbacks.

Signed-off-by: Lina Iyer 
---
 drivers/soc/qcom/rpmh.c | 150 
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 158 insertions(+)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index dff4c46be3af..6f60bb9a4dfa 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c

[...]

@@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
}
spin_unlock_irqrestore(>lock, flags);

+   /* First flush the cached batch requests */
+   ret = flush_batch(rc);
+   if (ret)
+   return ret;
+
/*
 * Nobody else should be calling this function other than system PM,,
 * hence we can run without locks.

This comment and the comment in the header of this function.


@@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
if (IS_ERR_OR_NULL(rc))
return -EINVAL;

+   invalidate_batch(rc);
+


Similarly to my comments in patch 7, aren't there races here with
adding new elements? After flush_batch, but before invalidate_batch,
somebody could call cache_batch, which would add new things on the end
of the array. These new items would be clobbered by invalidate_batch,
without having been flushed first.


Flush is a system PM function and is not called by drivers and is not
expected to run conncurrently with other threads using the RPMH library.

Thanks,
Lina

Re: [PATCH v2 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Lina Iyer]

On Wed, Feb 21 2018 at 23:25 +, Evan Green wrote:

Hello Lina,

On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer  wrote:

Platform drivers need make a lot of resource state requests at the same
time, say, at the start or end of an usecase. It can be quite
inefficient to send each request separately. Instead they can give the
RPMH library a batch of requests to be sent and wait on the whole
transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple
RPMH command sets. Each RPMH command set is set asynchronously and the
API blocks until all the command sets are complete and receive their
tx_done callbacks.

Signed-off-by: Lina Iyer 
---
 drivers/soc/qcom/rpmh.c | 150 
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 158 insertions(+)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index dff4c46be3af..6f60bb9a4dfa 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c

[...]

@@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
}
spin_unlock_irqrestore(>lock, flags);

+   /* First flush the cached batch requests */
+   ret = flush_batch(rc);
+   if (ret)
+   return ret;
+
/*
 * Nobody else should be calling this function other than system PM,,
 * hence we can run without locks.

This comment and the comment in the header of this function.


@@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
if (IS_ERR_OR_NULL(rc))
return -EINVAL;

+   invalidate_batch(rc);
+


Similarly to my comments in patch 7, aren't there races here with
adding new elements? After flush_batch, but before invalidate_batch,
somebody could call cache_batch, which would add new things on the end
of the array. These new items would be clobbered by invalidate_batch,
without having been flushed first.


Flush is a system PM function and is not called by drivers and is not
expected to run conncurrently with other threads using the RPMH library.

Thanks,
Lina

Re: [PATCH v2 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Evan Green]

Hello Lina,

On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer  wrote:
> Platform drivers need make a lot of resource state requests at the same
> time, say, at the start or end of an usecase. It can be quite
> inefficient to send each request separately. Instead they can give the
> RPMH library a batch of requests to be sent and wait on the whole
> transaction to be complete.
>
> rpmh_write_batch() is a blocking call that can be used to send multiple
> RPMH command sets. Each RPMH command set is set asynchronously and the
> API blocks until all the command sets are complete and receive their
> tx_done callbacks.
>
> Signed-off-by: Lina Iyer 
> ---
>  drivers/soc/qcom/rpmh.c | 150 
> 
>  include/soc/qcom/rpmh.h |   8 +++
>  2 files changed, 158 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index dff4c46be3af..6f60bb9a4dfa 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
[...]
> @@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
> }
> spin_unlock_irqrestore(>lock, flags);
>
> +   /* First flush the cached batch requests */
> +   ret = flush_batch(rc);
> +   if (ret)
> +   return ret;
> +
> /*
>  * Nobody else should be calling this function other than system PM,,
>  * hence we can run without locks.
> @@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
> if (IS_ERR_OR_NULL(rc))
> return -EINVAL;
>
> +   invalidate_batch(rc);
> +

Similarly to my comments in patch 7, aren't there races here with
adding new elements? After flush_batch, but before invalidate_batch,
somebody could call cache_batch, which would add new things on the end
of the array. These new items would be clobbered by invalidate_batch,
without having been flushed first.

-Evan

Re: [PATCH v2 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[Evan Green]

Hello Lina,

On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer  wrote:
> Platform drivers need make a lot of resource state requests at the same
> time, say, at the start or end of an usecase. It can be quite
> inefficient to send each request separately. Instead they can give the
> RPMH library a batch of requests to be sent and wait on the whole
> transaction to be complete.
>
> rpmh_write_batch() is a blocking call that can be used to send multiple
> RPMH command sets. Each RPMH command set is set asynchronously and the
> API blocks until all the command sets are complete and receive their
> tx_done callbacks.
>
> Signed-off-by: Lina Iyer 
> ---
>  drivers/soc/qcom/rpmh.c | 150 
> 
>  include/soc/qcom/rpmh.h |   8 +++
>  2 files changed, 158 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index dff4c46be3af..6f60bb9a4dfa 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
[...]
> @@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
> }
> spin_unlock_irqrestore(>lock, flags);
>
> +   /* First flush the cached batch requests */
> +   ret = flush_batch(rc);
> +   if (ret)
> +   return ret;
> +
> /*
>  * Nobody else should be calling this function other than system PM,,
>  * hence we can run without locks.
> @@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
> if (IS_ERR_OR_NULL(rc))
> return -EINVAL;
>
> +   invalidate_batch(rc);
> +

Similarly to my comments in patch 7, aren't there races here with
adding new elements? After flush_batch, but before invalidate_batch,
somebody could call cache_batch, which would add new things on the end
of the array. These new items would be clobbered by invalidate_batch,
without having been flushed first.

-Evan