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