| Message ID | 1560262374-67875-2-git-send-email-john.garry@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix ARM64 crash for accessing unmapped IO port regions | expand |
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. > > 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 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). That's a pretty important change and needs to be very clear in the commit log. 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. 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. > 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. > 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. 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 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. > - 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); \ > } 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); \ > \ > - if (entry && entry->ops) \ > - entry->ops->out(entry->hostdata, \ > - addr, value, sizeof(type)); \ > + if (range && range->ops) \ > + range->ops->out(range->hostdata, \ > + addr, value, sz); \ > else \ > WARN_ON_ONCE(1); \ > } \ > } \ > \ > -void logic_ins##bw(unsigned long addr, void *buffer, \ > - unsigned int count) \ > +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > { \ > if (addr < MMIO_UPPER_LIMIT) { \ > - reads##bw(PCI_IOBASE + addr, buffer, count); \ > + reads##bw(PCI_IOBASE + addr, buf, cnt); \ > } 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); \ > \ > - if (entry && entry->ops) \ > - entry->ops->ins(entry->hostdata, \ > - addr, buffer, sizeof(type), count); \ > + if (range && range->ops) \ > + range->ops->ins(range->hostdata, \ > + addr, buf, sz, cnt); \ > else \ > WARN_ON_ONCE(1); \ > } \ > \ > } \ > \ > -void logic_outs##bw(unsigned long addr, const void *buffer, \ > - unsigned int count) \ > +void logic_outs##bw(unsigned long addr, const void *buf, \ > + unsigned int cnt) \ > { \ > if (addr < MMIO_UPPER_LIMIT) { \ > - writes##bw(PCI_IOBASE + addr, buffer, count); \ > + writes##bw(PCI_IOBASE + addr, buf, cnt); \ > } 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); \ > \ > - if (entry && entry->ops) \ > - entry->ops->outs(entry->hostdata, \ > - addr, buffer, sizeof(type), count); \ > + if (range && range->ops) \ > + range->ops->outs(range->hostdata, \ > + addr, buf, sz, cnt); \ > else \ > WARN_ON_ONCE(1); \ > } \ > } > > +#else /* CONFIG_INDIRECT_PIO */ > + > +#define BUILD_LOGIC_IO(bw, type) \ > +type logic_in##bw(unsigned long addr) \ > +{ \ > + type ret = (type)~0; \ > + \ > + if (addr < MMIO_UPPER_LIMIT) \ > + ret = read##bw(PCI_IOBASE + addr); \ > + return ret; \ > +} \ > + \ > +void logic_out##bw(type value, unsigned long addr) \ > +{ \ > + if (addr < MMIO_UPPER_LIMIT) \ > + write##bw(value, PCI_IOBASE + addr); \ > +} \ > + \ > +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ > +{ \ > + if (addr < MMIO_UPPER_LIMIT) \ > + reads##bw(PCI_IOBASE + addr, buf, cnt); \ > +} \ > + \ > +void logic_outs##bw(unsigned long addr, const void *buf, \ > + unsigned int cnt) \ > +{ \ > + if (addr < MMIO_UPPER_LIMIT) \ > + writes##bw(PCI_IOBASE + addr, buf, cnt); \ > +} > +#endif /* CONFIG_INDIRECT_PIO */ > + > BUILD_LOGIC_IO(b, u8) > EXPORT_SYMBOL(logic_inb); > EXPORT_SYMBOL(logic_insb); > @@ -277,4 +312,4 @@ EXPORT_SYMBOL(logic_insl); > EXPORT_SYMBOL(logic_outl); > EXPORT_SYMBOL(logic_outsl); > > -#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */ > +#endif /* PCI_IOBASE */ > -- > 2.17.1 >
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
On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: Another thought here: > 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); \ > \ > - 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); \ Could this be simplified a little by requiring callers to set range->ops for LOGIC_PIO_INDIRECT ranges *before* calling logic_pio_register_range()? E.g., hisi_lpc_probe(...) { range = devm_kzalloc(...); range->flags = LOGIC_PIO_INDIRECT; range->ops = &hisi_lpc_ops; logic_pio_register_range(range); ... and logic_pio_register_range(struct logic_pio_hwaddr *new_range) { if (new_range->flags == LOGIC_PIO_INDIRECT && !new_range->ops) return -EINVAL; ... Then maybe you wouldn't need to check range->ops in the accessors. Bjorn
On 13/06/2019 14:58, Bjorn Helgaas wrote: > On Tue, Jun 11, 2019 at 10:12:52PM +0800, John Garry wrote: > Another thought here: > >> 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); \ >> \ >> - 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); Hi Bjorn, \ > > Could this be simplified a little by requiring callers to set > range->ops for LOGIC_PIO_INDIRECT ranges *before* calling > logic_pio_register_range()? E.g., > > hisi_lpc_probe(...) > { > range = devm_kzalloc(...); > range->flags = LOGIC_PIO_INDIRECT; > range->ops = &hisi_lpc_ops; > logic_pio_register_range(range); > ... > > and > > logic_pio_register_range(struct logic_pio_hwaddr *new_range) > { > if (new_range->flags == LOGIC_PIO_INDIRECT && !new_range->ops) > return -EINVAL; > ... > > Then maybe you wouldn't need to check range->ops in the accessors. > I think I know the reason why it was done this way. So currently there is no method to unregister a logical PIO region (the old code leaked ranges as well). As such, if hisi_lpc_probe() fails after we register the logical PIO range, there would be a range registered but no actual host backing it. So we set the ops at the point at which the probe cannot fail to avoid a potential problem. And now I realise that there is a bug in the code - range is allocated with devm_kzalloc and is passed to logic_pio_register_range(). As such, if the hisi_lpc_probe() goes on to fail, then this memory would be free'd and we have an issue. PCI code should be ok as it uses kzalloc(). The simplest solution is to not change the logical PIO API to allocate this memory itself, but rather make hisi_lpc_probe() use kzalloc(). And, if we go this way, we can use your idea to set the ops. I'll spin a separate patch for this. Thanks, John > Bjorn > > . >
On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: > On 13/06/2019 03:39, Bjorn Helgaas wrote: > > 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. What's the reason for testing PCI_IOBASE instead of CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, but it does make this much more complicated to review. Bjorn
On 13/06/2019 21:09, Bjorn Helgaas wrote: > On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: >> On 13/06/2019 03:39, Bjorn Helgaas wrote: >>> 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. > Hi Bjorn, > What's the reason for testing PCI_IOBASE instead of > CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, > but it does make this much more complicated to review. For ARM64, we have PCI_IOBASE defined but may not have CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only selected by CONFIG_HISILICON_LPC. As such, we should make this change also for when CONFIG_INDIRECT_PIO is not defined. Thanks, John > > Bjorn > > . >
On Fri, Jun 14, 2019 at 10:02:52AM +0100, John Garry wrote: > On 13/06/2019 21:09, Bjorn Helgaas wrote: > > On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: > > > On 13/06/2019 03:39, Bjorn Helgaas wrote: > > > > 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. > > > What's the reason for testing PCI_IOBASE instead of > > CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, > > but it does make this much more complicated to review. > > For ARM64, we have PCI_IOBASE defined but may not have > CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only > selected by CONFIG_HISILICON_LPC. > > As such, we should make this change also for when > CONFIG_INDIRECT_PIO is not defined. OK. This is all very important for the commit log -- we need to understand what arches are affected and the reason they need it. Since the goal of this series is to fix an ARM64-specific issue, and the typical port I/O model is for each arch to #define its own inb(), maybe it would make sense to move the "#define inb logic_inb" from linux/logic_pio.h to arm64/include/asm/io.h? The "#ifndef inb" arrangement gets pretty complicated when it occurs more than one place (asm-generic/io.h and logic_pio.h) and we have to start worrying about the ordering of #includes. Bjorn
On 14/06/2019 12:50, Bjorn Helgaas wrote: > On Fri, Jun 14, 2019 at 10:02:52AM +0100, John Garry wrote: >> On 13/06/2019 21:09, Bjorn Helgaas wrote: >>> On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote: >>>> On 13/06/2019 03:39, Bjorn Helgaas wrote: >>>>> 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. >> >>> What's the reason for testing PCI_IOBASE instead of >>> CONFIG_INDIRECT_PIO? If there's a reason it's needed, that's fine, >>> but it does make this much more complicated to review. >> >> For ARM64, we have PCI_IOBASE defined but may not have >> CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only >> selected by CONFIG_HISILICON_LPC. >> >> As such, we should make this change also for when >> CONFIG_INDIRECT_PIO is not defined. Hi Bjorn, > > OK. This is all very important for the commit log -- we need to > understand what arches are affected and the reason they need it. Right, and to repeat, this would affect other archs which define PCI_IOBASE and don't have custom IO port accessors definitions. There are a few remaining even after the recent arch clear out . I have it at arm64, microblaze, and unicore32. Arch m68k defines PCI_IOBASE but seems to have its own IO port accessors. Same again for some arm machines. At least I should cc those arch maintainers. > > Since the goal of this series is to fix an ARM64-specific issue, "ARM64" was in the headline banner, but it would apply to other archs, as mentioned above. I should have made that clearer. and > the typical port I/O model is for each arch to #define its own inb(), > maybe it would make sense to move the "#define inb logic_inb" from > linux/logic_pio.h to arm64/include/asm/io.h? > CONFIG_INDIRECT_PIO has been indirectly enabled in ARM64 defconfig for some time, so I think that it's ok for ARM64 arch Kconfig to select it at this stage. From that, we could make the change suggested. And, in addition to that, we can make the change in this series just for CONFIG_INDIRECT_PIO. But I think that the other archs, above, could benefit from the changes in this series, so it would be shame to omit them. > The "#ifndef inb" arrangement gets pretty complicated when it occurs > more than one place (asm-generic/io.h and logic_pio.h) and we have to > start worrying about the ordering of #includes. I agree on that. Cheers, John > > Bjorn > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >
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) 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); \ \ - 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); \ } 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); \ \ - if (entry && entry->ops) \ - entry->ops->out(entry->hostdata, \ - addr, value, sizeof(type)); \ + if (range && range->ops) \ + range->ops->out(range->hostdata, \ + addr, value, sz); \ else \ WARN_ON_ONCE(1); \ } \ } \ \ -void logic_ins##bw(unsigned long addr, void *buffer, \ - unsigned int count) \ +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ { \ if (addr < MMIO_UPPER_LIMIT) { \ - reads##bw(PCI_IOBASE + addr, buffer, count); \ + reads##bw(PCI_IOBASE + addr, buf, cnt); \ } 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); \ \ - if (entry && entry->ops) \ - entry->ops->ins(entry->hostdata, \ - addr, buffer, sizeof(type), count); \ + if (range && range->ops) \ + range->ops->ins(range->hostdata, \ + addr, buf, sz, cnt); \ else \ WARN_ON_ONCE(1); \ } \ \ } \ \ -void logic_outs##bw(unsigned long addr, const void *buffer, \ - unsigned int count) \ +void logic_outs##bw(unsigned long addr, const void *buf, \ + unsigned int cnt) \ { \ if (addr < MMIO_UPPER_LIMIT) { \ - writes##bw(PCI_IOBASE + addr, buffer, count); \ + writes##bw(PCI_IOBASE + addr, buf, cnt); \ } 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); \ \ - if (entry && entry->ops) \ - entry->ops->outs(entry->hostdata, \ - addr, buffer, sizeof(type), count); \ + if (range && range->ops) \ + range->ops->outs(range->hostdata, \ + addr, buf, sz, cnt); \ else \ WARN_ON_ONCE(1); \ } \ } +#else /* CONFIG_INDIRECT_PIO */ + +#define BUILD_LOGIC_IO(bw, type) \ +type logic_in##bw(unsigned long addr) \ +{ \ + type ret = (type)~0; \ + \ + if (addr < MMIO_UPPER_LIMIT) \ + ret = read##bw(PCI_IOBASE + addr); \ + return ret; \ +} \ + \ +void logic_out##bw(type value, unsigned long addr) \ +{ \ + if (addr < MMIO_UPPER_LIMIT) \ + write##bw(value, PCI_IOBASE + addr); \ +} \ + \ +void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt) \ +{ \ + if (addr < MMIO_UPPER_LIMIT) \ + reads##bw(PCI_IOBASE + addr, buf, cnt); \ +} \ + \ +void logic_outs##bw(unsigned long addr, const void *buf, \ + unsigned int cnt) \ +{ \ + if (addr < MMIO_UPPER_LIMIT) \ + writes##bw(PCI_IOBASE + addr, buf, cnt); \ +} +#endif /* CONFIG_INDIRECT_PIO */ + BUILD_LOGIC_IO(b, u8) EXPORT_SYMBOL(logic_inb); EXPORT_SYMBOL(logic_insb); @@ -277,4 +312,4 @@ EXPORT_SYMBOL(logic_insl); EXPORT_SYMBOL(logic_outl); EXPORT_SYMBOL(logic_outsl); -#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */ +#endif /* PCI_IOBASE */
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. 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. 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. 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(-)