* drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING @ 2015-04-09 11:22 Valentin Rothberg 2015-04-09 13:49 ` Rob Clark 0 siblings, 1 reply; 12+ messages in thread From: Valentin Rothberg @ 2015-04-09 11:22 UTC (permalink / raw) To: hali Cc: dri-devel, linux-kernel, airlied, robdclark, pebolle, rupran, stefan.hengelein Hi Hai, your commit d5af49c92a8a ("drm/msm/mdp5: Enable DSI connector in msm drm driver") in today's Linux next tree adds an #ifdef with CONFIG_MSM_BUS_SCALING as condition. MSM_BUS_SCALING is not defined in Kconfig, so the code in this #ifdef block won't be compiled at its current state. I saw some references on this Kconfig option in other files; is there a reason for the absence of MSM_BUS_SCALING? I found this issue with ./scripts/checkkconfigsymbols.py by diffing yesterday's and today's next tree. Kind regards, Valentin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 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 14:20 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Rob Clark @ 2015-04-09 13:49 UTC (permalink / raw) To: Valentin Rothberg Cc: Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, Paul Bolle, rupran, stefan.hengelein On Thu, Apr 9, 2015 at 7:22 AM, Valentin Rothberg <valentinrothberg@gmail.com> wrote: > Hi Hai, > > your commit d5af49c92a8a ("drm/msm/mdp5: Enable DSI connector in msm drm > driver") in today's Linux next tree adds an #ifdef with CONFIG_MSM_BUS_SCALING > as condition. MSM_BUS_SCALING is not defined in Kconfig, so the code in this > #ifdef block won't be compiled at its current state. > > I saw some references on this Kconfig option in other files; is there a > reason for the absence of MSM_BUS_SCALING? right now, it is something that only exists in downstream kernels (for example, android device kernels).. but in those kernels it is mandatory to use, as by default the memory/bus is downclocked and the display would underflow if we did not request sufficient bandwidth. It only exists right now in the upstream kernel to simplify backporting to various device kernels BR, -R > I found this issue with ./scripts/checkkconfigsymbols.py by diffing yesterday's > and today's next tree. > > Kind regards, > Valentin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 13:49 ` Rob Clark @ 2015-04-09 14:20 ` Greg KH 2015-04-09 14:50 ` Rob Clark 0 siblings, 1 reply; 12+ messages in thread From: Greg KH @ 2015-04-09 14:20 UTC (permalink / raw) To: Rob Clark Cc: Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, Paul Bolle, rupran, stefan.hengelein On Thu, Apr 09, 2015 at 09:49:58AM -0400, Rob Clark wrote: > On Thu, Apr 9, 2015 at 7:22 AM, Valentin Rothberg > <valentinrothberg@gmail.com> wrote: > > Hi Hai, > > > > your commit d5af49c92a8a ("drm/msm/mdp5: Enable DSI connector in msm drm > > driver") in today's Linux next tree adds an #ifdef with CONFIG_MSM_BUS_SCALING > > as condition. MSM_BUS_SCALING is not defined in Kconfig, so the code in this > > #ifdef block won't be compiled at its current state. > > > > I saw some references on this Kconfig option in other files; is there a > > reason for the absence of MSM_BUS_SCALING? > > right now, it is something that only exists in downstream kernels (for > example, android device kernels).. but in those kernels it is > mandatory to use, as by default the memory/bus is downclocked and the > display would underflow if we did not request sufficient bandwidth. > > It only exists right now in the upstream kernel to simplify > backporting to various device kernels That's crazy. You are asking upstream to maintain code in order to just make out of tree crap easier to maintain, which you don't have any plan to ever upstream? That causes havoc on static analysis tools and prevents anyone from ever being able to even change the code for new api changes and test build it. If this was in a subsystem that I maintain, I'd delete it tomorrow. But in the end, it's up to David to decide if he wants to waste the cycles or not. Ick ick ick. greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 14:20 ` Greg KH @ 2015-04-09 14:50 ` Rob Clark 2015-04-09 17:07 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Rob Clark @ 2015-04-09 14:50 UTC (permalink / raw) To: Greg KH Cc: Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, Paul Bolle, rupran, stefan.hengelein On Thu, Apr 9, 2015 at 10:20 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 09, 2015 at 09:49:58AM -0400, Rob Clark wrote: >> On Thu, Apr 9, 2015 at 7:22 AM, Valentin Rothberg >> <valentinrothberg@gmail.com> wrote: >> > Hi Hai, >> > >> > your commit d5af49c92a8a ("drm/msm/mdp5: Enable DSI connector in msm drm >> > driver") in today's Linux next tree adds an #ifdef with CONFIG_MSM_BUS_SCALING >> > as condition. MSM_BUS_SCALING is not defined in Kconfig, so the code in this >> > #ifdef block won't be compiled at its current state. >> > >> > I saw some references on this Kconfig option in other files; is there a >> > reason for the absence of MSM_BUS_SCALING? >> >> right now, it is something that only exists in downstream kernels (for >> example, android device kernels).. but in those kernels it is >> mandatory to use, as by default the memory/bus is downclocked and the >> display would underflow if we did not request sufficient bandwidth. >> >> It only exists right now in the upstream kernel to simplify >> backporting to various device kernels > > That's crazy. You are asking upstream to maintain code in order to just > make out of tree crap easier to maintain, which you don't have any plan > to ever upstream? That causes havoc on static analysis tools and > prevents anyone from ever being able to even change the code for new api > changes and test build it. Hey, don't blame me for the downstream kernels. But at various points in time I've had to backport drm/msm to various device kernels in order to work on the userspace/mesa end of things. (And, well, there are other crazy folks out there who want to get open source graphics drivers working on various phones/tablets.) It was a choice to make my life easier. You know, because reverse engineering a gpu is a walk in the park.. BR, -R > If this was in a subsystem that I maintain, I'd delete it tomorrow. But > in the end, it's up to David to decide if he wants to waste the cycles > or not. > > Ick ick ick. > > greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 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:12 ` Paul Bolle 0 siblings, 2 replies; 12+ messages in thread From: Greg KH @ 2015-04-09 17:07 UTC (permalink / raw) To: Rob Clark Cc: Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, Paul Bolle, rupran, stefan.hengelein On Thu, Apr 09, 2015 at 10:50:58AM -0400, Rob Clark wrote: > On Thu, Apr 9, 2015 at 10:20 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Apr 09, 2015 at 09:49:58AM -0400, Rob Clark wrote: > >> On Thu, Apr 9, 2015 at 7:22 AM, Valentin Rothberg > >> <valentinrothberg@gmail.com> wrote: > >> > Hi Hai, > >> > > >> > your commit d5af49c92a8a ("drm/msm/mdp5: Enable DSI connector in msm drm > >> > driver") in today's Linux next tree adds an #ifdef with CONFIG_MSM_BUS_SCALING > >> > as condition. MSM_BUS_SCALING is not defined in Kconfig, so the code in this > >> > #ifdef block won't be compiled at its current state. > >> > > >> > I saw some references on this Kconfig option in other files; is there a > >> > reason for the absence of MSM_BUS_SCALING? > >> > >> right now, it is something that only exists in downstream kernels (for > >> example, android device kernels).. but in those kernels it is > >> mandatory to use, as by default the memory/bus is downclocked and the > >> display would underflow if we did not request sufficient bandwidth. > >> > >> It only exists right now in the upstream kernel to simplify > >> backporting to various device kernels > > > > That's crazy. You are asking upstream to maintain code in order to just > > make out of tree crap easier to maintain, which you don't have any plan > > to ever upstream? That causes havoc on static analysis tools and > > prevents anyone from ever being able to even change the code for new api > > changes and test build it. > > Hey, don't blame me for the downstream kernels. But at various points > in time I've had to backport drm/msm to various device kernels in > order to work on the userspace/mesa end of things. (And, well, there > are other crazy folks out there who want to get open source graphics > drivers working on various phones/tablets.) It was a choice to make > my life easier. You know, because reverse engineering a gpu is a walk > in the park.. 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? confused, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 17:07 ` Greg KH @ 2015-04-09 18:02 ` Rob Clark 2015-04-09 18:12 ` Paul Bolle 1 sibling, 0 replies; 12+ messages in thread From: Rob Clark @ 2015-04-09 18:02 UTC (permalink / raw) To: Greg KH Cc: Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, Paul Bolle, rupran, stefan.hengelein On Thu, Apr 9, 2015 at 1:07 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 09, 2015 at 10:50:58AM -0400, Rob Clark wrote: >> On Thu, Apr 9, 2015 at 10:20 AM, Greg KH <gregkh@linuxfoundation.org> wrote: >> > On Thu, Apr 09, 2015 at 09:49:58AM -0400, Rob Clark wrote: >> >> On Thu, Apr 9, 2015 at 7:22 AM, Valentin Rothberg >> >> <valentinrothberg@gmail.com> wrote: >> >> > Hi Hai, >> >> > >> >> > your commit d5af49c92a8a ("drm/msm/mdp5: Enable DSI connector in msm drm >> >> > driver") in today's Linux next tree adds an #ifdef with CONFIG_MSM_BUS_SCALING >> >> > as condition. MSM_BUS_SCALING is not defined in Kconfig, so the code in this >> >> > #ifdef block won't be compiled at its current state. >> >> > >> >> > I saw some references on this Kconfig option in other files; is there a >> >> > reason for the absence of MSM_BUS_SCALING? >> >> >> >> right now, it is something that only exists in downstream kernels (for >> >> example, android device kernels).. but in those kernels it is >> >> mandatory to use, as by default the memory/bus is downclocked and the >> >> display would underflow if we did not request sufficient bandwidth. >> >> >> >> It only exists right now in the upstream kernel to simplify >> >> backporting to various device kernels >> > >> > That's crazy. You are asking upstream to maintain code in order to just >> > make out of tree crap easier to maintain, which you don't have any plan >> > to ever upstream? That causes havoc on static analysis tools and >> > prevents anyone from ever being able to even change the code for new api >> > changes and test build it. >> >> Hey, don't blame me for the downstream kernels. But at various points >> in time I've had to backport drm/msm to various device kernels in >> order to work on the userspace/mesa end of things. (And, well, there >> are other crazy folks out there who want to get open source graphics >> drivers working on various phones/tablets.) It was a choice to make >> my life easier. You know, because reverse engineering a gpu is a walk >> in the park.. > > 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? > For devices where I cannot run an upstream kernel yet, I backport latest upstream drm (mostly 'cp -r' with as little changes as possible, cherrypicking other dependencies outside of drm) to the device kernel. Basically that lets me develop against upstream drm in parallel with the kernel-msm folks (hopefully) getting their pieces upstream. If I had to wait for all the clocks/regulators/gpio/etc drivers that I depend on to land upstream, I'd pretty much only be able to start when a given SoC was already a bit old (and add to that 6-12mos or so to get mesa into mesa into good shape on a new gpu generation, by the time the end user gets something usable the device would already be obsolete) Ideally we get to the point where I don't need to do this.. downstream vendor kernels are generally a PITA.. but for the time being, it seems like the most practical way for me to do things. BR, -R > confused, > > greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 17:07 ` Greg KH 2015-04-09 18:02 ` Rob Clark @ 2015-04-09 18:12 ` Paul Bolle 2015-04-09 18:54 ` Rob Clark 1 sibling, 1 reply; 12+ messages in thread From: Paul Bolle @ 2015-04-09 18:12 UTC (permalink / raw) To: Greg KH Cc: Rob Clark, Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, rupran, stefan.hengelein 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. Paul Bolle ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 18:12 ` Paul Bolle @ 2015-04-09 18:54 ` Rob Clark 2015-04-09 19:38 ` Paul Bolle 2015-04-09 19:44 ` Valentin Rothberg 0 siblings, 2 replies; 12+ messages in thread From: Rob Clark @ 2015-04-09 18:54 UTC (permalink / raw) To: Paul Bolle Cc: Greg KH, Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, rupran, stefan.hengelein 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 BR, -R > > Paul Bolle > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 18:54 ` Rob Clark @ 2015-04-09 19:38 ` Paul Bolle 2015-04-09 19:44 ` Valentin Rothberg 1 sibling, 0 replies; 12+ messages in thread From: Paul Bolle @ 2015-04-09 19:38 UTC (permalink / raw) To: Rob Clark Cc: Greg KH, Valentin Rothberg, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, rupran, stefan.hengelein On Thu, 2015-04-09 at 14:54 -0400, Rob Clark wrote: > 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) This has comes up before too. My thoughts are basically that since it's just a few blocks of code (I think we're discussing less than 200 lines of code split over nine files here) it's hard to see why it would be such a burden to carry those blocks in a separate tree until everything can be submitted in actual working condition. Paul Bolle ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 18:54 ` Rob Clark 2015-04-09 19:38 ` Paul Bolle @ 2015-04-09 19:44 ` Valentin Rothberg 2015-04-09 20:20 ` Rob Clark 1 sibling, 1 reply; 12+ messages in thread From: Valentin Rothberg @ 2015-04-09 19:44 UTC (permalink / raw) To: Rob Clark Cc: Paul Bolle, Greg KH, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, rupran, stefan.hengelein 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. 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! Kind regards, Valentin > > BR, > -R > > > > > Paul Bolle > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 19:44 ` Valentin Rothberg @ 2015-04-09 20:20 ` Rob Clark 2015-04-10 6:04 ` Valentin Rothberg 0 siblings, 1 reply; 12+ messages in thread From: Rob Clark @ 2015-04-09 20:20 UTC (permalink / raw) To: Valentin Rothberg Cc: Paul Bolle, Greg KH, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, rupran, stefan.hengelein 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 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 >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: drm/msm/mdp5: undefined CONFIG_MSM_BUS_SCALING 2015-04-09 20:20 ` Rob Clark @ 2015-04-10 6:04 ` Valentin Rothberg 0 siblings, 0 replies; 12+ messages in thread From: Valentin Rothberg @ 2015-04-10 6:04 UTC (permalink / raw) To: Rob Clark Cc: Paul Bolle, Greg KH, Hai Li, dri-devel, Linux Kernel Mailing List, David Airlie, rupran, stefan.hengelein 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 > >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-04-10 6:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 14:20 ` Greg KH 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:12 ` Paul Bolle 2015-04-09 18:54 ` Rob Clark 2015-04-09 19:38 ` Paul Bolle 2015-04-09 19:44 ` Valentin Rothberg 2015-04-09 20:20 ` Rob Clark 2015-04-10 6:04 ` Valentin Rothberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).