From: Vaibhav Jain <vaibhav@linux.ibm.com>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
linuxppc-dev@lists.ozlabs.org, linux-nvdimm@lists.01.org,
linux-kernel@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RESEND PATCH v7 4/5] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods
Date: Wed, 20 May 2020 15:49:37 +0530 [thread overview]
Message-ID: <87y2pmx612.fsf@linux.ibm.com> (raw)
In-Reply-To: <87a723f5fs.fsf@linux.ibm.com>
Thanks for reviewing this patch Aneesh.
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
> ....
>
> +
>> +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
>> +struct nd_pdsm_cmd_pkg {
>> + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
>> + __s32 cmd_status; /* Out: Sub-cmd status returned back */
>> + __u16 payload_offset; /* In: offset from start of struct */
>> + __u16 payload_version; /* In/Out: version of the payload */
>> + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
>> +} __packed;
>
> that payload_offset can be avoided if we prevent userspace to user a
> different variant of nd_pdsm_cmd_pkg which different header. We can keep
> things simpler if we can always find payload at
> nd_pdsm_cmd_pkg->payload.
Had introduced this member to handle case where new fields are added to
'struct nd_pdsm_cmd_pkg' without having to break the ABI. But agree with
the point that you made later that this can be simplified by replacing
'payload_offset' with a set of reserved variables. Will address this in
next iteration of this patchset.
>
>> +
>> +/*
>> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
>> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
>> + */
>> +enum papr_scm_pdsm {
>> + PAPR_SCM_PDSM_MIN = 0x0,
>> + PAPR_SCM_PDSM_MAX,
>> +};
>> +
>> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
>> +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
>> +{
>> + return (struct nd_pdsm_cmd_pkg *) cmd;
>> +}
>> +
>> +/* Return the payload pointer for a given pcmd */
>> +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
>> +{
>> + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
>> + return NULL;
>> + else
>> + return (void *)((__u8 *) pcmd + pcmd->payload_offset);
>> +}
>> +
>
> we need to make sure userspace is not passing a wrong payload_offset.
Agree, that this function should have more strict checking for
payload_offset field. However will be getting rid of
'payload_offset' all together in the next iteration as you previously
suggested.
> and in the next patch you do
>
> + /* Copy the health struct to the payload */
> + memcpy(pdsm_cmd_to_payload(pkg), &p->health, copysize);
> + pkg->hdr.nd_fw_size = copysize;
> +
Yes this is already being done in the patchset and changes proposed to
this pdsm_cmd_to_payload() should not impact other patches as
pdsm_cmd_to_payload() abstracts rest of the code from how to access the
payload.
> All this can be simplified if you can keep payload at
> nd_pdsm_cmd_pkg->payload.
>
> If you still want to have the ability to extend the header, then added a
> reserved field similar to nd_cmd_pkg.
>
Agree to this and will address this in V8.
>
> -aneesh
--
Cheers
~ Vaibhav
next prev parent reply other threads:[~2020-05-20 10:20 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-19 19:00 [RESEND PATCH v7 0/5] powerpc/papr_scm: Add support for reporting nvdimm health Vaibhav Jain
2020-05-19 19:00 ` [RESEND PATCH v7 1/5] powerpc: Document details on H_SCM_HEALTH hcall Vaibhav Jain
2020-05-19 19:00 ` [RESEND PATCH v7 2/5] seq_buf: Export seq_buf_printf() to external modules Vaibhav Jain
2020-05-20 17:01 ` Christoph Hellwig
2020-05-19 19:00 ` [RESEND PATCH v7 3/5] powerpc/papr_scm: Fetch nvdimm health information from PHYP Vaibhav Jain
2020-05-20 14:54 ` Ira Weiny
2020-05-20 17:15 ` Vaibhav Jain
2020-05-21 14:31 ` Michael Ellerman
2020-05-21 16:59 ` Vaibhav Jain
2020-05-21 23:32 ` Ira Weiny
2020-05-19 19:00 ` [RESEND PATCH v7 4/5] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods Vaibhav Jain
2020-05-20 7:09 ` Aneesh Kumar K.V
2020-05-20 10:19 ` Vaibhav Jain [this message]
2020-05-20 15:32 ` Ira Weiny
2020-05-20 18:37 ` Vaibhav Jain
2020-05-21 6:48 ` Michael Ellerman
2020-05-22 7:38 ` [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: " Vaibhav Jain
2020-05-25 12:00 ` Vaibhav Jain
2020-05-26 12:14 ` Michael Ellerman
2020-05-19 19:00 ` [RESEND PATCH v7 5/5] powerpc/papr_scm: Implement support for PAPR_SCM_PDSM_HEALTH Vaibhav Jain
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=87y2pmx612.fsf@linux.ibm.com \
--to=vaibhav@linux.ibm.com \
--cc=aneesh.kumar@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=rostedt@goodmis.org \
/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 NNTP newsgroup(s).