From: John Garry <john.garry@huawei.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: <lorenzo.pieralisi@arm.com>, <arnd@arndb.de>, <linux-pci@vger.kernel.org>, <rjw@rjwysocki.net>, <linux-arm-kernel@lists.infradead.org>, <will.deacon@arm.com>, <wangkefeng.wang@huawei.com>, <linuxarm@huawei.com>, <andriy.shevchenko@linux.intel.com>, <linux-kernel@vger.kernel.org>, <catalin.marinas@arm.com> Subject: Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO Date: Thu, 13 Jun 2019 10:39:12 +0100 [thread overview] Message-ID: <8ef228f8-97cb-e40e-ea6b-410b80a845cf@huawei.com> (raw) In-Reply-To: <20190613023947.GD13533@google.com> On 13/06/2019 03:39, Bjorn Helgaas wrote: > On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: >> Currently we only use logical PIO low-level accessors for when >> CONFIG_INDIRECT_PIO is enabled. >> >> Otherwise we just use inb/out et all directly. >> Hi Bjorn, thanks for checking this. >> It is useful to now use logical PIO accessors for all cases, so we can add >> legality checks to accesses. Such a check would be for ensuring that the >> PCI IO port has been IO remapped prior to the access. > > IIUC, *this* patch doesn't actually add any ok, fine. I suppose that the subsequent patches in the series describe the motivation. additional checks, so no > need to mention that in this commit log. > > One thing this patch *does* do is "#define inb logic_inb" whenever > PCI_IOBASE is defined (we used to do that #define only when > CONFIG_INDIRECT_PIO was defined). Yes, right. > That's a pretty important change and needs to be very clear in the commit log. ok > > I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on > ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, > so this potentially affects arches other than ARM64. It would do. It would affect any arch which defines PCI_IOBASE and does not have arch-specific definition of inb et all. > > If possible, split out the cleanup patches as below and make the patch > that does this PCI_IOBASE change as small as possible so we can > evaluate that change by itself. > Fine >> Using the logical PIO accesses will add a little processing overhead, but >> that's ok as IO port accesses are relatively slow anyway. >> >> Some changes are also made to stop spilling so many lines under >> CONFIG_INDIRECT_PIO. > > "Some changes are also made" is a good hint to me that this patch > might be able to be split up :) > >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> include/linux/logic_pio.h | 7 ++-- >> lib/logic_pio.c | 83 ++++++++++++++++++++++++++++----------- >> 2 files changed, 63 insertions(+), 27 deletions(-) >> >> diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h >> index cbd9d8495690..06d22b2ec99f 100644 >> --- a/include/linux/logic_pio.h >> +++ b/include/linux/logic_pio.h >> @@ -37,7 +37,7 @@ struct logic_pio_host_ops { >> size_t dwidth, unsigned int count); >> }; >> >> -#ifdef CONFIG_INDIRECT_PIO >> +#if defined(PCI_IOBASE) > > Why change the #ifdef style? I understand these are equivalent, but > unless there's a movement to change from "#ifdef X" to "#if defined(X)" > I wouldn't bother. Not intentional. I can keep this style. > >> u8 logic_inb(unsigned long addr); >> void logic_outb(u8 value, unsigned long addr); >> void logic_outw(u16 value, unsigned long addr); >> @@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); >> #define outsl logic_outsl >> #endif >> >> +#if defined(CONFIG_INDIRECT_PIO) >> /* >> * We reserve 0x4000 bytes for Indirect IO as so far this library is only >> * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO >> @@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); >> */ >> #define PIO_INDIRECT_SIZE 0x4000 >> #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE) >> -#else >> +#else /* CONFIG_INDIRECT_PIO */ >> #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT >> #endif /* CONFIG_INDIRECT_PIO */ >> - >> +#endif /* PCI_IOBASE */ >> struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode); >> unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, >> resource_size_t hw_addr, resource_size_t size); >> diff --git a/lib/logic_pio.c b/lib/logic_pio.c >> index feea48fd1a0d..40d9428010e1 100644 >> --- a/lib/logic_pio.c >> +++ b/lib/logic_pio.c >> @@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) >> return ~0UL; >> } >> >> -#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) >> +#if defined(PCI_IOBASE) >> +#if defined(CONFIG_INDIRECT_PIO) >> #define BUILD_LOGIC_IO(bw, type) \ >> type logic_in##bw(unsigned long addr) \ >> { \ >> @@ -200,11 +201,11 @@ type logic_in##bw(unsigned long addr) \ >> if (addr < MMIO_UPPER_LIMIT) { \ >> ret = read##bw(PCI_IOBASE + addr); \ >> } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ >> - struct logic_pio_hwaddr *entry = find_io_range(addr); \ >> + struct logic_pio_hwaddr *range = find_io_range(addr); \ >> + size_t sz = sizeof(type); \ > > I don't mind changing "entry" to "range" and adding "sz". But that > could be done in a separate "no functional change" patch that is > trivial to review, which would make *this* patch smaller and easier to > review. ok > > Another "no functional change" simplification patch would be to > replace this: > > type ret = (type)~0; > > if (addr < MMIO_UPPER_LIMIT) { > ret = read##bw(...); > } else if (...) { > if (range && range->ops) > ret = range->ops->in(...); > else > WARN_ON_ONCE(); > } > return ret; > > with this: > > if (addr < MMIO_UPPER_LIMIT) > return read##bw(...); > > if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { > if (range && range->ops) > return range->ops->in(...); > else > WARN_ON_ONCE(); > } > > return (type)~0; > > Finally, I think the end result would be a little easier to read if > you restructured the #ifdefs like this: > > #ifdef PCI_IOBASE > #define BUILD_LOGIC_IO(...) > type logic_in##bw(...) > { > if (addr < MMIO_UPPER_LIMIT) > return read##bw(...); > > #ifdef CONFIG_INDIRECT_PIO I get your idea, but I don't think that that we can have an ifdef in macros (BUILD_LOGIC_IO) like this. > if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { > if (range && range->ops) > return range->ops->in(...); > else > WARN_ON_ONCE(); > } > #endif > > return (type)~0; > } > > That does mean a CONFIG_INDIRECT_PIO #ifdef in each in/out/ins/outs > builder, but it's more localized so I think it's easier to understand > that INDIRECT_PIO is just adding a new case to the default path. I'll see what I can do to improve the flow. But any change would also depend on your idea in response to patch v2, to unify the 2 types of logic_inb. > >> - if (entry && entry->ops) \ >> - ret = entry->ops->in(entry->hostdata, \ >> - addr, sizeof(type)); \ >> + if (range && range->ops) \ >> + ret = range->ops->in(range->hostdata, addr, sz);\ >> else \ >> WARN_ON_ONCE(1); \ >> } \ >> @@ -216,49 +217,83 @@ void logic_out##bw(type value, unsigned long addr) \ >> if (addr < MMIO_UPPER_LIMIT) { \ >> write##bw(value, PCI_IOBASE + addr); \ thanks again
WARNING: multiple messages have this Message-ID (diff)
From: John Garry <john.garry@huawei.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: rjw@rjwysocki.net, wangkefeng.wang@huawei.com, lorenzo.pieralisi@arm.com, arnd@arndb.de, linux-pci@vger.kernel.org, will.deacon@arm.com, linuxarm@huawei.com, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO Date: Thu, 13 Jun 2019 10:39:12 +0100 [thread overview] Message-ID: <8ef228f8-97cb-e40e-ea6b-410b80a845cf@huawei.com> (raw) In-Reply-To: <20190613023947.GD13533@google.com> On 13/06/2019 03:39, Bjorn Helgaas wrote: > On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: >> Currently we only use logical PIO low-level accessors for when >> CONFIG_INDIRECT_PIO is enabled. >> >> Otherwise we just use inb/out et all directly. >> Hi Bjorn, thanks for checking this. >> It is useful to now use logical PIO accessors for all cases, so we can add >> legality checks to accesses. Such a check would be for ensuring that the >> PCI IO port has been IO remapped prior to the access. > > IIUC, *this* patch doesn't actually add any ok, fine. I suppose that the subsequent patches in the series describe the motivation. additional checks, so no > need to mention that in this commit log. > > One thing this patch *does* do is "#define inb logic_inb" whenever > PCI_IOBASE is defined (we used to do that #define only when > CONFIG_INDIRECT_PIO was defined). Yes, right. > That's a pretty important change and needs to be very clear in the commit log. ok > > I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on > ARM64, but PCI_IOBASE is defined on most arches via asm-generic/io.h, > so this potentially affects arches other than ARM64. It would do. It would affect any arch which defines PCI_IOBASE and does not have arch-specific definition of inb et all. > > If possible, split out the cleanup patches as below and make the patch > that does this PCI_IOBASE change as small as possible so we can > evaluate that change by itself. > Fine >> Using the logical PIO accesses will add a little processing overhead, but >> that's ok as IO port accesses are relatively slow anyway. >> >> Some changes are also made to stop spilling so many lines under >> CONFIG_INDIRECT_PIO. > > "Some changes are also made" is a good hint to me that this patch > might be able to be split up :) > >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> include/linux/logic_pio.h | 7 ++-- >> lib/logic_pio.c | 83 ++++++++++++++++++++++++++++----------- >> 2 files changed, 63 insertions(+), 27 deletions(-) >> >> diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h >> index cbd9d8495690..06d22b2ec99f 100644 >> --- a/include/linux/logic_pio.h >> +++ b/include/linux/logic_pio.h >> @@ -37,7 +37,7 @@ struct logic_pio_host_ops { >> size_t dwidth, unsigned int count); >> }; >> >> -#ifdef CONFIG_INDIRECT_PIO >> +#if defined(PCI_IOBASE) > > Why change the #ifdef style? I understand these are equivalent, but > unless there's a movement to change from "#ifdef X" to "#if defined(X)" > I wouldn't bother. Not intentional. I can keep this style. > >> u8 logic_inb(unsigned long addr); >> void logic_outb(u8 value, unsigned long addr); >> void logic_outw(u16 value, unsigned long addr); >> @@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); >> #define outsl logic_outsl >> #endif >> >> +#if defined(CONFIG_INDIRECT_PIO) >> /* >> * We reserve 0x4000 bytes for Indirect IO as so far this library is only >> * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO >> @@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count); >> */ >> #define PIO_INDIRECT_SIZE 0x4000 >> #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE) >> -#else >> +#else /* CONFIG_INDIRECT_PIO */ >> #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT >> #endif /* CONFIG_INDIRECT_PIO */ >> - >> +#endif /* PCI_IOBASE */ >> struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode); >> unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode, >> resource_size_t hw_addr, resource_size_t size); >> diff --git a/lib/logic_pio.c b/lib/logic_pio.c >> index feea48fd1a0d..40d9428010e1 100644 >> --- a/lib/logic_pio.c >> +++ b/lib/logic_pio.c >> @@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) >> return ~0UL; >> } >> >> -#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) >> +#if defined(PCI_IOBASE) >> +#if defined(CONFIG_INDIRECT_PIO) >> #define BUILD_LOGIC_IO(bw, type) \ >> type logic_in##bw(unsigned long addr) \ >> { \ >> @@ -200,11 +201,11 @@ type logic_in##bw(unsigned long addr) \ >> if (addr < MMIO_UPPER_LIMIT) { \ >> ret = read##bw(PCI_IOBASE + addr); \ >> } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ >> - struct logic_pio_hwaddr *entry = find_io_range(addr); \ >> + struct logic_pio_hwaddr *range = find_io_range(addr); \ >> + size_t sz = sizeof(type); \ > > I don't mind changing "entry" to "range" and adding "sz". But that > could be done in a separate "no functional change" patch that is > trivial to review, which would make *this* patch smaller and easier to > review. ok > > Another "no functional change" simplification patch would be to > replace this: > > type ret = (type)~0; > > if (addr < MMIO_UPPER_LIMIT) { > ret = read##bw(...); > } else if (...) { > if (range && range->ops) > ret = range->ops->in(...); > else > WARN_ON_ONCE(); > } > return ret; > > with this: > > if (addr < MMIO_UPPER_LIMIT) > return read##bw(...); > > if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { > if (range && range->ops) > return range->ops->in(...); > else > WARN_ON_ONCE(); > } > > return (type)~0; > > Finally, I think the end result would be a little easier to read if > you restructured the #ifdefs like this: > > #ifdef PCI_IOBASE > #define BUILD_LOGIC_IO(...) > type logic_in##bw(...) > { > if (addr < MMIO_UPPER_LIMIT) > return read##bw(...); > > #ifdef CONFIG_INDIRECT_PIO I get your idea, but I don't think that that we can have an ifdef in macros (BUILD_LOGIC_IO) like this. > if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { > if (range && range->ops) > return range->ops->in(...); > else > WARN_ON_ONCE(); > } > #endif > > return (type)~0; > } > > That does mean a CONFIG_INDIRECT_PIO #ifdef in each in/out/ins/outs > builder, but it's more localized so I think it's easier to understand > that INDIRECT_PIO is just adding a new case to the default path. I'll see what I can do to improve the flow. But any change would also depend on your idea in response to patch v2, to unify the 2 types of logic_inb. > >> - if (entry && entry->ops) \ >> - ret = entry->ops->in(entry->hostdata, \ >> - addr, sizeof(type)); \ >> + if (range && range->ops) \ >> + ret = range->ops->in(range->hostdata, addr, sz);\ >> else \ >> WARN_ON_ONCE(1); \ >> } \ >> @@ -216,49 +217,83 @@ void logic_out##bw(type value, unsigned long addr) \ >> if (addr < MMIO_UPPER_LIMIT) { \ >> write##bw(value, PCI_IOBASE + addr); \ thanks again _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-06-13 15:46 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-11 14:12 [PATCH v4 0/3] Fix ARM64 crash for accessing unmapped IO port regions John Garry 2019-06-11 14:12 ` John Garry 2019-06-11 14:12 ` [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO John Garry 2019-06-11 14:12 ` John Garry 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 2:39 ` Bjorn Helgaas 2019-06-13 9:39 ` John Garry [this message] 2019-06-13 9:39 ` John Garry 2019-06-13 20:09 ` Bjorn Helgaas 2019-06-13 20:09 ` Bjorn Helgaas 2019-06-14 9:02 ` John Garry 2019-06-14 9:02 ` John Garry 2019-06-14 11:50 ` Bjorn Helgaas 2019-06-14 11:50 ` Bjorn Helgaas 2019-06-14 12:22 ` John Garry 2019-06-14 12:22 ` John Garry 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-13 13:58 ` Bjorn Helgaas 2019-06-13 15:21 ` John Garry 2019-06-13 15:21 ` John Garry 2019-06-11 14:12 ` [PATCH v4 2/3] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions John Garry 2019-06-11 14:12 ` John Garry 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 3:20 ` Bjorn Helgaas 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 7:47 ` Arnd Bergmann 2019-06-13 10:17 ` John Garry 2019-06-13 10:17 ` John Garry 2019-06-13 13:46 ` Bjorn Helgaas 2019-06-13 13:46 ` Bjorn Helgaas 2019-06-13 14:09 ` John Garry 2019-06-13 14:09 ` John Garry 2019-06-11 14:12 ` [PATCH v4 3/3] lib: logic_pio: Fix up a print John Garry 2019-06-11 14:12 ` John Garry
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=8ef228f8-97cb-e40e-ea6b-410b80a845cf@huawei.com \ --to=john.garry@huawei.com \ --cc=andriy.shevchenko@linux.intel.com \ --cc=arnd@arndb.de \ --cc=catalin.marinas@arm.com \ --cc=helgaas@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linuxarm@huawei.com \ --cc=lorenzo.pieralisi@arm.com \ --cc=rjw@rjwysocki.net \ --cc=wangkefeng.wang@huawei.com \ --cc=will.deacon@arm.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: linkBe 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.