Hi Loic,
On 10/24/18 10:24 AM, Loic PALLARDY wrote:
> Hi Suman,
>
>> -----Original Message-----
>> From: Suman Anna <s-anna@xxxxxx>
>> Sent: mercredi 24 octobre 2018 00:14
>> To: Loic PALLARDY <loic.pallardy@xxxxxx>; bjorn.andersson@xxxxxxxxxx;
>> ohad@xxxxxxxxxx
>> Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx>;
>> benjamin.gaignard@xxxxxxxxxx
>> Subject: Re: [PATCH v4 10/17] remoteproc: add helper function to check
>> carveout device address
>>
>> Hi Loic,
>>
>> On 7/27/18 8:14 AM, Loic Pallardy wrote:
>>> This patch introduces a function to verify that a specified carveout
>>> is fitting request device address and associated length
>>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
>>> ---
>>> drivers/remoteproc/remoteproc_core.c | 47
>> ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 47 insertions(+)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>>> index 1e0fe3e..5dd5edf 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -259,6 +259,53 @@ struct rproc_mem_entry *
>>> return mem;
>>> }
>>>
>>> +/**
>>> + * rproc_check_carveout_da() - Check specified carveout da configuration
>>> + * @rproc: handle of a remote processor
>>> + * @mem: pointer on carveout to check
>>> + * @da: area device address
>>> + * @len: associated area size
>>> + *
>>> + * This function is a helper function to verify requested device area
>> (couple
>>> + * da, len) is part of specified carevout.
>>
>> %s/carevout/carveout/
> OK
>>
>>> + *
>>> + * Return: 0 if carveout matchs request else -ENOMEM
>>
>> %s/matchs/matches/
> OK
>>
>>> + */
>>> +int rproc_check_carveout_da(struct rproc *rproc, struct
>> rproc_mem_entry *mem,
>>
>> static int since this seems to be only a local function.
> OK
>>
>>> + u32 da, u32 len)
>>> +{
>>> + struct device *dev = &rproc->dev;
>>> + int delta = 0;
>>> +
>>> + /* Check requested resource length */
>>> + if (len > mem->len) {
>>> + dev_err(dev, "Registered carveout doesn't fit len
>> request\n");
>>> + return -ENOMEM;
>>
>> ENOMEM not typically used for these kind of errors, you were probably
>> inclined to used this since it is dealing with memory.
>
> -EINVAL will be better
>>
>>> + }
>>> +
>>
>> Both the below codepaths are exercised only when da is not
>> FW_RSC_ADDR_ANY, and you are returning 0 otherwise (which is the case of
>> matches as per your description above). Is that what you really want -
>> should it be an error
>
> Yes when da is equal to FW_RSC_ADDR_ANY we should check the length too
Can you update the comments in the function description accordingly as
well, the current code silently returns 0 if da = FW_RSC_ADDR_ANY.
>
>>
>>> + if (da != FW_RSC_ADDR_ANY && mem->da == FW_RSC_ADDR_ANY)
>> {
>>> + /* Update existing carveout da */
>>> + mem->da = da;
>>
>> Where would you need to update this?
> In that case, we have 2 carveouts with the same name.
> One has some fixed requests. The other one has none.
> The goal here is to align both on the one which has the strongest requirements.
> I think length is missing.
It almost looks like there is a need for range overlap checks on all the
carveouts after all of them are registered, and error out if any overlap
irrespective of the name schema.
regards
Suman
>
> Regards,
> Loic
>
>>
>> regards
>> Suman
>>
>>> + } else if (da != FW_RSC_ADDR_ANY && mem->da !=
>> FW_RSC_ADDR_ANY) {
>>> + delta = da - mem->da;
>>> +
>>> + /* Check requested resource belongs to registered carveout
>> */
>>> + if (delta < 0) {
>>> + dev_err(dev,
>>> + "Registered carveout doesn't fit da
>> request\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + if (delta + len > mem->len) {
>>> + dev_err(dev,
>>> + "Registered carveout doesn't fit len
>> request\n");
>>> + return -ENOMEM;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>
>>
>>> +}
>>> +
>>> int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>>> {
>>> struct rproc *rproc = rvdev->rproc;
>>>
>
