From: Jehoon Park <jehoon.park@samsung.com>
To: Davidlohr Bueso <dave@stgolabs.net>
Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev,
Alison Schofield <alison.schofield@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Ira Weiny <ira.weiny@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
Dave Jiang <dave.jiang@intel.com>,
Jonathan Cameron <jonathan.cameron@huawei.com>,
Kyungsan Kim <ks0204.kim@samsung.com>,
Junhyeok Im <junhyeok.im@samsung.com>
Subject: Re: [ndctl PATCH v2 2/2] cxl: add 'set-alert-config' command to cxl tool
Date: Tue, 5 Sep 2023 15:29:18 +0900 [thread overview]
Message-ID: <20230905062918.GA8186@jehoon-Precision-7920-Tower> (raw)
In-Reply-To: <v76symilqy5onz6n3y7e47r5xqpaf53nunbb5kz2pcesvjocw7@42o6dotjncu5>
[-- Attachment #1: Type: text/plain, Size: 3617 bytes --]
On Sat, Sep 02, 2023 at 08:49:58PM -0700, Davidlohr Bueso wrote:
> On Mon, 07 Aug 2023, Jehoon Park wrote:
>
> > Add a new command: 'set-alert-config', which configures device's warning alert.
>
> The example in the cover-letter should be here and also mention explicitly that
> the get counterpart is via cxl-list -A, like you have in the manpage.
>
Thank you for comments.
I will reorganize and update cover letter and commit message for clear
explanation about the patchset.
> >
> > Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
> > ---
> > Documentation/cxl/cxl-set-alert-config.txt | 96 +++++++++
> > Documentation/cxl/meson.build | 1 +
> > cxl/builtin.h | 1 +
> > cxl/cxl.c | 1 +
> > cxl/memdev.c | 220 ++++++++++++++++++++-
> > 5 files changed, 318 insertions(+), 1 deletion(-)
> > create mode 100644 Documentation/cxl/cxl-set-alert-config.txt
> >
> > diff --git a/Documentation/cxl/cxl-set-alert-config.txt b/Documentation/cxl/cxl-set-alert-config.txt
> > new file mode 100644
> > index 0000000..c905f7c
> > --- /dev/null
> > +++ b/Documentation/cxl/cxl-set-alert-config.txt
> > @@ -0,0 +1,96 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +cxl-set-alert-config(1)
> > +=======================
> > +
> > +NAME
> > +----
> > +cxl-set-alert-config - set the warning alert threshold on a CXL memdev
> > +
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'cxl set-alert-config <mem0> [<mem1>..<memN>] [<options>]'
> > +
> > +DESCRIPTION
> > +-----------
> > +CXL device raises an alert when its health status is changed. Critical alert
> > +shall automatically be configured by the device after a device reset.
> > +If supported, programmable warning thresholds also be initialized to vendor
> > +recommended defaults, then could be configured by the user.
> > +
> > +Use this command to configure warning alert thresholds of a device.
> > +Having issued this command, the newly requested warning thresholds would
> > +override the previously programmed warning thresholds.
> > +
> > +To enable warning alert, set both 'threshold=value' and 'alert=on'. To disable
> > +warning alert, set only 'alert=off'. Other cases would cause errors.
>
> So what's the point of having to use double parameter to enable the warning?
> Just do alert=threshold if you've established that threshold=N and alert=off is
> not valid.
>
IMHO, using two separate parameters would be better because it would be safer
by protecting a misuse out of an user. I think it is important to clearly
interpret user's intention.
Also, CXL spec (8.2.9.8.3.3) defines two payload fields, threshold and
enablement, separetely.
Please also remind that the 'ndctl-inject-smart' command already uses two
parameters, threshold and alarm, to set smart threshold.
> > +
> > +Use "cxl list -m <memdev> -A" to examine the programming warning threshold
> > +capabilities of a device.
> > +
> > +EXAMPLES
> > +--------
> > +Set warning threshold to 30 and enable alert for life used.
> > +[verse]
> > +cxl set-alert-config mem0 -L 30 --life-used-alert=on
> > +
> > +Disable warning alert for device over temperature.
> > +[verse]
> > +cxl set-alert-config mem0 --over-temperature-alert=off
> > +
> > +OPTIONS
> > +-------
> > +<memory device(s)>::
> > +include::memdev-option.txt[]
> > +
> > +-v::
> > +--verbose=::
> > + Turn on verbose debug messages in the library (if libcxl was built with
> > + logging and debug enabled).
>
> Should be at the end.
>
Agree
>
> Thanks,
> Davidlohr
Sincerely,
Jehoon
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
prev parent reply other threads:[~2023-09-05 6:35 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20230807063148epcas2p44093513a86545a3d1b0c3e0900f7ff4b@epcas2p4.samsung.com>
2023-08-07 6:33 ` [ndctl PATCH v2 0/2] Add support for Set Alert Configuration mailbox command Jehoon Park
[not found] ` <CGME20230807063154epcas2p474cf88b864ca6c87f656853546256416@epcas2p4.samsung.com>
2023-08-07 6:33 ` [ndctl PATCH v2 1/2] libcxl: add " Jehoon Park
[not found] ` <CGME20230807063200epcas2p2c573a655c3b35de5aeb7e188430cca9a@epcas2p2.samsung.com>
2023-08-07 6:33 ` [ndctl PATCH v2 2/2] cxl: add 'set-alert-config' command to cxl tool Jehoon Park
2023-09-03 3:49 ` Davidlohr Bueso
2023-09-05 6:29 ` Jehoon Park [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230905062918.GA8186@jehoon-Precision-7920-Tower \
--to=jehoon.park@samsung.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=ira.weiny@intel.com \
--cc=jonathan.cameron@huawei.com \
--cc=junhyeok.im@samsung.com \
--cc=ks0204.kim@samsung.com \
--cc=linux-cxl@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).