All of lore.kernel.org
 help / color / mirror / Atom feed
From: Valentin Rothberg <valentinrothberg@gmail.com>
To: Rob Clark <robdclark@gmail.com>
Cc: Paul Bolle <pebolle@tiscali.nl>,
	Greg KH <gregkh@linuxfoundation.org>,
	Hai Li <hali@codeaurora.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	David Airlie <airlied@linux.ie>,
	rupran@einserver.de, stefan.hengelein@fau.de
Subject: Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING
Date: Fri, 10 Apr 2015 08:04:30 +0200	[thread overview]
Message-ID: <20150410060430.GA11962@station> (raw)
In-Reply-To: <CAF6AEGsnFqJ9CSSv-tEPW1cqaTSp90GohkvO=9+06tzgMx9o8A@mail.gmail.com>

On Thu, Apr 09, 2015 at 04:20:45PM -0400, Rob Clark wrote:
> On Thu, Apr 9, 2015 at 3:44 PM, Valentin Rothberg
> <valentinrothberg@gmail.com> wrote:
> > On Thu, Apr 09, 2015 at 02:54:29PM -0400, Rob Clark wrote:
> >> On Thu, Apr 9, 2015 at 2:12 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> >> > On Thu, 2015-04-09 at 19:07 +0200, Greg KH wrote:
> >> >> I really don't understand.  Why is this code in the kernel tree if it
> >> >> can't be built?  How does anyone use this?  By taking it and copying it
> >> >> where?  If it can't be built, and no one can update it, and of course
> >> >> not run it, why is it here?  What good is this code doing sitting here?
> >> >
> >> > The Erlangen bot (courtesy of Valentin, Stefan, and Andreas) has taken
> >> > over what I've been doing for quite some time, but doing it much more
> >> > thoroughly. And my experience tells me that the reports they'll send in
> >> > will trigger more discussions like this one.
> >> >
> >> > A lesson I learned from my daily checks for Kconfig oddities is that
> >> > people go to great lengths defending unbuildable code. (Do a web search
> >> > for ATHEROS_AR231X to find a discussion that dragged on for over three
> >> > years!) Personally I stopped caring after someone insisted on having a
> >> > file in the tree that was in no way connected to the build system: not a
> >> > single line in any of the Makefiles pointed at it. So, as far as I'm
> >> > concerned, if people can't point at a patch pending, somehow, somewhere,
> >> > that would make their code buildable one might as well delete the code.
> >> >
> >> > I really think it's as simple as that.
> >> >
> >>
> >> In the example you reference, sure it is as simple as that.  But here
> >> we are not talking about files that aren't even referenced by build
> >> system.  We are talking about a driver which does build and run on
> >> upstream kernel, and which has a few small #ifdef blocks to simplify
> >> backporting to downstream kernels (which we still do need to use for
> >> some generations and some devices)
> >>
> >> Sure, I'd love never to have to deal with a downstream kernel.  But
> >> really.. I didn't create the downstream mess in the arm/android
> >> ecosystem, I'm just trying to cope with it as best as possible.. don't
> >> hate the player, hate the game :-P
> >
> > I really understand your point.  But I also see conflicting interests.
> >
> > The goal of static analysis tools such as Paul's scripts, undertaker or
> > scripts/checkkconfigsymbols.py is to detect and ideally avoid certain
> > kind of bugs.  Having to deal with intentional dead code or entirely
> > dead files makes such analysis quite challenging.  The main issue for
> > the tools is that as soon as there is a CONFIG_ prefixed identifier, it
> > will be treated as a Kconfig variable.  Strictly speaking, it's
> > violating the Kconfig naming convention for the upstream case.
> >
> > Then there is another issue maintaining the code, studying the code,
> > making any kind of analysis.  How should people know which code is meant
> > for upstream, downstream or other streams?  Currently I am working on
> > detecting deprecated functions, data types, etc.  If there were too many
> > of such downstream #ifdefs, it would inherently complicate affords.
> 
> Hmm, admittedly, I hadn't really considered the static analysis case
> before today..
> 
> If at all possible, I would like to keep those, at least for the time
> being, since it is one less thing for me to mess up on backports.
> 
> Not sure if a comment tag could help make things clear (for humans and
> tools), ie.
> 
> #ifdef CONFIG_FOO
> /* downstream bonghits */
> ...
> #endif

The main problem for those analyzers is the 'CONFIG_' prefix.  This
prefix is reserved for Kconfig options only.  Using #ifdef FOO instead
avoids tools to run into this trap.  A comment would also be very
helpful.  The tools would be happy then : )

Kind regards,
 Valentin

> no idea if that would be trivial or difficult to implement?  If the
> latter, I can drop those parts of the code.  But if at all possible,
> I'm always a fan of giving myself less things to screw up.
> 
> > So I try to discourage such cases for the aforementioned reasons.  But
> > that's just my humble opinion and for sure my own interests : )
> >
> > In any case, thank you a lot for taking the time explain everything in
> > such nice detail.  I learned a lot!
> 
> No problem, and thanks for your work
> 
> BR,
> -R
> 
> > Kind regards,
> >  Valentin
> >
> >>
> >> BR,
> >> -R
> >>
> >> >
> >> > Paul Bolle
> >> >

      reply	other threads:[~2015-04-10  6:04 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-09 11:22 drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING Valentin Rothberg
2015-04-09 13:49 ` Rob Clark
2015-04-09 13:49   ` Rob Clark
2015-04-09 14:20   ` Greg KH
2015-04-09 14:50     ` Rob Clark
2015-04-09 14:50       ` Rob Clark
2015-04-09 17:07       ` Greg KH
2015-04-09 18:02         ` Rob Clark
2015-04-09 18:02           ` Rob Clark
2015-04-09 18:12         ` Paul Bolle
2015-04-09 18:54           ` Rob Clark
2015-04-09 18:54             ` Rob Clark
2015-04-09 19:38             ` Paul Bolle
2015-04-09 19:38               ` Paul Bolle
2015-04-09 19:44             ` Valentin Rothberg
2015-04-09 20:20               ` Rob Clark
2015-04-09 20:20                 ` Rob Clark
2015-04-10  6:04                 ` Valentin Rothberg [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150410060430.GA11962@station \
    --to=valentinrothberg@gmail.com \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hali@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pebolle@tiscali.nl \
    --cc=robdclark@gmail.com \
    --cc=rupran@einserver.de \
    --cc=stefan.hengelein@fau.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.