On Tue, Apr 24, 2012 at 11:15:26AM -0400, Paul Gortmaker wrote:
> On 12-04-24 04:25 AM, James Bottomley wrote:
> > On Mon, 2012-04-23 at 21:43 -0400, Paul Gortmaker wrote:
> >> On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
> >> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>> On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote:
> >>>> On 12-04-22 10:20 PM, Stephen Cameron wrote:
> >>>>> On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker
> >>>>> <paul.gortmaker@xxxxxxxxxxxxx> wrote:
> >>>>>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> >>>>>> <scameron@xxxxxxxxxxxxxxxxxx> wrote:
> >>>>>>> From: Stephen M. Cameron <scameron@xxxxxxxxxxxxxxxxxx>
> >>>>>>>
> >>>>>>> Signed-off-by: Stephen M. Cameron <scameron@xxxxxxxxxxxxxxxxxx>
> >>>>>>
> >>>>>> You've not written a commit log, so I'm left guessing what the
> >>>>>> intended rationale is here. COMPAT, X86 and PCI_MSI are
> >>>>>> I believe all bool, so why make this change? To me it gives
> >>>>>> a misleading message that some level of modular awareness
> >>>>>> is needed here, when there really isn't any such need.
> >>>>>
> >>>>> Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e
> >>>>>
> >>>>> Using IS_ENABLED() within C (vs. within CPP #if statements) in its
> >>>>> current form requires us to actually define every possible bool/tristate
> >>>>> Kconfig option twice (__enabled_* and __enabled_*_MODULE variants).
> >>>>> [...]
> >>>>>
> >>>>> and so I kind of thought IS_ENABLED is the new way to do that sort of thing.
> >>>
> >>> I don't think you need to change the driver unless there's a good
> >>> reason. In kernel terms, it's usually better to wait a bit to see if
> >>> the wheels fall off any particular bandwagon before leaping on it ...
> >>>
> >>>> In my opinion, IS_ENABLED can/should be used when you have:
> >>>>
> >>>> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> >>>>
> >>>> i.e. "is this driver built in, or can it be loaded as a module?"
> >>>> The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto
> >>>> generated by Kbuild infrastructure.
> >>>>
> >>>> It will do the Right Thing even for cases where CONFIG_FOO_MODULE
> >>>> is impossible, but it does as I've said, give the wrong impression
> >>>> that there is some possibility of modular presence. At least to
> >>>> those who are familiar with the implementation and why it exists.
> >>>> [I'll grant you that the name doesn't convey the use case too well.]
> >>>>
> >>>>>
> >>>>> Perhaps I'm wrong about that. Obviously the patch is not _needed_ for
> >>>>> things to work.
> >>>>
> >>>> I don't think we want to go and mass replace all existing cases of
> >>>>
> >>>> #ifdef CONFIG_SOME_BOOL
> >>>>
> >>>> with:
> >>>>
> >>>> #if IS_ENABLED(CONFIG_SOME_BOOL)
> >>>>
> >>>> There is no value add. However, replacing instances of:
> >>>>
> >>>> #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
> >>>>
> >>>> with:
> >>>>
> >>>> #if IS_ENABLED(CONFIG_FOO)
> >>>>
> >>>> is in my opinion, a reasonable thing to do. It is shorter, and
> >>>> it does hide the internally generated _MODULE suffixed "shadow"
> >>>> variables from appearing directly in the main source files. And
> >>>> it tells you that the author was thinking about both the built
> >>>> in and the modular cases when they wrote it.
> >>>
> >>> To be honest, I think we want to use #if IS_ENABLED() for all cases
> >>> going forwards, including the boolean case.
> >>
> >> I guess we will have to agree to disagree then.
> >>
> >>>
> >>> The reason is that it's an easier design pattern. If we have the #ifdef
> >>> CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can
> >>> be modular or not it's just creating pointless rules that someone will
> >>> annoy us all by enforcing in a checkpatch or some other script. Whereas
> >>> #if IS_ENABLED(CONFIG_X) is obvious and simple.
> >>
> >> What exactly is an "easier design pattern", and what are the gains?
> >
> > It means you only have one rule:
> >
> > everything is #if IS_ENABLED
> >
> > vs two arbitrary ones
>
> Again I'll have to disagree with you categorizing them as "arbitrary".
>
> >
> > If it's a boolean symbol, you use #ifdef else if it's tristate, use #if
> > IS_ENABLED
> >
> > It's obvious to me the former is simpler.
>
> So, something in you doesn't go "eeech" when you see:
>
> #if IS_ENABLED(CONFIG_ARM)
>
> ...that treats ARM as if it is some kind of driver, or feature
> that can simply be enabled at will, alongside of any other
> features? Or worse, when people take it to the next level
> and do:
>
> -#ifdef CONFIG_SPARC
> - boo();
> - bar();
> - baz();
> -#endif
> + if (IS_ENABLED(CONFIG_SPARC)) {
> + boo();
> + bar();
> + baz();
> + }
>
> These are kinds of instances that I hope we don't entertain
> as being improvements, or use cases we'd adopt as defaults.
>
> >
> >> To me, it has nothing to do with rules, and what the checkpatch folks do
> >> or do not do. The line "#ifdef CONFIG_FOO" conveys one specific piece
> >> of information. The line "#if IS_ENABLED(CONFIG_FOO) conveys
> >> a different piece of information, which is a superset of the former.
> >
> > That's the point ... why bother with the former? If the latter is a
> > superset and you always do the latter, the rule is simpler.
> >
> >> If you blindly convert all of them, you throw away information that would
> >> otherwise be available to the code reader. I would not support that.
> >
> > Well, a) I'm not advocating converting everything. I'm advocating using
> > the superset for everything going forwards.
> >
> > I don't understand what information would be lost: All use if #ifdef vs
> > #if IS_ENABLED tells you is whether the author thought the symbol was
> > boolean or tristate. I don't see how that's useful at all and it will
> > cause problems if a symbol changes (some non-modular code may become
> > modular for instance) ... then we have to go and chase it through the
> > code base.
>
> Hopefully the above makes my concerns more clear -- IS_ENABLED should
> not be used blindly without thought. Some things like core arch configs
> will never _ever_ be modular. Seeing IS_ENABLED used on those kinds of
> options just seems ugly to me. That is what was in this original patch.
>
> But using it on some driver that isn't currently modular, but might be
> made so eventually is reasonable. Maybe you will still call that
> distinction arbitrary, and the arch use case not ugly enough to matter.
>
> Nor IMHO should IS_ENABLED be used as a tool to engage a holy war on
> eradicating all #ifdef/#endif block. Just like "goto", sensible uses
> of such blocks can make things more readable, not less.
>
> Your point earlier about recommending people show restraint before
> rushing out to use the shiny new tool just because they can, was
> a good one -- I agree wholeheartedly with that.
>
> Thanks,
> Paul.
>
> >
> > James
> >
> >
Also, in "[PATCH 13/17] hpsa: factor out hpsa_free_irqs_and_disable_msix", I did this:
[...]
-#ifdef CONFIG_PCI_MSI
- if (h->msix_vector)
- pci_disable_msix(h->pdev);
- else if (h->msi_vector)
- pci_disable_msi(h->pdev);
-#endif /* CONFIG_PCI_MSI */
+ if (!IS_ENABLED(PCI_MSI))
+ return;
+ if (h->msix_vector) {
+ if (h->pdev->msix_enabled)
+ pci_disable_msix(h->pdev);
+ } else if (h->msi_vector) {
+ if (h->pdev->msi_enabled)
+ pci_disable_msi(h->pdev);
+ }
+}
[...]
Presumably I should not use IS_ENABLED in this instance either.
-- steve
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
