| Message ID | 20211125124605.25915-5-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pci: mvebu: Various fixes | expand |
On Thu, Nov 25, 2021 at 01:45:54PM +0100, Pali Rohár wrote: > Function mvebu_pcie_hw_rd_conf() does not handle invalid size. So correctly > set read value to all-ones and return appropriate error return value > PCIBIOS_BAD_REGISTER_NUMBER like in mvebu_pcie_hw_wr_conf() function. > > Signed-off-by: Pali Rohár <pali@kernel.org> > Cc: stable@vger.kernel.org Is there a bug that this fixes? If not, I would drop the stable tag (as I see Lorenzo already did, thanks!). > --- > drivers/pci/controller/pci-mvebu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > index 08274132cdfb..19c6ee298442 100644 > --- a/drivers/pci/controller/pci-mvebu.c > +++ b/drivers/pci/controller/pci-mvebu.c > @@ -261,6 +261,9 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port, > case 4: > *val = readl_relaxed(conf_data); > break; > + default: > + *val = 0xffffffff; > + return PCIBIOS_BAD_REGISTER_NUMBER; Might be the right thing to do, but there are many config accessors that do not set *val to ~0 before returning PCIBIOS_BAD_REGISTER_NUMBER: pci_bus_read_config_byte (and word, dword) # PCI_OP_READ(), *val unchanged pci_generic_config_read # *val = 32-bit value pci_user_read_config_byte (...) # PCI_USER_READ_CONFIG(), *val unchanged sh7786_pcie_read # *val unchanged dw_pcie_read # *val = 0 mobiveil_pcie_read # *val = 0 faraday_raw_pci_read_config # *val = 32-bit value ixp4xx_pci_read_config # *val unchanged orion5x_pci_hw_rd_conf # *val = 32-bit value orion_pcie_rd_conf # *val = 32-bit value bonito64_pcibios_read # *val = 32-bit value loongson_pcibios_read # *val = 32-bit value msc_pcibios_read # *val = 32-bit value ar724x_pci_read # *val unchanged bcm1480_pcibios_read # *val = 32-bit value _altera_pcie_cfg_read # *val = 32-bit value rockchip_pcie_rd_own_conf # *val = 0 rockchip_pcie_rd_other_conf # *val = 0 pci_bridge_emul_conf_read # may depend on op? There are more, but I got tired of looking. I actually didn't see any that set *val to ~0. I think the check in PCI_OP_READ() means that most accessors will never see an invalid "size". > } > > return PCIBIOS_SUCCESSFUL; > -- > 2.20.1 >
On Fri, Jan 07, 2022 at 12:45:48PM -0600, Bjorn Helgaas wrote: > On Thu, Nov 25, 2021 at 01:45:54PM +0100, Pali Rohár wrote: > > Function mvebu_pcie_hw_rd_conf() does not handle invalid size. So correctly > > set read value to all-ones and return appropriate error return value > > PCIBIOS_BAD_REGISTER_NUMBER like in mvebu_pcie_hw_wr_conf() function. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Cc: stable@vger.kernel.org > > Is there a bug that this fixes? If not, I would drop the stable tag > (as I see Lorenzo already did, thanks!). > > > --- > > drivers/pci/controller/pci-mvebu.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > index 08274132cdfb..19c6ee298442 100644 > > --- a/drivers/pci/controller/pci-mvebu.c > > +++ b/drivers/pci/controller/pci-mvebu.c > > @@ -261,6 +261,9 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port, > > case 4: > > *val = readl_relaxed(conf_data); > > break; > > + default: > > + *val = 0xffffffff; > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > Might be the right thing to do, but there are many config accessors > that do not set *val to ~0 before returning > PCIBIOS_BAD_REGISTER_NUMBER: I think a better question would be - how can this function be called with a size that isn't 1, 2 or 4? I suppose if someone were to add another PCI_OP_READ/PCI_OP_WRITE. However... they really need to audit every implementation if they do that. The generic implementation does this: if (size == 1) *val = readb(addr); else if (size == 2) *val = readw(addr); else *val = readl(addr); and therefore completely ignores the size if it isn't 1 or 2. So I don't think this is something that needs fixing. If we're going to fix this in drivers, shouldn't we fix the generic implementation too?
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c index 08274132cdfb..19c6ee298442 100644 --- a/drivers/pci/controller/pci-mvebu.c +++ b/drivers/pci/controller/pci-mvebu.c @@ -261,6 +261,9 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port, case 4: *val = readl_relaxed(conf_data); break; + default: + *val = 0xffffffff; + return PCIBIOS_BAD_REGISTER_NUMBER; } return PCIBIOS_SUCCESSFUL;
Function mvebu_pcie_hw_rd_conf() does not handle invalid size. So correctly set read value to all-ones and return appropriate error return value PCIBIOS_BAD_REGISTER_NUMBER like in mvebu_pcie_hw_wr_conf() function. Signed-off-by: Pali Rohár <pali@kernel.org> Cc: stable@vger.kernel.org --- drivers/pci/controller/pci-mvebu.c | 3 +++ 1 file changed, 3 insertions(+)