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

Groups > linux.kernel > #1518906 > unrolled thread

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

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 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops Joerg Roedel <joro@8bytes.org> - 2016-11-10 12:50 +0100
    Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-11 02:20 +0100
      Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops Joerg Roedel <joro@8bytes.org> - 2016-11-11 11:40 +0100

#1518906 — Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

On Fri, Oct 21, 2016 at 06:52:53PM +0100, Robin Murphy wrote:
> > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > -{
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > -		return NULL;
> 
> I *think* that if we did the initial check thus:
> 
> 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> 		return NULL;
> 
> it shouldn't be necessary to split the function at all - we then just
> wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> in the 32-bit ARM case they just don't run as that can never be true.

This would be a good improvement. Magnus, Robin, can either of you send
a follow-on patch to implement this suggestion? I have applied these
patches to my arm/renesas branch (not pushed yet). The patch can be
based on it.


	Joerg

[toc] | [next] | [standalone]

#1519499

Hello,

On Thursday 10 Nov 2016 12:42:06 Joerg Roedel wrote:
> On Fri, Oct 21, 2016 at 06:52:53PM +0100, Robin Murphy wrote:
> > > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > > -{
> > > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > > -		return NULL;
> > 
> > I *think* that if we did the initial check thus:
> > 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> > 	
> > 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> > 		
> > 		return NULL;
> > 
> > it shouldn't be necessary to split the function at all - we then just
> > wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> > in the 32-bit ARM case they just don't run as that can never be true.
> 
> This would be a good improvement. Magnus, Robin, can either of you send
> a follow-on patch to implement this suggestion? I have applied these
> patches to my arm/renesas branch (not pushed yet). The patch can be
> based on it.

I like the suggestion too, a patch is on its way.

Joerg, as I've sent a few comments about the other patches (sorry for the late 
review, I got delayed by KS and LPC), the follow-up patch should probably be 
squashed into this one when Magnus addresses my comments. Could you please 
hold off pushing the arm/renesas branch until Magnus replies to this ?

-- 
Regards,

Laurent Pinchart

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

#1519681

On Fri, Nov 11, 2016 at 03:13:32AM +0200, Laurent Pinchart wrote:
> Joerg, as I've sent a few comments about the other patches (sorry for the late 
> review, I got delayed by KS and LPC), the follow-up patch should probably be 
> squashed into this one when Magnus addresses my comments. Could you please 
> hold off pushing the arm/renesas branch until Magnus replies to this ?

Okay, I wait for a re-post and replace the patches in my tree then.



	Joerg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web