RE: [PATCH] omap:pm: Fix boot-time errors with debugfs disabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Thursday, May 19, 2011 7:28 PM
> To: Premi, Sanjeev
> Cc: Hilman, Kevin; linux-omap@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with 
> debugfs disabled
> 
> On Thu, May 19, 2011 at 05:30, Premi, Sanjeev <premi@xxxxxx> wrote:
> >> -----Original Message-----
> >> From: Hilman, Kevin
> >> Sent: Wednesday, May 18, 2011 10:04 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH] omap:pm: Fix boot-time errors with
> >> debugfs disabled
> >>
> >> Hi Sanjeev,
> >>
> >> Sanjeev Premi <premi@xxxxxx> writes:
> >>
> >> > There is an implicit assumption in current implementation that
> >> > debugfs is always enabled.
> >> >
> >> > When debugfs is disabled, these errors are noticed during boot:
> >> >   omap_voltage_late_init: Unable to create voltage 
> debugfs main dir
> >> >   vdd_debugfs_init: Unable to create debugfs directory 
> for vdd_mpu
> >> >   vdd_debugfs_init: Unable to create debugfs directory 
> for vdd_core
> >> >
> >> > This patch fixes these errors by enclosing code related 
> to debugfs
> >> > in #ifdef CONFIG_DEBUG_FS..#endif.
> >> >
> >> > Boot tested on OMAP3EVM.
> >> >
> >> > Signed-off-by: Sanjeev Premi <premi@xxxxxx>
> >>
> >> Sorry for the delay here, I've been on the road and finally
> >> catching up
> >> on the list.
> >>
> >> Basically, I'm in the process of a pretty major cleanup of
> >> the VC/VP and
> >> SR layers.  For example, in my pm-wip/voltdm_* branches, 
> the debugfs
> >> interface to the voltage layer has been completely 
> removed.  I'm also
> >> thinking of removing the SR debugfs interface also, as I 
> don't really
> >> think we need a userspace interface for this.  A 
> board-level interface
> >> is probably enough (/me waits for flame from Nishanth :)
> >>
> >> That being said, your approach below isn't quite right in 
> its usage of
> >> #ifdefs.  Use of #ifdefs like this is frowned upon for 
> many reasons.
> >> For starters, take a look the '#ifdefs are ugly' section of
> >> Documentation/SubmittingPatches.
> >
> > [sp] I don't like #ifdefs either but each time we cannot create
> >     a new file changes like this.
> >
> >     The current code is a mess with debugfs used too frequently.
> >     And - all of it is not for debug. The location of ifdefs in
> >     in the patch illustrates it quite well.
> >
> >     BTW, this isn't the only use of ifdefs in a C file in Linux.
> in reality the only reason you've had to do this patch was because we
> had a wicked handling of debugfs entries in voltage layer - with
> voltdm_c these are all gone. further any entries remaining (e.g. SR)
> are:
> dentry for debugfs file -> just a minor overhead not 
> deserving a #ifdef
> all other functions of debugfs (as per include/linux/debugfs.h) when
> debugfs is disabled in .config will be static inlined and we will not
> need any #ifdefs at all
> 
> The real pending question is about functional SR debugfs entries that
> need to loose it's critical functionality.

[sp] good argument for future not the present and past. Fact is that
     "wicked handling of debugfs entries" made their way.

     I am not worried about the patch in or out - few folks who were
     stuck on the issue would have used it anyway. But, whether each
     fix for today can be postponed for future restructuring.

     #ifdefs were just easy target for the postponement.

~sanjeev

> Regards,
> Nishanth Menon
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux