| Message ID | 20220928192730.809441-1-anirudh.venkataramanan@intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [net-next] ixgbe: Remove unnecessary use of kmap() | expand |
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; > > 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;
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
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; 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,
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(-)