On 12 February 2016 at 08:28, Alexander Shishkin
[off-list ref] wrote:

Chunyan Zhang [off-list ref] writes:

quoted

+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.

Regardless of the semantic associated to the word "invariant" in the
English language, the documentation characterises a channel configured
in best effort delivery mode as "invariant".  This is also the
opposite of the "guaranteed" mode where packets are guaranteed to be
delivered.  Adding a few comments here is probably a good idea.

quoted

+
+     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.

Yes, that was the intended behaviour - to allow user space to see in
what mode channels are configured (guaranteed/invariant).

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 interface asks if the channel is configured in "guaranteed" mode.
If not and because there isn't another mode available, it is
automatically in "invariant" mode.

But as I pointed out in my earlier email this may no longer needed.
One has to issue a system call anyway, might as well just go ahead
with the configuration request.

Thanks,
Mathieu

The idea behind set_options ioctl is that the user specifies a bit mask
of options that he/she wants to set.

[snip]

quoted

+#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