Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5d40d5b3.1c69fb81.6047f.1cc3@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAJZ5v0hj+e3+LZ+J1eOAT2REQne_J6aAXzkKVb0tJM4u9u--Rw@mail.gmail.com>
References: <20190730024309.233728-1-trong@android.com> <CANA+-vBKg_W88Oy_wJs1NNYaZ2ciJKO=Mrs47etYTDNXUKW9Uw@mail.gmail.com> <5d4090ea.1c69fb81.d5cab.4dcd@mx.google.com> <2085893.cJkfNvi94x@kreacher> <5d40c41f.1c69fb81.ac63f.947f@mx.google.com> <CAJZ5v0hj+e3+LZ+J1eOAT2REQne_J6aAXzkKVb0tJM4u9u--Rw@mail.gmail.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Tri Vo <trong@android.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Hridya Valsaraju <hridya@google.com>,
        Sandeep Patil <sspatil@google.com>,
        Kalesh Singh <kaleshsingh@google.com>,
        Ravi Chandra Sadineni <ravisadineni@chromium.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Cc: Android Kernel" <kernel-team@android.com>,
        kbuild test robot <lkp@intel.com>
From:   Stephen Boyd <swboyd@chromium.org>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
User-Agent: alot/0.8.1
Date:   Tue, 30 Jul 2019 16:41:38 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Rafael J. Wysocki (2019-07-30 16:05:55)
> On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > > >
> > > > Using the same prefix for the class and the device name is quite co=
mmon.
> > > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN=
. The
> > > > class name could be changed to wakeup_source perhaps (i.e.
> > > > /sys/class/wakeup_source/wakeupN)?
> > >
> > > Alternatively /sys/class/wakeup/wsN
> > >
> >
> > Or /sys/class/wakeup/eventN? It's your bikeshed to paint.
>=20
> So actually the underlying problem here is that device_wakeup_enable()
> tries to register a wakeup source and then attach it to the device to
> avoid calling possibly sleeping functions under a spinlock.

Agreed, that is one problem.

>=20
> However, it should be possible to call wakeup_source_create(name)
> first, then attach the wakeup source to the device (after checking for
> presence), and then invoke wakeup_source_add() (after dropping the
> lock).  If the wakeup source virtual device registration is done in
> wakeup_source_add(), that should avoid the problem altogether without
> having to introduce extra complexity.

While reordering the code to do what you describe will fix this specific
duplicate name problem, it won't fix the general problem with reusing
device names from one bus on a different bus/class. We can run into the
same problem when two buses name their devices the same name and then we
attempt to attach a wakeup source to those two devices. Or we can have a
problem where a virtual wakeup is made with the same name, and again
we'll try to make a duplicate named device. Using something like 'event'
or 'wakeup' or 'ws' as the prefix avoids this problem and keeps things
clean.

We should probably avoid letting the same virtual wakeup source be made
with the same name anyway, because userspace will be confused about what
virtual wakeup it is otherwise. I concede that using the name of the
wakeup source catches this problem without adding extra code.

Either way, I'd like to see what you outline implemented so that we
don't need to do more work than is necessary when userspace writes to
the file. I just don't want to see us need to change the name of the
wakeup device later on and then add a 'name' attribute to the class so
that we can avoid name collisions due to various buses controlling the
string we use to create the name of the wakeup device.