Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683453 > unrolled thread

Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching memory ownership

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership Stephen Boyd <sboyd@codeaurora.org> - 2017-07-08 01:00 +0200
    Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-07-11 01:40 +0200
    Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership "Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.org> - 2017-07-12 13:10 +0200
      Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-07-12 21:20 +0200
      Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 08:00 +0200
        Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership "Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.org> - 2017-07-13 09:40 +0200
          Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching  memory ownership Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 10:00 +0200

#1683453 — Re: [PATCH v6 1/4] firmware: scm: Add new SCM call API for switching memory ownership

On 06/22, Avaneesh Kumar Dwivedi wrote:
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 6e6d561..cdfe986 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>  }
>  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>  
> +/**
> + * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
> + *
> + * @mem_addr: mem region whose ownership need to be reassigned
> + * @mem_sz:   size of the region.
> + * @srcvm:    vmid for current set of owners, each set bit in
> + *            flag indicate a unique owner
> + * @newvm:    array having new owners and corrsponding permission
> + *            flags
> + * @dest_cnt: number of owners in next set.
> + * Return next set of owners on success.
> + */
> +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
> +			struct qcom_scm_vmperm *newvm, int dest_cnt)
> +{
> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;

Why do we need this? Just curious if we can drop this.

> +	struct qcom_scm_current_perm_info *destvm;
> +	struct qcom_scm_mem_map_info *mem;
> +	phys_addr_t memory_phys;
> +	phys_addr_t dest_phys;
> +	phys_addr_t src_phys;
> +	size_t mem_all_sz;
> +	size_t memory_sz;
> +	size_t dest_sz;
> +	size_t src_sz;
> +	int next_vm;
> +	__le32 *src;
> +	void *ptr;
> +	int ret;
> +	int len;
> +	int i;
> +
> +	src_sz = hweight_long(srcvm) * sizeof(*src);
> +	memory_sz = sizeof(*mem);
> +	dest_sz = dest_cnt*sizeof(*destvm);
> +	mem_all_sz = src_sz + memory_sz + dest_sz;
> +
> +	ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> +		&src_phys, GFP_KERNEL, dma_attrs);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	/* Fill source vmid detail */
> +	src = (__le32 *)ptr;

Cast is necessary?

> +	len = hweight_long(srcvm);
> +	for (i = 0; i < len; i++) {
> +		src[i] = cpu_to_le32(ffs(srcvm) - 1);
> +		srcvm ^= 1 << (ffs(srcvm) - 1);
> +	}
> +
> +	/* Fill details of mem buff to map */
> +	mem = ptr + ALIGN(src_sz, SZ_64);
> +	memory_phys = src_phys + ALIGN(src_sz, SZ_64);
> +	mem[0].mem_addr = cpu_to_le64(mem_addr);
> +	mem[0].mem_size = cpu_to_le64(mem_sz);
> +
> +	next_vm = 0;
> +	/* Fill details of next vmid detail */
> +	destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
> +	dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
> +	for (i = 0; i < dest_cnt; i++) {
> +		destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
> +		destvm[i].perm = cpu_to_le32(newvm[i].perm);
> +		destvm[i].ctx = 0;
> +		destvm[i].ctx_size = 0;
> +		next_vm |= BIT(newvm[i].vmid);
> +	}
> +
> +	ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
> +		memory_sz, src_phys, src_sz, dest_phys, dest_sz);
> +	dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> +		ptr, src_phys, dma_attrs);
> +	if (ret == 0)
> +		return next_vm;
> +	else if (ret > 0)
> +		return -ret;

This still confuses me. Do we really just pass whatever the
firmware tells us the error code is up to the caller? Shouldn't
we be remapping the scm errors we receive to normal linux errnos?

> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_scm_assign_mem);
> +
>  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>  				     unsigned long idx)
>  {

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1684691

On Fri 07 Jul 15:49 PDT 2017, Stephen Boyd wrote:

> On 06/22, Avaneesh Kumar Dwivedi wrote:
> > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
[..]
> > +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
> > +			struct qcom_scm_vmperm *newvm, int dest_cnt)
> > +{
> > +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> 
> Why do we need this? Just curious if we can drop this.
> 

As far as I can see this attribute has no affect on the allocation,
unless you have attached an iommu to the device. So in the current setup
it should be perfectly fine to get the memory from dma_alloc_coherent()

> > +	struct qcom_scm_current_perm_info *destvm;
> > +	struct qcom_scm_mem_map_info *mem;
> > +	phys_addr_t memory_phys;
> > +	phys_addr_t dest_phys;
> > +	phys_addr_t src_phys;
> > +	size_t mem_all_sz;
> > +	size_t memory_sz;
> > +	size_t dest_sz;
> > +	size_t src_sz;
> > +	int next_vm;
> > +	__le32 *src;
> > +	void *ptr;
> > +	int ret;
> > +	int len;
> > +	int i;
> > +
> > +	src_sz = hweight_long(srcvm) * sizeof(*src);
> > +	memory_sz = sizeof(*mem);
> > +	dest_sz = dest_cnt*sizeof(*destvm);
> > +	mem_all_sz = src_sz + memory_sz + dest_sz;
> > +
> > +	ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> > +		&src_phys, GFP_KERNEL, dma_attrs);
> > +	if (!ptr)
> > +		return -ENOMEM;
> > +
> > +	/* Fill source vmid detail */
> > +	src = (__le32 *)ptr;
> 
> Cast is necessary?
> 
> > +	len = hweight_long(srcvm);
> > +	for (i = 0; i < len; i++) {
> > +		src[i] = cpu_to_le32(ffs(srcvm) - 1);
> > +		srcvm ^= 1 << (ffs(srcvm) - 1);
> > +	}
> > +
> > +	/* Fill details of mem buff to map */
> > +	mem = ptr + ALIGN(src_sz, SZ_64);
> > +	memory_phys = src_phys + ALIGN(src_sz, SZ_64);
> > +	mem[0].mem_addr = cpu_to_le64(mem_addr);
> > +	mem[0].mem_size = cpu_to_le64(mem_sz);
> > +
> > +	next_vm = 0;
> > +	/* Fill details of next vmid detail */
> > +	destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
> > +	dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
> > +	for (i = 0; i < dest_cnt; i++) {
> > +		destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
> > +		destvm[i].perm = cpu_to_le32(newvm[i].perm);
> > +		destvm[i].ctx = 0;
> > +		destvm[i].ctx_size = 0;
> > +		next_vm |= BIT(newvm[i].vmid);
> > +	}
> > +
> > +	ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
> > +		memory_sz, src_phys, src_sz, dest_phys, dest_sz);
> > +	dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> > +		ptr, src_phys, dma_attrs);
> > +	if (ret == 0)
> > +		return next_vm;
> > +	else if (ret > 0)
> > +		return -ret;
> 
> This still confuses me. Do we really just pass whatever the
> firmware tells us the error code is up to the caller? Shouldn't
> we be remapping the scm errors we receive to normal linux errnos?
> 

I agree, the one case I've seen this not being 0 on "success" it was 15
and I consider it wrong to return ENOTBLK when this happens.

It would be better to just return -EINVAL in this case.

> > +	return ret;
> > +}

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1685700

On 7/8/2017 4:19 AM, Stephen Boyd wrote:
> On 06/22, Avaneesh Kumar Dwivedi wrote:
>> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
>> index 6e6d561..cdfe986 100644
>> --- a/drivers/firmware/qcom_scm-64.c
>> +++ b/drivers/firmware/qcom_scm-64.c
>> @@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>   
>> +/**
>> + * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
>> + *
>> + * @mem_addr: mem region whose ownership need to be reassigned
>> + * @mem_sz:   size of the region.
>> + * @srcvm:    vmid for current set of owners, each set bit in
>> + *            flag indicate a unique owner
>> + * @newvm:    array having new owners and corrsponding permission
>> + *            flags
>> + * @dest_cnt: number of owners in next set.
>> + * Return next set of owners on success.
>> + */
>> +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
>> +			struct qcom_scm_vmperm *newvm, int dest_cnt)
>> +{
>> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> Why do we need this? Just curious if we can drop this.
The force contiguous flag is required with dma_alloc_attrs() api to 
allocate memory from physically contiguous zone.
I am not sure, are you saying that api will work without the attribute 
or you mean i shall use some api which does not take explicit attribute?
>
>> +	struct qcom_scm_current_perm_info *destvm;
>> +	struct qcom_scm_mem_map_info *mem;
>> +	phys_addr_t memory_phys;
>> +	phys_addr_t dest_phys;
>> +	phys_addr_t src_phys;
>> +	size_t mem_all_sz;
>> +	size_t memory_sz;
>> +	size_t dest_sz;
>> +	size_t src_sz;
>> +	int next_vm;
>> +	__le32 *src;
>> +	void *ptr;
>> +	int ret;
>> +	int len;
>> +	int i;
>> +
>> +	src_sz = hweight_long(srcvm) * sizeof(*src);
>> +	memory_sz = sizeof(*mem);
>> +	dest_sz = dest_cnt*sizeof(*destvm);
>> +	mem_all_sz = src_sz + memory_sz + dest_sz;
>> +
>> +	ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
>> +		&src_phys, GFP_KERNEL, dma_attrs);
>> +	if (!ptr)
>> +		return -ENOMEM;
>> +
>> +	/* Fill source vmid detail */
>> +	src = (__le32 *)ptr;
> Cast is necessary?
i removed many type casting but few still lingering, will check and 
remove whatever unnecessary.
>
>> +	len = hweight_long(srcvm);
>> +	for (i = 0; i < len; i++) {
>> +		src[i] = cpu_to_le32(ffs(srcvm) - 1);
>> +		srcvm ^= 1 << (ffs(srcvm) - 1);
>> +	}
>> +
>> +	/* Fill details of mem buff to map */
>> +	mem = ptr + ALIGN(src_sz, SZ_64);
>> +	memory_phys = src_phys + ALIGN(src_sz, SZ_64);
>> +	mem[0].mem_addr = cpu_to_le64(mem_addr);
>> +	mem[0].mem_size = cpu_to_le64(mem_sz);
>> +
>> +	next_vm = 0;
>> +	/* Fill details of next vmid detail */
>> +	destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>> +	dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
>> +	for (i = 0; i < dest_cnt; i++) {
>> +		destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
>> +		destvm[i].perm = cpu_to_le32(newvm[i].perm);
>> +		destvm[i].ctx = 0;
>> +		destvm[i].ctx_size = 0;
>> +		next_vm |= BIT(newvm[i].vmid);
>> +	}
>> +
>> +	ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
>> +		memory_sz, src_phys, src_sz, dest_phys, dest_sz);
>> +	dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
>> +		ptr, src_phys, dma_attrs);
>> +	if (ret == 0)
>> +		return next_vm;
>> +	else if (ret > 0)
>> +		return -ret;
> This still confuses me. Do we really just pass whatever the
> firmware tells us the error code is up to the caller? Shouldn't
> we be remapping the scm errors we receive to normal linux errnos?
because i do not know in advance what exactly will be the return error 
code, moreover there are number of error codes which are returned in 
case of failure
so if i have to return linux error code, i can not do one to one mapping 
of error code and will have to return single error code for all failure.
let me know your comments further on this.+ return ret;
>> +}
>> +EXPORT_SYMBOL(qcom_scm_assign_mem);
>> +
>>   static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>>   				     unsigned long idx)
>>   {

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1685988

On Wed 12 Jul 04:03 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:

> 
> 
> On 7/8/2017 4:19 AM, Stephen Boyd wrote:
> > On 06/22, Avaneesh Kumar Dwivedi wrote:
> > > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> > > index 6e6d561..cdfe986 100644
> > > --- a/drivers/firmware/qcom_scm-64.c
> > > +++ b/drivers/firmware/qcom_scm-64.c
> > > @@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
> > >   }
> > >   EXPORT_SYMBOL(qcom_scm_pas_shutdown);
> > > +/**
> > > + * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
> > > + *
> > > + * @mem_addr: mem region whose ownership need to be reassigned
> > > + * @mem_sz:   size of the region.
> > > + * @srcvm:    vmid for current set of owners, each set bit in
> > > + *            flag indicate a unique owner
> > > + * @newvm:    array having new owners and corrsponding permission
> > > + *            flags
> > > + * @dest_cnt: number of owners in next set.
> > > + * Return next set of owners on success.
> > > + */
> > > +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
> > > +			struct qcom_scm_vmperm *newvm, int dest_cnt)
> > > +{
> > > +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> > Why do we need this? Just curious if we can drop this.
> The force contiguous flag is required with dma_alloc_attrs() api to allocate
> memory from physically contiguous zone.
> I am not sure, are you saying that api will work without the attribute or
> you mean i shall use some api which does not take explicit attribute?
> > 
> > > +	struct qcom_scm_current_perm_info *destvm;
> > > +	struct qcom_scm_mem_map_info *mem;
> > > +	phys_addr_t memory_phys;
> > > +	phys_addr_t dest_phys;
> > > +	phys_addr_t src_phys;
> > > +	size_t mem_all_sz;
> > > +	size_t memory_sz;
> > > +	size_t dest_sz;
> > > +	size_t src_sz;
> > > +	int next_vm;
> > > +	__le32 *src;
> > > +	void *ptr;
> > > +	int ret;
> > > +	int len;
> > > +	int i;
> > > +
> > > +	src_sz = hweight_long(srcvm) * sizeof(*src);
> > > +	memory_sz = sizeof(*mem);
> > > +	dest_sz = dest_cnt*sizeof(*destvm);
> > > +	mem_all_sz = src_sz + memory_sz + dest_sz;
> > > +
> > > +	ptr = dma_alloc_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> > > +		&src_phys, GFP_KERNEL, dma_attrs);
> > > +	if (!ptr)
> > > +		return -ENOMEM;
> > > +
> > > +	/* Fill source vmid detail */
> > > +	src = (__le32 *)ptr;
> > Cast is necessary?
> i removed many type casting but few still lingering, will check and remove
> whatever unnecessary.
> > 
> > > +	len = hweight_long(srcvm);
> > > +	for (i = 0; i < len; i++) {
> > > +		src[i] = cpu_to_le32(ffs(srcvm) - 1);
> > > +		srcvm ^= 1 << (ffs(srcvm) - 1);
> > > +	}
> > > +
> > > +	/* Fill details of mem buff to map */
> > > +	mem = ptr + ALIGN(src_sz, SZ_64);
> > > +	memory_phys = src_phys + ALIGN(src_sz, SZ_64);
> > > +	mem[0].mem_addr = cpu_to_le64(mem_addr);
> > > +	mem[0].mem_size = cpu_to_le64(mem_sz);
> > > +
> > > +	next_vm = 0;
> > > +	/* Fill details of next vmid detail */
> > > +	destvm = ptr + ALIGN(memory_sz, SZ_64) + ALIGN(src_sz, SZ_64);
> > > +	dest_phys = memory_phys + ALIGN(memory_sz, SZ_64);
> > > +	for (i = 0; i < dest_cnt; i++) {
> > > +		destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
> > > +		destvm[i].perm = cpu_to_le32(newvm[i].perm);
> > > +		destvm[i].ctx = 0;
> > > +		destvm[i].ctx_size = 0;
> > > +		next_vm |= BIT(newvm[i].vmid);
> > > +	}
> > > +
> > > +	ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
> > > +		memory_sz, src_phys, src_sz, dest_phys, dest_sz);
> > > +	dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> > > +		ptr, src_phys, dma_attrs);
> > > +	if (ret == 0)
> > > +		return next_vm;
> > > +	else if (ret > 0)
> > > +		return -ret;
> > This still confuses me. Do we really just pass whatever the
> > firmware tells us the error code is up to the caller? Shouldn't
> > we be remapping the scm errors we receive to normal linux errnos?
> because i do not know in advance what exactly will be the return error code,
> moreover there are number of error codes which are returned in case of
> failure
> so if i have to return linux error code, i can not do one to one mapping of
> error code and will have to return single error code for all failure.
> let me know your comments further on this.+ return ret;

Returning a single error code for all these cases (e.g. -EINVAL) is
fine.


You could amend this by translating some of the results to other codes,
if that makes sense. But I'm not aware of the kind of errors this
interface can return, so it's hard to advice on this.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1686266

On 07/12, Dwivedi, Avaneesh Kumar (avani) wrote:
> 
> 
> On 7/8/2017 4:19 AM, Stephen Boyd wrote:
> >On 06/22, Avaneesh Kumar Dwivedi wrote:
> >>diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> >>index 6e6d561..cdfe986 100644
> >>--- a/drivers/firmware/qcom_scm-64.c
> >>+++ b/drivers/firmware/qcom_scm-64.c
> >>@@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
> >>  }
> >>  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
> >>+/**
> >>+ * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
> >>+ *
> >>+ * @mem_addr: mem region whose ownership need to be reassigned
> >>+ * @mem_sz:   size of the region.
> >>+ * @srcvm:    vmid for current set of owners, each set bit in
> >>+ *            flag indicate a unique owner
> >>+ * @newvm:    array having new owners and corrsponding permission
> >>+ *            flags
> >>+ * @dest_cnt: number of owners in next set.
> >>+ * Return next set of owners on success.
> >>+ */
> >>+int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
> >>+			struct qcom_scm_vmperm *newvm, int dest_cnt)
> >>+{
> >>+	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> >Why do we need this? Just curious if we can drop this.
> The force contiguous flag is required with dma_alloc_attrs() api to
> allocate memory from physically contiguous zone.
> I am not sure, are you saying that api will work without the
> attribute or you mean i shall use some api which does not take
> explicit attribute?

Does physically contiguous zone mean some CMA carveout? I wasn't
aware of a carveout for scm devices. I'm still not following the
reasoning here.

I'm saying that I don't understand why we need this flag. It
feels like this sort of constraint would apply all over the scm
driver if it was true, hence the confusion.

> >>+
> >>+	ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
> >>+		memory_sz, src_phys, src_sz, dest_phys, dest_sz);
> >>+	dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
> >>+		ptr, src_phys, dma_attrs);
> >>+	if (ret == 0)
> >>+		return next_vm;
> >>+	else if (ret > 0)
> >>+		return -ret;
> >This still confuses me. Do we really just pass whatever the
> >firmware tells us the error code is up to the caller? Shouldn't
> >we be remapping the scm errors we receive to normal linux errnos?
> because i do not know in advance what exactly will be the return
> error code, moreover there are number of error codes which are
> returned in case of failure
> so if i have to return linux error code, i can not do one to one
> mapping of error code and will have to return single error code for
> all failure.
> let me know your comments further on this.+ return ret;

Yes, returning -EINVAL all the time is fine if we can't remap the
error. In fact, we should probably do what we do downstream and
print out the error value returned from the firmware to the
kernel log and then return some sane errno up to the caller. That
way the few people who know what the error codes mean can tell us
why the scm call failed.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686318

On 7/13/2017 11:24 AM, Stephen Boyd wrote:
> On 07/12, Dwivedi, Avaneesh Kumar (avani) wrote:
>>
>> On 7/8/2017 4:19 AM, Stephen Boyd wrote:
>>> On 06/22, Avaneesh Kumar Dwivedi wrote:
>>>> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
>>>> index 6e6d561..cdfe986 100644
>>>> --- a/drivers/firmware/qcom_scm-64.c
>>>> +++ b/drivers/firmware/qcom_scm-64.c
>>>> @@ -292,6 +304,86 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>>>>   }
>>>>   EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>>> +/**
>>>> + * qcom_scm_assign_mem() - Make a secure call to reassign memory ownership
>>>> + *
>>>> + * @mem_addr: mem region whose ownership need to be reassigned
>>>> + * @mem_sz:   size of the region.
>>>> + * @srcvm:    vmid for current set of owners, each set bit in
>>>> + *            flag indicate a unique owner
>>>> + * @newvm:    array having new owners and corrsponding permission
>>>> + *            flags
>>>> + * @dest_cnt: number of owners in next set.
>>>> + * Return next set of owners on success.
>>>> + */
>>>> +int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, int srcvm,
>>>> +			struct qcom_scm_vmperm *newvm, int dest_cnt)
>>>> +{
>>>> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>>> Why do we need this? Just curious if we can drop this.
>> The force contiguous flag is required with dma_alloc_attrs() api to
>> allocate memory from physically contiguous zone.
>> I am not sure, are you saying that api will work without the
>> attribute or you mean i shall use some api which does not take
>> explicit attribute?
> Does physically contiguous zone mean some CMA carveout? I wasn't
> aware of a carveout for scm devices. I'm still not following the
> reasoning here.
the memory will be allocated from common carveout, there is no scm 
device specific
carveout. i will use dma_alloc_coherent() and will drop off this flag.

we need physical contigious zone to fill and pass scm call parameters to TZ.
>
> I'm saying that I don't understand why we need this flag. It
> feels like this sort of constraint would apply all over the scm
> driver if it was true, hence the confusion.
>
>>>> +
>>>> +	ret = __qcom_scm_assign_mem(__scm->dev, memory_phys,
>>>> +		memory_sz, src_phys, src_sz, dest_phys, dest_sz);
>>>> +	dma_free_attrs(__scm->dev, ALIGN(mem_all_sz, SZ_64),
>>>> +		ptr, src_phys, dma_attrs);
>>>> +	if (ret == 0)
>>>> +		return next_vm;
>>>> +	else if (ret > 0)
>>>> +		return -ret;
>>> This still confuses me. Do we really just pass whatever the
>>> firmware tells us the error code is up to the caller? Shouldn't
>>> we be remapping the scm errors we receive to normal linux errnos?
>> because i do not know in advance what exactly will be the return
>> error code, moreover there are number of error codes which are
>> returned in case of failure
>> so if i have to return linux error code, i can not do one to one
>> mapping of error code and will have to return single error code for
>> all failure.
>> let me know your comments further on this.+ return ret;
> Yes, returning -EINVAL all the time is fine if we can't remap the
> error. In fact, we should probably do what we do downstream and
> print out the error value returned from the firmware to the
> kernel log and then return some sane errno up to the caller. That
> way the few people who know what the error codes mean can tell us
> why the scm call failed.
OK, will do same.
just last thing to ask, should i resend all 4 patches together again or 
only one patch in v7 version.
as chnage will be in only 1 out of 4 patches.
>

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1686347

On 07/13, Dwivedi, Avaneesh Kumar (avani) wrote:
> OK, will do same.
> just last thing to ask, should i resend all 4 patches together again
> or only one patch in v7 version.
> as chnage will be in only 1 out of 4 patches.

I would resend all of them. That's standard practice for patch
series.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web