Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 18:54:10 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Alison Schofield <alison.schofield@intel.com>
Cc:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Tsuchiya Yuto <kitakar@gmail.com>,
        Martiros Shakhzadyan <vrzh@vrzh.net>,
        Hans de Goede <hdegoede@redhat.com>,
        linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, outreachy@lists.linux.dev
Subject: Re: [PATCH] staging: media: atomisp: Use kmap_local_page() in
 hmm_store()
Message-ID: <Yld+wpMe1C51bKU3@iweiny-desk3>
References: <20220413225531.9425-1-fmdefrancesco@gmail.com>
 <20220414004454.GA1243697@alison-desk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220414004454.GA1243697@alison-desk>
Precedence: bulk

On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote:
> On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page()
> > where it is feasible. The same is true for kmap_atomic().
> > 
> > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic
> > context and, if so, it calls kmap_atomic(), if not, it calls kmap().
> > 
> > First of all, in_atomic() shouldn't be used in drivers. This macro
> > cannot always detect atomic context; in particular, it cannot know
> > about held spinlocks in non-preemptible kernels.
> > 
> > Notwithstanding what it is said above, this code doesn't need to care
> > whether or not it is executing in atomic context. It can simply use
> > kmap_local_page() / kunmap_local() that can instead do the mapping /
> > unmapping regardless of the context.
> > 
> > With kmap_local_page(), the mapping is per thread, CPU local and not
> > globally visible. Therefore, hmm_store()() is a function where the use
> > of kmap_local_page() in place of both kmap() and kmap_atomic() is
> > correctly suited.
> > 
> > Convert the calls of kmap() / kunmap() and kmap_atomic() /
> > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the
> > unnecessary tests which test if the code is in atomic context.
> > 
> 
> Not specifically about this patch, but more generally about all
> such conversions - is there a 'proof' that shows this just works

Just code inspection.  Most of them that I have done have been compile tested
only.  Part of the key is that des is a local variable and is not aliased by
anything outside this function.

> or do we need to test each one. If the latter, then how?

Generally there is no test if we don't have the hardware.  Some of the more
difficult conversions will probably need to have some testing done but that
will need to be discussed with the subsystem maintainers at the time.

Ira

> 
> 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > index 46ac082cd3f1..54188197c3dc 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
> >  		idx = (virt - bo->start) >> PAGE_SHIFT;
> >  		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
> >  
> > -		if (in_atomic())
> > -			des = (char *)kmap_atomic(bo->page_obj[idx].page);
> > -		else
> > -			des = (char *)kmap(bo->page_obj[idx].page);
> > +		des = (char *)kmap_local_page(bo->page_obj[idx].page);
> >  
> >  		if (!des) {
> >  			dev_err(atomisp_dev,
> > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
> >  
> >  		clflush_cache_range(des, len);
> >  
> > -		if (in_atomic())
> > -			/*
> > -			 * Note: kunmap_atomic requires return addr from
> > -			 * kmap_atomic, not the page. See linux/highmem.h
> > -			 */
> > -			kunmap_atomic(des - offset);
> > -		else
> > -			kunmap(bo->page_obj[idx].page);
> > +		kunmap_local(des);
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.34.1
> > 
> >