Re: [PATCH v4 17/22] iommu/vt-d: Avoid duplicated code for PASID setup

Jacob Pan
2019-06-24T23:41:38Z

On Tue, 18 Jun 2019 17:03:35 +0100
Jonathan Cameron <jonathan.cameron@huawei.com> wrote:

> On Sun, 9 Jun 2019 06:44:17 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
>
> > After each setup for PASID entry, related translation caches must
> > be flushed. We can combine duplicated code into one function which
> > is less error prone.
> >
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Formatting nitpick below ;)
>
> Otherwise it's cut and paste
> > ---
> > drivers/iommu/intel-pasid.c | 48
> > +++++++++++++++++---------------------------- 1 file changed, 18
> > insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-pasid.c
> > b/drivers/iommu/intel-pasid.c index 1e25539..1ff2ecc 100644
> > --- a/drivers/iommu/intel-pasid.c
> > +++ b/drivers/iommu/intel-pasid.c
> > @@ -522,6 +522,21 @@ void intel_pasid_tear_down_entry(struct
> > intel_iommu *iommu, devtlb_invalidation_with_pasid(iommu, dev,
> > pasid); }
> >
> > +static inline void pasid_flush_caches(struct intel_iommu *iommu,
> > + struct pasid_entry *pte,
> > + int pasid, u16 did)
> > +{
> > + if (!ecap_coherent(iommu->ecap))
> > + clflush_cache_range(pte, sizeof(*pte));
> > +
> > + if (cap_caching_mode(iommu->cap)) {
> > + pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > + iotlb_invalidation_with_pasid(iommu, did, pasid);
> > + } else
> > + iommu_flush_write_buffer(iommu);
>
> I have some vague recollection kernel style says use brackets around
> single lines if other blocks in an if / else stack have multiple
> lines..
>
> I checked, this case is specifically called out
>
> https://www.kernel.org/doc/html/v5.1/process/coding-style.html
> > +
> This blank line doesn't add anything either ;)
indeed. i will add the braces and remove the blank line.

Thanks for looking it up.
> > +}
> > +
> > /*
> > * Set up the scalable mode pasid table entry for first only
> > * translation type.
> > @@ -567,16 +582,7 @@ int intel_pasid_setup_first_level(struct
> > intel_iommu *iommu, /* Setup Present and PASID Granular Transfer
> > Type: */ pasid_set_translation_type(pte, 1);
> > pasid_set_present(pte);
> > -
> > - if (!ecap_coherent(iommu->ecap))
> > - clflush_cache_range(pte, sizeof(*pte));
> > -
> > - if (cap_caching_mode(iommu->cap)) {
> > - pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > - iotlb_invalidation_with_pasid(iommu, did, pasid);
> > - } else {
> > - iommu_flush_write_buffer(iommu);
> > - }
> > + pasid_flush_caches(iommu, pte, pasid, did);
> >
> > return 0;
> > }
> > @@ -640,16 +646,7 @@ int intel_pasid_setup_second_level(struct
> > intel_iommu *iommu, */
> > pasid_set_sre(pte);
> > pasid_set_present(pte);
> > -
> > - if (!ecap_coherent(iommu->ecap))
> > - clflush_cache_range(pte, sizeof(*pte));
> > -
> > - if (cap_caching_mode(iommu->cap)) {
> > - pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > - iotlb_invalidation_with_pasid(iommu, did, pasid);
> > - } else {
> > - iommu_flush_write_buffer(iommu);
> > - }
> > + pasid_flush_caches(iommu, pte, pasid, did);
> >
> > return 0;
> > }
> > @@ -683,16 +680,7 @@ int intel_pasid_setup_pass_through(struct
> > intel_iommu *iommu, */
> > pasid_set_sre(pte);
> > pasid_set_present(pte);
> > -
> > - if (!ecap_coherent(iommu->ecap))
> > - clflush_cache_range(pte, sizeof(*pte));
> > -
> > - if (cap_caching_mode(iommu->cap)) {
> > - pasid_cache_invalidation_with_pasid(iommu, did,
> > pasid);
> > - iotlb_invalidation_with_pasid(iommu, did, pasid);
> > - } else {
> > - iommu_flush_write_buffer(iommu);
> > - }
> > + pasid_flush_caches(iommu, pte, pasid, did);
> >
> > return 0;
> > }
>
>

[Jacob Pan]