Message-ID: <20120425151130.GI11802@beardog.cce.hp.com>
Date:	Wed, 25 Apr 2012 10:11:30 -0500
From:	scameron@...rdog.cce.hp.com
To:	Paul Gortmaker <paul.gortmaker@...driver.com>
Cc:	James Bottomley <James.Bottomley@...senpartnership.com>,
	Stephen Cameron <stephenmcameron@...il.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	matthew.gates@...com, thenzl@...hat.com, akpm@...ux-foundation.org,
	mikem@...rdog.cce.hp.com, scameron@...rdog.cce.hp.com
Subject: Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

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@...senpartnership.com> 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@...driver.com> wrote:
> >>>>>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> >>>>>> <scameron@...rdog.cce.hp.com> wrote:
> >>>>>>> From: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
> >>>>>>>
> >>>>>>> Signed-off-by: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
> >>>>>>
> >>>>>> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists