Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Konrad,

On Wednesday 20 August 2014 09:02:50 Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> > On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

[snip]

> > > >> Okay, since you add these call-backs to all drivers I think I can
> > > >> live with not doing a pointer check here.
> > > > 
> > > > I suggested doing a
> > > > 
> > > > if (ops is not NULL)
> > > > 	return ops();
> > > > else
> > > > 	return default_ops();
> > > > 
> > > > to avoid modifying all drivers. I'm not sure why that wasn't received
> > > > with much enthusiasm.
> > > 
> > > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > > so I implemented what the majority wanted. :-)
> > 
> > I'm not blaming you :-) I was just wondering what their rationale was.
> 
> a). To be inline with the usage of this API.
> 
>     For this particular case the other functions (but maybe I missed some)
>     follow the idea that they implement the ops->func for every operation
>     and they don't have an fallback.

That's because the other functions are mandatory, while this particular one is 
just an optimization and can be implemented generically.

> b)  Follow what x86 maintainers prefer (based on other API calls,
>     such as x86_init or any other platform overrides).
> 
> c). When there are new IOMMUs it has to take in-to account all of the
>     function ops. If they fail to implement all of them the kernel
>     crashes instead of working (but maybe working incorrectly).

I don't think that applies in this case, the generic implementation should 
work in all cases.

> d). Lastly, we also want the bloat of kernel. If the compiler decides
>     to roll in the fallback implementation for 'iommu_map_sg' in - it will
>     needlessly expand the kernel wherever we use 'iommu_map_sg' call with
>     a fallback implementation (which might not be called 99% of time).
> 
>     Having the little inline static just do 'func_ops->func' will
>     stop the compiler from optimizing too much and convert this just
>     to a simple function.

That's an argument I can buy.

-- 
Regards,

Laurent Pinchart