Re: [Intel-wired-lan] [PATCH net-next] ixgbe: Remove unnecessary use of kmap()

[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>]

On 9/28/2022 1:40 PM, Alexander Duyck wrote:
> On Wed, Sep 28, 2022 at 12:26 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> wrote:
>>
>> Pages for Rx are allocated in ixgbe_alloc_mapped_page() using
>> dev_alloc_pages(), which does the allocation using flags GFP_ATOMIC |
>> __GFP_NOWARN. Pages allocated thus can't come from highmem, so remove
>> calls to kmap() and kunmap().
>>
>> While here, also remove the local variable "match", and just return
>> true/false.
>>
>> I wasn't able to get a hold of a system with an ixgbe NIC to test this
>> change. This is a pretty trivial change and I don't expect anything to
>> break, but a "Tested-by" from someone who has the hardware would be nice.
>>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> index 04f453e..90c0e0b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> @@ -1960,20 +1960,17 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
>>                                       unsigned int frame_size)
>>   {
>>          unsigned char *data;
>> -       bool match = true;
>>
>>          frame_size >>= 1;
>>
>> -       data = kmap(rx_buffer->page) + rx_buffer->page_offset;
>> +       data = page_address(rx_buffer->page) + rx_buffer->page_offset;

Okay, Fabio has already made this exact change and it's currently in 
Tony's kernel tree (next-queue:dev-queue). I missed it because I was 
barking up the wrong (kernel) tree. My apologies for the thrash.

>>
>>          if (data[3] != 0xFF ||
>>              data[frame_size + 10] != 0xBE ||
>>              data[frame_size + 12] != 0xAF)
>> -               match = false;
>> -
>> -       kunmap(rx_buffer->page);
>> +               return false;
>>
>> -       return match;
>> +       return true;
>>   }
>>
>>   static u16 ixgbe_clean_test_rings(struct ixgbe_ring *rx_ring,
> 
> Rather than bothering with the return true/false why not just return
> the compare value? So it would be:
>          return data[3] == 0xFF && data[frame_size + 10] == 0xBE &&
> data[frame_size + 12] == 0xAF;

Sure, I can do a standalone patch for this.

Ani


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan