[PATCH net-next 0/5] Remove uses of kmap_atomic()

[PATCH net-next 0/5] Remove uses of kmap_atomic()

[Anirudh Venkataramanan]

kmap_atomic() is being deprecated. This little series replaces the last
few uses of kmap_atomic() in ethernet drivers with either page_address()
or kmap_local_page().

Anirudh Venkataramanan (5):
  ch_ktls: Use kmap_local_page() instead of kmap_atomic()
  sfc: Use kmap_local_page() instead of kmap_atomic()
  cassini: Remove unnecessary use of kmap_atomic()
  cassini: Use kmap_local_page() instead of kmap_atomic()
  sunvnet: Use kmap_local_page() instead of kmap_atomic()

 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 ++---
 drivers/net/ethernet/sfc/tx.c                 |  4 +-
 drivers/net/ethernet/sun/cassini.c            | 40 ++++++-------------
 drivers/net/ethernet/sun/sunvnet_common.c     |  4 +-
 4 files changed, 22 insertions(+), 36 deletions(-)


base-commit: b4b221bd79a1c698d9653e3ae2c3cb61cdc9aee7
-- 
2.37.2

[PATCH net-next 1/5] ch_ktls: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

kmap_atomic() is being deprecated in favor of kmap_local_page().
Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
and kunmap_local() respectively.

Note that kmap_atomic() disables preemption and page-fault processing,
but kmap_local_page() doesn't. Converting the former to the latter is safe
only if there isn't an implicit dependency on preemption and page-fault
handling being disabled, which does appear to be the case here.

Also note that the page being mapped is not allocated by the driver,
and so the driver doesn't know if the page is in normal memory. This is the
reason kmap_local_page() is used as opposed to page_address().

I don't have hardware, so this change has only been compile tested.

Cc: Ayush Sawal <ayush.sawal@chelsio.com>
Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
Cc: Rohit Maheshwari <rohitm@chelsio.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
 .../ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index da9973b..d95f230 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -1853,24 +1853,24 @@ static int chcr_short_record_handler(struct chcr_ktls_info *tx_info,
 				i++;
 			}
 			f = &record->frags[i];
-			vaddr = kmap_atomic(skb_frag_page(f));
+			vaddr = kmap_local_page(skb_frag_page(f));
 
 			data = vaddr + skb_frag_off(f)  + remaining;
 			frag_delta = skb_frag_size(f) - remaining;
 
 			if (frag_delta >= prior_data_len) {
 				memcpy(prior_data, data, prior_data_len);
-				kunmap_atomic(vaddr);
+				kunmap_local(vaddr);
 			} else {
 				memcpy(prior_data, data, frag_delta);
-				kunmap_atomic(vaddr);
+				kunmap_local(vaddr);
 				/* get the next page */
 				f = &record->frags[i + 1];
-				vaddr = kmap_atomic(skb_frag_page(f));
+				vaddr = kmap_local_page(skb_frag_page(f));
 				data = vaddr + skb_frag_off(f);
 				memcpy(prior_data + frag_delta,
 				       data, (prior_data_len - frag_delta));
-				kunmap_atomic(vaddr);
+				kunmap_local(vaddr);
 			}
 			/* reset tcp_seq as per the prior_data_required len */
 			tcp_seq -= prior_data_len;
-- 
2.37.2

Re: [PATCH net-next 1/5] ch_ktls: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On giovedì 17 novembre 2022 23:25:53 CET Anirudh Venkataramanan wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page().
> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> and kunmap_local() respectively.
> 
> Note that kmap_atomic() disables preemption and page-fault processing,
> but kmap_local_page() doesn't. Converting the former to the latter is safe
> only if there isn't an implicit dependency on preemption and page-fault
> handling being disabled, which does appear to be the case here.
> 
> Also note that the page being mapped is not allocated by the driver,
> and so the driver doesn't know if the page is in normal memory. This is the
> reason kmap_local_page() is used as opposed to page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ayush Sawal <ayush.sawal@chelsio.com>
> Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
> Cc: Rohit Maheshwari <rohitm@chelsio.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  .../ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c index
> da9973b..d95f230 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> @@ -1853,24 +1853,24 @@ static int chcr_short_record_handler(struct
> chcr_ktls_info *tx_info, i++;
>  			}
>  			f = &record->frags[i];
> -			vaddr = kmap_atomic(skb_frag_page(f));
> +			vaddr = kmap_local_page(skb_frag_page(f));
> 
>  			data = vaddr + skb_frag_off(f)  + remaining;
>  			frag_delta = skb_frag_size(f) - remaining;
> 
>  			if (frag_delta >= prior_data_len) {
>  				memcpy(prior_data, data, 
prior_data_len);
> -				kunmap_atomic(vaddr);
> +				kunmap_local(vaddr);
>  			} else {
>  				memcpy(prior_data, data, frag_delta);
> -				kunmap_atomic(vaddr);
> +				kunmap_local(vaddr);
>  				/* get the next page */
>  				f = &record->frags[i + 1];
> -				vaddr = kmap_atomic(skb_frag_page(f));
> +				vaddr = 
kmap_local_page(skb_frag_page(f));
>  				data = vaddr + skb_frag_off(f);
>  				memcpy(prior_data + frag_delta,
>  				       data, (prior_data_len - 
frag_delta));
> -				kunmap_atomic(vaddr);
> +				kunmap_local(vaddr);
>  			}
>  			/* reset tcp_seq as per the prior_data_required 
len */
>  			tcp_seq -= prior_data_len;
> --
> 2.37.2

The last conversion could have been done with memcpy_from_page(). However, 
it's not that a big deal. Therefore...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

Re: [PATCH net-next 1/5] ch_ktls: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

On 11/18/2022 12:14 AM, Fabio M. De Francesco wrote:
> On giovedì 17 novembre 2022 23:25:53 CET Anirudh Venkataramanan wrote:
>> kmap_atomic() is being deprecated in favor of kmap_local_page().
>> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
>> and kunmap_local() respectively.
>>
>> Note that kmap_atomic() disables preemption and page-fault processing,
>> but kmap_local_page() doesn't. Converting the former to the latter is safe
>> only if there isn't an implicit dependency on preemption and page-fault
>> handling being disabled, which does appear to be the case here.
>>
>> Also note that the page being mapped is not allocated by the driver,
>> and so the driver doesn't know if the page is in normal memory. This is the
>> reason kmap_local_page() is used as opposed to page_address().
>>
>> I don't have hardware, so this change has only been compile tested.
>>
>> Cc: Ayush Sawal <ayush.sawal@chelsio.com>
>> Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
>> Cc: Rohit Maheshwari <rohitm@chelsio.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>> ---
>>   .../ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
>> b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c index
>> da9973b..d95f230 100644
>> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
>> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
>> @@ -1853,24 +1853,24 @@ static int chcr_short_record_handler(struct
>> chcr_ktls_info *tx_info, i++;
>>   			}
>>   			f = &record->frags[i];
>> -			vaddr = kmap_atomic(skb_frag_page(f));
>> +			vaddr = kmap_local_page(skb_frag_page(f));
>>
>>   			data = vaddr + skb_frag_off(f)  + remaining;
>>   			frag_delta = skb_frag_size(f) - remaining;
>>
>>   			if (frag_delta >= prior_data_len) {
>>   				memcpy(prior_data, data,
> prior_data_len);
>> -				kunmap_atomic(vaddr);
>> +				kunmap_local(vaddr);
>>   			} else {
>>   				memcpy(prior_data, data, frag_delta);
>> -				kunmap_atomic(vaddr);
>> +				kunmap_local(vaddr);
>>   				/* get the next page */
>>   				f = &record->frags[i + 1];
>> -				vaddr = kmap_atomic(skb_frag_page(f));
>> +				vaddr =
> kmap_local_page(skb_frag_page(f));
>>   				data = vaddr + skb_frag_off(f);
>>   				memcpy(prior_data + frag_delta,
>>   				       data, (prior_data_len -
> frag_delta));
>> -				kunmap_atomic(vaddr);
>> +				kunmap_local(vaddr);
>>   			}
>>   			/* reset tcp_seq as per the prior_data_required
> len */
>>   			tcp_seq -= prior_data_len;
>> --
>> 2.37.2
> 
> The last conversion could have been done with memcpy_from_page(). However,
> it's not that a big deal. Therefore...
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Yeah, using memcpy_from_page() is cleaner. I'll update this patch, and 
probably 4/5 too.

Thanks!
Ani

Re: [PATCH net-next 1/5] ch_ktls: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On venerdì 18 novembre 2022 19:27:56 CET Anirudh Venkataramanan wrote:
> On 11/18/2022 12:14 AM, Fabio M. De Francesco wrote:
> > On giovedì 17 novembre 2022 23:25:53 CET Anirudh Venkataramanan wrote:
> >> kmap_atomic() is being deprecated in favor of kmap_local_page().
> >> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> >> and kunmap_local() respectively.
> >> 
> >> Note that kmap_atomic() disables preemption and page-fault processing,
> >> but kmap_local_page() doesn't. Converting the former to the latter is 
safe
> >> only if there isn't an implicit dependency on preemption and page-fault
> >> handling being disabled, which does appear to be the case here.
> >> 
> >> Also note that the page being mapped is not allocated by the driver,
> >> and so the driver doesn't know if the page is in normal memory. This is 
the
> >> reason kmap_local_page() is used as opposed to page_address().
> >> 
> >> I don't have hardware, so this change has only been compile tested.
> >> 
> >> Cc: Ayush Sawal <ayush.sawal@chelsio.com>
> >> Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
> >> Cc: Rohit Maheshwari <rohitm@chelsio.com>
> >> Cc: Ira Weiny <ira.weiny@intel.com>
> >> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> >> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> >> ---
> >> 
> >>   .../ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/
chcr_ktls.c
> >> b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c index
> >> da9973b..d95f230 100644
> >> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> >> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> >> @@ -1853,24 +1853,24 @@ static int chcr_short_record_handler(struct
> >> chcr_ktls_info *tx_info, i++;
> >> 
> >>   			}
> >>   			f = &record->frags[i];
> >> 
> >> -			vaddr = kmap_atomic(skb_frag_page(f));
> >> +			vaddr = kmap_local_page(skb_frag_page(f));
> >> 
> >>   			data = vaddr + skb_frag_off(f)  + remaining;
> >>   			frag_delta = skb_frag_size(f) - remaining;
> >>   			
> >>   			if (frag_delta >= prior_data_len) {
> >>   			
> >>   				memcpy(prior_data, data,
> > 
> > prior_data_len);
> > 
> >> -				kunmap_atomic(vaddr);
> >> +				kunmap_local(vaddr);
> >> 
> >>   			} else {
> >>   			
> >>   				memcpy(prior_data, data, frag_delta);
> >> 
> >> -				kunmap_atomic(vaddr);
> >> +				kunmap_local(vaddr);
> >> 
> >>   				/* get the next page */
> >>   				f = &record->frags[i + 1];
> >> 
> >> -				vaddr = kmap_atomic(skb_frag_page(f));
> >> +				vaddr =
> > 
> > kmap_local_page(skb_frag_page(f));
> > 
> >>   				data = vaddr + skb_frag_off(f);
> >>   				memcpy(prior_data + frag_delta,
> >>   				
> >>   				       data, (prior_data_len -
> > 
> > frag_delta));
> > 
> >> -				kunmap_atomic(vaddr);
> >> +				kunmap_local(vaddr);
> >> 
> >>   			}
> >>   			/* reset tcp_seq as per the prior_data_required
> > 
> > len */
> > 
> >>   			tcp_seq -= prior_data_len;
> >> 
> >> --
> >> 2.37.2
> > 
> > The last conversion could have been done with memcpy_from_page(). However,
> > it's not that a big deal. Therefore...
> > 
> > Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Yeah, using memcpy_from_page() is cleaner. I'll update this patch, and
> probably 4/5 too.
> 
> Thanks!
> Ani

Well, I didn't ask you for a second version. This is why you already see my 
"Reviewed-by:" tag. I'm OK with your changes. I just warned you that 
maintainers might ask, so I'd wait and see. However it's up to you.

However, if you decide to send this patch with memcpy_from_page(), why you 
are not sure about 4/5? Since you decided to send 1/5 again, what does prevent 
you from updating also 4/5?

Fabio  

Re: [PATCH net-next 1/5] ch_ktls: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

On 11/18/2022 12:18 PM, Fabio M. De Francesco wrote:
> On venerdì 18 novembre 2022 19:27:56 CET Anirudh Venkataramanan wrote:
>> On 11/18/2022 12:14 AM, Fabio M. De Francesco wrote:
>>> On giovedì 17 novembre 2022 23:25:53 CET Anirudh Venkataramanan wrote:
>>>> kmap_atomic() is being deprecated in favor of kmap_local_page().
>>>> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
>>>> and kunmap_local() respectively.
>>>>
>>>> Note that kmap_atomic() disables preemption and page-fault processing,
>>>> but kmap_local_page() doesn't. Converting the former to the latter is
> safe
>>>> only if there isn't an implicit dependency on preemption and page-fault
>>>> handling being disabled, which does appear to be the case here.
>>>>
>>>> Also note that the page being mapped is not allocated by the driver,
>>>> and so the driver doesn't know if the page is in normal memory. This is
> the
>>>> reason kmap_local_page() is used as opposed to page_address().
>>>>
>>>> I don't have hardware, so this change has only been compile tested.
>>>>
>>>> Cc: Ayush Sawal <ayush.sawal@chelsio.com>
>>>> Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
>>>> Cc: Rohit Maheshwari <rohitm@chelsio.com>
>>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>>> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>>>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>>>> ---
>>>>
>>>>    .../ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/
> chcr_ktls.c
>>>> b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c index
>>>> da9973b..d95f230 100644
>>>> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
>>>> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
>>>> @@ -1853,24 +1853,24 @@ static int chcr_short_record_handler(struct
>>>> chcr_ktls_info *tx_info, i++;
>>>>
>>>>    			}
>>>>    			f = &record->frags[i];
>>>>
>>>> -			vaddr = kmap_atomic(skb_frag_page(f));
>>>> +			vaddr = kmap_local_page(skb_frag_page(f));
>>>>
>>>>    			data = vaddr + skb_frag_off(f)  + remaining;
>>>>    			frag_delta = skb_frag_size(f) - remaining;
>>>>    			
>>>>    			if (frag_delta >= prior_data_len) {
>>>>    			
>>>>    				memcpy(prior_data, data,
>>>
>>> prior_data_len);
>>>
>>>> -				kunmap_atomic(vaddr);
>>>> +				kunmap_local(vaddr);
>>>>
>>>>    			} else {
>>>>    			
>>>>    				memcpy(prior_data, data, frag_delta);
>>>>
>>>> -				kunmap_atomic(vaddr);
>>>> +				kunmap_local(vaddr);
>>>>
>>>>    				/* get the next page */
>>>>    				f = &record->frags[i + 1];
>>>>
>>>> -				vaddr = kmap_atomic(skb_frag_page(f));
>>>> +				vaddr =
>>>
>>> kmap_local_page(skb_frag_page(f));
>>>
>>>>    				data = vaddr + skb_frag_off(f);
>>>>    				memcpy(prior_data + frag_delta,
>>>>    				
>>>>    				       data, (prior_data_len -
>>>
>>> frag_delta));
>>>
>>>> -				kunmap_atomic(vaddr);
>>>> +				kunmap_local(vaddr);
>>>>
>>>>    			}
>>>>    			/* reset tcp_seq as per the prior_data_required
>>>
>>> len */
>>>
>>>>    			tcp_seq -= prior_data_len;
>>>>
>>>> --
>>>> 2.37.2
>>>
>>> The last conversion could have been done with memcpy_from_page(). However,
>>> it's not that a big deal. Therefore...
>>>
>>> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>>
>> Yeah, using memcpy_from_page() is cleaner. I'll update this patch, and
>> probably 4/5 too.
>>
>> Thanks!
>> Ani
> 
> Well, I didn't ask you for a second version. This is why you already see my
> "Reviewed-by:" tag. I'm OK with your changes. I just warned you that
> maintainers might ask, so I'd wait and see. However it's up to you.

I understand and appreciate your "Reviewed-by", but that doesn't mean 
further improvements aren't possible. I believe using memcpy_from_page() 
is better, and plan to do this in v2.

> 
> However, if you decide to send this patch with memcpy_from_page(), why you
> are not sure about 4/5? Since you decided to send 1/5 again, what does prevent
> you from updating also 4/5?

I hadn't seen patch 4/5 when I replied to you. Since then I have, and so 
I'll be updating 4/5 to use memcpy_from_page() as well.

Ani

Re: [PATCH net-next 1/5] ch_ktls: Use kmap_local_page() instead of kmap_atomic()

[Ira Weiny]

On Thu, Nov 17, 2022 at 02:25:53PM -0800, Venkataramanan, Anirudh wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page().
> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> and kunmap_local() respectively.
> 
> Note that kmap_atomic() disables preemption and page-fault processing,
> but kmap_local_page() doesn't. Converting the former to the latter is safe
> only if there isn't an implicit dependency on preemption and page-fault
> handling being disabled, which does appear to be the case here.
                                 does not?

Also, say 'is not the case here'.  Appearances are not enough.  But we know
that this code is safe doing only memcpy's.

> 
> Also note that the page being mapped is not allocated by the driver,
> and so the driver doesn't know if the page is in normal memory. This is the
> reason kmap_local_page() is used as opposed to page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ayush Sawal <ayush.sawal@chelsio.com>
> Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com>
> Cc: Rohit Maheshwari <rohitm@chelsio.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  .../ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> index da9973b..d95f230 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> @@ -1853,24 +1853,24 @@ static int chcr_short_record_handler(struct chcr_ktls_info *tx_info,
>  				i++;
>  			}
>  			f = &record->frags[i];
> -			vaddr = kmap_atomic(skb_frag_page(f));
> +			vaddr = kmap_local_page(skb_frag_page(f));
>  
>  			data = vaddr + skb_frag_off(f)  + remaining;
>  			frag_delta = skb_frag_size(f) - remaining;
>  
>  			if (frag_delta >= prior_data_len) {
>  				memcpy(prior_data, data, prior_data_len);
> -				kunmap_atomic(vaddr);
> +				kunmap_local(vaddr);
>  			} else {
>  				memcpy(prior_data, data, frag_delta);
> -				kunmap_atomic(vaddr);
> +				kunmap_local(vaddr);
>  				/* get the next page */
>  				f = &record->frags[i + 1];
> -				vaddr = kmap_atomic(skb_frag_page(f));
> +				vaddr = kmap_local_page(skb_frag_page(f));
>  				data = vaddr + skb_frag_off(f);
>  				memcpy(prior_data + frag_delta,
>  				       data, (prior_data_len - frag_delta));
> -				kunmap_atomic(vaddr);
> +				kunmap_local(vaddr);

Agree with Fabio that this needs to be memcpy_from_page().  We should be
consistent in using it.

Ira

>  			}
>  			/* reset tcp_seq as per the prior_data_required len */
>  			tcp_seq -= prior_data_len;
> -- 
> 2.37.2
> 

[PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

kmap_atomic() is being deprecated in favor of kmap_local_page().
Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
and kunmap_local() respectively.

Note that kmap_atomic() disables preemption and page-fault processing, but
kmap_local_page() doesn't. Converting the former to the latter is safe only
if there isn't an implicit dependency on preemption and page-fault handling
being disabled, which does appear to be the case here.

Also note that the page being mapped is not allocated by the driver, and so
the driver doesn't know if the page is in normal memory. This is the reason
kmap_local_page() is used as opposed to page_address().

I don't have hardware, so this change has only been compile tested.

Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Cc: Edward Cree <ecree.xilinx@gmail.com>
Cc: Martin Habets <habetsm.xilinx@gmail.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
 drivers/net/ethernet/sfc/tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index c5f88f7..4ed4082 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -207,11 +207,11 @@ static void efx_skb_copy_bits_to_pio(struct efx_nic *efx, struct sk_buff *skb,
 		skb_frag_t *f = &skb_shinfo(skb)->frags[i];
 		u8 *vaddr;
 
-		vaddr = kmap_atomic(skb_frag_page(f));
+		vaddr = kmap_local_page(skb_frag_page(f));
 
 		efx_memcpy_toio_aligned_cb(efx, piobuf, vaddr + skb_frag_off(f),
 					   skb_frag_size(f), copy_buf);
-		kunmap_atomic(vaddr);
+		kunmap_local(vaddr);
 	}
 
 	EFX_WARN_ON_ONCE_PARANOID(skb_shinfo(skb)->frag_list);
-- 
2.37.2

Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On giovedì 17 novembre 2022 23:25:54 CET Anirudh Venkataramanan wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page().
> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> and kunmap_local() respectively.
> 
> Note that kmap_atomic() disables preemption and page-fault processing, but
> kmap_local_page() doesn't. Converting the former to the latter is safe only
> if there isn't an implicit dependency on preemption and page-fault handling
> being disabled, which does appear to be the case here.

NIT: It is always possible to disable explicitly along with the conversion.
However, you are noticing that we don't need to do it.

I'm noticing the same wording in other of your patches, but there are no 
problems with them. 

> 
> Also note that the page being mapped is not allocated by the driver, and so
> the driver doesn't know if the page is in normal memory. This is the reason
> kmap_local_page() is used as opposed to page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Cc: Edward Cree <ecree.xilinx@gmail.com>
> Cc: Martin Habets <habetsm.xilinx@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  drivers/net/ethernet/sfc/tx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> index c5f88f7..4ed4082 100644
> --- a/drivers/net/ethernet/sfc/tx.c
> +++ b/drivers/net/ethernet/sfc/tx.c
> @@ -207,11 +207,11 @@ static void efx_skb_copy_bits_to_pio(struct efx_nic
> *efx, struct sk_buff *skb, skb_frag_t *f = &skb_shinfo(skb)->frags[i];
>  		u8 *vaddr;
> 
> -		vaddr = kmap_atomic(skb_frag_page(f));
> +		vaddr = kmap_local_page(skb_frag_page(f));
> 
>  		efx_memcpy_toio_aligned_cb(efx, piobuf, vaddr + 
skb_frag_off(f),
>  					   skb_frag_size(f), 
copy_buf);
> -		kunmap_atomic(vaddr);
> +		kunmap_local(vaddr);
>  	}
> 
>  	EFX_WARN_ON_ONCE_PARANOID(skb_shinfo(skb)->frag_list);
> --
> 2.37.2

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

On 11/18/2022 12:23 AM, Fabio M. De Francesco wrote:
> On giovedì 17 novembre 2022 23:25:54 CET Anirudh Venkataramanan wrote:
>> kmap_atomic() is being deprecated in favor of kmap_local_page().
>> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
>> and kunmap_local() respectively.
>>
>> Note that kmap_atomic() disables preemption and page-fault processing, but
>> kmap_local_page() doesn't. Converting the former to the latter is safe only
>> if there isn't an implicit dependency on preemption and page-fault handling
>> being disabled, which does appear to be the case here.
> 
> NIT: It is always possible to disable explicitly along with the conversion.

Fair enough. I suppose "convert" is broader than "replace". How about this:

"Replacing the former with the latter is safe only if there isn't an 
implicit dependency on preemption and page-fault handling being 
disabled, which does appear to be the case here."

Ani

Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On venerdì 18 novembre 2022 18:47:52 CET Anirudh Venkataramanan wrote:
> On 11/18/2022 12:23 AM, Fabio M. De Francesco wrote:
> > On giovedì 17 novembre 2022 23:25:54 CET Anirudh Venkataramanan wrote:
> >> kmap_atomic() is being deprecated in favor of kmap_local_page().
> >> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> >> and kunmap_local() respectively.
> >> 
> >> Note that kmap_atomic() disables preemption and page-fault processing, 
but
> >> kmap_local_page() doesn't. Converting the former to the latter is safe 
only
> >> if there isn't an implicit dependency on preemption and page-fault 
handling
> >> being disabled, which does appear to be the case here.
> > 
> > NIT: It is always possible to disable explicitly along with the 
conversion.
> 
> Fair enough. I suppose "convert" is broader than "replace". How about this:
> 
> "Replacing the former with the latter is safe only if there isn't an
> implicit dependency on preemption and page-fault handling being
> disabled, which does appear to be the case here."
> 
> Ani

Let's start with 2/5 because it looks that here we are talking of a sensitive 
subject. Yesterday something triggered the necessity to make a patch for 
highmem.rst for clarifying that these conversions can _always_ be addressed.  

I sent it to Ira and I'm waiting for his opinion before submitting it.

The me explain better... the point is that all kmap_atomic(), despite the 
differences, _can_ be converted to kmap_local_page().

What I care of is the safety of the conversions. I trust your commit message 
where you say that you inspected the code and that "there isn't an implicit 
dependency on preemption and page-fault handling being disabled".

I was talking about something very different: what if the code between mapping 
and unmapping was relying on implicit page-faults and/or preemption disable? I 
read between the lines that you consider a conversion of that kind something 
that cannot be addressed because "kmap_atomic() disables preemption and page-
fault processing, but kmap_local_page() doesn't" (which is true).

The point is that you have the possibility to convert also in this 
hypothetical case by doing something like the following.

Old code:

ptr = kmap_atomic(page);
do something with ptr;
kunmap_atomic(ptr);

You checked the code and understood that that "something" can only be carried 
out with page-faults disabled (just an example). Conversion:

pagefault_disable();
ptr = kmap_local_page(page);
do something with ptr;
kunmap_local(ptr);
pagefault_enable();

I'm not asking to reword your commit message only for the purpose to clear 
that your changes are "safe" because you checked the code and can reasonably 
affirm that the conversion doesn't depend on further disables.

I just said it to make you notice that every kmap_atomic() conversion to 
kmap_local_page() is "safe", but only if you really understand the code and 
act accordingly.

I'm too wordy, Ira said it so many times. Unfortunately, I'm not able to 
optimize English text and need to improve. I'm sorry.

Does my long explanation make any sense to you?

If so, I'm happy. I'm not asking to send v2. I just desired that you realize 
(1) how tricky these conversions may be and therefore how much important is 
not to do them mechanically (2) how to better craft your next commit message 
(if you want to keep on helping with these conversions).

I'm OK with this patch. Did you see my tag? :-)

Thanks for helping,

Fabio  

Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

On 11/18/2022 11:26 AM, Fabio M. De Francesco wrote:
> On venerdì 18 novembre 2022 18:47:52 CET Anirudh Venkataramanan wrote:
>> On 11/18/2022 12:23 AM, Fabio M. De Francesco wrote:
>>> On giovedì 17 novembre 2022 23:25:54 CET Anirudh Venkataramanan wrote:
>>>> kmap_atomic() is being deprecated in favor of kmap_local_page().
>>>> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
>>>> and kunmap_local() respectively.
>>>>
>>>> Note that kmap_atomic() disables preemption and page-fault processing,
> but
>>>> kmap_local_page() doesn't. Converting the former to the latter is safe
> only
>>>> if there isn't an implicit dependency on preemption and page-fault
> handling
>>>> being disabled, which does appear to be the case here.
>>>
>>> NIT: It is always possible to disable explicitly along with the
> conversion.
>>
>> Fair enough. I suppose "convert" is broader than "replace". How about this:
>>
>> "Replacing the former with the latter is safe only if there isn't an
>> implicit dependency on preemption and page-fault handling being
>> disabled, which does appear to be the case here."
>>
>> Ani
> 
> Let's start with 2/5 because it looks that here we are talking of a sensitive
> subject. Yesterday something triggered the necessity to make a patch for
> highmem.rst for clarifying that these conversions can _always_ be addressed.
> 
> I sent it to Ira and I'm waiting for his opinion before submitting it.
> 
> The me explain better... the point is that all kmap_atomic(), despite the
> differences, _can_ be converted to kmap_local_page().
> 
> What I care of is the safety of the conversions. I trust your commit message
> where you say that you inspected the code and that "there isn't an implicit
> dependency on preemption and page-fault handling being disabled".
> 
> I was talking about something very different: what if the code between mapping
> and unmapping was relying on implicit page-faults and/or preemption disable? I
> read between the lines that you consider a conversion of that kind something
> that cannot be addressed because "kmap_atomic() disables preemption and page-
> fault processing, but kmap_local_page() doesn't" (which is true).

No, I wasn't saying (or implying) this at all, nor did I think it 
could/would be interpreted this way.

I was trying to say that a straight-up replacing kmap_atomic() with 
kmap_local_page() would not be functionally safe if the code in between 
the mapping and unmapping relied on page-faults and/or preemption being 
disabled.

> 
> The point is that you have the possibility to convert also in this
> hypothetical case by doing something like the following.
> 
> Old code:
> 
> ptr = kmap_atomic(page);
> do something with ptr;
> kunmap_atomic(ptr);
> 
> You checked the code and understood that that "something" can only be carried
> out with page-faults disabled (just an example). Conversion:
> 
> pagefault_disable();
> ptr = kmap_local_page(page);
> do something with ptr;
> kunmap_local(ptr);
> pagefault_enable();
> 
> I'm not asking to reword your commit message only for the purpose to clear
> that your changes are "safe" because you checked the code and can reasonably
> affirm that the conversion doesn't depend on further disables.
> 
> I just said it to make you notice that every kmap_atomic() conversion to
> kmap_local_page() is "safe", but only if you really understand the code and
> act accordingly.
> 
> I'm too wordy, Ira said it so many times. Unfortunately, I'm not able to
> optimize English text and need to improve. I'm sorry.
> 
> Does my long explanation make any sense to you?
> 
> If so, I'm happy. I'm not asking to send v2. I just desired that you realize
> (1) how tricky these conversions may be and therefore how much important is
> not to do them mechanically (2) how to better craft your next commit message
> (if you want to keep on helping with these conversions).

Appreciate the explanation, but as I explained above, what you read 
between the lines isn't what I was saying. I am most certainly not 
making these changes mechanically.

> 
> I'm OK with this patch. Did you see my tag? :-)

I did, and thanks!

FYI, I will be sending a v2 for the series anyway (with the 
memcpy_from_page()) changes, and so I am planning to update the commit 
messages to hopefully be a little clearer to you.

Ani

Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap_atomic()

[Ira Weiny]

On Thu, Nov 17, 2022 at 02:25:54PM -0800, Venkataramanan, Anirudh wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page().
> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> and kunmap_local() respectively.
> 
> Note that kmap_atomic() disables preemption and page-fault processing, but
> kmap_local_page() doesn't. Converting the former to the latter is safe only
> if there isn't an implicit dependency on preemption and page-fault handling
> being disabled, which does appear to be the case here.

Oh reading this here I see you meant that 'there is not an implicit
dependency'...  :-/  Ok read that too quick.

Still it is not an appearance it is true right?

Ok

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> Also note that the page being mapped is not allocated by the driver, and so
> the driver doesn't know if the page is in normal memory. This is the reason
> kmap_local_page() is used as opposed to page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Cc: Edward Cree <ecree.xilinx@gmail.com>
> Cc: Martin Habets <habetsm.xilinx@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  drivers/net/ethernet/sfc/tx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> index c5f88f7..4ed4082 100644
> --- a/drivers/net/ethernet/sfc/tx.c
> +++ b/drivers/net/ethernet/sfc/tx.c
> @@ -207,11 +207,11 @@ static void efx_skb_copy_bits_to_pio(struct efx_nic *efx, struct sk_buff *skb,
>  		skb_frag_t *f = &skb_shinfo(skb)->frags[i];
>  		u8 *vaddr;
>  
> -		vaddr = kmap_atomic(skb_frag_page(f));
> +		vaddr = kmap_local_page(skb_frag_page(f));
>  
>  		efx_memcpy_toio_aligned_cb(efx, piobuf, vaddr + skb_frag_off(f),
>  					   skb_frag_size(f), copy_buf);
> -		kunmap_atomic(vaddr);
> +		kunmap_local(vaddr);
>  	}
>  
>  	EFX_WARN_ON_ONCE_PARANOID(skb_shinfo(skb)->frag_list);
> -- 
> 2.37.2
> 

[PATCH net-next 3/5] cassini: Remove unnecessary use of kmap_atomic()

[Anirudh Venkataramanan]

Pages for Rx buffers are allocated in cas_page_alloc() using either
GFP_ATOMIC or GFP_KERNEL. Memory allocated with GFP_KERNEL/GFP_ATOMIC
can't come from highmem and so there's no need to kmap() them. Just use
page_address() instead.

I don't have hardware, so this change has only been compile tested.

Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
 drivers/net/ethernet/sun/cassini.c | 34 ++++++++++--------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/sun/cassini.c b/drivers/net/ethernet/sun/cassini.c
index 0aca193..2f66cfc 100644
--- a/drivers/net/ethernet/sun/cassini.c
+++ b/drivers/net/ethernet/sun/cassini.c
@@ -1915,7 +1915,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 	int off, swivel = RX_SWIVEL_OFF_VAL;
 	struct cas_page *page;
 	struct sk_buff *skb;
-	void *addr, *crcaddr;
+	void *crcaddr;
 	__sum16 csum;
 	char *p;
 
@@ -1936,7 +1936,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 	skb_reserve(skb, swivel);
 
 	p = skb->data;
-	addr = crcaddr = NULL;
+	crcaddr = NULL;
 	if (hlen) { /* always copy header pages */
 		i = CAS_VAL(RX_COMP2_HDR_INDEX, words[1]);
 		page = cp->rx_pages[CAS_VAL(RX_INDEX_RING, i)][CAS_VAL(RX_INDEX_NUM, i)];
@@ -1948,12 +1948,10 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 			i += cp->crc_size;
 		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr + off,
 					i, DMA_FROM_DEVICE);
-		addr = cas_page_map(page->buffer);
-		memcpy(p, addr + off, i);
+		memcpy(p, page_address(page->buffer) + off, i);
 		dma_sync_single_for_device(&cp->pdev->dev,
 					   page->dma_addr + off, i,
 					   DMA_FROM_DEVICE);
-		cas_page_unmap(addr);
 		RX_USED_ADD(page, 0x100);
 		p += hlen;
 		swivel = 0;
@@ -1984,12 +1982,11 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 		/* make sure we always copy a header */
 		swivel = 0;
 		if (p == (char *) skb->data) { /* not split */
-			addr = cas_page_map(page->buffer);
-			memcpy(p, addr + off, RX_COPY_MIN);
+			memcpy(p, page_address(page->buffer) + off,
+			       RX_COPY_MIN);
 			dma_sync_single_for_device(&cp->pdev->dev,
 						   page->dma_addr + off, i,
 						   DMA_FROM_DEVICE);
-			cas_page_unmap(addr);
 			off += RX_COPY_MIN;
 			swivel = RX_COPY_MIN;
 			RX_USED_ADD(page, cp->mtu_stride);
@@ -2036,10 +2033,8 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 			RX_USED_ADD(page, hlen + cp->crc_size);
 		}
 
-		if (cp->crc_size) {
-			addr = cas_page_map(page->buffer);
-			crcaddr  = addr + off + hlen;
-		}
+		if (cp->crc_size)
+			crcaddr = page_address(page->buffer) + off + hlen;
 
 	} else {
 		/* copying packet */
@@ -2061,12 +2056,10 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 			i += cp->crc_size;
 		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr + off,
 					i, DMA_FROM_DEVICE);
-		addr = cas_page_map(page->buffer);
-		memcpy(p, addr + off, i);
+		memcpy(p, page_address(page->buffer) + off, i);
 		dma_sync_single_for_device(&cp->pdev->dev,
 					   page->dma_addr + off, i,
 					   DMA_FROM_DEVICE);
-		cas_page_unmap(addr);
 		if (p == (char *) skb->data) /* not split */
 			RX_USED_ADD(page, cp->mtu_stride);
 		else
@@ -2081,20 +2074,17 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 						page->dma_addr,
 						dlen + cp->crc_size,
 						DMA_FROM_DEVICE);
-			addr = cas_page_map(page->buffer);
-			memcpy(p, addr, dlen + cp->crc_size);
+			memcpy(p, page_address(page->buffer), dlen + cp->crc_size);
 			dma_sync_single_for_device(&cp->pdev->dev,
 						   page->dma_addr,
 						   dlen + cp->crc_size,
 						   DMA_FROM_DEVICE);
-			cas_page_unmap(addr);
 			RX_USED_ADD(page, dlen + cp->crc_size);
 		}
 end_copy_pkt:
-		if (cp->crc_size) {
-			addr    = NULL;
+		if (cp->crc_size)
 			crcaddr = skb->data + alloclen;
-		}
+
 		skb_put(skb, alloclen);
 	}
 
@@ -2103,8 +2093,6 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 		/* checksum includes FCS. strip it out. */
 		csum = csum_fold(csum_partial(crcaddr, cp->crc_size,
 					      csum_unfold(csum)));
-		if (addr)
-			cas_page_unmap(addr);
 	}
 	skb->protocol = eth_type_trans(skb, cp->dev);
 	if (skb->protocol == htons(ETH_P_IP)) {
-- 
2.37.2

Re: [PATCH net-next 3/5] cassini: Remove unnecessary use of kmap_atomic()

[Fabio M. De Francesco]

On giovedì 17 novembre 2022 23:25:55 CET Anirudh Venkataramanan wrote:
> Pages for Rx buffers are allocated in cas_page_alloc() using either
> GFP_ATOMIC or GFP_KERNEL. Memory allocated with GFP_KERNEL/GFP_ATOMIC
> can't come from highmem and so there's no need to kmap() them. Just use
> page_address() instead.
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  drivers/net/ethernet/sun/cassini.c | 34 ++++++++++--------------------
>  1 file changed, 11 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/cassini.c
> b/drivers/net/ethernet/sun/cassini.c index 0aca193..2f66cfc 100644
> --- a/drivers/net/ethernet/sun/cassini.c
> +++ b/drivers/net/ethernet/sun/cassini.c
> @@ -1915,7 +1915,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, int off, swivel = RX_SWIVEL_OFF_VAL;
>  	struct cas_page *page;
>  	struct sk_buff *skb;
> -	void *addr, *crcaddr;
> +	void *crcaddr;
>  	__sum16 csum;
>  	char *p;
> 
> @@ -1936,7 +1936,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, skb_reserve(skb, swivel);
> 
>  	p = skb->data;
> -	addr = crcaddr = NULL;
> +	crcaddr = NULL;
>  	if (hlen) { /* always copy header pages */
>  		i = CAS_VAL(RX_COMP2_HDR_INDEX, words[1]);
>  		page = cp->rx_pages[CAS_VAL(RX_INDEX_RING, i)]
[CAS_VAL(RX_INDEX_NUM, i)];
> @@ -1948,12 +1948,10 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, i += cp->crc_size;
>  		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr + 
off,
>  					i, DMA_FROM_DEVICE);
> -		addr = cas_page_map(page->buffer);
> -		memcpy(p, addr + off, i);
> +		memcpy(p, page_address(page->buffer) + off, i);
>  		dma_sync_single_for_device(&cp->pdev->dev,
>  					   page->dma_addr + off, i,
>  					   DMA_FROM_DEVICE);
> -		cas_page_unmap(addr);
>  		RX_USED_ADD(page, 0x100);
>  		p += hlen;
>  		swivel = 0;
> @@ -1984,12 +1982,11 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, /* make sure we always copy a header */
>  		swivel = 0;
>  		if (p == (char *) skb->data) { /* not split */
> -			addr = cas_page_map(page->buffer);
> -			memcpy(p, addr + off, RX_COPY_MIN);
> +			memcpy(p, page_address(page->buffer) + off,
> +			       RX_COPY_MIN);
>  			dma_sync_single_for_device(&cp->pdev->dev,
>  						   page->dma_addr 
+ off, i,
>  						   
DMA_FROM_DEVICE);
> -			cas_page_unmap(addr);
>  			off += RX_COPY_MIN;
>  			swivel = RX_COPY_MIN;
>  			RX_USED_ADD(page, cp->mtu_stride);
> @@ -2036,10 +2033,8 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, RX_USED_ADD(page, hlen + cp->crc_size);
>  		}
> 
> -		if (cp->crc_size) {
> -			addr = cas_page_map(page->buffer);
> -			crcaddr  = addr + off + hlen;
> -		}
> +		if (cp->crc_size)
> +			crcaddr = page_address(page->buffer) + off + 
hlen;
> 
>  	} else {
>  		/* copying packet */
> @@ -2061,12 +2056,10 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, i += cp->crc_size;
>  		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr + 
off,
>  					i, DMA_FROM_DEVICE);
> -		addr = cas_page_map(page->buffer);
> -		memcpy(p, addr + off, i);
> +		memcpy(p, page_address(page->buffer) + off, i);
>  		dma_sync_single_for_device(&cp->pdev->dev,
>  					   page->dma_addr + off, i,
>  					   DMA_FROM_DEVICE);
> -		cas_page_unmap(addr);
>  		if (p == (char *) skb->data) /* not split */
>  			RX_USED_ADD(page, cp->mtu_stride);
>  		else
> @@ -2081,20 +2074,17 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, page->dma_addr,
>  						dlen + cp-
>crc_size,
>  						DMA_FROM_DEVICE);
> -			addr = cas_page_map(page->buffer);
> -			memcpy(p, addr, dlen + cp->crc_size);
> +			memcpy(p, page_address(page->buffer), dlen + cp-
>crc_size);
>  			dma_sync_single_for_device(&cp->pdev->dev,
>  						   page->dma_addr,
>  						   dlen + cp-
>crc_size,
>  						   
DMA_FROM_DEVICE);
> -			cas_page_unmap(addr);
>  			RX_USED_ADD(page, dlen + cp->crc_size);
>  		}
>  end_copy_pkt:
> -		if (cp->crc_size) {
> -			addr    = NULL;
> +		if (cp->crc_size)
>  			crcaddr = skb->data + alloclen;
> -		}
> +

This is a different logical change. Some maintainers I met would have asked  
for a separate patch, but I'm OK with it being here.

>  		skb_put(skb, alloclen);
>  	}
> 
> @@ -2103,8 +2093,6 @@ static int cas_rx_process_pkt(struct cas *cp, struct
> cas_rx_comp *rxc, /* checksum includes FCS. strip it out. */
>  		csum = csum_fold(csum_partial(crcaddr, cp->crc_size,
>  					      csum_unfold(csum)));
> -		if (addr)
> -			cas_page_unmap(addr);
>  	}
>  	skb->protocol = eth_type_trans(skb, cp->dev);
>  	if (skb->protocol == htons(ETH_P_IP)) {
> --
> 2.37.2

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

Re: [PATCH net-next 3/5] cassini: Remove unnecessary use of kmap_atomic()

[Anirudh Venkataramanan]

On 11/18/2022 12:35 AM, Fabio M. De Francesco wrote:
> On giovedì 17 novembre 2022 23:25:55 CET Anirudh Venkataramanan wrote:
>> Pages for Rx buffers are allocated in cas_page_alloc() using either
>> GFP_ATOMIC or GFP_KERNEL. Memory allocated with GFP_KERNEL/GFP_ATOMIC
>> can't come from highmem and so there's no need to kmap() them. Just use
>> page_address() instead.
>>
>> I don't have hardware, so this change has only been compile tested.
>>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>> ---
>>   drivers/net/ethernet/sun/cassini.c | 34 ++++++++++--------------------
>>   1 file changed, 11 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/cassini.c
>> b/drivers/net/ethernet/sun/cassini.c index 0aca193..2f66cfc 100644
>> --- a/drivers/net/ethernet/sun/cassini.c
>> +++ b/drivers/net/ethernet/sun/cassini.c
>> @@ -1915,7 +1915,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, int off, swivel = RX_SWIVEL_OFF_VAL;
>>   	struct cas_page *page;
>>   	struct sk_buff *skb;
>> -	void *addr, *crcaddr;
>> +	void *crcaddr;
>>   	__sum16 csum;
>>   	char *p;
>>
>> @@ -1936,7 +1936,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, skb_reserve(skb, swivel);
>>
>>   	p = skb->data;
>> -	addr = crcaddr = NULL;
>> +	crcaddr = NULL;
>>   	if (hlen) { /* always copy header pages */
>>   		i = CAS_VAL(RX_COMP2_HDR_INDEX, words[1]);
>>   		page = cp->rx_pages[CAS_VAL(RX_INDEX_RING, i)]
> [CAS_VAL(RX_INDEX_NUM, i)];
>> @@ -1948,12 +1948,10 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, i += cp->crc_size;
>>   		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr +
> off,
>>   					i, DMA_FROM_DEVICE);
>> -		addr = cas_page_map(page->buffer);
>> -		memcpy(p, addr + off, i);
>> +		memcpy(p, page_address(page->buffer) + off, i);
>>   		dma_sync_single_for_device(&cp->pdev->dev,
>>   					   page->dma_addr + off, i,
>>   					   DMA_FROM_DEVICE);
>> -		cas_page_unmap(addr);
>>   		RX_USED_ADD(page, 0x100);
>>   		p += hlen;
>>   		swivel = 0;
>> @@ -1984,12 +1982,11 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, /* make sure we always copy a header */
>>   		swivel = 0;
>>   		if (p == (char *) skb->data) { /* not split */
>> -			addr = cas_page_map(page->buffer);
>> -			memcpy(p, addr + off, RX_COPY_MIN);
>> +			memcpy(p, page_address(page->buffer) + off,
>> +			       RX_COPY_MIN);
>>   			dma_sync_single_for_device(&cp->pdev->dev,
>>   						   page->dma_addr
> + off, i,
>>   						
> DMA_FROM_DEVICE);
>> -			cas_page_unmap(addr);
>>   			off += RX_COPY_MIN;
>>   			swivel = RX_COPY_MIN;
>>   			RX_USED_ADD(page, cp->mtu_stride);
>> @@ -2036,10 +2033,8 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, RX_USED_ADD(page, hlen + cp->crc_size);
>>   		}
>>
>> -		if (cp->crc_size) {
>> -			addr = cas_page_map(page->buffer);
>> -			crcaddr  = addr + off + hlen;
>> -		}
>> +		if (cp->crc_size)
>> +			crcaddr = page_address(page->buffer) + off +
> hlen;
>>
>>   	} else {
>>   		/* copying packet */
>> @@ -2061,12 +2056,10 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, i += cp->crc_size;
>>   		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr +
> off,
>>   					i, DMA_FROM_DEVICE);
>> -		addr = cas_page_map(page->buffer);
>> -		memcpy(p, addr + off, i);
>> +		memcpy(p, page_address(page->buffer) + off, i);
>>   		dma_sync_single_for_device(&cp->pdev->dev,
>>   					   page->dma_addr + off, i,
>>   					   DMA_FROM_DEVICE);
>> -		cas_page_unmap(addr);
>>   		if (p == (char *) skb->data) /* not split */
>>   			RX_USED_ADD(page, cp->mtu_stride);
>>   		else
>> @@ -2081,20 +2074,17 @@ static int cas_rx_process_pkt(struct cas *cp, struct
>> cas_rx_comp *rxc, page->dma_addr,
>>   						dlen + cp-
>> crc_size,
>>   						DMA_FROM_DEVICE);
>> -			addr = cas_page_map(page->buffer);
>> -			memcpy(p, addr, dlen + cp->crc_size);
>> +			memcpy(p, page_address(page->buffer), dlen + cp-
>> crc_size);
>>   			dma_sync_single_for_device(&cp->pdev->dev,
>>   						   page->dma_addr,
>>   						   dlen + cp-
>> crc_size,
>>   						
> DMA_FROM_DEVICE);
>> -			cas_page_unmap(addr);
>>   			RX_USED_ADD(page, dlen + cp->crc_size);
>>   		}
>>   end_copy_pkt:
>> -		if (cp->crc_size) {
>> -			addr    = NULL;
>> +		if (cp->crc_size)
>>   			crcaddr = skb->data + alloclen;
>> -		}
>> +
> 
> This is a different logical change. Some maintainers I met would have asked
> for a separate patch, but I'm OK with it being here.

cas_page_map()/cap_page_unmap() were using addr. Once these went away 
addr became unnecessary. It would be weird to leave the declaration, 
init and reinit for a variable that's not of any use, and so it was 
removed as well. It's cleaner this way.

Ani

Re: [PATCH net-next 3/5] cassini: Remove unnecessary use of kmap_atomic()

[Fabio M. De Francesco]

On venerdì 18 novembre 2022 18:55:36 CET Anirudh Venkataramanan wrote:
> On 11/18/2022 12:35 AM, Fabio M. De Francesco wrote:
> > On giovedì 17 novembre 2022 23:25:55 CET Anirudh Venkataramanan wrote:
> >> Pages for Rx buffers are allocated in cas_page_alloc() using either
> >> GFP_ATOMIC or GFP_KERNEL. Memory allocated with GFP_KERNEL/GFP_ATOMIC
> >> can't come from highmem and so there's no need to kmap() them. Just use
> >> page_address() instead.
> >> 
> >> I don't have hardware, so this change has only been compile tested.
> >> 
> >> Cc: Ira Weiny <ira.weiny@intel.com>
> >> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> >> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> >> ---
> >> 
> >>   drivers/net/ethernet/sun/cassini.c | 34 ++++++++++--------------------
> >>   1 file changed, 11 insertions(+), 23 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/sun/cassini.c
> >> b/drivers/net/ethernet/sun/cassini.c index 0aca193..2f66cfc 100644
> >> --- a/drivers/net/ethernet/sun/cassini.c
> >> +++ b/drivers/net/ethernet/sun/cassini.c
> >> @@ -1915,7 +1915,7 @@ static int cas_rx_process_pkt(struct cas *cp, 
struct
> >> cas_rx_comp *rxc, int off, swivel = RX_SWIVEL_OFF_VAL;
> >> 
> >>   	struct cas_page *page;
> >>   	struct sk_buff *skb;
> >> 
> >> -	void *addr, *crcaddr;
> >> +	void *crcaddr;
> >> 
> >>   	__sum16 csum;
> >>   	char *p;
> >> 
> >> @@ -1936,7 +1936,7 @@ static int cas_rx_process_pkt(struct cas *cp, 
struct
> >> cas_rx_comp *rxc, skb_reserve(skb, swivel);
> >> 
> >>   	p = skb->data;
> >> 
> >> -	addr = crcaddr = NULL;
> >> +	crcaddr = NULL;
> >> 
> >>   	if (hlen) { /* always copy header pages */
> >>   	
> >>   		i = CAS_VAL(RX_COMP2_HDR_INDEX, words[1]);
> >>   		page = cp->rx_pages[CAS_VAL(RX_INDEX_RING, i)]
> > 
> > [CAS_VAL(RX_INDEX_NUM, i)];
> > 
> >> @@ -1948,12 +1948,10 @@ static int cas_rx_process_pkt(struct cas *cp,
> >> struct
> >> cas_rx_comp *rxc, i += cp->crc_size;
> >> 
> >>   		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr +
> > 
> > off,
> > 
> >>   					i, DMA_FROM_DEVICE);
> >> 
> >> -		addr = cas_page_map(page->buffer);
> >> -		memcpy(p, addr + off, i);
> >> +		memcpy(p, page_address(page->buffer) + off, i);
> >> 
> >>   		dma_sync_single_for_device(&cp->pdev->dev,
> >>   		
> >>   					   page->dma_addr + off, i,
> >>   					   DMA_FROM_DEVICE);
> >> 
> >> -		cas_page_unmap(addr);
> >> 
> >>   		RX_USED_ADD(page, 0x100);
> >>   		p += hlen;
> >>   		swivel = 0;
> >> 
> >> @@ -1984,12 +1982,11 @@ static int cas_rx_process_pkt(struct cas *cp,
> >> struct
> >> cas_rx_comp *rxc, /* make sure we always copy a header */
> >> 
> >>   		swivel = 0;
> >>   		if (p == (char *) skb->data) { /* not split */
> >> 
> >> -			addr = cas_page_map(page->buffer);
> >> -			memcpy(p, addr + off, RX_COPY_MIN);
> >> +			memcpy(p, page_address(page->buffer) + off,
> >> +			       RX_COPY_MIN);
> >> 
> >>   			dma_sync_single_for_device(&cp->pdev->dev,
> >>   			
> >>   						   page->dma_addr
> > 
> > + off, i,
> > 
> > DMA_FROM_DEVICE);
> > 
> >> -			cas_page_unmap(addr);
> >> 
> >>   			off += RX_COPY_MIN;
> >>   			swivel = RX_COPY_MIN;
> >>   			RX_USED_ADD(page, cp->mtu_stride);
> >> 
> >> @@ -2036,10 +2033,8 @@ static int cas_rx_process_pkt(struct cas *cp, 
struct
> >> cas_rx_comp *rxc, RX_USED_ADD(page, hlen + cp->crc_size);
> >> 
> >>   		}
> >> 
> >> -		if (cp->crc_size) {
> >> -			addr = cas_page_map(page->buffer);
> >> -			crcaddr  = addr + off + hlen;
> >> -		}
> >> +		if (cp->crc_size)
> >> +			crcaddr = page_address(page->buffer) + off +
> > 
> > hlen;
> > 
> >>   	} else {
> >>   	
> >>   		/* copying packet */
> >> 
> >> @@ -2061,12 +2056,10 @@ static int cas_rx_process_pkt(struct cas *cp,
> >> struct
> >> cas_rx_comp *rxc, i += cp->crc_size;
> >> 
> >>   		dma_sync_single_for_cpu(&cp->pdev->dev, page->dma_addr +
> > 
> > off,
> > 
> >>   					i, DMA_FROM_DEVICE);
> >> 
> >> -		addr = cas_page_map(page->buffer);
> >> -		memcpy(p, addr + off, i);
> >> +		memcpy(p, page_address(page->buffer) + off, i);
> >> 
> >>   		dma_sync_single_for_device(&cp->pdev->dev,
> >>   		
> >>   					   page->dma_addr + off, i,
> >>   					   DMA_FROM_DEVICE);
> >> 
> >> -		cas_page_unmap(addr);
> >> 
> >>   		if (p == (char *) skb->data) /* not split */
> >>   		
> >>   			RX_USED_ADD(page, cp->mtu_stride);
> >>   		
> >>   		else
> >> 
> >> @@ -2081,20 +2074,17 @@ static int cas_rx_process_pkt(struct cas *cp,
> >> struct
> >> cas_rx_comp *rxc, page->dma_addr,
> >> 
> >>   						dlen + cp-
> >> 
> >> crc_size,
> >> 
> >>   						DMA_FROM_DEVICE);
> >> 
> >> -			addr = cas_page_map(page->buffer);
> >> -			memcpy(p, addr, dlen + cp->crc_size);
> >> +			memcpy(p, page_address(page->buffer), dlen + cp-
> >> crc_size);
> >> 
> >>   			dma_sync_single_for_device(&cp->pdev->dev,
> >>   			
> >>   						   page->dma_addr,
> >>   						   dlen + cp-
> >> 
> >> crc_size,
> > 
> > DMA_FROM_DEVICE);
> > 
> >> -			cas_page_unmap(addr);
> >> 
> >>   			RX_USED_ADD(page, dlen + cp->crc_size);
> >>   		
> >>   		}
> >>   
> >>   end_copy_pkt:
> >> -		if (cp->crc_size) {
> >> -			addr    = NULL;
> >> +		if (cp->crc_size)
> >> 
> >>   			crcaddr = skb->data + alloclen;
> >> 
> >> -		}
> >> +
> > 
> > This is a different logical change. Some maintainers I met would have 
asked
> > for a separate patch, but I'm OK with it being here.
> 
> cas_page_map()/cap_page_unmap() were using addr. Once these went away
> addr became unnecessary. It would be weird to leave the declaration,
> init and reinit for a variable that's not of any use, and so it was
> removed as well. It's cleaner this way.
> 
> Ani

Oh, sorry. You are right here. I didn't notice this connection. I probably 
took too little time to read your code carefully and interpreted those two 
lines another way :-(

Thanks,

Fabio

[PATCH net-next 4/5] cassini: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

kmap_atomic() is being deprecated in favor of kmap_local_page().
Replace kmap_atomic() and kunmap_atomic() with kmap_local_page() and
kunmap_local() respectively. cas_page_map() and cas_page_unmap() aren't
really useful anymore, so get rid of these as well.

Note that kmap_atomic() disables preemption and page-fault processing,
but kmap_local_page() doesn't. Converting the former to the latter is safe
only if there isn't an implicit dependency on preemption and page-fault
handling being disabled, which does appear to be the case here.

Also note that the page being mapped is not allocated by the driver,
and so the driver doesn't know if the page is in normal memory. This is the
reason kmap_local_page() is used as opposed to page_address().

I don't have hardware, so this change has only been compile tested.

Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
 drivers/net/ethernet/sun/cassini.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sun/cassini.c b/drivers/net/ethernet/sun/cassini.c
index 2f66cfc..3e632b0 100644
--- a/drivers/net/ethernet/sun/cassini.c
+++ b/drivers/net/ethernet/sun/cassini.c
@@ -90,8 +90,6 @@
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
 
-#define cas_page_map(x)      kmap_atomic((x))
-#define cas_page_unmap(x)    kunmap_atomic((x))
 #define CAS_NCPUS            num_online_cpus()
 
 #define cas_skb_release(x)  netif_rx(x)
@@ -2788,11 +2786,11 @@ static inline int cas_xmit_tx_ringN(struct cas *cp, int ring,
 				      ctrl, 0);
 			entry = TX_DESC_NEXT(ring, entry);
 
-			addr = cas_page_map(skb_frag_page(fragp));
+			addr = kmap_local_page(skb_frag_page(fragp));
 			memcpy(tx_tiny_buf(cp, ring, entry),
 			       addr + skb_frag_off(fragp) + len - tabort,
 			       tabort);
-			cas_page_unmap(addr);
+			kunmap_local(addr);
 			mapping = tx_tiny_map(cp, ring, entry, tentry);
 			len     = tabort;
 		}
-- 
2.37.2

Re: [PATCH net-next 4/5] cassini: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On giovedì 17 novembre 2022 23:25:56 CET Anirudh Venkataramanan wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page().
> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page() and
> kunmap_local() respectively. cas_page_map() and cas_page_unmap() aren't
> really useful anymore, so get rid of these as well.
> 
> Note that kmap_atomic() disables preemption and page-fault processing,
> but kmap_local_page() doesn't. Converting the former to the latter is safe
> only if there isn't an implicit dependency on preemption and page-fault
> handling being disabled, which does appear to be the case here.

Same NIT: again, conversions would be possible with the addition of explicit 
call(s) for disable page faults and preemption. As I said, I have no problems 
with this inaccurate description. Please see 2/5, I don't think it should 
prevent the patch to be applied.

> 
> Also note that the page being mapped is not allocated by the driver,
> and so the driver doesn't know if the page is in normal memory. This is the
> reason kmap_local_page() is used as opposed to page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  drivers/net/ethernet/sun/cassini.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/cassini.c
> b/drivers/net/ethernet/sun/cassini.c index 2f66cfc..3e632b0 100644
> --- a/drivers/net/ethernet/sun/cassini.c
> +++ b/drivers/net/ethernet/sun/cassini.c
> @@ -90,8 +90,6 @@
>  #include <linux/uaccess.h>
>  #include <linux/jiffies.h>
> 
> -#define cas_page_map(x)      kmap_atomic((x))
> -#define cas_page_unmap(x)    kunmap_atomic((x))
>  #define CAS_NCPUS            num_online_cpus()
> 
>  #define cas_skb_release(x)  netif_rx(x)
> @@ -2788,11 +2786,11 @@ static inline int cas_xmit_tx_ringN(struct cas *cp,
> int ring, ctrl, 0);
>  			entry = TX_DESC_NEXT(ring, entry);
> 
> -			addr = cas_page_map(skb_frag_page(fragp));
> +			addr = kmap_local_page(skb_frag_page(fragp));
>  			memcpy(tx_tiny_buf(cp, ring, entry),
>  			       addr + skb_frag_off(fragp) + len - 
tabort,
>  			       tabort);
> -			cas_page_unmap(addr);
> +			kunmap_local(addr);

memcpy_from_page() would be better suited.
Please remember to use memcpy_{from,to}_page() where they are better suited.
However, they would not change the logic, so I'm OK with this change too.

>  			mapping = tx_tiny_map(cp, ring, entry, tentry);
>  			len     = tabort;
>  		}
> --
> 2.37.2

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Feel free to forward my tag, if maintainers require the use of the above-
mentioned helpers and ask for v2.

Thanks,

Fabio

[PATCH net-next 5/5] sunvnet: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

kmap_atomic() is being deprecated in favor of kmap_local_page().
Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
and kunmap_local() respectively.

Note that kmap_atomic() disables preemption and page-fault processing,
but kmap_local_page() doesn't. Converting the former to the latter is safe
only if there isn't an implicit dependency on preemption and page-fault
handling being disabled, which does appear to be the case here.

Also note that the page being mapped is not allocated by the driver, and so
the driver doesn't know if the page is in normal memory. This is the reason
kmap_local_page() is used as opposed to page_address().

I don't have hardware, so this change has only been compile tested.

Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
 drivers/net/ethernet/sun/sunvnet_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
index 80fde5f..a6211b9 100644
--- a/drivers/net/ethernet/sun/sunvnet_common.c
+++ b/drivers/net/ethernet/sun/sunvnet_common.c
@@ -1085,13 +1085,13 @@ static inline int vnet_skb_map(struct ldc_channel *lp, struct sk_buff *skb,
 		u8 *vaddr;
 
 		if (nc < ncookies) {
-			vaddr = kmap_atomic(skb_frag_page(f));
+			vaddr = kmap_local_page(skb_frag_page(f));
 			blen = skb_frag_size(f);
 			blen += 8 - (blen & 7);
 			err = ldc_map_single(lp, vaddr + skb_frag_off(f),
 					     blen, cookies + nc, ncookies - nc,
 					     map_perm);
-			kunmap_atomic(vaddr);
+			kunmap_local(vaddr);
 		} else {
 			err = -EMSGSIZE;
 		}
-- 
2.37.2

Re: [PATCH net-next 5/5] sunvnet: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On giovedì 17 novembre 2022 23:25:57 CET Anirudh Venkataramanan wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page().
> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> and kunmap_local() respectively.
> 
> Note that kmap_atomic() disables preemption and page-fault processing,
> but kmap_local_page() doesn't. Converting the former to the latter is safe
> only if there isn't an implicit dependency on preemption and page-fault
> handling being disabled, which does appear to be the case here.
> 
> Also note that the page being mapped is not allocated by the driver, and so
> the driver doesn't know if the page is in normal memory. This is the reason
> kmap_local_page() is used as opposed to page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> ---
>  drivers/net/ethernet/sun/sunvnet_common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c
> b/drivers/net/ethernet/sun/sunvnet_common.c index 80fde5f..a6211b9 100644
> --- a/drivers/net/ethernet/sun/sunvnet_common.c
> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
> @@ -1085,13 +1085,13 @@ static inline int vnet_skb_map(struct ldc_channel 
*lp,
> struct sk_buff *skb, u8 *vaddr;
> 
>  		if (nc < ncookies) {
> -			vaddr = kmap_atomic(skb_frag_page(f));
> +			vaddr = kmap_local_page(skb_frag_page(f));
>  			blen = skb_frag_size(f);
>  			blen += 8 - (blen & 7);
>  			err = ldc_map_single(lp, vaddr + 
skb_frag_off(f),
>  					     blen, cookies + nc, 
ncookies - nc,
>  					     map_perm);
> -			kunmap_atomic(vaddr);
> +			kunmap_local(vaddr);
>  		} else {
>  			err = -EMSGSIZE;
>  		}
> --
> 2.37.2

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

Re: [PATCH net-next 5/5] sunvnet: Use kmap_local_page() instead of kmap_atomic()

[Fabio M. De Francesco]

On venerdì 18 novembre 2022 10:11:12 CET Fabio M. De Francesco wrote:
> On giovedì 17 novembre 2022 23:25:57 CET Anirudh Venkataramanan wrote:
> > kmap_atomic() is being deprecated in favor of kmap_local_page().
> > Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
> > and kunmap_local() respectively.
> > 
> > Note that kmap_atomic() disables preemption and page-fault processing,
> > but kmap_local_page() doesn't. Converting the former to the latter is safe
> > only if there isn't an implicit dependency on preemption and page-fault
> > handling being disabled, which does appear to be the case here.
> > 
> > Also note that the page being mapped is not allocated by the driver, and 
so
> > the driver doesn't know if the page is in normal memory. This is the 
reason
> > kmap_local_page() is used as opposed to page_address().
> > 
> > I don't have hardware, so this change has only been compile tested.
> > 
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
> > ---
> > 
> >  drivers/net/ethernet/sun/sunvnet_common.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/sun/sunvnet_common.c
> > b/drivers/net/ethernet/sun/sunvnet_common.c index 80fde5f..a6211b9 100644
> > --- a/drivers/net/ethernet/sun/sunvnet_common.c
> > +++ b/drivers/net/ethernet/sun/sunvnet_common.c
> > @@ -1085,13 +1085,13 @@ static inline int vnet_skb_map(struct ldc_channel
> 
> *lp,
> 
> > struct sk_buff *skb, u8 *vaddr;
> > 
> >  		if (nc < ncookies) {
> > 
> > -			vaddr = kmap_atomic(skb_frag_page(f));
> > +			vaddr = kmap_local_page(skb_frag_page(f));
> > 
> >  			blen = skb_frag_size(f);
> >  			blen += 8 - (blen & 7);
> >  			err = ldc_map_single(lp, vaddr +
> 
> skb_frag_off(f),
> 
> >  					     blen, cookies + nc,
> 
> ncookies - nc,
> 
> >  					     map_perm);
> > 
> > -			kunmap_atomic(vaddr);
> > +			kunmap_local(vaddr);
> > 
> >  		} else {
> >  		
> >  			err = -EMSGSIZE;
> >  		
> >  		}
> > 
> > --
> > 2.37.2
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Thanks,
> 
> Fabio

Now that we are at 5/5 again. I'd like to point again to what worries me:

"Converting the former to the latter is safe only if there isn't an implicit 
dependency on preemption and page-fault handling being disabled, ...".

If I was able to convey my thoughts this is what you should get from my long 
email: 

"Converting the former to the latter is _always_ safe if there isn't an 
implicit dependency on preemption and page-fault handling being disabled and 
also when the above-mentioned implicit dependency is present, but in the 
latter case only if calling pagefault_disable() and/or preempt_disable() with 
kmap_local_page(). These disables are not required here because...".

As you demonstrated none of your nine patches need any explicit disable along 
with kmap_local_page().

Do my two emails make any sense to you?
However, your patches are good. If you decide to make them perfect use those 
helpers we've been talking about.

Again thanks,

Fabio


Fabio

Re: [PATCH net-next 5/5] sunvnet: Use kmap_local_page() instead of kmap_atomic()

[Anirudh Venkataramanan]

On 11/18/2022 12:45 PM, Fabio M. De Francesco wrote:
> On venerdì 18 novembre 2022 10:11:12 CET Fabio M. De Francesco wrote:
> 
> Now that we are at 5/5 again. I'd like to point again to what worries me:
> 
> "Converting the former to the latter is safe only if there isn't an implicit
> dependency on preemption and page-fault handling being disabled, ...".
> 
> If I was able to convey my thoughts this is what you should get from my long
> email:
> 
> "Converting the former to the latter is _always_ safe if there isn't an
> implicit dependency on preemption and page-fault handling being disabled and
> also when the above-mentioned implicit dependency is present, but in the
> latter case only if calling pagefault_disable() and/or preempt_disable() with
> kmap_local_page(). These disables are not required here because...".
> 
> As you demonstrated none of your nine patches need any explicit disable along
> with kmap_local_page().
> 
> Do my two emails make any sense to you?
> However, your patches are good. If you decide to make them perfect use those
> helpers we've been talking about.

Hi Fabio,

I'll be posting v2 next week. I've incorporated feedback from our 
discussion, and hopefully the things I've added make sense. If not, 
let's continue talking.

Ani

Re: [PATCH net-next 0/5] Remove uses of kmap_atomic()

[Leon Romanovsky]

On Thu, Nov 17, 2022 at 02:25:52PM -0800, Anirudh Venkataramanan wrote:
> kmap_atomic() is being deprecated. This little series replaces the last
> few uses of kmap_atomic() in ethernet drivers with either page_address()
> or kmap_local_page().
> 
> Anirudh Venkataramanan (5):
>   ch_ktls: Use kmap_local_page() instead of kmap_atomic()
>   sfc: Use kmap_local_page() instead of kmap_atomic()
>   cassini: Remove unnecessary use of kmap_atomic()
>   cassini: Use kmap_local_page() instead of kmap_atomic()
>   sunvnet: Use kmap_local_page() instead of kmap_atomic()
> 
>  .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 10 ++---
>  drivers/net/ethernet/sfc/tx.c                 |  4 +-


>  drivers/net/ethernet/sun/cassini.c            | 40 ++++++-------------
>  drivers/net/ethernet/sun/sunvnet_common.c     |  4 +-

Dave, Jakub, Paolo
I wonder if these drivers can be simply deleted.

Thanks

>  4 files changed, 22 insertions(+), 36 deletions(-)
> 
> 
> base-commit: b4b221bd79a1c698d9653e3ae2c3cb61cdc9aee7
> -- 
> 2.37.2
> 

Re: [PATCH net-next 0/5] Remove uses of kmap_atomic()

[Jakub Kicinski]

On Tue, 22 Nov 2022 13:29:03 +0200 Leon Romanovsky wrote:
> >  drivers/net/ethernet/sun/cassini.c            | 40 ++++++-------------
> >  drivers/net/ethernet/sun/sunvnet_common.c     |  4 +-  
> 
> Dave, Jakub, Paolo
> I wonder if these drivers can be simply deleted.

My thought as well. It's just a matter of digging thru the history,
platform code and the web to find potential users and contacting them. 

Re: [PATCH net-next 0/5] Remove uses of kmap_atomic()

[Anirudh Venkataramanan]

On 11/22/2022 10:50 AM, Jakub Kicinski wrote:
> On Tue, 22 Nov 2022 13:29:03 +0200 Leon Romanovsky wrote:
>>>   drivers/net/ethernet/sun/cassini.c            | 40 ++++++-------------
>>>   drivers/net/ethernet/sun/sunvnet_common.c     |  4 +-
>>
>> Dave, Jakub, Paolo
>> I wonder if these drivers can be simply deleted.
> 
> My thought as well. It's just a matter of digging thru the history,
> platform code and the web to find potential users and contacting them.

I did a little bit of digging on these two files. Here's what I found.

For the cassini driver, I don't see any recent patches that fix an end 
user visible issue. There are clean ups, updates to use newer kernel 
APIs, and some build/memory leak fixes. I checked as far back as 2011. 
There are web references to some issues in kernel v2.6. I didn't see 
anything more recent.

The code in sunvnet_common.c seems to be common code that's used by

[1] "Sun4v LDOM Virtual Switch Driver" (ldmvsw.c, kconfig flag 
CONFIG_LDMVSW)

[2] "Sun LDOM virtual network driver" (sunvnet.c, kconfig flag 
CONFIG_SUNVNET).

These two seem to have had some feature updates around 2017, but 
otherwise the situation is the same as cassini.

Ani

Re: [PATCH net-next 0/5] Remove uses of kmap_atomic()

[Leon Romanovsky]

On Tue, Nov 22, 2022 at 01:06:09PM -0800, Anirudh Venkataramanan wrote:
> On 11/22/2022 10:50 AM, Jakub Kicinski wrote:
> > On Tue, 22 Nov 2022 13:29:03 +0200 Leon Romanovsky wrote:
> > > >   drivers/net/ethernet/sun/cassini.c            | 40 ++++++-------------
> > > >   drivers/net/ethernet/sun/sunvnet_common.c     |  4 +-
> > > 
> > > Dave, Jakub, Paolo
> > > I wonder if these drivers can be simply deleted.
> > 
> > My thought as well. It's just a matter of digging thru the history,
> > platform code and the web to find potential users and contacting them.
> 
> I did a little bit of digging on these two files. Here's what I found.
> 
> For the cassini driver, I don't see any recent patches that fix an end user
> visible issue. There are clean ups, updates to use newer kernel APIs, and
> some build/memory leak fixes. I checked as far back as 2011. There are web
> references to some issues in kernel v2.6. I didn't see anything more recent.
> 
> The code in sunvnet_common.c seems to be common code that's used by
> 
> [1] "Sun4v LDOM Virtual Switch Driver" (ldmvsw.c, kconfig flag
> CONFIG_LDMVSW)
> 
> [2] "Sun LDOM virtual network driver" (sunvnet.c, kconfig flag
> CONFIG_SUNVNET).
> 
> These two seem to have had some feature updates around 2017, but otherwise
> the situation is the same as cassini.

If there is a pole to delete them, I vote for deletion. :)

Thanks

> 
> Ani

Re: [PATCH net-next 0/5] Remove uses of kmap_atomic()

[Anirudh Venkataramanan]

On 11/22/2022 11:34 PM, Leon Romanovsky wrote:
> On Tue, Nov 22, 2022 at 01:06:09PM -0800, Anirudh Venkataramanan wrote:
>> On 11/22/2022 10:50 AM, Jakub Kicinski wrote:
>>> On Tue, 22 Nov 2022 13:29:03 +0200 Leon Romanovsky wrote:
>>>>>    drivers/net/ethernet/sun/cassini.c            | 40 ++++++-------------
>>>>>    drivers/net/ethernet/sun/sunvnet_common.c     |  4 +-
>>>>
>>>> Dave, Jakub, Paolo
>>>> I wonder if these drivers can be simply deleted.
>>>
>>> My thought as well. It's just a matter of digging thru the history,
>>> platform code and the web to find potential users and contacting them.
>>
>> I did a little bit of digging on these two files. Here's what I found.
>>
>> For the cassini driver, I don't see any recent patches that fix an end user
>> visible issue. There are clean ups, updates to use newer kernel APIs, and
>> some build/memory leak fixes. I checked as far back as 2011. There are web
>> references to some issues in kernel v2.6. I didn't see anything more recent.
>>
>> The code in sunvnet_common.c seems to be common code that's used by
>>
>> [1] "Sun4v LDOM Virtual Switch Driver" (ldmvsw.c, kconfig flag
>> CONFIG_LDMVSW)
>>
>> [2] "Sun LDOM virtual network driver" (sunvnet.c, kconfig flag
>> CONFIG_SUNVNET).
>>
>> These two seem to have had some feature updates around 2017, but otherwise
>> the situation is the same as cassini.
> 
> If there is a pole to delete them, I vote for deletion. :)

Perhaps I should put out a patchset that does this. That would give 
users or anyone who cares another opportunity to chime in. If we hear 
nothing, then we're good.

I am thinking I'll still include the conversions for cassini.c and 
sunvnet_common.c in my v2 series. In case we don't end up deleting these 
drivers for some reason, they'd at least not be using kmap_atomic() anymore.

Ani