| Message ID | 20160722052336.3340-1-ruscur@russell.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jul 22, 2016 at 03:23:36PM +1000, Russell Currey wrote: >On EEH events the kernel will print a dump of relevant registers. >If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform >doesn't have EEH support, etc) this information isn't readily available. > >Add a new debugfs handler to trigger a PHB register dump, so that this >information can be made available on demand. > >Signed-off-by: Russell Currey <ruscur@russell.cc> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >--- > arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > >diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >index 891fc4a..ada2f3c 100644 >--- a/arch/powerpc/platforms/powernv/pci-ioda.c >+++ b/arch/powerpc/platforms/powernv/pci-ioda.c >@@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe) > } > } > >+#ifdef CONFIG_DEBUG_FS >+static ssize_t pnv_pci_debug_write(struct file *filp, >+ const char __user *user_buf, >+ size_t count, loff_t *ppos) >+{ >+ struct pci_controller *hose = filp->private_data; >+ struct pnv_phb *phb; >+ int ret = 0; Needn't initialize @ret in advance. The code might be simpler, but it's only a personal preference: struct pci_controller *hose = filp->private_data; struct pnv_phb *phb = hose ? hose->private_data : NULL; if (!phb) return -ENODEV; >+ >+ if (!hose) >+ return -EFAULT; >+ >+ phb = hose->private_data; >+ if (!phb) >+ return -EFAULT; >+ >+ ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob, >+ PNV_PCI_DIAG_BUF_SIZE); >+ >+ if (!ret) >+ pnv_pci_dump_phb_diag_data(phb->hose, phb->diag.blob); >+ >+ return ret < 0 ? ret : count; return ret == OPAL_SUCCESS ? count : -EIO; >+} >+ >+static const struct file_operations pnv_pci_debug_ops = { >+ .open = simple_open, >+ .llseek = no_llseek, >+ .write = pnv_pci_debug_write, It might be reasonable to dump the diag-data on read if it is trying to do it on write. >+}; >+#endif /* CONFIG_DEBUG_FS */ >+ > static void pnv_pci_ioda_create_dbgfs(void) > { > #ifdef CONFIG_DEBUG_FS >@@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void) > if (!phb->dbgfs) > pr_warning("%s: Error on creating debugfs on PHB#%x\n", > __func__, hose->global_number); >+ >+ debugfs_create_file("regdump", 0200, phb->dbgfs, hose, >+ &pnv_pci_debug_ops); "diag-data" might be indicating or a better one you can name :) Thanks, Gavin > } > #endif /* CONFIG_DEBUG_FS */ > } >-- >2.9.0 >
On 07/21/2016 11:36 PM, Gavin Shan wrote: > On Fri, Jul 22, 2016 at 03:23:36PM +1000, Russell Currey wrote: >> On EEH events the kernel will print a dump of relevant registers. >> If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform >> doesn't have EEH support, etc) this information isn't readily available. >> >> Add a new debugfs handler to trigger a PHB register dump, so that this >> information can be made available on demand. >> >> Signed-off-by: Russell Currey <ruscur@russell.cc> > > Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > >> --- >> arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++++++++++++++++++++++ >> 1 file changed, 35 insertions(+) >> >> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >> index 891fc4a..ada2f3c 100644 >> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >> @@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe) >> } >> } >> >> +#ifdef CONFIG_DEBUG_FS >> +static ssize_t pnv_pci_debug_write(struct file *filp, >> + const char __user *user_buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct pci_controller *hose = filp->private_data; >> + struct pnv_phb *phb; >> + int ret = 0; > > Needn't initialize @ret in advance. The code might be simpler, but it's > only a personal preference: I believe its actually preferred that it not be initialized in advance so that the tooling can warn you about conditional code paths where you may have forgotten to set a value. Or as Gavin suggests to explicitly use error values in the return statements. -Tyrel > > struct pci_controller *hose = filp->private_data; > struct pnv_phb *phb = hose ? hose->private_data : NULL; > > if (!phb) > return -ENODEV; > >> + >> + if (!hose) >> + return -EFAULT; >> + >> + phb = hose->private_data; >> + if (!phb) >> + return -EFAULT; >> + >> + ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob, >> + PNV_PCI_DIAG_BUF_SIZE); >> + >> + if (!ret) >> + pnv_pci_dump_phb_diag_data(phb->hose, phb->diag.blob); >> + >> + return ret < 0 ? ret : count; > > return ret == OPAL_SUCCESS ? count : -EIO; > >> +} >> + >> +static const struct file_operations pnv_pci_debug_ops = { >> + .open = simple_open, >> + .llseek = no_llseek, >> + .write = pnv_pci_debug_write, > > It might be reasonable to dump the diag-data on read if it is trying > to do it on write. > >> +}; >> +#endif /* CONFIG_DEBUG_FS */ >> + >> static void pnv_pci_ioda_create_dbgfs(void) >> { >> #ifdef CONFIG_DEBUG_FS >> @@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void) >> if (!phb->dbgfs) >> pr_warning("%s: Error on creating debugfs on PHB#%x\n", >> __func__, hose->global_number); >> + >> + debugfs_create_file("regdump", 0200, phb->dbgfs, hose, >> + &pnv_pci_debug_ops); > > "diag-data" might be indicating or a better one you can name :) > > Thanks, > Gavin > >> } >> #endif /* CONFIG_DEBUG_FS */ >> } >> -- >> 2.9.0 >> > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
On Mon, Jul 25, 2016 at 10:53:49AM -0700, Tyrel Datwyler wrote: >On 07/21/2016 11:36 PM, Gavin Shan wrote: >> On Fri, Jul 22, 2016 at 03:23:36PM +1000, Russell Currey wrote: >>> On EEH events the kernel will print a dump of relevant registers. >>> If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform >>> doesn't have EEH support, etc) this information isn't readily available. >>> >>> Add a new debugfs handler to trigger a PHB register dump, so that this >>> information can be made available on demand. >>> >>> Signed-off-by: Russell Currey <ruscur@russell.cc> >> >> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> >>> --- >>> arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++++++++++++++++++++++ >>> 1 file changed, 35 insertions(+) >>> >>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>> index 891fc4a..ada2f3c 100644 >>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>> @@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe) >>> } >>> } >>> >>> +#ifdef CONFIG_DEBUG_FS >>> +static ssize_t pnv_pci_debug_write(struct file *filp, >>> + const char __user *user_buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct pci_controller *hose = filp->private_data; >>> + struct pnv_phb *phb; >>> + int ret = 0; >> >> Needn't initialize @ret in advance. The code might be simpler, but it's >> only a personal preference: > >I believe its actually preferred that it not be initialized in advance >so that the tooling can warn you about conditional code paths where you >may have forgotten to set a value. Or as Gavin suggests to explicitly >use error values in the return statements. > Yeah, the data type should be int64_t as well. Thanks, Gavin
On Fri, 2016-07-22 at 16:36 +1000, Gavin Shan wrote: > On Fri, Jul 22, 2016 at 03:23:36PM +1000, Russell Currey wrote: > > > > On EEH events the kernel will print a dump of relevant registers. > > If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform > > doesn't have EEH support, etc) this information isn't readily available. > > > > Add a new debugfs handler to trigger a PHB register dump, so that this > > information can be made available on demand. > > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> Hi Gavin, thanks for the review. > > > > > --- > > arch/powerpc/platforms/powernv/pci-ioda.c | 35 > > +++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c > > b/arch/powerpc/platforms/powernv/pci-ioda.c > > index 891fc4a..ada2f3c 100644 > > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > > @@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe > > *pe) > > } > > } > > > > +#ifdef CONFIG_DEBUG_FS > > +static ssize_t pnv_pci_debug_write(struct file *filp, > > + const char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct pci_controller *hose = filp->private_data; > > + struct pnv_phb *phb; > > + int ret = 0; > > Needn't initialize @ret in advance. The code might be simpler, but it's > only a personal preference: > > struct pci_controller *hose = filp->private_data; > struct pnv_phb *phb = hose ? hose->private_data : NULL; > > if (!phb) > return -ENODEV; > > > > > + > > + if (!hose) > > + return -EFAULT; > > + > > + phb = hose->private_data; > > + if (!phb) > > + return -EFAULT; > > + > > + ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob, > > + PNV_PCI_DIAG_BUF_SIZE); > > + > > + if (!ret) > > + pnv_pci_dump_phb_diag_data(phb->hose, phb->diag.blob); > > + > > + return ret < 0 ? ret : count; > > return ret == OPAL_SUCCESS ? count : -EIO; Yeah, that's much better. > > > > > +} > > + > > +static const struct file_operations pnv_pci_debug_ops = { > > + .open = simple_open, > > + .llseek = no_llseek, > > + .write = pnv_pci_debug_write, > > It might be reasonable to dump the diag-data on read if it is trying > to do it on write. I'm not sure about this one. I went with write since (at least, in my mind) writing to a file feels like triggering an action, although we're not actually reading any input. It also means that it works the same way as the other PHB debugfs entries (i.e. errinjct). I could rework it into a read that said something like "PHB#%x diag data dumped, check the kernel log", what do you think? > > > > > +}; > > +#endif /* CONFIG_DEBUG_FS */ > > + > > static void pnv_pci_ioda_create_dbgfs(void) > > { > > #ifdef CONFIG_DEBUG_FS > > @@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void) > > if (!phb->dbgfs) > > pr_warning("%s: Error on creating debugfs on PHB#%x\n", > > __func__, hose->global_number); > > + > > + debugfs_create_file("regdump", 0200, phb->dbgfs, hose, > > + &pnv_pci_debug_ops); > > "diag-data" might be indicating or a better one you can name :) > > Thanks, > Gavin > > > > > } > > #endif /* CONFIG_DEBUG_FS */ > > } > > -- > > 2.9.0 > >
Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes: > On 07/21/2016 11:36 PM, Gavin Shan wrote: >> On Fri, Jul 22, 2016 at 03:23:36PM +1000, Russell Currey wrote: >>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>> index 891fc4a..ada2f3c 100644 >>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>> @@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe) >>> } >>> } >>> >>> +#ifdef CONFIG_DEBUG_FS >>> +static ssize_t pnv_pci_debug_write(struct file *filp, >>> + const char __user *user_buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct pci_controller *hose = filp->private_data; >>> + struct pnv_phb *phb; >>> + int ret = 0; >> >> Needn't initialize @ret in advance. The code might be simpler, but it's >> only a personal preference: > > I believe its actually preferred that it not be initialized in advance > so that the tooling can warn you about conditional code paths where you > may have forgotten to set a value. Yeah that's right, it's preferable not to initialise it. It helps for complex if/else/switch cases, where you might accidentally have a path where you return without giving ret the right value. The other case is when someone modifies your code. For example if you have: int ret; if (foo) ret = do_foo(); else ret = 1; return ret; And then you add a case to the if: if (foo) ret = do_foo(); else if (bar) do_bar(); else ret = 1; The compiler will warn you that in the bar case you forget to initialise ret. Whereas if you initialised ret at the start then the compiler can't help you. There are times when it's cleaner to initialise the value at the start, eg. if you have many error cases and only one success case. But that should be a deliberate choice. cheers
Quoting Russell Currey (2016-07-22 15:23:36) > On EEH events the kernel will print a dump of relevant registers. > If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform > doesn't have EEH support, etc) this information isn't readily available. > > Add a new debugfs handler to trigger a PHB register dump, so that this > information can be made available on demand. This is a bit weird. It's a debugfs file, but when you read from it you get nothing (I think, you have no read() defined). When you write to it, regardless of what you write, the kernel spits some stuff out to dmesg and throws away whatever you wrote. Ideally pnv_pci_dump_phb_diag_data() would write its output to a buffer, which we could then either send to dmesg, or give to debugfs. But that might be more work than we want to do for this. If we just want a trigger file, then I think it'd be preferable to just use a simple attribute, with a set and no show, eg. something like: static int foo_set(void *data, u64 val) { if (val != 1) return -EINVAL; ... return 0; } DEFINE_SIMPLE_ATTRIBUTE(fops_foo, NULL, foo_set, "%llu\n"); That requires that you write "1" to the file to trigger the reg dump. > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index 891fc4a..ada2f3c 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void) > if (!phb->dbgfs) > pr_warning("%s: Error on creating debugfs on PHB#%x\n", > __func__, hose->global_number); > + > + debugfs_create_file("regdump", 0200, phb->dbgfs, hose, > + &pnv_pci_debug_ops); > } You shouldn't be trying to create the file if the directory create failed. So the check for (!phb->dbgfs) should probably print and then continue. And a better name would be "dump-regs", because it indicates that the file does something, rather than is something. cheers
On Tue, 2016-07-26 at 11:45 +1000, Michael Ellerman wrote: > Quoting Russell Currey (2016-07-22 15:23:36) > > > > On EEH events the kernel will print a dump of relevant registers. > > If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform > > doesn't have EEH support, etc) this information isn't readily available. > > > > Add a new debugfs handler to trigger a PHB register dump, so that this > > information can be made available on demand. > > This is a bit weird. > > It's a debugfs file, but when you read from it you get nothing (I think, > you have no read() defined). > > When you write to it, regardless of what you write, the kernel spits > some stuff out to dmesg and throws away whatever you wrote. > > Ideally pnv_pci_dump_phb_diag_data() would write its output to a buffer, > which we could then either send to dmesg, or give to debugfs. But that > might be more work than we want to do for this. > > If we just want a trigger file, then I think it'd be preferable to just > use a simple attribute, with a set and no show, eg. something like: > > static int foo_set(void *data, u64 val) > { > if (val != 1) > return -EINVAL; > > ... > > return 0; > } > > DEFINE_SIMPLE_ATTRIBUTE(fops_foo, NULL, foo_set, "%llu\n"); > > That requires that you write "1" to the file to trigger the reg dump. I don't think I can use this here. Triggering the diag dump on the given PHB (these are in /sys/kernel/debug/powerpc/PCI####), and that PHB is retrieved from the file handler. It looks like I have no access to the file struct if using a simple getter/setter. > > > > > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c > > b/arch/powerpc/platforms/powernv/pci-ioda.c > > index 891fc4a..ada2f3c 100644 > > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > > @@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void) > > if (!phb->dbgfs) > > pr_warning("%s: Error on creating debugfs on > > PHB#%x\n", > > __func__, hose->global_number); > > + > > + debugfs_create_file("regdump", 0200, phb->dbgfs, hose, > > + &pnv_pci_debug_ops); > > } > > You shouldn't be trying to create the file if the directory create failed. So > the check for (!phb->dbgfs) should probably print and then continue. Good catch. > > And a better name would be "dump-regs", because it indicates that the file > does > something, rather than is something. That is indeed better. > > cheers
Russell Currey <ruscur@russell.cc> writes: > On Tue, 2016-07-26 at 11:45 +1000, Michael Ellerman wrote: >> Quoting Russell Currey (2016-07-22 15:23:36) >> >> DEFINE_SIMPLE_ATTRIBUTE(fops_foo, NULL, foo_set, "%llu\n"); >> >> That requires that you write "1" to the file to trigger the reg dump. > > I don't think I can use this here. Triggering the diag dump on the given PHB > (these are in /sys/kernel/debug/powerpc/PCI####), and that PHB is retrieved from > the file handler. It looks like I have no access to the file struct if using a > simple getter/setter. You don't have access to the file struct, but you don't need it, can register the fops with a data pointer. So the DEFINE_SIMPLE_ATTRIBUTE() gives you a fops_foo, which you can then do: debugfs_create_file("dump-regs", 0200, phb->dbgfs, hose, &fops_foo)) And then in foo_set() data == hose. cheers
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 891fc4a..ada2f3c 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe) } } +#ifdef CONFIG_DEBUG_FS +static ssize_t pnv_pci_debug_write(struct file *filp, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pci_controller *hose = filp->private_data; + struct pnv_phb *phb; + int ret = 0; + + if (!hose) + return -EFAULT; + + phb = hose->private_data; + if (!phb) + return -EFAULT; + + ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob, + PNV_PCI_DIAG_BUF_SIZE); + + if (!ret) + pnv_pci_dump_phb_diag_data(phb->hose, phb->diag.blob); + + return ret < 0 ? ret : count; +} + +static const struct file_operations pnv_pci_debug_ops = { + .open = simple_open, + .llseek = no_llseek, + .write = pnv_pci_debug_write, +}; +#endif /* CONFIG_DEBUG_FS */ + static void pnv_pci_ioda_create_dbgfs(void) { #ifdef CONFIG_DEBUG_FS @@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void) if (!phb->dbgfs) pr_warning("%s: Error on creating debugfs on PHB#%x\n", __func__, hose->global_number); + + debugfs_create_file("regdump", 0200, phb->dbgfs, hose, + &pnv_pci_debug_ops); } #endif /* CONFIG_DEBUG_FS */ }
On EEH events the kernel will print a dump of relevant registers. If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform doesn't have EEH support, etc) this information isn't readily available. Add a new debugfs handler to trigger a PHB register dump, so that this information can be made available on demand. Signed-off-by: Russell Currey <ruscur@russell.cc> --- arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)