Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 9:46 PM, David Cohen wrote: > [snip] > >>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>> + if (~flags & IOVMF_DA_FIXED) >>> + flags |= IOVMF_DA_ANON; >> >> could we use only one? both are mutual exclusive, what happen if flag >> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >> IOVMF_DA_ANON. > > Then, what about introducing some MACRO? Better names? > > #define set_iovmf_da_anon(flags) > #define set_iovmf_da_fix(flags) > #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); >>> >>> I'd be happier with this approach, instead of the macros. :) >>> It's intuitive and very common on kernel. >>> than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? >>> >>> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. >>> I can resend my patch if we agree it's OK. >> >> sounds perfect to me. > > Not sure indeed if this change fits to this same patch. Looks like a > 4th patch sounds better. Indeed not. :) A new set is coming soon. Br, David > > Br, > > David Cohen > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
[snip] >> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >> + if (~flags & IOVMF_DA_FIXED) >> + flags |= IOVMF_DA_ANON; > > could we use only one? both are mutual exclusive, what happen if flag > is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of > IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) >>> >>> will they be used by the users? >>> >>> I think people are more used to use >>> >>> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); >> >> I'd be happier with this approach, instead of the macros. :) >> It's intuitive and very common on kernel. >> >>> >>> than >>> >>> set_iovmf_da_anon(flags) >>> set_iovmf_mmio(flags) >>> iommu_vmap(obj, da, sgt, flags); >>> >>> I don't have problem with the change, but I think how it is now is ok, >>> just that we don't we two bits to handle anon/fixed da, it can be >>> managed it only 1 bit (one flag), or is there a issue? >> >> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. >> I can resend my patch if we agree it's OK. > > sounds perfect to me. Not sure indeed if this change fits to this same patch. Looks like a 4th patch sounds better. Br, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 12:57 PM, David Cohen wrote: > Hi Hiroshi, Fernando, > > On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando > wrote: >> On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU wrote: >>> From: "ext Guzman Lugo, Fernando" >>> Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set >>> IOVMF_DA_FIXED/IOVMF_DA_ANON flags >>> Date: Tue, 8 Mar 2011 11:59:43 -0600 >>> >>>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote: >>>>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according >>>>> to input 'da' address when mapping memory: >>>>> da == 0: IOVMF_DA_ANON >>>>> da != 0: IOVMF_DA_FIXED >>>>> >>>>> It prevents IOMMU to map first page with fixed 'da'. To avoid such >>>>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now >>>>> come from the user. IOVMF_DA_ANON will be automatically set if >>>>> IOVMF_DA_FIXED isn't set. >>>>> >>>>> Signed-off-by: David Cohen >>>>> --- >>>>> arch/arm/plat-omap/iovmm.c | 12 >>>>> 1 files changed, 8 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c >>>>> index 11c9b76..dde9cb0 100644 >>>>> --- a/arch/arm/plat-omap/iovmm.c >>>>> +++ b/arch/arm/plat-omap/iovmm.c >>>>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const >>>>> struct sg_table *sgt, >>>>> flags &= IOVMF_HW_MASK; >>>>> flags |= IOVMF_DISCONT; >>>>> flags |= IOVMF_MMIO; >>>>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>>>> + if (~flags & IOVMF_DA_FIXED) >>>>> + flags |= IOVMF_DA_ANON; >>>> >>>> could we use only one? both are mutual exclusive, what happen if flag >>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >>>> IOVMF_DA_ANON. >>> >>> Then, what about introducing some MACRO? Better names? >>> >>> #define set_iovmf_da_anon(flags) >>> #define set_iovmf_da_fix(flags) >>> #define set_iovmf_mmio(flags) >> >> will they be used by the users? >> >> I think people are more used to use >> >> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); > > I'd be happier with this approach, instead of the macros. :) > It's intuitive and very common on kernel. > >> >> than >> >> set_iovmf_da_anon(flags) >> set_iovmf_mmio(flags) >> iommu_vmap(obj, da, sgt, flags); >> >> I don't have problem with the change, but I think how it is now is ok, >> just that we don't we two bits to handle anon/fixed da, it can be >> managed it only 1 bit (one flag), or is there a issue? > > We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. > I can resend my patch if we agree it's OK. sounds perfect to me. Regards, Fernando. > > Regards, > > David > >> >> Regards, >> Fernando. >>> .. >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Hi Hiroshi, Fernando, On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando wrote: > On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU wrote: >> From: "ext Guzman Lugo, Fernando" >> Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set >> IOVMF_DA_FIXED/IOVMF_DA_ANON flags >> Date: Tue, 8 Mar 2011 11:59:43 -0600 >> >>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote: >>>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according >>>> to input 'da' address when mapping memory: >>>> da == 0: IOVMF_DA_ANON >>>> da != 0: IOVMF_DA_FIXED >>>> >>>> It prevents IOMMU to map first page with fixed 'da'. To avoid such >>>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now >>>> come from the user. IOVMF_DA_ANON will be automatically set if >>>> IOVMF_DA_FIXED isn't set. >>>> >>>> Signed-off-by: David Cohen >>>> --- >>>> arch/arm/plat-omap/iovmm.c | 12 >>>> 1 files changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c >>>> index 11c9b76..dde9cb0 100644 >>>> --- a/arch/arm/plat-omap/iovmm.c >>>> +++ b/arch/arm/plat-omap/iovmm.c >>>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct >>>> sg_table *sgt, >>>> flags &= IOVMF_HW_MASK; >>>> flags |= IOVMF_DISCONT; >>>> flags |= IOVMF_MMIO; >>>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>>> + if (~flags & IOVMF_DA_FIXED) >>>> + flags |= IOVMF_DA_ANON; >>> >>> could we use only one? both are mutual exclusive, what happen if flag >>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >>> IOVMF_DA_ANON. >> >> Then, what about introducing some MACRO? Better names? >> >> #define set_iovmf_da_anon(flags) >> #define set_iovmf_da_fix(flags) >> #define set_iovmf_mmio(flags) > > will they be used by the users? > > I think people are more used to use > > iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. > > than > > set_iovmf_da_anon(flags) > set_iovmf_mmio(flags) > iommu_vmap(obj, da, sgt, flags); > > I don't have problem with the change, but I think how it is now is ok, > just that we don't we two bits to handle anon/fixed da, it can be > managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. Regards, David > > Regards, > Fernando. >> .. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU wrote: > From: "ext Guzman Lugo, Fernando" > Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set > IOVMF_DA_FIXED/IOVMF_DA_ANON flags > Date: Tue, 8 Mar 2011 11:59:43 -0600 > >> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote: >>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according >>> to input 'da' address when mapping memory: >>> da == 0: IOVMF_DA_ANON >>> da != 0: IOVMF_DA_FIXED >>> >>> It prevents IOMMU to map first page with fixed 'da'. To avoid such >>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now >>> come from the user. IOVMF_DA_ANON will be automatically set if >>> IOVMF_DA_FIXED isn't set. >>> >>> Signed-off-by: David Cohen >>> --- >>> arch/arm/plat-omap/iovmm.c | 12 >>> 1 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c >>> index 11c9b76..dde9cb0 100644 >>> --- a/arch/arm/plat-omap/iovmm.c >>> +++ b/arch/arm/plat-omap/iovmm.c >>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct >>> sg_table *sgt, >>> flags &= IOVMF_HW_MASK; >>> flags |= IOVMF_DISCONT; >>> flags |= IOVMF_MMIO; >>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>> + if (~flags & IOVMF_DA_FIXED) >>> + flags |= IOVMF_DA_ANON; >> >> could we use only one? both are mutual exclusive, what happen if flag >> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >> IOVMF_DA_ANON. > > Then, what about introducing some MACRO? Better names? > > #define set_iovmf_da_anon(flags) > #define set_iovmf_da_fix(flags) > #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? Regards, Fernando. > .. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
From: "ext Guzman Lugo, Fernando" Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags Date: Tue, 8 Mar 2011 11:59:43 -0600 > On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote: >> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according >> to input 'da' address when mapping memory: >> da == 0: IOVMF_DA_ANON >> da != 0: IOVMF_DA_FIXED >> >> It prevents IOMMU to map first page with fixed 'da'. To avoid such >> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now >> come from the user. IOVMF_DA_ANON will be automatically set if >> IOVMF_DA_FIXED isn't set. >> >> Signed-off-by: David Cohen >> --- >> arch/arm/plat-omap/iovmm.c | 12 >> 1 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c >> index 11c9b76..dde9cb0 100644 >> --- a/arch/arm/plat-omap/iovmm.c >> +++ b/arch/arm/plat-omap/iovmm.c >> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct >> sg_table *sgt, >> flags &= IOVMF_HW_MASK; >> flags |= IOVMF_DISCONT; >> flags |= IOVMF_MMIO; >> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >> + if (~flags & IOVMF_DA_FIXED) >> + flags |= IOVMF_DA_ANON; > > could we use only one? both are mutual exclusive, what happen if flag > is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of > IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) .. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote:
> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
> to input 'da' address when mapping memory:
> da == 0: IOVMF_DA_ANON
> da != 0: IOVMF_DA_FIXED
>
> It prevents IOMMU to map first page with fixed 'da'. To avoid such
> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
> come from the user. IOVMF_DA_ANON will be automatically set if
> IOVMF_DA_FIXED isn't set.
>
> Signed-off-by: David Cohen
> ---
> arch/arm/plat-omap/iovmm.c | 12
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 11c9b76..dde9cb0 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
> sg_table *sgt,
> flags &= IOVMF_HW_MASK;
> flags |= IOVMF_DISCONT;
> flags |= IOVMF_MMIO;
> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> + if (~flags & IOVMF_DA_FIXED)
> + flags |= IOVMF_DA_ANON;
could we use only one? both are mutual exclusive, what happen if flag
is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
IOVMF_DA_ANON.
Regards,
Fernando.
>
> da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
> if (IS_ERR_VALUE(da))
> @@ -713,7 +714,8 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t
> bytes, u32 flags)
> flags &= IOVMF_HW_MASK;
> flags |= IOVMF_DISCONT;
> flags |= IOVMF_ALLOC;
> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> + if (~flags & IOVMF_DA_FIXED)
> + flags |= IOVMF_DA_ANON;
>
> sgt = sgtable_alloc(bytes, flags, da, 0);
> if (IS_ERR(sgt)) {
> @@ -803,7 +805,8 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
> bytes,
> flags &= IOVMF_HW_MASK;
> flags |= IOVMF_LINEAR;
> flags |= IOVMF_MMIO;
> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> + if (~flags & IOVMF_DA_FIXED)
> + flags |= IOVMF_DA_ANON;
>
> da = __iommu_kmap(obj, da, pa, va, bytes, flags);
> if (IS_ERR_VALUE(da))
> @@ -862,7 +865,8 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t
> bytes, u32 flags)
> flags &= IOVMF_HW_MASK;
> flags |= IOVMF_LINEAR;
> flags |= IOVMF_ALLOC;
> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> + if (~flags & IOVMF_DA_FIXED)
> + flags |= IOVMF_DA_ANON;
>
> da = __iommu_kmap(obj, da, pa, va, bytes, flags);
> if (IS_ERR_VALUE(da))
> --
> 1.7.0.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
