All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: linux-pci@vger.kernel.org,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	linux-kernel@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
	netdev@vger.kernel.org, Tony Nguyen <anthony.l.nguyen@intel.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Paolo Abeni <pabeni@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH 3/8] igb: Use FIELD_GET() to extract Link Width
Date: Tue, 12 Sep 2023 11:34:03 +0100	[thread overview]
Message-ID: <20230912113403.00006c39@Huawei.com> (raw)
In-Reply-To: <20230911121501.21910-4-ilpo.jarvinen@linux.intel.com>

On Mon, 11 Sep 2023 15:14:56 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Use FIELD_GET() to extract PCIe Negotiated Link Width field instead of
> custom masking and shifting.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_mac.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/e1000_mac.c b/drivers/net/ethernet/intel/igb/e1000_mac.c
> index caf91c6f52b4..5a23b9cfec6c 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_mac.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_mac.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright(c) 2007 - 2018 Intel Corporation. */
>  
> +#include <linux/bitfield.h>
>  #include <linux/if_ether.h>
>  #include <linux/delay.h>
>  #include <linux/pci.h>
> @@ -50,9 +51,8 @@ s32 igb_get_bus_info_pcie(struct e1000_hw *hw)
>  			break;
>  		}
>  
> -		bus->width = (enum e1000_bus_width)((pcie_link_status &
> -						     PCI_EXP_LNKSTA_NLW) >>
> -						     PCI_EXP_LNKSTA_NLW_SHIFT);
> +		bus->width = (enum e1000_bus_width)FIELD_GET(PCI_EXP_LNKSTA_NLW,
> +							     pcie_link_status);

This cast is a bit ugly given it takes the values 0, 1, 2, 3 and
we extra a field that the spec says contains 1, 2, 4, 8 etc
Hence it only works because only 1 and 2 are used I think...  Not nice.


Also, whilst looking at this I note that e1000e has it's own defines
for PCIE_LINK_WIDTH_MASK and PCIE_LINK_WIDTH_SHIFT 

Looks like those should be changed to use the standard defines.

For extra giggles there are two e1000_bus_width enum definitions in different
headers.

Actual patch is good - just 'interesting' stuff noticed whilst looking at it :)

Jonathan


>  	}
>  
>  	reg = rd32(E1000_STATUS);

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

WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: <linux-pci@vger.kernel.org>, Bjorn Helgaas <helgaas@kernel.org>,
	"Jesse Brandeburg" <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	<intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/8] igb: Use FIELD_GET() to extract Link Width
Date: Tue, 12 Sep 2023 11:34:03 +0100	[thread overview]
Message-ID: <20230912113403.00006c39@Huawei.com> (raw)
In-Reply-To: <20230911121501.21910-4-ilpo.jarvinen@linux.intel.com>

On Mon, 11 Sep 2023 15:14:56 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Use FIELD_GET() to extract PCIe Negotiated Link Width field instead of
> custom masking and shifting.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_mac.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/e1000_mac.c b/drivers/net/ethernet/intel/igb/e1000_mac.c
> index caf91c6f52b4..5a23b9cfec6c 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_mac.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_mac.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright(c) 2007 - 2018 Intel Corporation. */
>  
> +#include <linux/bitfield.h>
>  #include <linux/if_ether.h>
>  #include <linux/delay.h>
>  #include <linux/pci.h>
> @@ -50,9 +51,8 @@ s32 igb_get_bus_info_pcie(struct e1000_hw *hw)
>  			break;
>  		}
>  
> -		bus->width = (enum e1000_bus_width)((pcie_link_status &
> -						     PCI_EXP_LNKSTA_NLW) >>
> -						     PCI_EXP_LNKSTA_NLW_SHIFT);
> +		bus->width = (enum e1000_bus_width)FIELD_GET(PCI_EXP_LNKSTA_NLW,
> +							     pcie_link_status);

This cast is a bit ugly given it takes the values 0, 1, 2, 3 and
we extra a field that the spec says contains 1, 2, 4, 8 etc
Hence it only works because only 1 and 2 are used I think...  Not nice.


Also, whilst looking at this I note that e1000e has it's own defines
for PCIE_LINK_WIDTH_MASK and PCIE_LINK_WIDTH_SHIFT 

Looks like those should be changed to use the standard defines.

For extra giggles there are two e1000_bus_width enum definitions in different
headers.

Actual patch is good - just 'interesting' stuff noticed whilst looking at it :)

Jonathan


>  	}
>  
>  	reg = rd32(E1000_STATUS);


  reply	other threads:[~2023-09-12 15:08 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-11 12:14 [PATCH 0/8] PCI/treewide: PCIe Link Width field access cleanup Ilpo Järvinen
2023-09-11 12:14 ` [PATCH 1/8] IB/hfi1: Use FIELD_GET() to extract Link Width Ilpo Järvinen
2023-09-12 10:26   ` Jonathan Cameron
2023-09-11 12:14 ` [PATCH 2/8] media: cobalt: " Ilpo Järvinen
2023-09-11 12:14 ` [Intel-wired-lan] [PATCH 3/8] igb: " Ilpo Järvinen
2023-09-11 12:14   ` Ilpo Järvinen
2023-09-12 10:34   ` Jonathan Cameron [this message]
2023-09-12 10:34     ` Jonathan Cameron
2023-09-12 12:11     ` [Intel-wired-lan] " Ilpo Järvinen
2023-09-12 12:11       ` Ilpo Järvinen
2023-09-11 12:14 ` [PATCH 4/8] PCI: tegra194: Use FIELD_GET()/FIELD_PREP() with Link Width fields Ilpo Järvinen
2023-09-12 10:35   ` Jonathan Cameron
2023-09-11 12:14 ` [PATCH 5/8] PCI: mvebu: Use FIELD_PREP() with Link Width Ilpo Järvinen
2023-09-11 12:14   ` Ilpo Järvinen
2023-09-11 12:14 ` [PATCH 6/8] PCI: Use FIELD_GET() to extract " Ilpo Järvinen
2023-09-11 12:15 ` [PATCH 7/8] scsi: esas2r: " Ilpo Järvinen
2023-09-12 10:38   ` Jonathan Cameron
2023-09-11 12:15 ` [PATCH 8/8] scsi: qla2xxx: " Ilpo Järvinen
2023-09-12 10:39   ` Jonathan Cameron
2023-09-12 10:24 ` [PATCH 0/8] PCI/treewide: PCIe Link Width field access cleanup Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230912113403.00006c39@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=anthony.l.nguyen@intel.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=helgaas@kernel.org \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=jesse.brandeburg@intel.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.