Re: [PATCH v2] driver core: Remove device link creation limitation

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Saravana Kannan <saravanak@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Dmitry Osipenko <digetx@gmail.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Lukas Wunner <lukas@wunner.de>, Jon Hunter <jonathanh@nvidia.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>
Subject: Re: [PATCH v2] driver core: Remove device link creation limitation
Date: Tue, 30 Jul 2019 00:03:47 +0200	[thread overview]
Message-ID: <CAJZ5v0gSyPn8sZ2jJ+nZ_nu1Y=8+7Pg+pPvKGSijUm_sKitk4Q@mail.gmail.com> (raw)
In-Reply-To: <CAGETcx9oqAJ-VoJnD0Y8k+W8cCGPDz--=amktSgW_sB4MEngDA@mail.gmail.com>

On Mon, Jul 29, 2019 at 11:43 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Jul 29, 2019 at 2:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Jul 29, 2019 at 10:47 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Rafael,
> > >
> > > This is the fix you need. Or something link this.
> > >
> > > I had asked you to reject DL_FLAG_MANAGED as an input flag if you are
> > > marking it as internal (in the comments). But looks like you were also
> > > trying to check for "undefined" bit positions. However, the check
> > > isn't correct because DL_MANAGED_FLAGS doesn't include (rightfully so)
> > > DL_FLAG_PM_RUNTIME and DL_FLAG_RPM_ACTIVE .
> > >
> > > I tried to write a DL_FLAG_EXTERNAL to include all the external flags,
> > > but that felt like a maintenance headache that's not worth carrying. I
> > > think it's simpler to just error out when internal flags being passed
> > > in and ignore any undefined bit positions.
> >
> > Well, IMO it is better to prevent people from passing unrecognized
> > flags to device_link_add() at all, even if that means some extra
> > effort when adding new flags.
>
> It isn't so much the extra effort that's a concern, but people might
> miss updating whatever grouping macro we use.
>
> >
> > I'll post an alternative fix shortly.
>
> You might want to move the MANAGED_FLAGs and other grouping macros
> into the header file then? So that if someone is adding new flags,
> it'll be less likely they'll forget to update the grouping macro?

They would need to update device_link_add() itself, so updating a
thing next to it does't seem to be so much of an issue.

Moreover, the "grouping macro" is not relevant for any users of the
API, just for device_link_add() itself, so I'm not sure how much
better it is to have it in the header.

And, of course, if anyone forgets to update the "grouping macro", they
will find that the new flags are rejected immediately.