On 12 February 2016 at 08:28, Alexander Shishkin
<alexander.shishkin@xxxxxxxxxxxxxxx> wrote:
> Chunyan Zhang <zhang.chunyan@xxxxxxxxxx> 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.
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.
>
>> +
>> + 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]
>
>> +#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
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
