From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
To: Chunyan Zhang <zhang.chunyan@linaro.org>, mathieu.poirier@linaro.org
Cc: mike.leach@arm.com, Michael.Williams@arm.com, al.grant@arm.com,
tor@ti.com, nicolas.guion@st.com, broonie@kernel.org,
robh@kernel.org, pratikp@codeaurora.org, corbet@lwn.net,
mark.rutland@arm.com, zhang.lyra@gmail.com,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-api@vger.kernel.org,
linux-doc@vger.kernel.org
Subject: Re: [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component
Date: Fri, 12 Feb 2016 17:28:16 +0200 [thread overview]
Message-ID: <87y4aq2bjj.fsf@ashishki-desk.ger.corp.intel.com> (raw)
In-Reply-To: <1454756672-12790-7-git-send-email-zhang.chunyan@linaro.org>
Chunyan Zhang <zhang.chunyan@linaro.org> writes:
> +static long stm_generic_set_options(struct stm_data *stm_data,
> + unsigned int master,
> + unsigned int channel,
> + unsigned int nr_chans,
> + unsigned long options)
> +{
> + struct stm_drvdata *drvdata = container_of(stm_data,
> + struct stm_drvdata, stm);
> + if (!(drvdata && drvdata->enable))
> + return -EINVAL;
> +
> + if (channel >= drvdata->numsp)
> + return -EINVAL;
> +
> + switch (options) {
> + case STM_OPTION_GUARANTEED:
> + set_bit(channel, drvdata->chs.guaranteed);
> + break;
> +
> + case STM_OPTION_INVARIANT:
> + clear_bit(channel, drvdata->chs.guaranteed);
> + break;
This is a bad interface. Firstly, neither option is described
anywhere. Secondly, I'm pretty sure "invariant" does not mean "not
guaranteed" in english, although this function seems to imply this.
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static long stm_generic_get_options(struct stm_data *stm_data,
> + unsigned int master,
> + unsigned int channel,
> + unsigned int nr_chans,
> + u64 *options)
> +{
> + struct stm_drvdata *drvdata = container_of(stm_data,
> + struct stm_drvdata, stm);
> + if (!(drvdata && drvdata->enable))
> + return -EINVAL;
> +
> + if (channel >= drvdata->numsp)
> + return -EINVAL;
> +
> + switch (*options) {
> + case STM_OPTION_GUARANTEED:
> + *options = test_bit(channel, drvdata->chs.guaranteed);
> + break;
This just doesn't work. @options here is an on-stack variable in
stm_char_ioctl(), hitherto uninitialized. The get_options ioctl command
as you implemented it doesn't fetch @options from userspace either, it
just passes a pointer to it to this callback, expecting that the
callback will set it so that it can be copy_to_user()ed back to the
user.
Then, when we figure this out, there is again the question of what
should one make of STM_OPTION_{GUARANTEED,INVARIANT} and how do they fit
into *options.
The idea behind set_options ioctl is that the user specifies a bit mask
of options that he/she wants to set.
[snip]
> +#ifndef __UAPI_CORESIGHT_STM_H_
> +#define __UAPI_CORESIGHT_STM_H_
> +
> +#define STM_FLAG_TIMESTAMPED BIT(3)
> +#define STM_FLAG_GUARANTEED BIT(7)
> +
> +enum {
> + STM_OPTION_GUARANTEED = 0,
> + STM_OPTION_INVARIANT,
> +};
Each of these guys could also use an explanation.
Regards,
--
Alex
next prev parent reply other threads:[~2016-02-12 15:28 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-06 11:04 [PATCH V3 0/6] Introduce CoreSight STM support Chunyan Zhang
2016-02-06 11:04 ` [PATCH V3 1/6] stm class: Add ioctl get_options interface Chunyan Zhang
2016-02-12 15:18 ` Alexander Shishkin
2016-02-12 18:33 ` Mathieu Poirier
2016-02-06 11:04 ` [PATCH V3 2/6] stm class: Support devices with multiple instances Chunyan Zhang
2016-02-06 11:04 ` [PATCH V3 3/6] stm class: provision for statically assigned masterIDs Chunyan Zhang
2016-02-06 11:04 ` [PATCH V3 4/6] Documentations: Add explanations of the case for non-configurable masters Chunyan Zhang
2016-02-06 11:04 ` [PATCH V3 5/6] coresight-stm: Bindings for System Trace Macrocell Chunyan Zhang
2016-02-06 11:04 ` [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component Chunyan Zhang
2016-02-11 16:59 ` Mathieu Poirier
2016-02-12 14:55 ` Michael Williams
2016-02-12 18:24 ` Mathieu Poirier
2016-02-17 3:18 ` Chunyan Zhang
2016-02-17 12:08 ` Michael Williams
2016-02-17 3:49 ` Chunyan Zhang
2016-02-12 15:28 ` Alexander Shishkin [this message]
2016-02-12 19:02 ` Mathieu Poirier
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=87y4aq2bjj.fsf@ashishki-desk.ger.corp.intel.com \
--to=alexander.shishkin@linux.intel.com \
--cc=Michael.Williams@arm.com \
--cc=al.grant@arm.com \
--cc=broonie@kernel.org \
--cc=corbet@lwn.net \
--cc=linux-api@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@arm.com \
--cc=nicolas.guion@st.com \
--cc=pratikp@codeaurora.org \
--cc=robh@kernel.org \
--cc=tor@ti.com \
--cc=zhang.chunyan@linaro.org \
--cc=zhang.lyra@gmail.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 NNTP newsgroup(s).