| Message ID | 1319242654-15534-2-git-send-email-marek.vasut@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Oct 21, 2011 at 20:16, Marek Vasut wrote: > The current implementation of debug doesn't play well with GCC4.6. > This implementation also fixes GCC4.6 complaints about unused variables > while maintaining code size. i think this patch should come last rather than first that way you fix all the warnings/failures *before* making the debug() code unconditional > +#define debug_cond(cond, fmt, args...) \ > + do { \ > + if (cond) \ > + printf(fmt, ##args); \ > + } while (0) > + > +#define debug(fmt, args...) \ > + debug_cond(_DEBUG, fmt, ##args) > + > +#define debugX(level, fmt, args...) \ > + debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args) i thought we were just going to punt debugX() ? -mike
Dear Mike Frysinger, In message <CAJaTeTpwpO6Kd-q=bmbzqPJMoNz_YiOWGRks=zGkhes0vUA=mg@mail.gmail.com> you wrote: > > > +#define debugX(level, fmt, args...) \ > > + debug_cond((_DEBUG && DEBUG >=3D (level)), fmt, ##args) > > i thought we were just going to punt debugX() ? Yes, please do! Best regards, Wolfgang Denk
Hi Marek, On Fri, Oct 21, 2011 at 5:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > The current implementation of debug doesn't play well with GCC4.6. > This implementation also fixes GCC4.6 complaints about unused variables > while maintaining code size. > > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Simon Glass <sjg@chromium.org> > --- > include/common.h | 20 ++++++++++++-------- > 1 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/include/common.h b/include/common.h > index eb19a44..c3b23551 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -116,20 +116,24 @@ typedef volatile unsigned char vu_char; > #include <flash.h> > #include <image.h> > > -#ifdef DEBUG > -#define debug(fmt,args...) printf (fmt ,##args) > -#define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args); > -#else > -#define debug(fmt,args...) > -#define debugX(level,fmt,args...) > -#endif /* DEBUG */ > - > #ifdef DEBUG > # define _DEBUG 1 > #else > # define _DEBUG 0 > #endif > > +#define debug_cond(cond, fmt, args...) \ Yes this is much nicer. Could perhaps add a little comment about how to use this and to avoid putting debug() inside #ifdef? > + do { \ > + if (cond) \ > + printf(fmt, ##args); \ > + } while (0) > + > +#define debug(fmt, args...) \ > + debug_cond(_DEBUG, fmt, ##args) > + > +#define debugX(level, fmt, args...) \ > + debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args) > + > /* > * An assertion is run-time check done in debug mode only. If DEBUG is not > * defined then it is skipped. If DEBUG is defined and the assertion fails, > -- > 1.7.6.3 > >
On Monday, October 24, 2011 06:31:19 AM Simon Glass wrote: > Hi Marek, > > On Fri, Oct 21, 2011 at 5:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > > The current implementation of debug doesn't play well with GCC4.6. > > This implementation also fixes GCC4.6 complaints about unused variables > > while maintaining code size. > > > > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com> > > Cc: Wolfgang Denk <wd@denx.de> > > Cc: Simon Glass <sjg@chromium.org> > > --- > > include/common.h | 20 ++++++++++++-------- > > 1 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/include/common.h b/include/common.h > > index eb19a44..c3b23551 100644 > > --- a/include/common.h > > +++ b/include/common.h > > @@ -116,20 +116,24 @@ typedef volatile unsigned char vu_char; > > #include <flash.h> > > #include <image.h> > > > > -#ifdef DEBUG > > -#define debug(fmt,args...) printf (fmt ,##args) > > -#define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args); > > -#else > > -#define debug(fmt,args...) > > -#define debugX(level,fmt,args...) > > -#endif /* DEBUG */ > > - > > #ifdef DEBUG > > # define _DEBUG 1 > > #else > > # define _DEBUG 0 > > #endif > > > > +#define debug_cond(cond, fmt, args...) \ > > Yes this is much nicer. Could perhaps add a little comment about how > to use this and to avoid putting debug() inside #ifdef? > > > + do { \ > > + if (cond) \ > > + printf(fmt, ##args); \ > > + } while (0) > > + > > +#define debug(fmt, args...) \ > > + debug_cond(_DEBUG, fmt, ##args) > > + > > +#define debugX(level, fmt, args...) \ > > + debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args) > > + > > /* > > * An assertion is run-time check done in debug mode only. If DEBUG is > > not * defined then it is skipped. If DEBUG is defined and the assertion > > fails, -- > > 1.7.6.3 Done, I pushed new patchset right now to git://git.denx.de/u-boot-marex.git , "debug" branch. It's around 50 patches now.
Dear Marek Vasut, In message <201110241021.15442.marek.vasut@gmail.com> you wrote: > ... > Done, I pushed new patchset right now to git://git.denx.de/u-boot-marex.git , > "debug" branch. It's around 50 patches now. Keep in mind that nobody cares about patches anywhere in any repository. What has not been sent to the mailing list and thus is captured in the list archives and patchwork simply does not exist. Best regards, Wolfgang Denk
> Dear Marek Vasut, > > In message <201110241021.15442.marek.vasut@gmail.com> you wrote: > > ... > > > Done, I pushed new patchset right now to > > git://git.denx.de/u-boot-marex.git , "debug" branch. It's around 50 > > patches now. > > Keep in mind that nobody cares about patches anywhere in any > repository. What has not been sent to the mailing list and thus is > captured in the list archives and patchwork simply does not exist. Hi Wolfgang, I know this very well, I just need to feedback and the patchset is really huge. I'd wait for you to merge some of the stuff from this patchset (you can merge what you feel fitting but the 0001 patch ...) and then rebase and resubmit the rest on top of that. Cheers
Dear Marek Vasut, In message <201110250211.07058.marek.vasut@gmail.com> you wrote: > > I know this very well, I just need to feedback and the patchset is really huge. What you attempt will not work. You may get some feedback on patches posted on the list. I will_never_ send any feedback to any stuff in some repository anywhere. I will never even try to read it. And I m pretty sure many others work like that, too. If the patch set is huge, then do yourself a favour and break it down into small, managable pieces. Don't try to do everything at once. Send a short sub-set first - maybe some 4...8 patches or so. Maybe even 10. Then let people digest these, and only when they are scheduled for merging start and post the next of your batches. _Nobody_ will review a 50 patch series. And nobdy will apply it either. > I'd wait for you to merge some of the stuff from this patchset (you can merge > what you feel fitting but the 0001 patch ...) and then rebase and resubmit the > rest on top of that. I'd rather you split the series in smaller, digestable batches. Best regards, Wolfgang Denk
> Dear Marek Vasut, > > In message <201110250211.07058.marek.vasut@gmail.com> you wrote: > > I know this very well, I just need to feedback and the patchset is really > > huge. > > What you attempt will not work. > > You may get some feedback on patches posted on the list. > > I will_never_ send any feedback to any stuff in some repository > anywhere. I will never even try to read it. And I m pretty sure many > others work like that, too. I expect that, all right. > > If the patch set is huge, then do yourself a favour and break it down > into small, managable pieces. Don't try to do everything at once. Well I'm not quite sure how you'd like to break a set of patches like this one into smaller chunks ... ? > > Send a short sub-set first - maybe some 4...8 patches or so. Maybe > even 10. Then let people digest these, and only when they are > scheduled for merging start and post the next of your batches. Well ... it's insane. Can you start picking patches from this one series which you feel are ok? I'll rebase the rest on that and submit it. The patches are pretty much independent anyway. > > _Nobody_ will review a 50 patch series. And nobdy will apply it > either. > > > I'd wait for you to merge some of the stuff from this patchset (you can > > merge what you feel fitting but the 0001 patch ...) and then rebase and > > resubmit the rest on top of that. > > I'd rather you split the series in smaller, digestable batches. Well see above. Can you cherrypick something from this series already which you feel fitting ? There's no dependency anyway. > > Best regards, > > Wolfgang Denk Cheers
Hi Marek, On 25/10/11 19:27, Marek Vasut wrote: >> Dear Marek Vasut, >> >> In message <201110250211.07058.marek.vasut@gmail.com> you wrote: >>> I know this very well, I just need to feedback and the patchset is really >>> huge. [snip] >> I'd rather you split the series in smaller, digestable batches. > > Well see above. Can you cherrypick something from this series already which you > feel fitting ? There's no dependency anyway. Why not break it down into /common, /drivers, /arch etc - does that get it down to less than ten per set? /drivers will probably by >10 so you can then go /drivers/serial, /drivers/mtd until there are less than ten - put these in a patchset called /drivers/various Regards, Graeme
> Hi Marek, > > On 25/10/11 19:27, Marek Vasut wrote: > >> Dear Marek Vasut, > >> > >> In message <201110250211.07058.marek.vasut@gmail.com> you wrote: > >>> I know this very well, I just need to feedback and the patchset is > >>> really huge. > > [snip] > > >> I'd rather you split the series in smaller, digestable batches. > > > > Well see above. Can you cherrypick something from this series already > > which you feel fitting ? There's no dependency anyway. > > Why not break it down into /common, /drivers, /arch etc - does that get it > down to less than ten per set? /drivers will probably by >10 so you can > then go /drivers/serial, /drivers/mtd until there are less than ten - put > these in a patchset called /drivers/various This sounds good, yes. Roger that. > > Regards, > > Graeme
Dear Marek Vasut, In message <201110251027.10375.marek.vasut@gmail.com> you wrote: > > > If the patch set is huge, then do yourself a favour and break it down > > into small, managable pieces. Don't try to do everything at once. > > Well I'm not quite sure how you'd like to break a set of patches like this one > into smaller chunks ... ? $ ls | split -l 8 ? :-) > Well ... it's insane. Can you start picking patches from this one series which > you feel are ok? I'll rebase the rest on that and submit it. The patches are > pretty much independent anyway. No, I cannot. I don't have the time to review 50+ patches and pick some that I "feel are ok". > Well see above. Can you cherrypick something from this series already which you > feel fitting ? There's no dependency anyway. Technically I could do that, of course. But within the given limitations of time and resources I will not be able to do that. Sorry, but I ask again to provide small, managable pieces. Best regards, Wolfgang Denk
On Sun, Oct 23, 2011 at 12:12 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Mike Frysinger, > > In message <CAJaTeTpwpO6Kd-q=bmbzqPJMoNz_YiOWGRks=zGkhes0vUA=mg@mail.gmail.com> you wrote: >> >> > +#define debugX(level, fmt, args...) \ >> > + debug_cond((_DEBUG && DEBUG >=3D (level)), fmt, ##args) >> >> i thought we were just going to punt debugX() ? > > Yes, please do! I'm working on a patch for the net code that benefits from the debugX macro. Especially for the net code, the debug traces can be overwhelming depending on the issue you are looking at. I recommend we keep it around. Thanks -Joe
Hi all, 2011/10/26 Joe Hershberger <joe.hershberger@gmail.com>: > On Sun, Oct 23, 2011 at 12:12 PM, Wolfgang Denk <wd@denx.de> wrote: >> Dear Mike Frysinger, >> > I'm working on a patch for the net code that benefits from the debugX > macro. Especially for the net code, the debug traces can be > overwhelming depending on the issue you are looking at. I recommend > we keep it around. > > Thanks > -Joe I agreed with Joe, I feel some debug code is necessary to keep it into levels since some you usually need to split debug level into 2 or 3 to help the debug work. A device driver which has been commit into the mainline source should be worked on most of platforms. However, it still might be need to do debug work on several different platform since the hardware integration might be different then introduce problem. Hence the debug codes in detail should not be removed in the mainline code, and it should be labeled with some debug levels. Those code will help the debug work in future. Moreover, some device driver will need to be fixed when the hardware will be fixed or upgraded and introduce different versions. Those hardware still follow a common behaviors, a common set of registers, even the 50% usage of the driver will be same. Hence keep debug code in deep level will be help. We can keep it until someone really have time to fix it.
Dear =?UTF-8?B?6aas5YWL5rOh?=, In message <CACCg+XP8zoKGproJ4t=QEQU4ZFYccAPPjsGkz72BKBwowcUQng@mail.gmail.com> you wrote: > > > I'm working on a patch for the net code that benefits from the debugX > > macro. Especially for the net code, the debug traces can be > > overwhelming depending on the issue you are looking at. I recommend > > we keep it around. ... > I agreed with Joe, And I disagree. After 11 years of U-Boot development, after collecting 2,515 C files, we have just 2 (in words: TWO) files that use this macro. And one of them with a single level only, so it's even useless there. > I feel some debug code is necessary to keep it into levels since some > you usually need to split debug level into 2 or 3 to help the debug work. Usually? In a single case out of 2.5k files? That's not usual, that's a mistake. I say: we get rid of this! Best regards, Wolfgang Denk
Hi Wolfgang, On Tue, Oct 25, 2011 at 4:59 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear =?UTF-8?B?6aas5YWL5rOh?=, > > In message <CACCg+XP8zoKGproJ4t=QEQU4ZFYccAPPjsGkz72BKBwowcUQng@mail.gmail.com> you wrote: >> >> > I'm working on a patch for the net code that benefits from the debugX >> > macro. Especially for the net code, the debug traces can be >> > overwhelming depending on the issue you are looking at. I recommend >> > we keep it around. > ... >> I agreed with Joe, > > And I disagree. In the net code, I separated out the debug prints based on * internal state changes * any traffic on the network * packets targeted to the device I found this very helpful in debugging to limit the volume of traces depending on what I was looking for. How would you recommend getting similar behavior without debugX? Thanks, Joe
diff --git a/include/common.h b/include/common.h index eb19a44..c3b23551 100644 --- a/include/common.h +++ b/include/common.h @@ -116,20 +116,24 @@ typedef volatile unsigned char vu_char; #include <flash.h> #include <image.h> -#ifdef DEBUG -#define debug(fmt,args...) printf (fmt ,##args) -#define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args); -#else -#define debug(fmt,args...) -#define debugX(level,fmt,args...) -#endif /* DEBUG */ - #ifdef DEBUG # define _DEBUG 1 #else # define _DEBUG 0 #endif +#define debug_cond(cond, fmt, args...) \ + do { \ + if (cond) \ + printf(fmt, ##args); \ + } while (0) + +#define debug(fmt, args...) \ + debug_cond(_DEBUG, fmt, ##args) + +#define debugX(level, fmt, args...) \ + debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args) + /* * An assertion is run-time check done in debug mode only. If DEBUG is not * defined then it is skipped. If DEBUG is defined and the assertion fails,