| Message ID | 20220812094058.16141-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: mvebu: Fix endianity when accessing pci emul bridge members | expand |
On 2022-08-12 10:40, Pali Rohár wrote: > PCI emul bridge members iolimitupper, iobaseupper, memlimit and membase are > of type __le16, so correctly access these members via le16_to_cpu() macros. > > Fixes: 4ded69473adb ("PCI: mvebu: Propagate errors when updating PCI_IO_BASE and PCI_MEM_BASE registers") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > drivers/pci/controller/pci-mvebu.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > index c1ffdb06c971..00ea0836b81a 100644 > --- a/drivers/pci/controller/pci-mvebu.c > +++ b/drivers/pci/controller/pci-mvebu.c > @@ -523,7 +523,7 @@ static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) > > /* Are the new iobase/iolimit values invalid? */ > if (conf->iolimit < conf->iobase || > - conf->iolimitupper < conf->iobaseupper) > + le16_to_cpu(conf->iolimitupper) < le16_to_cpu(conf->iobaseupper)) > return mvebu_pcie_set_window(port, port->io_target, port->io_attr, > &desired, &port->iowin); > > @@ -535,10 +535,10 @@ static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) > * is the CPU address. > */ > desired.remap = ((conf->iobase & 0xF0) << 8) | > - (conf->iobaseupper << 16); > + le16_to_cpu(conf->iobaseupper << 16); This will always give 0, even when natively LE. > desired.base = port->pcie->io.start + desired.remap; > desired.size = ((0xFFF | ((conf->iolimit & 0xF0) << 8) | > - (conf->iolimitupper << 16)) - > + le16_to_cpu(conf->iolimitupper << 16)) - Similarly here. Robin. > desired.remap) + > 1; > > @@ -552,7 +552,7 @@ static int mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) > struct pci_bridge_emul_conf *conf = &port->bridge.conf; > > /* Are the new membase/memlimit values invalid? */ > - if (conf->memlimit < conf->membase) > + if (le16_to_cpu(conf->memlimit) < le16_to_cpu(conf->membase)) > return mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, > &desired, &port->memwin); > > @@ -562,8 +562,8 @@ static int mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) > * window to setup, according to the PCI-to-PCI bridge > * specifications. > */ > - desired.base = ((conf->membase & 0xFFF0) << 16); > - desired.size = (((conf->memlimit & 0xFFF0) << 16) | 0xFFFFF) - > + desired.base = ((le16_to_cpu(conf->membase) & 0xFFF0) << 16); > + desired.size = (((le16_to_cpu(conf->memlimit) & 0xFFF0) << 16) | 0xFFFFF) - > desired.base + 1; > > return mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired,
On Friday 12 August 2022 11:32:59 Robin Murphy wrote: > On 2022-08-12 10:40, Pali Rohár wrote: > > PCI emul bridge members iolimitupper, iobaseupper, memlimit and membase are > > of type __le16, so correctly access these members via le16_to_cpu() macros. > > > > Fixes: 4ded69473adb ("PCI: mvebu: Propagate errors when updating PCI_IO_BASE and PCI_MEM_BASE registers") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > drivers/pci/controller/pci-mvebu.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > > index c1ffdb06c971..00ea0836b81a 100644 > > --- a/drivers/pci/controller/pci-mvebu.c > > +++ b/drivers/pci/controller/pci-mvebu.c > > @@ -523,7 +523,7 @@ static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) > > /* Are the new iobase/iolimit values invalid? */ > > if (conf->iolimit < conf->iobase || > > - conf->iolimitupper < conf->iobaseupper) > > + le16_to_cpu(conf->iolimitupper) < le16_to_cpu(conf->iobaseupper)) > > return mvebu_pcie_set_window(port, port->io_target, port->io_attr, > > &desired, &port->iowin); > > @@ -535,10 +535,10 @@ static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) > > * is the CPU address. > > */ > > desired.remap = ((conf->iobase & 0xF0) << 8) | > > - (conf->iobaseupper << 16); > > + le16_to_cpu(conf->iobaseupper << 16); > > This will always give 0, even when natively LE. You are right, I overlooked it and I put closing parenthesis at wrong place. Bit shifting should be applied after le to cpu conversion. I will fix it in V2. > > desired.base = port->pcie->io.start + desired.remap; > > desired.size = ((0xFFF | ((conf->iolimit & 0xF0) << 8) | > > - (conf->iolimitupper << 16)) - > > + le16_to_cpu(conf->iolimitupper << 16)) - > > Similarly here. > > Robin. > > > desired.remap) + > > 1; > > @@ -552,7 +552,7 @@ static int mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) > > struct pci_bridge_emul_conf *conf = &port->bridge.conf; > > /* Are the new membase/memlimit values invalid? */ > > - if (conf->memlimit < conf->membase) > > + if (le16_to_cpu(conf->memlimit) < le16_to_cpu(conf->membase)) > > return mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, > > &desired, &port->memwin); > > @@ -562,8 +562,8 @@ static int mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) > > * window to setup, according to the PCI-to-PCI bridge > > * specifications. > > */ > > - desired.base = ((conf->membase & 0xFFF0) << 16); > > - desired.size = (((conf->memlimit & 0xFFF0) << 16) | 0xFFFFF) - > > + desired.base = ((le16_to_cpu(conf->membase) & 0xFFF0) << 16); > > + desired.size = (((le16_to_cpu(conf->memlimit) & 0xFFF0) << 16) | 0xFFFFF) - > > desired.base + 1; > > return mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired,
Hi "Pali,
I love your patch! Perhaps something to improve:
[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on linus/master v5.19 next-20220812]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pali-Roh-r/PCI-mvebu-Fix-endianity-when-accessing-pci-emul-bridge-members/20220812-174306
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-randconfig-s041-20220812 (https://download.01.org/0day-ci/archive/20220813/202208130542.WpzYnXZ3-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/525898e0a8389f8bb560edbce3f07a3ec2550968
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Pali-Roh-r/PCI-mvebu-Fix-endianity-when-accessing-pci-emul-bridge-members/20220812-174306
git checkout 525898e0a8389f8bb560edbce3f07a3ec2550968
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm SHELL=/bin/bash drivers/pci/controller/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
drivers/pci/controller/pci-mvebu.c:538:25: sparse: sparse: restricted __le16 degrades to integer
>> drivers/pci/controller/pci-mvebu.c:538:25: sparse: sparse: cast to restricted __le16
drivers/pci/controller/pci-mvebu.c:541:26: sparse: sparse: restricted __le16 degrades to integer
drivers/pci/controller/pci-mvebu.c:541:26: sparse: sparse: cast to restricted __le16
vim +538 drivers/pci/controller/pci-mvebu.c
518
519 static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
520 {
521 struct mvebu_pcie_window desired = {};
522 struct pci_bridge_emul_conf *conf = &port->bridge.conf;
523
524 /* Are the new iobase/iolimit values invalid? */
525 if (conf->iolimit < conf->iobase ||
526 le16_to_cpu(conf->iolimitupper) < le16_to_cpu(conf->iobaseupper))
527 return mvebu_pcie_set_window(port, port->io_target, port->io_attr,
528 &desired, &port->iowin);
529
530 /*
531 * We read the PCI-to-PCI bridge emulated registers, and
532 * calculate the base address and size of the address decoding
533 * window to setup, according to the PCI-to-PCI bridge
534 * specifications. iobase is the bus address, port->iowin_base
535 * is the CPU address.
536 */
537 desired.remap = ((conf->iobase & 0xF0) << 8) |
> 538 le16_to_cpu(conf->iobaseupper << 16);
539 desired.base = port->pcie->io.start + desired.remap;
540 desired.size = ((0xFFF | ((conf->iolimit & 0xF0) << 8) |
541 le16_to_cpu(conf->iolimitupper << 16)) -
542 desired.remap) +
543 1;
544
545 return mvebu_pcie_set_window(port, port->io_target, port->io_attr, &desired,
546 &port->iowin);
547 }
548
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c index c1ffdb06c971..00ea0836b81a 100644 --- a/drivers/pci/controller/pci-mvebu.c +++ b/drivers/pci/controller/pci-mvebu.c @@ -523,7 +523,7 @@ static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) /* Are the new iobase/iolimit values invalid? */ if (conf->iolimit < conf->iobase || - conf->iolimitupper < conf->iobaseupper) + le16_to_cpu(conf->iolimitupper) < le16_to_cpu(conf->iobaseupper)) return mvebu_pcie_set_window(port, port->io_target, port->io_attr, &desired, &port->iowin); @@ -535,10 +535,10 @@ static int mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port) * is the CPU address. */ desired.remap = ((conf->iobase & 0xF0) << 8) | - (conf->iobaseupper << 16); + le16_to_cpu(conf->iobaseupper << 16); desired.base = port->pcie->io.start + desired.remap; desired.size = ((0xFFF | ((conf->iolimit & 0xF0) << 8) | - (conf->iolimitupper << 16)) - + le16_to_cpu(conf->iolimitupper << 16)) - desired.remap) + 1; @@ -552,7 +552,7 @@ static int mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) struct pci_bridge_emul_conf *conf = &port->bridge.conf; /* Are the new membase/memlimit values invalid? */ - if (conf->memlimit < conf->membase) + if (le16_to_cpu(conf->memlimit) < le16_to_cpu(conf->membase)) return mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired, &port->memwin); @@ -562,8 +562,8 @@ static int mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port) * window to setup, according to the PCI-to-PCI bridge * specifications. */ - desired.base = ((conf->membase & 0xFFF0) << 16); - desired.size = (((conf->memlimit & 0xFFF0) << 16) | 0xFFFFF) - + desired.base = ((le16_to_cpu(conf->membase) & 0xFFF0) << 16); + desired.size = (((le16_to_cpu(conf->memlimit) & 0xFFF0) << 16) | 0xFFFFF) - desired.base + 1; return mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired,
PCI emul bridge members iolimitupper, iobaseupper, memlimit and membase are of type __le16, so correctly access these members via le16_to_cpu() macros. Fixes: 4ded69473adb ("PCI: mvebu: Propagate errors when updating PCI_IO_BASE and PCI_MEM_BASE registers") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Pali Rohár <pali@kernel.org> --- drivers/pci/controller/pci-mvebu.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)