[PATCH 1/3] PCI: iproc: Add bus number parameter to read/write functions

[PATCH 1/3] PCI: iproc: Add bus number parameter to read/write functions

[Mark Tomlinson]

This makes the read/write functions more generic, allowing them to be
used from other places.

Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/pci/controller/pcie-iproc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index 8c7f875acf7f..2c836eede42c 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -660,13 +660,13 @@ static void __iomem *iproc_pcie_bus_map_cfg_bus(struct pci_bus *bus,
 				      where);
 }
 
-static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
+static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie, int busno,
 				       unsigned int devfn, int where,
 				       int size, u32 *val)
 {
 	void __iomem *addr;
 
-	addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
+	addr = iproc_pcie_map_cfg_bus(pcie, busno, devfn, where & ~0x3);
 	if (!addr) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -680,14 +680,14 @@ static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static int iproc_pci_raw_config_write32(struct iproc_pcie *pcie,
+static int iproc_pci_raw_config_write32(struct iproc_pcie *pcie, int busno,
 					unsigned int devfn, int where,
 					int size, u32 val)
 {
 	void __iomem *addr;
 	u32 mask, tmp;
 
-	addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
+	addr = iproc_pcie_map_cfg_bus(pcie, busno, devfn, where & ~0x3);
 	if (!addr)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
@@ -793,7 +793,7 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 	}
 
 	/* make sure we are not in EP mode */
-	iproc_pci_raw_config_read32(pcie, 0, PCI_HEADER_TYPE, 1, &hdr_type);
+	iproc_pci_raw_config_read32(pcie, 0, 0, PCI_HEADER_TYPE, 1, &hdr_type);
 	if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
 		dev_err(dev, "in EP mode, hdr=%#02x\n", hdr_type);
 		return -EFAULT;
@@ -803,15 +803,16 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 #define PCI_BRIDGE_CTRL_REG_OFFSET	0x43c
 #define PCI_CLASS_BRIDGE_MASK		0xffff00
 #define PCI_CLASS_BRIDGE_SHIFT		8
-	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
+	iproc_pci_raw_config_read32(pcie, 0, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				    4, &class);
 	class &= ~PCI_CLASS_BRIDGE_MASK;
 	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
-	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
+	iproc_pci_raw_config_write32(pcie, 0, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
 				     4, class);
 
 	/* check link status to see if link is active */
-	iproc_pci_raw_config_read32(pcie, 0, IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
+	iproc_pci_raw_config_read32(pcie, 0, 0,
+				    IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
 				    2, &link_status);
 	if (link_status & PCI_EXP_LNKSTA_NLW)
 		link_is_active = true;
@@ -821,19 +822,19 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie)
 #define PCI_TARGET_LINK_SPEED_MASK	0xf
 #define PCI_TARGET_LINK_SPEED_GEN2	0x2
 #define PCI_TARGET_LINK_SPEED_GEN1	0x1
-		iproc_pci_raw_config_read32(pcie, 0,
+		iproc_pci_raw_config_read32(pcie, 0, 0,
 					    IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2,
 					    4, &link_ctrl);
 		if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
 		    PCI_TARGET_LINK_SPEED_GEN2) {
 			link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
 			link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
-			iproc_pci_raw_config_write32(pcie, 0,
+			iproc_pci_raw_config_write32(pcie, 0, 0,
 					IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2,
 					4, link_ctrl);
 			msleep(100);
 
-			iproc_pci_raw_config_read32(pcie, 0,
+			iproc_pci_raw_config_read32(pcie, 0, 0,
 					IPROC_PCI_EXP_CAP + PCI_EXP_LNKSTA,
 					2, &link_status);
 			if (link_status & PCI_EXP_LNKSTA_NLW)
-- 
2.28.0

[PATCH 2/3] PCI: iproc: Stop using generic config read/write functions

[Mark Tomlinson]

The pci_generic_config_write32() function will give warning messages
whenever writing less than 4 bytes at a time. As there is nothing we can
do about this without changing the hardware, the message is just a
nuisance. So instead of using the generic functions, use the functions
that have already been written for reading/writing the config registers.

Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/pci/controller/pcie-iproc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index 2c836eede42c..68ecd3050529 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -709,12 +709,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 {
 	int ret;
 	struct iproc_pcie *pcie = iproc_data(bus);
+	int busno = bus->number;
 
 	iproc_pcie_apb_err_disable(bus, true);
 	if (pcie->iproc_cfg_read)
 		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
 	else
-		ret = pci_generic_config_read32(bus, devfn, where, size, val);
+		ret = iproc_pci_raw_config_read32(pcie, busno, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
 	return ret;
@@ -724,9 +725,11 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
 				     int where, int size, u32 val)
 {
 	int ret;
+	struct iproc_pcie *pcie = iproc_data(bus);
+	int busno = bus->number;
 
 	iproc_pcie_apb_err_disable(bus, true);
-	ret = pci_generic_config_write32(bus, devfn, where, size, val);
+	ret = iproc_pci_raw_config_write32(pcie, busno, devfn, where, size, val);
 	iproc_pcie_apb_err_disable(bus, false);
 
 	return ret;
-- 
2.28.0

Re: [PATCH 2/3] PCI: iproc: Stop using generic config read/write functions

[Bjorn Helgaas]

[+cc Lorenzo, Rob]

On Thu, Jul 30, 2020 at 03:37:46PM +1200, Mark Tomlinson wrote:
> The pci_generic_config_write32() function will give warning messages
> whenever writing less than 4 bytes at a time. As there is nothing we can
> do about this without changing the hardware, the message is just a
> nuisance. So instead of using the generic functions, use the functions
> that have already been written for reading/writing the config registers.

The reason that pci_generic_config_write32() message is there is
because, as the message says, a read/modify/write may corrupt bits in
adjacent registers.  

It makes me a little queasy to do these read/modify/write sequences
silently.  A generic driver doing an 8- or 16-bit config write has no
idea that the write may corrupt an adjacent register.  That leads to
bugs that are very difficult to debug and only reproducible on iProc.

The ratelimiting on the current pci_generic_config_write32() message
is based on the call site, not on the device.  That's not ideal: we
may emit several messages for device A, trigger ratelimiting, then do
a write for device B that doesn't generate a message.

I think it would be better to have a warning once per device, so if
XYZ device has a problem and we look at the dmesg log, we would find a
single message for device XYZ as a hint.  Would that reduce the
nuisance level enough?

So I think I did it wrong in fb2659230120 ("PCI: Warn on possible RW1C
corruption for sub-32 bit config writes").  Ratelimiting is the wrong
concept because what we want is a single warning per device, not a
limit on the similar messages for *all* devices, maybe something like
this:

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 79c4a2ef269a..e5f956b7e3b7 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -160,9 +160,12 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
 	 * write happen to have any RW1C (write-one-to-clear) bits set, we
 	 * just inadvertently cleared something we shouldn't have.
 	 */
-	dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
+	if (!(bus->unsafe_warn & (1 << devfn))) {
+		dev_warn(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
 			     size, pci_domain_nr(bus), bus->number,
 			     PCI_SLOT(devfn), PCI_FUNC(devfn), where);
+		bus->unsafe_warn |= 1 << devfn;
+	}
 
 	mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
 	tmp = readl(addr) & mask;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c79d83304e52..264b009fa4a6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -613,6 +613,7 @@ struct pci_bus {
 	unsigned char	primary;	/* Number of primary bridge */
 	unsigned char	max_bus_speed;	/* enum pci_bus_speed */
 	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
+	u8		unsafe_warn;	/* warned about R/M/W config write */
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
 	int		domain_nr;
 #endif

> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
>  drivers/pci/controller/pcie-iproc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index 2c836eede42c..68ecd3050529 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -709,12 +709,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>  {
>  	int ret;
>  	struct iproc_pcie *pcie = iproc_data(bus);
> +	int busno = bus->number;
>  
>  	iproc_pcie_apb_err_disable(bus, true);
>  	if (pcie->iproc_cfg_read)
>  		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>  	else
> -		ret = pci_generic_config_read32(bus, devfn, where, size, val);
> +		ret = iproc_pci_raw_config_read32(pcie, busno, devfn, where, size, val);
>  	iproc_pcie_apb_err_disable(bus, false);
>  
>  	return ret;
> @@ -724,9 +725,11 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>  				     int where, int size, u32 val)
>  {
>  	int ret;
> +	struct iproc_pcie *pcie = iproc_data(bus);
> +	int busno = bus->number;
>  
>  	iproc_pcie_apb_err_disable(bus, true);
> -	ret = pci_generic_config_write32(bus, devfn, where, size, val);
> +	ret = iproc_pci_raw_config_write32(pcie, busno, devfn, where, size, val);
>  	iproc_pcie_apb_err_disable(bus, false);
>  
>  	return ret;
> -- 
> 2.28.0
> 

Re: [PATCH 2/3] PCI: iproc: Stop using generic config read/write functions

[Ray Jui]

On 7/30/2020 9:09 AM, Bjorn Helgaas wrote:
> [+cc Lorenzo, Rob]
> 
> On Thu, Jul 30, 2020 at 03:37:46PM +1200, Mark Tomlinson wrote:
>> The pci_generic_config_write32() function will give warning messages
>> whenever writing less than 4 bytes at a time. As there is nothing we can
>> do about this without changing the hardware, the message is just a
>> nuisance. So instead of using the generic functions, use the functions
>> that have already been written for reading/writing the config registers.
> 
> The reason that pci_generic_config_write32() message is there is
> because, as the message says, a read/modify/write may corrupt bits in
> adjacent registers.  
> 
> It makes me a little queasy to do these read/modify/write sequences
> silently.  A generic driver doing an 8- or 16-bit config write has no
> idea that the write may corrupt an adjacent register.  That leads to
> bugs that are very difficult to debug and only reproducible on iProc.
> 
> The ratelimiting on the current pci_generic_config_write32() message
> is based on the call site, not on the device.  That's not ideal: we
> may emit several messages for device A, trigger ratelimiting, then do
> a write for device B that doesn't generate a message.
> 
> I think it would be better to have a warning once per device, so if
> XYZ device has a problem and we look at the dmesg log, we would find a
> single message for device XYZ as a hint.  Would that reduce the
> nuisance level enough?
> 

I'm in favor of this. I agree with you that we do need the warnings
because some PCIe config registers that are read/write to clear.

But the current amount of warning messages generated from these config
register access is quite massive and often concerns the users who are
less familiar with the reason/purpose of the warnings. We were asked
about these warnings by multiple customers. People freaked out when they
see "corrupt" in the warning messages, :)

Limiting the warning to once per device seems to be a reasonable
compromise to me.

Thanks,

Ray

> So I think I did it wrong in fb2659230120 ("PCI: Warn on possible RW1C
> corruption for sub-32 bit config writes").  Ratelimiting is the wrong
> concept because what we want is a single warning per device, not a
> limit on the similar messages for *all* devices, maybe something like
> this:
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 79c4a2ef269a..e5f956b7e3b7 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -160,9 +160,12 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
>  	 * write happen to have any RW1C (write-one-to-clear) bits set, we
>  	 * just inadvertently cleared something we shouldn't have.
>  	 */
> -	dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
> +	if (!(bus->unsafe_warn & (1 << devfn))) {
> +		dev_warn(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
>  			     size, pci_domain_nr(bus), bus->number,
>  			     PCI_SLOT(devfn), PCI_FUNC(devfn), where);
> +		bus->unsafe_warn |= 1 << devfn;
> +	}
>  
>  	mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
>  	tmp = readl(addr) & mask;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c79d83304e52..264b009fa4a6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -613,6 +613,7 @@ struct pci_bus {
>  	unsigned char	primary;	/* Number of primary bridge */
>  	unsigned char	max_bus_speed;	/* enum pci_bus_speed */
>  	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
> +	u8		unsafe_warn;	/* warned about R/M/W config write */
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	int		domain_nr;
>  #endif
> 
>> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
>> ---
>>  drivers/pci/controller/pcie-iproc.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
>> index 2c836eede42c..68ecd3050529 100644
>> --- a/drivers/pci/controller/pcie-iproc.c
>> +++ b/drivers/pci/controller/pcie-iproc.c
>> @@ -709,12 +709,13 @@ static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>>  {
>>  	int ret;
>>  	struct iproc_pcie *pcie = iproc_data(bus);
>> +	int busno = bus->number;
>>  
>>  	iproc_pcie_apb_err_disable(bus, true);
>>  	if (pcie->iproc_cfg_read)
>>  		ret = iproc_pcie_config_read(bus, devfn, where, size, val);
>>  	else
>> -		ret = pci_generic_config_read32(bus, devfn, where, size, val);
>> +		ret = iproc_pci_raw_config_read32(pcie, busno, devfn, where, size, val);
>>  	iproc_pcie_apb_err_disable(bus, false);
>>  
>>  	return ret;
>> @@ -724,9 +725,11 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>>  				     int where, int size, u32 val)
>>  {
>>  	int ret;
>> +	struct iproc_pcie *pcie = iproc_data(bus);
>> +	int busno = bus->number;
>>  
>>  	iproc_pcie_apb_err_disable(bus, true);
>> -	ret = pci_generic_config_write32(bus, devfn, where, size, val);
>> +	ret = iproc_pci_raw_config_write32(pcie, busno, devfn, where, size, val);
>>  	iproc_pcie_apb_err_disable(bus, false);
>>  
>>  	return ret;
>> -- 
>> 2.28.0
>>

Re: [PATCH 2/3] PCI: iproc: Stop using generic config read/write functions

[Bjorn Helgaas]

On Thu, Jul 30, 2020 at 09:36:14AM -0700, Ray Jui wrote:
> On 7/30/2020 9:09 AM, Bjorn Helgaas wrote:
> > On Thu, Jul 30, 2020 at 03:37:46PM +1200, Mark Tomlinson wrote:
> >> The pci_generic_config_write32() function will give warning messages
> >> whenever writing less than 4 bytes at a time. As there is nothing we can
> >> do about this without changing the hardware, the message is just a
> >> nuisance. So instead of using the generic functions, use the functions
> >> that have already been written for reading/writing the config registers.
> > 
> > The reason that pci_generic_config_write32() message is there is
> > because, as the message says, a read/modify/write may corrupt bits in
> > adjacent registers.  
> > 
> > It makes me a little queasy to do these read/modify/write sequences
> > silently.  A generic driver doing an 8- or 16-bit config write has no
> > idea that the write may corrupt an adjacent register.  That leads to
> > bugs that are very difficult to debug and only reproducible on iProc.
> > 
> > The ratelimiting on the current pci_generic_config_write32() message
> > is based on the call site, not on the device.  That's not ideal: we
> > may emit several messages for device A, trigger ratelimiting, then do
> > a write for device B that doesn't generate a message.
> > 
> > I think it would be better to have a warning once per device, so if
> > XYZ device has a problem and we look at the dmesg log, we would find a
> > single message for device XYZ as a hint.  Would that reduce the
> > nuisance level enough?
> 
> I'm in favor of this. I agree with you that we do need the warnings
> because some PCIe config registers that are read/write to clear.
> 
> But the current amount of warning messages generated from these config
> register access is quite massive and often concerns the users who are
> less familiar with the reason/purpose of the warnings. We were asked
> about these warnings by multiple customers. People freaked out when they
> see "corrupt" in the warning messages, :)

Yeah, I'm sure they would.  Hopefully the message makes it all the way
back to the hardware designers ;)

> Limiting the warning to once per device seems to be a reasonable
> compromise to me.

We (you, I mean :)) could also look at the particular warnings.  If
they're triggered by PCI core writes that are 8- or 16-bits when they
*could* be 32-bits, it might make sense to widen them.  I know there
are places that do 8-bit writes to 16-bit registers; maybe there are
similar ones to 32-bit registers.

Bjorn

Re: [PATCH 2/3] PCI: iproc: Stop using generic config read/write functions

[Mark Tomlinson]

On Thu, 2020-07-30 at 11:09 -0500, Bjorn Helgaas wrote:
> I think it would be better to have a warning once per device, so if
> XYZ device has a problem and we look at the dmesg log, we would find a
> single message for device XYZ as a hint.  Would that reduce the
> nuisance level enough?

We would be OK with that.

> So I think I did it wrong in fb2659230120 ("PCI: Warn on possible RW1C
> corruption for sub-32 bit config writes").  Ratelimiting is the wrong
> concept because what we want is a single warning per device, not a
> limit on the similar messages for *all* devices, maybe something like
> this:
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 79c4a2ef269a..e5f956b7e3b7 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -160,9 +160,12 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
>  	 * write happen to have any RW1C (write-one-to-clear) bits set, we
>  	 * just inadvertently cleared something we shouldn't have.
>  	 */
> -	dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
> +	if (!(bus->unsafe_warn & (1 << devfn))) {
> +		dev_warn(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
>  			     size, pci_domain_nr(bus), bus->number,
>  			     PCI_SLOT(devfn), PCI_FUNC(devfn), where);
> +		bus->unsafe_warn |= 1 << devfn;
> +	}

As I understand it, devfn is an 8-bit value with five bits of device
and three bits of function. So (1 << devfn) is not going to fit in an
8-bit mask. Am I missing something here? (I do admit that my PCI
knowledge is not great).

Re: [PATCH 2/3] PCI: iproc: Stop using generic config read/write functions

[Bjorn Helgaas]

On Thu, Jul 30, 2020 at 10:58:03PM +0000, Mark Tomlinson wrote:
> On Thu, 2020-07-30 at 11:09 -0500, Bjorn Helgaas wrote:
> > I think it would be better to have a warning once per device, so if
> > XYZ device has a problem and we look at the dmesg log, we would find a
> > single message for device XYZ as a hint.  Would that reduce the
> > nuisance level enough?
> 
> We would be OK with that.
> 
> > So I think I did it wrong in fb2659230120 ("PCI: Warn on possible RW1C
> > corruption for sub-32 bit config writes").  Ratelimiting is the wrong
> > concept because what we want is a single warning per device, not a
> > limit on the similar messages for *all* devices, maybe something like
> > this:
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 79c4a2ef269a..e5f956b7e3b7 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -160,9 +160,12 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
> >  	 * write happen to have any RW1C (write-one-to-clear) bits set, we
> >  	 * just inadvertently cleared something we shouldn't have.
> >  	 */
> > -	dev_warn_ratelimited(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
> > +	if (!(bus->unsafe_warn & (1 << devfn))) {
> > +		dev_warn(&bus->dev, "%d-byte config write to %04x:%02x:%02x.%d offset %#x may corrupt adjacent RW1C bits\n",
> >  			     size, pci_domain_nr(bus), bus->number,
> >  			     PCI_SLOT(devfn), PCI_FUNC(devfn), where);
> > +		bus->unsafe_warn |= 1 << devfn;
> > +	}
> 
> As I understand it, devfn is an 8-bit value with five bits of device
> and three bits of function. So (1 << devfn) is not going to fit in an
> 8-bit mask. Am I missing something here? (I do admit that my PCI
> knowledge is not great).

You're not missing anything, I just screwed up.  What I was really
*hoping* to do was just put a bit in the pci_dev, but of course, these
functions don't have a pci_dev.  256 bits in the bus seems like a
little overkill though.  Maybe we just give up on the exact device and
warn only once per *bus* instead of once per device.

Bjorn

[PATCH 3/3] PCI: iproc: Set affinity mask on MSI interrupts

[Mark Tomlinson]

The core interrupt code expects the irq_set_affinity call to update the
effective affinity for the interrupt. This was not being done, so update
iproc_msi_irq_set_affinity() to do so.

Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/pci/controller/pcie-iproc-msi.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc-msi.c b/drivers/pci/controller/pcie-iproc-msi.c
index 3176ad3ab0e5..908475d27e0e 100644
--- a/drivers/pci/controller/pcie-iproc-msi.c
+++ b/drivers/pci/controller/pcie-iproc-msi.c
@@ -209,15 +209,20 @@ static int iproc_msi_irq_set_affinity(struct irq_data *data,
 	struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
 	int target_cpu = cpumask_first(mask);
 	int curr_cpu;
+	int ret;
 
 	curr_cpu = hwirq_to_cpu(msi, data->hwirq);
 	if (curr_cpu == target_cpu)
-		return IRQ_SET_MASK_OK_DONE;
+		ret = IRQ_SET_MASK_OK_DONE;
+	else {
+		/* steer MSI to the target CPU */
+		data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
+		ret = IRQ_SET_MASK_OK;
+	}
 
-	/* steer MSI to the target CPU */
-	data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
+	irq_data_update_effective_affinity(data, cpumask_of(target_cpu));
 
-	return IRQ_SET_MASK_OK;
+	return ret;
 }
 
 static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,
-- 
2.28.0

Re: [PATCH 3/3] PCI: iproc: Set affinity mask on MSI interrupts

[Ray Jui]

On 7/29/2020 8:37 PM, Mark Tomlinson wrote:
> The core interrupt code expects the irq_set_affinity call to update the
> effective affinity for the interrupt. This was not being done, so update
> iproc_msi_irq_set_affinity() to do so.
> 
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
>  drivers/pci/controller/pcie-iproc-msi.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-iproc-msi.c b/drivers/pci/controller/pcie-iproc-msi.c
> index 3176ad3ab0e5..908475d27e0e 100644
> --- a/drivers/pci/controller/pcie-iproc-msi.c
> +++ b/drivers/pci/controller/pcie-iproc-msi.c
> @@ -209,15 +209,20 @@ static int iproc_msi_irq_set_affinity(struct irq_data *data,
>  	struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
>  	int target_cpu = cpumask_first(mask);
>  	int curr_cpu;
> +	int ret;
>  
>  	curr_cpu = hwirq_to_cpu(msi, data->hwirq);
>  	if (curr_cpu == target_cpu)
> -		return IRQ_SET_MASK_OK_DONE;
> +		ret = IRQ_SET_MASK_OK_DONE;
> +	else {
> +		/* steer MSI to the target CPU */
> +		data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
> +		ret = IRQ_SET_MASK_OK;
> +	}
>  
> -	/* steer MSI to the target CPU */
> -	data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
> +	irq_data_update_effective_affinity(data, cpumask_of(target_cpu));
>  
> -	return IRQ_SET_MASK_OK;
> +	return ret;
>  }
>  
>  static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,
> 

This change looks good to me. Thanks.

Reviewed-by: Ray Jui <ray.jui@broadcom.com>

Re: [PATCH 3/3] PCI: iproc: Set affinity mask on MSI interrupts

[Scott Branden]

On 2020-07-29 8:37 p.m., Mark Tomlinson wrote:
> The core interrupt code expects the irq_set_affinity call to update the
> effective affinity for the interrupt. This was not being done, so update
> iproc_msi_irq_set_affinity() to do so.
>
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
Should this have a Fixes: added to it?
> ---
>  drivers/pci/controller/pcie-iproc-msi.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-iproc-msi.c b/drivers/pci/controller/pcie-iproc-msi.c
> index 3176ad3ab0e5..908475d27e0e 100644
> --- a/drivers/pci/controller/pcie-iproc-msi.c
> +++ b/drivers/pci/controller/pcie-iproc-msi.c
> @@ -209,15 +209,20 @@ static int iproc_msi_irq_set_affinity(struct irq_data *data,
>  	struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
>  	int target_cpu = cpumask_first(mask);
>  	int curr_cpu;
> +	int ret;
>  
>  	curr_cpu = hwirq_to_cpu(msi, data->hwirq);
>  	if (curr_cpu == target_cpu)
> -		return IRQ_SET_MASK_OK_DONE;
> +		ret = IRQ_SET_MASK_OK_DONE;
> +	else {
> +		/* steer MSI to the target CPU */
> +		data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
> +		ret = IRQ_SET_MASK_OK;
> +	}
>  
> -	/* steer MSI to the target CPU */
> -	data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
> +	irq_data_update_effective_affinity(data, cpumask_of(target_cpu));
>  
> -	return IRQ_SET_MASK_OK;
> +	return ret;
>  }
>  
>  static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,