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;
MIME-Version: 1.0
References: <20191029223214.18889-1-linux@roeck-us.net> <CAC5umyhc=6yULiLwXu65VDvDk2cBiF0R9O39B-T5ftapJfj0rQ@mail.gmail.com>
 <e62b6763-0d1b-3359-6d3b-cb31e96bb862@roeck-us.net>
In-Reply-To: <e62b6763-0d1b-3359-6d3b-cb31e96bb862@roeck-us.net>
From:   Akinobu Mita <akinobu.mita@gmail.com>
Date:   Thu, 31 Oct 2019 22:44:26 +0900
Message-ID: <CAC5umygHEcVJQskfX0Ef4Z5Ti5B1-zKbmvvAbPPi4YtcQsV-WA@mail.gmail.com>
Subject: Re: [PATCH v2] nvme: Add hardware monitoring support
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-nvme@lists.infradead.org,
        Linux PM <linux-pm@vger.kernel.org>,
        Chris Healy <cphealy@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

2019=E5=B9=B410=E6=9C=8831=E6=97=A5(=E6=9C=A8) 11:20 Guenter Roeck <linux@r=
oeck-us.net>:
>
> On 10/30/19 4:16 AM, Akinobu Mita wrote:
> > 2019=E5=B9=B410=E6=9C=8830=E6=97=A5(=E6=B0=B4) 7:32 Guenter Roeck <linu=
x@roeck-us.net>:
> >>
> >> nvme devices report temperature information in the controller informat=
ion
> >> (for limits) and in the smart log. Currently, the only means to retrie=
ve
> >> this information is the nvme command line interface, which requires
> >> super-user privileges.
> >>
> >> At the same time, it would be desirable to use NVME temperature inform=
ation
> >> for thermal control.
> >>
> >> This patch adds support to read NVME temperatures from the kernel usin=
g the
> >> hwmon API and adds temperature zones for NVME drives. The thermal subs=
ystem
> >> can use this information to set thermal policies, and userspace can ac=
cess
> >> it using libsensors and/or the "sensors" command.
> >>
> >> Example output from the "sensors" command:
> >>
> >> nvme0-pci-0100
> >> Adapter: PCI adapter
> >> Composite:    +39.0=C2=B0C  (high =3D +85.0=C2=B0C, crit =3D +85.0=C2=
=B0C)
> >> Sensor 1:     +39.0=C2=B0C
> >> Sensor 2:     +41.0=C2=B0C
> >>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> v2: Use devm_kfree() to release memory in error path
> >>
> >>   drivers/nvme/host/Kconfig      |  10 ++
> >>   drivers/nvme/host/Makefile     |   1 +
> >>   drivers/nvme/host/core.c       |   5 +
> >>   drivers/nvme/host/nvme-hwmon.c | 163 +++++++++++++++++++++++++++++++=
++
> >>   drivers/nvme/host/nvme.h       |   8 ++
> >>   5 files changed, 187 insertions(+)
> >>   create mode 100644 drivers/nvme/host/nvme-hwmon.c
> >>
> >> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> >> index 2b36f052bfb9..aeb49e16e386 100644
> >> --- a/drivers/nvme/host/Kconfig
> >> +++ b/drivers/nvme/host/Kconfig
> >> @@ -23,6 +23,16 @@ config NVME_MULTIPATH
> >>             /dev/nvmeXnY device will show up for each NVMe namespaces,
> >>             even if it is accessible through multiple controllers.
> >>
> >> +config NVME_HWMON
> >> +       bool "NVME hardware monitoring"
> >> +       depends on (NVME_CORE=3Dy && HWMON=3Dy) || (NVME_CORE=3Dm && H=
WMON)
> >> +       help
> >> +         This provides support for NVME hardware monitoring. If enabl=
ed,
> >> +         a hardware monitoring device will be created for each NVME d=
rive
> >> +         in the system.
> >> +
> >> +         If unsure, say N.
> >> +
> >>   config NVME_FABRICS
> >>          tristate
> >>
> >> diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
> >> index 8a4b671c5f0c..03de4797a877 100644
> >> --- a/drivers/nvme/host/Makefile
> >> +++ b/drivers/nvme/host/Makefile
> >> @@ -14,6 +14,7 @@ nvme-core-$(CONFIG_TRACING)           +=3D trace.o
> >>   nvme-core-$(CONFIG_NVME_MULTIPATH)     +=3D multipath.o
> >>   nvme-core-$(CONFIG_NVM)                        +=3D lightnvm.o
> >>   nvme-core-$(CONFIG_FAULT_INJECTION_DEBUG_FS)   +=3D fault_inject.o
> >> +nvme-core-$(CONFIG_NVME_HWMON)         +=3D nvme-hwmon.o
> >>
> >>   nvme-y                                 +=3D pci.o
> >>
> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >> index fa7ba09dca77..fc1d4b146717 100644
> >> --- a/drivers/nvme/host/core.c
> >> +++ b/drivers/nvme/host/core.c
> >> @@ -2796,6 +2796,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> >>          ctrl->oncs =3D le16_to_cpu(id->oncs);
> >>          ctrl->mtfa =3D le16_to_cpu(id->mtfa);
> >>          ctrl->oaes =3D le32_to_cpu(id->oaes);
> >> +       ctrl->wctemp =3D le16_to_cpu(id->wctemp);
> >> +       ctrl->cctemp =3D le16_to_cpu(id->cctemp);
> >> +
> >>          atomic_set(&ctrl->abort_limit, id->acl + 1);
> >>          ctrl->vwc =3D id->vwc;
> >>          if (id->mdts)
> >> @@ -2897,6 +2900,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> >>
> >>          ctrl->identified =3D true;
> >>
> >> +       nvme_hwmon_init(ctrl);
> >> +
> >>          return 0;
> >>
> >>   out_free:
> >
> > The nvme_init_identify() can be called multiple time in nvme ctrl's
> > lifetime (e.g 'nvme reset /dev/nvme*' or suspend/resume paths), so
> > should we need to prevent nvme_hwmon_init() from registering hwmon
> > device more than twice?
> >
> > In the nvme thermal zone patchset[1], thernal zone is registered in
> > nvme_init_identify and unregistered in nvme_stop_ctrl().
> >
>
> Doesn't that mean that the initialization should happen in nvme_start_ctr=
l()
> and not here ?

Seems possible.  But I would like to ask maintainers' opinion.