| Message ID | 1465840609-136143-1-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: Sorry for the late reply. I compiled/tested this on linux-next on a Skylake system and it looks ok to me except for a couple of minor comments. > This patch does the following: > - refactors code to use recently introduced DEFINE_DEBUGFS_ATTRIBUTE() macro > - makes absence of DEBUG_FS non-fatal error > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/platform/x86/intel_pmc_core.c | 45 ++++++++--------------------------- > drivers/platform/x86/intel_pmc_core.h | 5 ++-- > 2 files changed, 13 insertions(+), 37 deletions(-) > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > index e57f923..c29f59b 100644 > --- a/drivers/platform/x86/intel_pmc_core.c > +++ b/drivers/platform/x86/intel_pmc_core.c > @@ -23,7 +23,6 @@ > #include <linux/init.h> > #include <linux/io.h> > #include <linux/pci.h> > -#include <linux/seq_file.h> > > #include <asm/cpu_device_id.h> > #include <asm/intel-family.h> > @@ -78,30 +77,18 @@ int intel_pmc_slp_s0_counter_read(u32 *data) > } > EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); > > -#if IS_ENABLED(CONFIG_DEBUG_FS) > -static int pmc_core_dev_state_show(struct seq_file *s, void *unused) > +static int pmc_core_dev_state_get(void *data, u64 *val) > { > - struct pmc_dev *pmcdev = s->private; > - u32 counter_val; > + struct pmc_dev *pmcdev = data; I am hoping that data being passed here is similar to inode.i_private so that we get the address of pmc struct here. > + u32 value; > > - counter_val = pmc_core_reg_read(pmcdev, > - SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > - seq_printf(s, "%u\n", pmc_core_adjust_slp_s0_step(counter_val)); > + value = pmc_core_reg_read(pmcdev, SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > + *val = pmc_core_adjust_slp_s0_step(value); > > return 0; > } > > -static int pmc_core_dev_state_open(struct inode *inode, struct file *file) > -{ > - return single_open(file, pmc_core_dev_state_show, inode->i_private); > -} > - > -static const struct file_operations pmc_core_dev_state_ops = { > - .open = pmc_core_dev_state_open, > - .read = seq_read, > - .llseek = seq_lseek, > - .release = single_release, > -}; > +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu"); > > static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev) > { > @@ -113,12 +100,12 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) > struct dentry *dir, *file; > > dir = debugfs_create_dir("pmc_core", NULL); > - if (!dir) > + if (IS_ERR_OR_NULL(dir)) > return -ENOMEM; > > pmcdev->dbgfs_dir = dir; > file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, Should we consider using debugfs_create_file_unsafe ? > - dir, pmcdev, &pmc_core_dev_state_ops); > + dir, pmcdev, &pmc_core_dev_state); > > if (!file) { > pmc_core_dbgfs_unregister(pmcdev); > @@ -127,16 +114,6 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) > > return 0; > } > -#else > -static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) > -{ > - return 0; > -} > - > -static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev) > -{ > -} > -#endif /* CONFIG_DEBUG_FS */ > > static const struct x86_cpu_id intel_pmc_core_ids[] = { > { X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_MOBILE, X86_FEATURE_MWAIT, > @@ -183,10 +160,8 @@ static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) > } > > err = pmc_core_dbgfs_register(pmcdev); > - if (err < 0) { > - dev_err(&dev->dev, "PMC Core: debugfs register failed.\n"); > - return err; > - } > + if (err < 0) > + dev_warn(&dev->dev, "PMC Core: debugfs register failed.\n"); > > pmc.has_slp_s0_res = true; > return 0; > diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h > index a9dadaf..9689b92 100644 > --- a/drivers/platform/x86/intel_pmc_core.h > +++ b/drivers/platform/x86/intel_pmc_core.h > @@ -23,11 +23,14 @@ > > /* Sunrise Point Power Management Controller PCI Device ID */ > #define SPT_PMC_PCI_DEVICE_ID 0x9d21 > + > #define SPT_PMC_BASE_ADDR_OFFSET 0x48 > #define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET 0x13c > #define SPT_PMC_MMIO_REG_LEN 0x100 > #define SPT_PMC_SLP_S0_RES_COUNTER_STEP 0x64 > > +struct dentry; > + > /** > * struct pmc_dev - pmc device structure > * @base_addr: comtains pmc base address > @@ -42,9 +45,7 @@ > struct pmc_dev { > u32 base_addr; > void __iomem *regbase; > -#if IS_ENABLED(CONFIG_DEBUG_FS) > struct dentry *dbgfs_dir; > -#endif /* CONFIG_DEBUG_FS */ > bool has_slp_s0_res; > }; > > -- > 2.8.1 >
On Thu, 2016-06-23 at 20:56 +0530, Rajneesh Bhardwaj wrote: > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > Sorry for the late reply. I compiled/tested this on linux-next on a > Skylake > system and it looks ok to me except for a couple of minor comments. > > > +static int pmc_core_dev_state_get(void *data, u64 *val) > > { > > - struct pmc_dev *pmcdev = s->private; > > - u32 counter_val; > > + struct pmc_dev *pmcdev = data; > > I am hoping that data being passed here is similar to inode.i_private > so that we get the address of pmc struct here. Yes, I went through the debugfs support code to be sure. > > +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, > > pmc_core_dev_state_get, NULL, "%llu"); > > pmcdev->dbgfs_dir = dir; > > file = debugfs_create_file("slp_s0_residency_usec", S_IFREG > > | S_IRUGO, > > Should we consider using debugfs_create_file_unsafe ? But why? I would choose safest option among others if there is no strong objection and explanation.
On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > This patch does the following: > - refactors code to use recently introduced DEFINE_DEBUGFS_ATTRIBUTE() macro > - makes absence of DEBUG_FS non-fatal error > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/platform/x86/intel_pmc_core.c | 45 ++++++++--------------------------- > drivers/platform/x86/intel_pmc_core.h | 5 ++-- > 2 files changed, 13 insertions(+), 37 deletions(-) > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > index e57f923..c29f59b 100644 > --- a/drivers/platform/x86/intel_pmc_core.c > +++ b/drivers/platform/x86/intel_pmc_core.c > @@ -23,7 +23,6 @@ > #include <linux/init.h> > #include <linux/io.h> > #include <linux/pci.h> > -#include <linux/seq_file.h> > > #include <asm/cpu_device_id.h> > #include <asm/intel-family.h> > @@ -78,30 +77,18 @@ int intel_pmc_slp_s0_counter_read(u32 *data) > } > EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); > > -#if IS_ENABLED(CONFIG_DEBUG_FS) > -static int pmc_core_dev_state_show(struct seq_file *s, void *unused) > +static int pmc_core_dev_state_get(void *data, u64 *val) > { > - struct pmc_dev *pmcdev = s->private; > - u32 counter_val; > + struct pmc_dev *pmcdev = data; > + u32 value; > > - counter_val = pmc_core_reg_read(pmcdev, > - SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > - seq_printf(s, "%u\n", pmc_core_adjust_slp_s0_step(counter_val)); > + value = pmc_core_reg_read(pmcdev, SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > + *val = pmc_core_adjust_slp_s0_step(value); > We were ensuring a new line for printing the output value with seq_printf whereas in this case the output is printed along with the shell prompt. Sometimes this could be harder to read. > return 0; > } > > -static int pmc_core_dev_state_open(struct inode *inode, struct file *file) > -{ > - return single_open(file, pmc_core_dev_state_show, inode->i_private); > -} > - > -static const struct file_operations pmc_core_dev_state_ops = { > - .open = pmc_core_dev_state_open, > - .read = seq_read, > - .llseek = seq_lseek, > - .release = single_release, > -}; > +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu"); > > static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev) > { > @@ -113,12 +100,12 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) > struct dentry *dir, *file; > > dir = debugfs_create_dir("pmc_core", NULL); > - if (!dir) > + if (IS_ERR_OR_NULL(dir)) > return -ENOMEM; > > pmcdev->dbgfs_dir = dir; > file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, > - dir, pmcdev, &pmc_core_dev_state_ops); > + dir, pmcdev, &pmc_core_dev_state); > > if (!file) { > pmc_core_dbgfs_unregister(pmcdev); > @@ -127,16 +114,6 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) > > return 0; > } > -#else > -static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) > -{ > - return 0; > -} > - > -static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev) > -{ > -} > -#endif /* CONFIG_DEBUG_FS */ > > static const struct x86_cpu_id intel_pmc_core_ids[] = { > { X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_MOBILE, X86_FEATURE_MWAIT, > @@ -183,10 +160,8 @@ static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) > } > > err = pmc_core_dbgfs_register(pmcdev); > - if (err < 0) { > - dev_err(&dev->dev, "PMC Core: debugfs register failed.\n"); > - return err; > - } > + if (err < 0) > + dev_warn(&dev->dev, "PMC Core: debugfs register failed.\n"); > > pmc.has_slp_s0_res = true; > return 0; > diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h > index a9dadaf..9689b92 100644 > --- a/drivers/platform/x86/intel_pmc_core.h > +++ b/drivers/platform/x86/intel_pmc_core.h > @@ -23,11 +23,14 @@ > > /* Sunrise Point Power Management Controller PCI Device ID */ > #define SPT_PMC_PCI_DEVICE_ID 0x9d21 > + > #define SPT_PMC_BASE_ADDR_OFFSET 0x48 > #define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET 0x13c > #define SPT_PMC_MMIO_REG_LEN 0x100 > #define SPT_PMC_SLP_S0_RES_COUNTER_STEP 0x64 > > +struct dentry; > + Why do we need this ? > /** > * struct pmc_dev - pmc device structure > * @base_addr: comtains pmc base address > @@ -42,9 +45,7 @@ > struct pmc_dev { > u32 base_addr; > void __iomem *regbase; > -#if IS_ENABLED(CONFIG_DEBUG_FS) > struct dentry *dbgfs_dir; > -#endif /* CONFIG_DEBUG_FS */ > bool has_slp_s0_res; > }; > > -- > 2.8.1 >
On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote: > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > This patch does the following: > > - refactors code to use recently introduced > > DEFINE_DEBUGFS_ATTRIBUTE() macro > > - makes absence of DEBUG_FS non-fatal error > > - counter_val = pmc_core_reg_read(pmcdev, > > - SPT_PMC_SLP_S0_RES_COUNTER_ > > OFFSET); > > - seq_printf(s, "%u\n", > > pmc_core_adjust_slp_s0_step(counter_val)); > > + value = pmc_core_reg_read(pmcdev, > > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > + *val = pmc_core_adjust_slp_s0_step(value); > > > > We were ensuring a new line for printing the output value with > seq_printf > whereas in this case the output is printed along with the shell > prompt. > Sometimes this could be harder to read. This would be a difference in the behaviour to a generic assumption (since this new macro is going to be used by plenty of simple attributes under debugfs). If you still consider it's inappropriate I would recommend to propose a fix globally then. Regarding to change behaviour of the current implementation I think Darren can tell his opinion. (For me at least pros and cons of new format quite obvious). > > > +struct dentry; > > + > > Why do we need this ? Since we are using pointer to undefined type. > > > /** > > * struct pmc_dev - pmc device structure > > * @base_addr: comtains pmc base address > > @@ -42,9 +45,7 @@ > > struct pmc_dev { > > u32 base_addr; > > void __iomem *regbase; > > -#if IS_ENABLED(CONFIG_DEBUG_FS) > > struct dentry *dbgfs_dir; > > -#endif /* CONFIG_DEBUG_FS */
On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote: > > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > > This patch does the following: > > > - refactors code to use recently introduced > > > DEFINE_DEBUGFS_ATTRIBUTE() macro > > > - makes absence of DEBUG_FS non-fatal error > > > > > - counter_val = pmc_core_reg_read(pmcdev, > > > - SPT_PMC_SLP_S0_RES_COUNTER_ > > > OFFSET); > > > - seq_printf(s, "%u\n", > > > pmc_core_adjust_slp_s0_step(counter_val)); > > > + value = pmc_core_reg_read(pmcdev, > > > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > > + *val = pmc_core_adjust_slp_s0_step(value); > > > > > > > We were ensuring a new line for printing the output value with > > seq_printf > > whereas in this case the output is printed along with the shell > > prompt. > > Sometimes this could be harder to read. > > This would be a difference in the behaviour to a generic assumption > (since this new macro is going to be used by plenty of simple attributes > under debugfs). If you still consider it's inappropriate I would > recommend to propose a fix globally then. > > Regarding to change behaviour of the current implementation I think > Darren can tell his opinion. (For me at least pros and cons of new > format quite obvious). > While this is good for simple attributes i see a potential issue with this approch when we want to display multiple attributes for a single debugfs entry. We typically use seq_printf in a loop and display multiline output for a single debugfs entry. How do we go about the scenarios where we want to display formatted multiline debug information? > > > > > +struct dentry; > > > + > > > > Why do we need this ? > > Since we are using pointer to undefined type. > I think we can omit this line if we chose to take this approach. > > > > > /** > > > * struct pmc_dev - pmc device structure > > > * @base_addr: comtains pmc base address > > > @@ -42,9 +45,7 @@ > > > struct pmc_dev { > > > u32 base_addr; > > > void __iomem *regbase; > > > -#if IS_ENABLED(CONFIG_DEBUG_FS) > > > struct dentry *dbgfs_dir; > > > -#endif /* CONFIG_DEBUG_FS */ > > > > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy > -- > To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > > On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote: > > > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > > > This patch does the following: > > > > - refactors code to use recently introduced > > > > DEFINE_DEBUGFS_ATTRIBUTE() macro > > > > - makes absence of DEBUG_FS non-fatal error > > > > > > > > - counter_val = pmc_core_reg_read(pmcdev, > > > > - SPT_PMC_SLP_S0_RES_COUN > > > > TER_ > > > > OFFSET); > > > > - seq_printf(s, "%u\n", > > > > pmc_core_adjust_slp_s0_step(counter_val)); > > > > + value = pmc_core_reg_read(pmcdev, > > > > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > > > + *val = pmc_core_adjust_slp_s0_step(value); > > > > > > > > > > We were ensuring a new line for printing the output value with > > > seq_printf > > > whereas in this case the output is printed along with the shell > > > prompt. > > > Sometimes this could be harder to read. > > > > This would be a difference in the behaviour to a generic assumption > > (since this new macro is going to be used by plenty of simple > > attributes > > under debugfs). If you still consider it's inappropriate I would > > recommend to propose a fix globally then. > > > > Regarding to change behaviour of the current implementation I think > > Darren can tell his opinion. (For me at least pros and cons of new > > format quite obvious). > > > > While this is good for simple attributes i see a potential issue with > this > approch when we want to display multiple attributes for a single > debugfs > entry. > We typically use seq_printf in a loop and display multiline output > for a single debugfs entry. How do we go about the scenarios where we > want > to display formatted multiline debug information? We are talking about abstract case or is it change coming to this very driver? > > > > > > > > +struct dentry; > > > > + > > > > > > Why do we need this ? > > > > Since we are using pointer to undefined type. > > > > I think we can omit this line if we chose to take this approach. Yeah, it does not produce a warning on new compilers, though I think Darren can share his opinion.
On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote: > On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > > > On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote: > > > > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > > > > This patch does the following: > > > > > - refactors code to use recently introduced > > > > > DEFINE_DEBUGFS_ATTRIBUTE() macro > > > > > - makes absence of DEBUG_FS non-fatal error > > > > > > > > > > > - counter_val = pmc_core_reg_read(pmcdev, > > > > > - SPT_PMC_SLP_S0_RES_COUN > > > > > TER_ > > > > > OFFSET); > > > > > - seq_printf(s, "%u\n", > > > > > pmc_core_adjust_slp_s0_step(counter_val)); > > > > > + value = pmc_core_reg_read(pmcdev, > > > > > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > > > > + *val = pmc_core_adjust_slp_s0_step(value); > > > > > > > > > > > > > We were ensuring a new line for printing the output value with > > > > seq_printf > > > > whereas in this case the output is printed along with the shell > > > > prompt. > > > > Sometimes this could be harder to read. > > > > > > This would be a difference in the behaviour to a generic assumption > > > (since this new macro is going to be used by plenty of simple > > > attributes > > > under debugfs). If you still consider it's inappropriate I would > > > recommend to propose a fix globally then. > > > > > > Regarding to change behaviour of the current implementation I think > > > Darren can tell his opinion. (For me at least pros and cons of new > > > format quite obvious). > > > > > > > While this is good for simple attributes i see a potential issue with > > this > > approch when we want to display multiple attributes for a single > > debugfs > > entry. > > We typically use seq_printf in a loop and display multiline output > > for a single debugfs entry. How do we go about the scenarios where we > > want > > to display formatted multiline debug information? > > We are talking about abstract case or is it change coming to this very > driver? > We will be sending few patches for this very driver shortly and one such feature would require multiline formatted output for a single debugfs entry. Can we have seq_printf for such cases and use DEFINE_DEBUGFS_ATTRIBUTE for other simple attributes? Doing this might raise some coding style consistency concerns though so lets wait to hear Darren's opinion on this patch. > > > > > > > > > > > +struct dentry; > > > > > + > > > > > > > > Why do we need this ? > > > > > > Since we are using pointer to undefined type. > > > > > > > I think we can omit this line if we chose to take this approach. > > Yeah, it does not produce a warning on new compilers, though I think > Darren can share his opinion. > Sure. > > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy i> -- > To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-06-29 at 17:46 +0530, Rajneesh Bhardwaj wrote: > On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote: > > On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > > > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > > > While this is good for simple attributes i see a potential issue > > > with > > > this > > > approch when we want to display multiple attributes for a single > > > debugfs > > > entry. > > > We typically use seq_printf in a loop and display multiline > > > output > > > for a single debugfs entry. How do we go about the scenarios where > > > we > > > want > > > to display formatted multiline debug information? > > > > We are talking about abstract case or is it change coming to this > > very > > driver? > > > > We will be sending few patches for this very driver shortly and one > such > feature would require multiline formatted output for a single debugfs > entry. If it touches the same attribute it needs to be seen first before applying this one. > > Can we have seq_printf for such cases and use DEFINE_DEBUGFS_ATTRIBUTE > for > other simple attributes? Doing this might raise some coding style > consistency > concerns though so lets wait to hear Darren's opinion on this patch. For the rest we have to see the implementation and decide how to proceed (since it's a debugfs dedicated for _debugging_) we might change it in the way we like. Though I don't encourage people to do this. I like interfaces on which people thought before implementing.
On Wed, Jun 29, 2016 at 03:48:25PM +0300, Andy Shevchenko wrote: > On Wed, 2016-06-29 at 17:46 +0530, Rajneesh Bhardwaj wrote: > > On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote: > > > On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > > > > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > > > > > While this is good for simple attributes i see a potential issue > > > > with > > > > this > > > > approch when we want to display multiple attributes for a single > > > > debugfs > > > > entry. > > > > We typically use seq_printf in a loop and display multiline > > > > output > > > > for a single debugfs entry. How do we go about the scenarios where > > > > we > > > > want > > > > to display formatted multiline debug information? > > > > > > We are talking about abstract case or is it change coming to this > > > very > > > driver? > > > > > > > We will be sending few patches for this very driver shortly and one > > such > > feature would require multiline formatted output for a single debugfs > > entry. > > If it touches the same attribute it needs to be seen first before > applying this one. > No, this attribute wont be touched. > > > > Can we have seq_printf for such cases and use DEFINE_DEBUGFS_ATTRIBUTE > > for > > other simple attributes? Doing this might raise some coding style > > consistency > > concerns though so lets wait to hear Darren's opinion on this patch. > > For the rest we have to see the implementation and decide how to proceed > (since it's a debugfs dedicated for _debugging_) we might change it in > the way we like. Though I don't encourage people to do this. I like > interfaces on which people thought before implementing. > Sure, i am fine with DEFINE_DEBUGFS_ATTRIBUTE but would like to leave the scope for having seq_printf for future patches i.e. linux/seq_file.h and thus would have a dependency on struct file_operations for seq_read. > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy
On Wed, 2016-06-29 at 18:50 +0530, Rajneesh Bhardwaj wrote: > On Wed, Jun 29, 2016 at 03:48:25PM +0300, Andy Shevchenko wrote: > > On Wed, 2016-06-29 at 17:46 +0530, Rajneesh Bhardwaj wrote: > > > On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote: > > > > On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > > > > > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko > > > > > wrote: > > > > > > > > > If it touches the same attribute it needs to be seen first before > > applying this one. > > > > No, this attribute wont be touched. So, if there is no objections (taking into consideration minors you mentioned) I think the patch can be applied. > Can we have seq_printf for such cases and use > > > DEFINE_DEBUGFS_ATTRIBUTE > > > for > > > other simple attributes? Doing this might raise some coding style > > > consistency > > > concerns though so lets wait to hear Darren's opinion on this > > > patch. > > > > For the rest we have to see the implementation and decide how to > > proceed > > (since it's a debugfs dedicated for _debugging_) we might change it > > in > > the way we like. Though I don't encourage people to do this. I like > > interfaces on which people thought before implementing. > > > > Sure, i am fine with DEFINE_DEBUGFS_ATTRIBUTE but would like to leave > the scope > for having seq_printf for future patches i.e. linux/seq_file.h and > thus would > have a dependency on struct file_operations for seq_read. Of course! No-one is insisting to use only the macro and simple attributes.
On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote: > > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > > This patch does the following: > > > - refactors code to use recently introduced > > > DEFINE_DEBUGFS_ATTRIBUTE() macro > > > - makes absence of DEBUG_FS non-fatal error > > > > > - counter_val = pmc_core_reg_read(pmcdev, > > > - SPT_PMC_SLP_S0_RES_COUNTER_ > > > OFFSET); > > > - seq_printf(s, "%u\n", > > > pmc_core_adjust_slp_s0_step(counter_val)); > > > + value = pmc_core_reg_read(pmcdev, > > > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > > + *val = pmc_core_adjust_slp_s0_step(value); > > > > > > > We were ensuring a new line for printing the output value with > > seq_printf > > whereas in this case the output is printed along with the shell > > prompt. > > Sometimes this could be harder to read. > > This would be a difference in the behaviour to a generic assumption > (since this new macro is going to be used by plenty of simple attributes > under debugfs). If you still consider it's inappropriate I would > recommend to propose a fix globally then. Agreed. > > Regarding to change behaviour of the current implementation I think > Darren can tell his opinion. (For me at least pros and cons of new > format quite obvious). We should use the available subsystem tools for boilerplate reduction whenever possible. Fortunately, debugfs is debug, and we do not require that it provide a consistent user interface like with sysfs (via a version attribute). > > > > > > +struct dentry; > > > + > > > > Why do we need this ? > > Since we are using pointer to undefined type. Erm... why is dentry an undefined type? > > > > > > /** > > > * struct pmc_dev - pmc device structure > > > * @base_addr: comtains pmc base address > > > @@ -42,9 +45,7 @@ > > > struct pmc_dev { > > > u32 base_addr; > > > void __iomem *regbase; > > > -#if IS_ENABLED(CONFIG_DEBUG_FS) > > > struct dentry *dbgfs_dir; > > > -#endif /* CONFIG_DEBUG_FS */ > > > > -- > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy >
On Wed, Jun 29, 2016 at 05:46:04PM +0530, Rajneesh Bhardwaj wrote: > On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote: > > On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > > > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko wrote: > > > > On Tue, 2016-06-28 at 14:35 +0530, Rajneesh Bhardwaj wrote: > > > > > On Mon, Jun 13, 2016 at 08:56:49PM +0300, Andy Shevchenko wrote: > > > > > > This patch does the following: > > > > > > - refactors code to use recently introduced > > > > > > DEFINE_DEBUGFS_ATTRIBUTE() macro > > > > > > - makes absence of DEBUG_FS non-fatal error > > > > > > > > > > > > > > - counter_val = pmc_core_reg_read(pmcdev, > > > > > > - SPT_PMC_SLP_S0_RES_COUN > > > > > > TER_ > > > > > > OFFSET); > > > > > > - seq_printf(s, "%u\n", > > > > > > pmc_core_adjust_slp_s0_step(counter_val)); > > > > > > + value = pmc_core_reg_read(pmcdev, > > > > > > SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); > > > > > > + *val = pmc_core_adjust_slp_s0_step(value); > > > > > > > > > > > > > > > > We were ensuring a new line for printing the output value with > > > > > seq_printf > > > > > whereas in this case the output is printed along with the shell > > > > > prompt. > > > > > Sometimes this could be harder to read. > > > > > > > > This would be a difference in the behaviour to a generic assumption > > > > (since this new macro is going to be used by plenty of simple > > > > attributes > > > > under debugfs). If you still consider it's inappropriate I would > > > > recommend to propose a fix globally then. > > > > > > > > Regarding to change behaviour of the current implementation I think > > > > Darren can tell his opinion. (For me at least pros and cons of new > > > > format quite obvious). > > > > > > > > > > While this is good for simple attributes i see a potential issue with > > > this > > > approch when we want to display multiple attributes for a single > > > debugfs > > > entry. > > > We typically use seq_printf in a loop and display multiline output > > > for a single debugfs entry. How do we go about the scenarios where we > > > want > > > to display formatted multiline debug information? > > > > We are talking about abstract case or is it change coming to this very > > driver? > > > > We will be sending few patches for this very driver shortly and one such > feature would require multiline formatted output for a single debugfs entry. > > Can we have seq_printf for such cases and use DEFINE_DEBUGFS_ATTRIBUTE for > other simple attributes? Doing this might raise some coding style consistency > concerns though so lets wait to hear Darren's opinion on this patch. > > > > > > > > > > > > > > > +struct dentry; > > > > > > + > > > > > > > > > > Why do we need this ? > > > > > > > > Since we are using pointer to undefined type. > > > > > > > > > > I think we can omit this line if we chose to take this approach. > > > > Yeah, it does not produce a warning on new compilers, though I think > > Darren can share his opinion. > > > > Sure. I'm not sure why we need it. dentry isn't debugfs specific. When is it undefined?
On Wed, Jun 29, 2016 at 04:33:50PM +0300, Andy Shevchenko wrote: > On Wed, 2016-06-29 at 18:50 +0530, Rajneesh Bhardwaj wrote: > > On Wed, Jun 29, 2016 at 03:48:25PM +0300, Andy Shevchenko wrote: > > > On Wed, 2016-06-29 at 17:46 +0530, Rajneesh Bhardwaj wrote: > > > > On Wed, Jun 29, 2016 at 02:49:45PM +0300, Andy Shevchenko wrote: > > > > > On Wed, 2016-06-29 at 13:33 +0530, Rajneesh Bhardwaj wrote: > > > > > > On Tue, Jun 28, 2016 at 01:05:07PM +0300, Andy Shevchenko > > > > > > wrote: > > > > > > > > > > > > > > If it touches the same attribute it needs to be seen first before > > > applying this one. > > > > > > > No, this attribute wont be touched. > > So, if there is no objections (taking into consideration minors you > mentioned) I think the patch can be applied. > > > Can we have seq_printf for such cases and use > > > > DEFINE_DEBUGFS_ATTRIBUTE > > > > for > > > > other simple attributes? Doing this might raise some coding style > > > > consistency > > > > concerns though so lets wait to hear Darren's opinion on this > > > > patch. > > > > > > For the rest we have to see the implementation and decide how to > > > proceed > > > (since it's a debugfs dedicated for _debugging_) we might change it > > > in > > > the way we like. Though I don't encourage people to do this. I like > > > interfaces on which people thought before implementing. > > > > > > > Sure, i am fine with DEFINE_DEBUGFS_ATTRIBUTE but would like to leave > > the scope > > for having seq_printf for future patches i.e. linux/seq_file.h and > > thus would > > have a dependency on struct file_operations for seq_read. > > Of course! No-one is insisting to use only the macro and simple > attributes. Agreed. I do wonder if some of the newer debugfs types like array and regset would be sufficient for your needs. Whenever you can reuse an abstraction of the subsystem through higher level interfaces, I encourage you to do so as it reduces code size and potential programming errors.
On Fri, 2016-07-01 at 14:24 -0700, Darren Hart wrote: > > > > +struct dentry; > > > > + > > > > > > Why do we need this ? > > > > Since we are using pointer to undefined type. > > Erm... why is dentry an undefined type? In this header it's undefined. Though I can't get any warning using recent compilers, thus the change is not needed.
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index e57f923..c29f59b 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -23,7 +23,6 @@ #include <linux/init.h> #include <linux/io.h> #include <linux/pci.h> -#include <linux/seq_file.h> #include <asm/cpu_device_id.h> #include <asm/intel-family.h> @@ -78,30 +77,18 @@ int intel_pmc_slp_s0_counter_read(u32 *data) } EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read); -#if IS_ENABLED(CONFIG_DEBUG_FS) -static int pmc_core_dev_state_show(struct seq_file *s, void *unused) +static int pmc_core_dev_state_get(void *data, u64 *val) { - struct pmc_dev *pmcdev = s->private; - u32 counter_val; + struct pmc_dev *pmcdev = data; + u32 value; - counter_val = pmc_core_reg_read(pmcdev, - SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); - seq_printf(s, "%u\n", pmc_core_adjust_slp_s0_step(counter_val)); + value = pmc_core_reg_read(pmcdev, SPT_PMC_SLP_S0_RES_COUNTER_OFFSET); + *val = pmc_core_adjust_slp_s0_step(value); return 0; } -static int pmc_core_dev_state_open(struct inode *inode, struct file *file) -{ - return single_open(file, pmc_core_dev_state_show, inode->i_private); -} - -static const struct file_operations pmc_core_dev_state_ops = { - .open = pmc_core_dev_state_open, - .read = seq_read, - .llseek = seq_lseek, - .release = single_release, -}; +DEFINE_DEBUGFS_ATTRIBUTE(pmc_core_dev_state, pmc_core_dev_state_get, NULL, "%llu"); static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev) { @@ -113,12 +100,12 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) struct dentry *dir, *file; dir = debugfs_create_dir("pmc_core", NULL); - if (!dir) + if (IS_ERR_OR_NULL(dir)) return -ENOMEM; pmcdev->dbgfs_dir = dir; file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO, - dir, pmcdev, &pmc_core_dev_state_ops); + dir, pmcdev, &pmc_core_dev_state); if (!file) { pmc_core_dbgfs_unregister(pmcdev); @@ -127,16 +114,6 @@ static int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) return 0; } -#else -static inline int pmc_core_dbgfs_register(struct pmc_dev *pmcdev) -{ - return 0; -} - -static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev) -{ -} -#endif /* CONFIG_DEBUG_FS */ static const struct x86_cpu_id intel_pmc_core_ids[] = { { X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_MOBILE, X86_FEATURE_MWAIT, @@ -183,10 +160,8 @@ static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id) } err = pmc_core_dbgfs_register(pmcdev); - if (err < 0) { - dev_err(&dev->dev, "PMC Core: debugfs register failed.\n"); - return err; - } + if (err < 0) + dev_warn(&dev->dev, "PMC Core: debugfs register failed.\n"); pmc.has_slp_s0_res = true; return 0; diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h index a9dadaf..9689b92 100644 --- a/drivers/platform/x86/intel_pmc_core.h +++ b/drivers/platform/x86/intel_pmc_core.h @@ -23,11 +23,14 @@ /* Sunrise Point Power Management Controller PCI Device ID */ #define SPT_PMC_PCI_DEVICE_ID 0x9d21 + #define SPT_PMC_BASE_ADDR_OFFSET 0x48 #define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET 0x13c #define SPT_PMC_MMIO_REG_LEN 0x100 #define SPT_PMC_SLP_S0_RES_COUNTER_STEP 0x64 +struct dentry; + /** * struct pmc_dev - pmc device structure * @base_addr: comtains pmc base address @@ -42,9 +45,7 @@ struct pmc_dev { u32 base_addr; void __iomem *regbase; -#if IS_ENABLED(CONFIG_DEBUG_FS) struct dentry *dbgfs_dir; -#endif /* CONFIG_DEBUG_FS */ bool has_slp_s0_res; };
This patch does the following: - refactors code to use recently introduced DEFINE_DEBUGFS_ATTRIBUTE() macro - makes absence of DEBUG_FS non-fatal error Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/platform/x86/intel_pmc_core.c | 45 ++++++++--------------------------- drivers/platform/x86/intel_pmc_core.h | 5 ++-- 2 files changed, 13 insertions(+), 37 deletions(-)