Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvram_write_byte()
On Sun, 30 Dec 2018, I wrote:
> On Sat, 29 Dec 2018, LEROY Christophe wrote:
>
> > Finn Thain a ?crit?:
> >
> > > Make use of arch_nvram_ops in device drivers so that the nvram_* function
> > > exports can be removed.
> > >
> > > Since they are no longer global symbols, rename the PPC32 nvram_*
> > > functions
> > > appropriately.
> > >
> > > Signed-off-by: Finn Thain
> > > ---
> > > arch/powerpc/kernel/setup_32.c | 8
> > > drivers/char/generic_nvram.c | 4 ++--
> > > drivers/video/fbdev/controlfb.c| 4 ++--
> > > drivers/video/fbdev/imsttfb.c | 4 ++--
> > > drivers/video/fbdev/matrox/matroxfb_base.c | 2 +-
> > > drivers/video/fbdev/platinumfb.c | 4 ++--
> > > drivers/video/fbdev/valkyriefb.c | 4 ++--
> > > 7 files changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/setup_32.c
> > > b/arch/powerpc/kernel/setup_32.c
> > > index e0d045677472..bdbe6acbef11 100644
> > > --- a/arch/powerpc/kernel/setup_32.c
> > > +++ b/arch/powerpc/kernel/setup_32.c
> > > @@ -152,20 +152,18 @@ __setup("l3cr=", ppc_setup_l3cr);
> > >
> > > #ifdef CONFIG_GENERIC_NVRAM
> > >
> > > -unsigned char nvram_read_byte(int addr)
> > > +static unsigned char ppc_nvram_read_byte(int addr)
> > > {
> > > if (ppc_md.nvram_read_val)
> > > return ppc_md.nvram_read_val(addr);
> > > return 0xff;
> > > }
> > > -EXPORT_SYMBOL(nvram_read_byte);
> > >
> > > -void nvram_write_byte(unsigned char val, int addr)
> > > +static void ppc_nvram_write_byte(unsigned char val, int addr)
> > > {
> > > if (ppc_md.nvram_write_val)
> > > ppc_md.nvram_write_val(addr, val);
> > > }
> > > -EXPORT_SYMBOL(nvram_write_byte);
> > >
> > > static ssize_t ppc_nvram_get_size(void)
> > > {
> > > @@ -182,6 +180,8 @@ static long ppc_nvram_sync(void)
> > > }
> > >
> > > const struct nvram_ops arch_nvram_ops = {
> > > + .read_byte = ppc_nvram_read_byte,
> > > + .write_byte = ppc_nvram_write_byte,
> > > .get_size = ppc_nvram_get_size,
> > > .sync = ppc_nvram_sync,
> > > };
> > > diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> > > index f32d5663de95..41b76bf9614e 100644
> > > --- a/drivers/char/generic_nvram.c
> > > +++ b/drivers/char/generic_nvram.c
> > > @@ -48,7 +48,7 @@ static ssize_t read_nvram(struct file *file, char __user
> > > *buf,
> > > if (*ppos >= nvram_len)
> > > return 0;
> > > for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count)
> > > - if (__put_user(nvram_read_byte(i), p))
> > > + if (__put_user(arch_nvram_ops.read_byte(i), p))
> >
> > Instead of modifying all drivers (in this patch and previous ones related to
> > other arches), wouldn't it be better to add helpers like the following in
> > nvram.h:
> >
> > Static inline unsigned char nvram_read_byte(int addr)
> > {
> >return arch_nvram_ops.read_byte(addr);
> > }
> >
>
> Is there some benefit, or is that just personal taste?
>
> Avoiding changes to call sites avoids code review, but I think 1) the
> thinkpad_acpi changes have already been reviewed and 2) the fbdev changes
> need review anyway.
>
> Your suggesion would add several new entities and one extra layer of
> indirection.
>
Contrary to what I said above, this kind of double indirection could be
useful if it allows us to avoid the kind of double indirection which Arnd
objected to (which arises when an arch_nvram_ops method invokes a ppc_md
method). For example,
static inline unsigned char nvram_read_byte(int addr)
{
#ifdef CONFIG_PPC
return ppc_md.nvram_read_byte(addr);
#else
return arch_nvram_ops.read_byte(addr);
#endif
}
I'll try this approach for v9 if there are no objections. It may be the
least invasive approach. Also, arch_nvram_ops can remain const.
--
Re: [PATCH] PCI: Add no-D3 quirk for Mellanox ConnectX-[45]
On Sat, 2019-01-05 at 10:51 -0700, Jason Gunthorpe wrote: > > > Interesting. I've investigated this further, though I don't have as > > many new clues as I'd like. The problem occurs reliably, at least on > > one particular type of machine (a POWER8 "Garrison" with ConnectX-4). > > I don't yet know if it occurs with other machines, I'm having trouble > > getting access to other machines with a suitable card. I didn't > > manage to reproduce it on a different POWER8 machine with a > > ConnectX-5, but I don't know if it's the difference in machine or > > difference in card revision that's important. > > Make sure the card has the latest firmware is always good advice.. > > > So possibilities that occur to me: > > * It's something specific about how the vfio-pci driver uses D3 > > state - have you tried rebinding your device to vfio-pci? > > * It's something specific about POWER, either the kernel or the PCI > > bridge hardware > > * It's something specific about this particular type of machine > > Does the EEH indicate what happend to actually trigger it? In a very cryptic way that requires manual parsing using non-public docs sadly but yes. From the look of it, it's a completion timeout. Looks to me like we don't get a response to a config space access during the change of D state. I don't know if it's the write of the D3 state itself or the read back though (it's probably detected on the read back or a subsequent read, but that doesn't tell me which specific one failed). Some extra logging in OPAL might help pin that down by checking the InA error state in the config accessor after the config write (and polling on it for a while as from a CPU perspective I don't knw if the write is synchronous, probably not). Cheers, Ben.
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-4.21-2 tag
The pull request you sent on Sat, 05 Jan 2019 00:00:03 +1100: > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git > tags/powerpc-4.21-2 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/f1c2f8857c5aa6c92aa903bc06437503422e5925 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-4.21-2 tag
On Sat, Jan 5, 2019 at 3:53 AM Michael Ellerman wrote: > > powerpc: Drop use of 'type' from access_ok() (2019-01-04 23:07:59 +1100) > > I see you've fixed this in your tree already. No problem. It was my mess-up to begin with, and I just did the obvious resolution (only difference between our fixes was whitespace, and that I obviously also fixed up sparc32 as well). Linus
Re: [PATCH] PCI: Add no-D3 quirk for Mellanox ConnectX-[45]
On Fri, Jan 04, 2019 at 02:44:01PM +1100, David Gibson wrote: > On Thu, Dec 06, 2018 at 08:45:09AM +0200, Leon Romanovsky wrote: > > On Thu, Dec 06, 2018 at 03:19:51PM +1100, David Gibson wrote: > > > Mellanox ConnectX-5 IB cards (MT27800) seem to cause a call trace when > > > unbound from their regular driver and attached to vfio-pci in order to > > > pass > > > them through to a guest. > > > > > > This goes away if the disable_idle_d3 option is used, so it looks like a > > > problem with the hardware handling D3 state. To fix that more > > > permanently, > > > use a device quirk to disable D3 state for these devices. > > > > > > We do this by renaming the existing quirk_no_ata_d3() more generally and > > > attaching it to the ConnectX-[45] devices (0x15b3:0x1013). > > > > > > Signed-off-by: David Gibson > > > drivers/pci/quirks.c | 17 +++-- > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > > > > Hi David, > > > > Thank for your patch, > > > > I would like to reproduce the calltrace before moving forward, > > but have trouble to reproduce the original issue. > > > > I'm working with vfio-pci and CX-4/5 cards on daily basis, > > tried manually enter into D3 state now, and it worked for me. > > Interesting. I've investigated this further, though I don't have as > many new clues as I'd like. The problem occurs reliably, at least on > one particular type of machine (a POWER8 "Garrison" with ConnectX-4). > I don't yet know if it occurs with other machines, I'm having trouble > getting access to other machines with a suitable card. I didn't > manage to reproduce it on a different POWER8 machine with a > ConnectX-5, but I don't know if it's the difference in machine or > difference in card revision that's important. Make sure the card has the latest firmware is always good advice.. > So possibilities that occur to me: > * It's something specific about how the vfio-pci driver uses D3 > state - have you tried rebinding your device to vfio-pci? > * It's something specific about POWER, either the kernel or the PCI > bridge hardware > * It's something specific about this particular type of machine Does the EEH indicate what happend to actually trigger it? Jason
Re: use generic DMA mapping code in powerpc V4
Next step: c446404b041130fbd9d1772d184f24715cf2362f (powerpc/dma: remove dma_nommu_mmap_coherent) git clone git://git.infradead.org/users/hch/misc.git -b powerpc-dma.6 a git checkout c446404b041130fbd9d1772d184f24715cf2362f Output: Note: checking out 'c446404b041130fbd9d1772d184f24715cf2362f'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by performing another checkout. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -b with the checkout command again. Example: git checkout -b HEAD is now at c446404... powerpc/dma: remove dma_nommu_mmap_coherent - Link to the Git: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/powerpc-dma.6 Result: PASEMI onboard ethernet works and the X5000 (P5020 board) boots. -- Christian
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-4.21-2 tag
Hi Linus, Michael Ellerman writes: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA1 > > Hi Linus, > > Please pull some powerpc fixes for 4.21: ... > > for you to fetch changes up to 074400a7be61250d9f0ccec07d5c35ffec4d8d22: > > powerpc: Drop use of 'type' from access_ok() (2019-01-04 23:07:59 +1100) I see you've fixed this in your tree already. If you want to skip this pull and avoid having two fixes for the access_ok() stuff in the history that would be fine by me. The rest of this pull request is all very minor and could wait until next week, I could pull out the redundant access_ok() fix and generate a new tag later in the week. cheers > - -- > powerpc fixes for 4.21 #2 > > A fix for the recent access_ok() change, which broke the build. We recently > added a use of type in order to squash a warning elsewhere about type being > unused. > > A handful of other minor build fixes, and one defconfig update. > > Thanks to: > Christian Lamparter, Christophe Leroy, Diana Craciun, Mathieu Malaterre. > > - -- > Christian Lamparter (1): > powerpc/4xx/ocm: Fix compilation error due to PAGE_KERNEL usage > > Diana Craciun (1): > powerpc/fsl: Fixed warning: orphan section `__btb_flush_fixup' > > Mathieu Malaterre (1): > powerpc: Drop use of 'type' from access_ok() > > Michael Ellerman (4): > powerpc/4xx/ocm: Fix phys_addr_t printf warnings > powerpc/configs: Add PPC4xx_OCM to ppc40x_defconfig > KVM: PPC: Book3S HV: radix: Fix uninitialized var build error > Merge branch 'master' into fixes > > > arch/powerpc/configs/ppc40x_defconfig | 1 + > arch/powerpc/include/asm/uaccess.h | 2 +- > arch/powerpc/kernel/head_booke.h | 18 -- > arch/powerpc/kvm/book3s_64_mmu_radix.c | 2 +- > arch/powerpc/platforms/4xx/ocm.c | 10 +- > 5 files changed, 20 insertions(+), 13 deletions(-) > -BEGIN PGP SIGNATURE- > > iQIcBAEBAgAGBQJcL1eOAAoJEFHr6jzI4aWA86wP/3V5zlUNtsyXpcCx60PNIMya > 6z6oWVKoM92nMIyfWuoxKBEbylSyixFSxOsKoiVJDyigW5WWNjsPQ1KOUD2OVn0V > OQhj2aDXwUtxdlQ5dK0H9lodsUKUEw1/saqcgi2OS8iDDa8kJIEemDe29Omf5DyE > U2RKl++nhAfK8BLfMRCAIqL+TVZq9Zy+zNwbVtsmj7lAlw+Csb4KV3YvlJ7b6pjD > Ntm6O3X8sy7GtJdMpeBDOMTbWuvIQpefVH135jS1IjJZbrafZK6a4gQ9cs5mx68d > +/nyzkcjy83eRGkv3QY5Y+vEa0wkFdBb6Nf0MH63xCPVYVjom6pRUGL+RPldi2GO > bmieDXavlicX9y+BCQSKASTzNo2uR2heF31aCnr6bsOSeT8ZNGbIwgHuGGqrzN+8 > qLQb00hA0uKlbNgPpNoyYO5K3khntR348HfH0Uwlm3Nttvn1TEh87c9L19FcdlMw > SlA6kpJ6328FkEv5FUPu7v7764dPgOQNImPKnsYdhbUxhHnDMx33LzsUEQQcYWpM > 3nsKQMW1yqMH2BG8u9LQtZErFanX+RNm81OsP38F8nvGe1RJkoq7R7MSooZ9tzs2 > Ymf8W+KNofKmXVAPh7AyKQzpy0a/sEqAXmtTPBf7bsdwU5PQB0+LMf0yzyZpEp2U > 3eFcseqDoO32ScDZQUYt > =Y+yv > -END PGP SIGNATURE-
