From: Bjorn Helgaas <bhelgaas@google.com>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 9 +++++----
drivers/pci/quirks.c | 2 +-
include/uapi/linux/pci_regs.h | 1 +
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..6e551f34ec63 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -8,6 +8,7 @@
#define dev_fmt(fmt) "DPC: " fmt
+#include <linux/bitfield.h>
#include <linux/aer.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
@@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
/* Get First Error Pointer */
pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
- first_error = (dpc_status & 0x1f00) >> 8;
+ first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
if ((status & ~mask) & (1 << i))
@@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
status, source);
- reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
- ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
+ reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
+ ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
pci_warn(pdev, "%s detected\n",
(reason == 0) ? "unmasked uncorrectable error" :
(reason == 1) ? "ERR_NONFATAL" :
@@ -338,7 +339,7 @@ void pci_dpc_init(struct pci_dev *pdev)
/* Quirks may set dpc_rp_log_size if device or firmware is buggy */
if (!pdev->dpc_rp_log_size) {
pdev->dpc_rp_log_size =
- (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+ FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
pci_err(pdev, "RP PIO log size %u is invalid\n",
pdev->dpc_rp_log_size);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..a9fdc2e3f110 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
return;
- if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
+ if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
pci_info(dev, "Overriding RP PIO Log Size to 4\n");
dev->dpc_rp_log_size = 4;
}
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 833e5fb40ea5..e97a06b50f95 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1046,6 +1046,7 @@
#define PCI_EXP_DPC_STATUS_INTERRUPT 0x0008 /* Interrupt Status */
#define PCI_EXP_DPC_RP_BUSY 0x0010 /* Root Port Busy */
#define PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
+#define PCI_EXP_DPC_STATUS_FIRST_ERR 0x1f00 /* RP PIO First Error Ptr */
#define PCI_EXP_DPC_SOURCE_ID 0x0A /* DPC Source Identifier */
--
2.34.1On Tue, 10 Oct 2023 15:44:32 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
A question about what 'rules' you are applying for using these macros
vs choosing not not do so. Personally I prefer using them even for
flag fields mostly because it makes the code more consistent and
the compiler should remove any unnecessary shifts that result.
> ---
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..a9fdc2e3f110 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
> if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
This is what I'm commenting on below.
> return;
>
> - if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> + if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
Why do this one and not the one just above?
In both cases extracting a field then comparing it to 0, I'm not sure
it makes sense to care if that field is 1 bit or multiple bit.
> pci_info(dev, "Overriding RP PIO Log Size to 4\n");
> dev->dpc_rp_log_size = 4;
> }
On Wed, 11 Oct 2023, Jonathan Cameron wrote:
> On Tue, 10 Oct 2023 15:44:32 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> A question about what 'rules' you are applying for using these macros
> vs choosing not not do so. Personally I prefer using them even for
> flag fields mostly because it makes the code more consistent and
> the compiler should remove any unnecessary shifts that result.
>
> > ---
>
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index eeec1d6f9023..a9fdc2e3f110 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
> > if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
>
> This is what I'm commenting on below.
>
> > return;
> >
> > - if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> > + if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
>
> Why do this one and not the one just above?
> In both cases extracting a field then comparing it to 0, I'm not sure
> it makes sense to care if that field is 1 bit or multiple bit.
I cannot speak for Bjorn but at least I've left flag checks untouched
(but when pulling the flag into a variable, I've made it with FIELD_GET()).
In anycase, that seems minor issue though compared with defined values of
the field being incompatible with the FIELD_GET()ed value (when the shift
is non-zero). I wish there would be good solution to that but so far I've
not come up anything that would be short and simple enough.
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/pcie/dpc.c | 9 +++++----
> drivers/pci/quirks.c | 2 +-
> include/uapi/linux/pci_regs.h | 1 +
> 3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 3ceed8e3de41..6e551f34ec63 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -8,6 +8,7 @@
>
> #define dev_fmt(fmt) "DPC: " fmt
>
> +#include <linux/bitfield.h>
> #include <linux/aer.h>
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
>
> /* Get First Error Pointer */
> pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> - first_error = (dpc_status & 0x1f00) >> 8;
> + first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
>
> for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> if ((status & ~mask) & (1 << i))
> @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> status, source);
>
> - reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> - ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> + reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> pci_warn(pdev, "%s detected\n",
> (reason == 0) ? "unmasked uncorrectable error" :
> (reason == 1) ? "ERR_NONFATAL" :
BTW, it seems we're doing overlapping work here with many of these
patches. It takes some time to think these through one by one, I don't
just autorun through them with coccinelle so I've not posted my changes
yet.
I went to a different direction here and named all the reasons too with
defines and used & to get the reason in order to be able to compare with
the named reasons.
You also missed convering one 0xfff4 to use define (although I suspect it
never was your goal to go beyond FIELD_GET() here).
> @@ -338,7 +339,7 @@ void pci_dpc_init(struct pci_dev *pdev)
> /* Quirks may set dpc_rp_log_size if device or firmware is buggy */
> if (!pdev->dpc_rp_log_size) {
> pdev->dpc_rp_log_size =
> - (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
> + FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
> if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
> pci_err(pdev, "RP PIO log size %u is invalid\n",
> pdev->dpc_rp_log_size);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..a9fdc2e3f110 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
> if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
> return;
>
> - if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> + if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
> pci_info(dev, "Overriding RP PIO Log Size to 4\n");
> dev->dpc_rp_log_size = 4;
> }
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 833e5fb40ea5..e97a06b50f95 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1046,6 +1046,7 @@
> #define PCI_EXP_DPC_STATUS_INTERRUPT 0x0008 /* Interrupt Status */
> #define PCI_EXP_DPC_RP_BUSY 0x0010 /* Root Port Busy */
> #define PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
> +#define PCI_EXP_DPC_STATUS_FIRST_ERR 0x1f00 /* RP PIO First Error Ptr */
If you prefer consistency, there already FEP used for "First
Error Pointer" used in another define.
> #define PCI_EXP_DPC_SOURCE_ID 0x0A /* DPC Source Identifier */
--
i.
On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> > drivers/pci/pcie/dpc.c | 9 +++++----
> > drivers/pci/quirks.c | 2 +-
> > include/uapi/linux/pci_regs.h | 1 +
> > 3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3ceed8e3de41..6e551f34ec63 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -8,6 +8,7 @@
> >
> > #define dev_fmt(fmt) "DPC: " fmt
> >
> > +#include <linux/bitfield.h>
> > #include <linux/aer.h>
> > #include <linux/delay.h>
> > #include <linux/interrupt.h>
> > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> >
> > /* Get First Error Pointer */
> > pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > - first_error = (dpc_status & 0x1f00) >> 8;
> > + first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> >
> > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > if ((status & ~mask) & (1 << i))
> > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > status, source);
> >
> > - reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > - ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > + reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > pci_warn(pdev, "%s detected\n",
> > (reason == 0) ? "unmasked uncorrectable error" :
> > (reason == 1) ? "ERR_NONFATAL" :
>
> BTW, it seems we're doing overlapping work here with many of these
> patches. It takes some time to think these through one by one, I don't
> just autorun through them with coccinelle so I've not posted my changes
> yet.
>
> I went to a different direction here and named all the reasons too with
> defines and used & to get the reason in order to be able to compare with
> the named reasons.
>
> You also missed convering one 0xfff4 to use define (although I suspect it
> never was your goal to go beyond FIELD_GET() here).
Pure FIELD_GET() and FIELD_PREP() was my goal.
If you have patches you prefer, I'll drop mine. I did these about a
year ago and it seemed like the time to do something with them since
you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
work. Since we've started, I'd like to get as much of it done this
cycle as possible.
Bjorn
On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
> On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > >
> > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > shift value. No functional change intended.
> > >
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > > drivers/pci/pcie/dpc.c | 9 +++++----
> > > drivers/pci/quirks.c | 2 +-
> > > include/uapi/linux/pci_regs.h | 1 +
> > > 3 files changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > index 3ceed8e3de41..6e551f34ec63 100644
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -8,6 +8,7 @@
> > >
> > > #define dev_fmt(fmt) "DPC: " fmt
> > >
> > > +#include <linux/bitfield.h>
> > > #include <linux/aer.h>
> > > #include <linux/delay.h>
> > > #include <linux/interrupt.h>
> > > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > >
> > > /* Get First Error Pointer */
> > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > > - first_error = (dpc_status & 0x1f00) >> 8;
> > > + first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> > >
> > > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > if ((status & ~mask) & (1 << i))
> > > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > > pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > > status, source);
> > >
> > > - reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > > - ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > > + reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > pci_warn(pdev, "%s detected\n",
> > > (reason == 0) ? "unmasked uncorrectable error" :
> > > (reason == 1) ? "ERR_NONFATAL" :
> >
> > BTW, it seems we're doing overlapping work here with many of these
> > patches. It takes some time to think these through one by one, I don't
> > just autorun through them with coccinelle so I've not posted my changes
> > yet.
> >
> > I went to a different direction here and named all the reasons too with
> > defines and used & to get the reason in order to be able to compare with
> > the named reasons.
> >
> > You also missed convering one 0xfff4 to use define (although I suspect it
> > never was your goal to go beyond FIELD_GET() here).
>
> Pure FIELD_GET() and FIELD_PREP() was my goal.
>
> If you have patches you prefer, I'll drop mine. I did these about a
> year ago and it seemed like the time to do something with them since
> you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> work. Since we've started, I'd like to get as much of it done this
> cycle as possible.
Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting
more and more complicated. I can build a nice set of small changes about
what remains to do in DPC on top of your patch.
--
i.
On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
> On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
>
> > On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > >
> > > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > > shift value. No functional change intended.
> > > >
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > > drivers/pci/pcie/dpc.c | 9 +++++----
> > > > drivers/pci/quirks.c | 2 +-
> > > > include/uapi/linux/pci_regs.h | 1 +
> > > > 3 files changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > index 3ceed8e3de41..6e551f34ec63 100644
> > > > --- a/drivers/pci/pcie/dpc.c
> > > > +++ b/drivers/pci/pcie/dpc.c
> > > > @@ -8,6 +8,7 @@
> > > >
> > > > #define dev_fmt(fmt) "DPC: " fmt
> > > >
> > > > +#include <linux/bitfield.h>
> > > > #include <linux/aer.h>
> > > > #include <linux/delay.h>
> > > > #include <linux/interrupt.h>
> > > > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > > >
> > > > /* Get First Error Pointer */
> > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > > > - first_error = (dpc_status & 0x1f00) >> 8;
> > > > + first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > > if ((status & ~mask) & (1 << i))
> > > > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > > > pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > > > status, source);
> > > >
> > > > - reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > > > - ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > > > + reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > > pci_warn(pdev, "%s detected\n",
> > > > (reason == 0) ? "unmasked uncorrectable error" :
> > > > (reason == 1) ? "ERR_NONFATAL" :
> > >
> > > BTW, it seems we're doing overlapping work here with many of these
> > > patches. It takes some time to think these through one by one, I don't
> > > just autorun through them with coccinelle so I've not posted my changes
> > > yet.
> > >
> > > I went to a different direction here and named all the reasons too with
> > > defines and used & to get the reason in order to be able to compare with
> > > the named reasons.
> > >
> > > You also missed convering one 0xfff4 to use define (although I suspect it
> > > never was your goal to go beyond FIELD_GET() here).
> >
> > Pure FIELD_GET() and FIELD_PREP() was my goal.
> >
> > If you have patches you prefer, I'll drop mine. I did these about a
> > year ago and it seemed like the time to do something with them since
> > you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> > work. Since we've started, I'd like to get as much of it done this
> > cycle as possible.
>
> Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting
> more and more complicated. I can build a nice set of small changes about
> what remains to do in DPC on top of your patch.
Err, actually, there's still the naming of the define, should _FEP be used
for First Error Pointer for consistency? You should make that small change
into your patch if you think _FEP is better because of consistency.
--
i.
On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
> On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
>
> > On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
> >
> > > On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > > > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > >
> > > > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > > > shift value. No functional change intended.
> > > > >
> > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > ---
> > > > > drivers/pci/pcie/dpc.c | 9 +++++----
> > > > > drivers/pci/quirks.c | 2 +-
> > > > > include/uapi/linux/pci_regs.h | 1 +
> > > > > 3 files changed, 7 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > index 3ceed8e3de41..6e551f34ec63 100644
> > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > @@ -8,6 +8,7 @@
> > > > >
> > > > > #define dev_fmt(fmt) "DPC: " fmt
> > > > >
> > > > > +#include <linux/bitfield.h>
> > > > > #include <linux/aer.h>
> > > > > #include <linux/delay.h>
> > > > > #include <linux/interrupt.h>
> > > > > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > > > >
> > > > > /* Get First Error Pointer */
> > > > > pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > > > > - first_error = (dpc_status & 0x1f00) >> 8;
> > > > > + first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> > > > >
> > > > > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > > > if ((status & ~mask) & (1 << i))
> > > > > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > > > > pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > > > > status, source);
> > > > >
> > > > > - reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > > > > - ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > > > > + reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > > > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > > > pci_warn(pdev, "%s detected\n",
> > > > > (reason == 0) ? "unmasked uncorrectable error" :
> > > > > (reason == 1) ? "ERR_NONFATAL" :
> > > >
> > > > BTW, it seems we're doing overlapping work here with many of these
> > > > patches. It takes some time to think these through one by one, I don't
> > > > just autorun through them with coccinelle so I've not posted my changes
> > > > yet.
> > > >
> > > > I went to a different direction here and named all the reasons too with
> > > > defines and used & to get the reason in order to be able to compare with
> > > > the named reasons.
> > > >
> > > > You also missed convering one 0xfff4 to use define (although I suspect it
> > > > never was your goal to go beyond FIELD_GET() here).
> > >
> > > Pure FIELD_GET() and FIELD_PREP() was my goal.
> > >
> > > If you have patches you prefer, I'll drop mine. I did these about a
> > > year ago and it seemed like the time to do something with them since
> > > you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> > > work. Since we've started, I'd like to get as much of it done this
> > > cycle as possible.
> >
> > Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting
> > more and more complicated. I can build a nice set of small changes about
> > what remains to do in DPC on top of your patch.
>
> Err, actually, there's still the naming of the define, should _FEP be used
> for First Error Pointer for consistency? You should make that small change
> into your patch if you think _FEP is better because of consistency.
There's also #include order so it seems you should just drop the patch, I
can handle this along my series.
--
i.
On Wed, 11 Oct 2023, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
>
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> > drivers/pci/pcie/dpc.c | 9 +++++----
> > drivers/pci/quirks.c | 2 +-
> > include/uapi/linux/pci_regs.h | 1 +
> > 3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3ceed8e3de41..6e551f34ec63 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -8,6 +8,7 @@
> >
> > #define dev_fmt(fmt) "DPC: " fmt
> >
> > +#include <linux/bitfield.h>
> > #include <linux/aer.h>
> > #include <linux/delay.h>
> > #include <linux/interrupt.h>
> > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> >
> > /* Get First Error Pointer */
> > pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > - first_error = (dpc_status & 0x1f00) >> 8;
> > + first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> >
> > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > if ((status & ~mask) & (1 << i))
> > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > status, source);
> >
> > - reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > - ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > + reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > pci_warn(pdev, "%s detected\n",
> > (reason == 0) ? "unmasked uncorrectable error" :
> > (reason == 1) ? "ERR_NONFATAL" :
>
> BTW, it seems we're doing overlapping work here with many of these
> patches. It takes some time to think these through one by one, I don't
> just autorun through them with coccinelle so I've not posted my changes
> yet.
>
> I went to a different direction here and named all the reasons too with
> defines and used & to get the reason in order to be able to compare with
> the named reasons.
>
> You also missed convering one 0xfff4 to use define (although I suspect it
> never was your goal to go beyond FIELD_GET() here).
I posted my approach onto the list so you can see what it looks like:
https://lore.kernel.org/linux-pci/20231013112004.4239-1-ilpo.jarvinen@linux.intel.com/T/#t
(It will obviously conflict with this patch so both cannot be applied as
is).
--
i.
© 2016 - 2026 Red Hat, Inc.