[PATCH] stm class: Adding master and channel parameter to ioctl function

[PATCH] stm class: Adding master and channel parameter to ioctl function

[Mathieu Poirier]

Hey Alex,

Have a look at the following patch and see if you agree with my approach.  If so
simply add the code to a third version. 

Thanks,
Mathieu

>From 70b4709b668ef59b303dabeff73ed850a4980cfc Mon Sep 17 00:00:00 2001
From: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Thu, 26 Mar 2015 15:33:03 -0600
Subject: [PATCH] stm class: Adding master and channel parameter to ioctl
 function

Just like the "write" function, architecture specific device need
to know about master and channels to do the correct operations on
a ioctl call.

Since this information is embedded in the stm_file structure that
is private to the stm class core, adding extra parameters to convey
the values is simple and clean.

Signed-off-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/hwtracing/stm/core.c | 12 ++++++++++--
 include/linux/stm.h          |  5 +++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 9e82634590dc..ca71b06bbfea 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -511,9 +511,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		return stm_char_policy_get_ioctl(stmf, (void __user *)arg);
 
 	default:
-		if (stm_data->ioctl)
-			err = stm_data->ioctl(stm_data, cmd, arg);
+		if (stm_data->ioctl) {
+			/* users shouldn't call device specific ioctl before
+			 * getting a channel using the proper potocol
+			 */
+			if (!stmf->output.nr_chans)
+				return -EINVAL;
 
+			err = stm_data->ioctl(stm_data, stmf->output.master,
+					      stmf->output.channel, cmd, arg);
+
+		}
 		break;
 	}
 
diff --git a/include/linux/stm.h b/include/linux/stm.h
index 976c94d8f17f..84dd83c47fe7 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -62,8 +62,9 @@ struct stm_data {
 					unsigned int);
 	void			(*unlink)(struct stm_data *, unsigned int,
 					  unsigned int);
-	long			(*ioctl)(struct stm_data *, unsigned int,
-					 unsigned long);
+	long			(*ioctl)(struct stm_data *,
+					 unsigned int, unsigned int,
+					 unsigned int, unsigned long);
 };
 
 int stm_register_device(struct device *parent, struct stm_data *stm_data,
-- 
1.9.1

Re: [PATCH] stm class: Adding master and channel parameter to ioctl function

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> Hey Alex,

Hi Mathieu,

> Have a look at the following patch and see if you agree with my approach.  If so
> simply add the code to a third version. 

Greg's comment about the two levels of ioctls got me thinking in the
direction of adding callbacks to stm_data on case-by-case basis and then
I realized that we might actually move these private callbacks into the
stm core as well (see my followup to the coresight-stm driver patch).

So, I suggest that we work through the ioctl commands that you need for
coresight stm and try to see how they fit into the generic scheme of
things and if we still find that we need implementation-specific ioctl
commands, then we try to shape them as individual callbacks rather than
just .ioctl().

> diff --git a/include/linux/stm.h b/include/linux/stm.h
> index 976c94d8f17f..84dd83c47fe7 100644
> --- a/include/linux/stm.h
> +++ b/include/linux/stm.h
> @@ -62,8 +62,9 @@ struct stm_data {
>  					unsigned int);
>  	void			(*unlink)(struct stm_data *, unsigned int,
>  					  unsigned int);
> -	long			(*ioctl)(struct stm_data *, unsigned int,
> -					 unsigned long);
> +	long			(*ioctl)(struct stm_data *,
> +					 unsigned int, unsigned int,
> +					 unsigned int, unsigned long);

We might go even further and pass a struct stm_output (which also
contains number of channels) pointer here, having first moved it from
drivers/... to include/linux/stm.h, and while at it also do the same to
the other callbacks that pass master/channel around. Does this make
sense to you?

Regards,
--
Alex

Re: [PATCH] stm class: Adding master and channel parameter to ioctl function

[Mathieu Poirier]

On 30 March 2015 at 08:19, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> Hey Alex,
>
> Hi Mathieu,
>
>> Have a look at the following patch and see if you agree with my approach.  If so
>> simply add the code to a third version.
>
> Greg's comment about the two levels of ioctls got me thinking in the
> direction of adding callbacks to stm_data on case-by-case basis and then
> I realized that we might actually move these private callbacks into the
> stm core as well (see my followup to the coresight-stm driver patch).
>
> So, I suggest that we work through the ioctl commands that you need for
> coresight stm and try to see how they fit into the generic scheme of
> things and if we still find that we need implementation-specific ioctl
> commands, then we try to shape them as individual callbacks rather than
> just .ioctl().

Yeah, I'm pretty sure we can come up with something better here.

>
>> diff --git a/include/linux/stm.h b/include/linux/stm.h
>> index 976c94d8f17f..84dd83c47fe7 100644
>> --- a/include/linux/stm.h
>> +++ b/include/linux/stm.h
>> @@ -62,8 +62,9 @@ struct stm_data {
>>                                       unsigned int);
>>       void                    (*unlink)(struct stm_data *, unsigned int,
>>                                         unsigned int);
>> -     long                    (*ioctl)(struct stm_data *, unsigned int,
>> -                                      unsigned long);
>> +     long                    (*ioctl)(struct stm_data *,
>> +                                      unsigned int, unsigned int,
>> +                                      unsigned int, unsigned long);
>
> We might go even further and pass a struct stm_output (which also
> contains number of channels) pointer here, having first moved it from
> drivers/... to include/linux/stm.h, and while at it also do the same to
> the other callbacks that pass master/channel around. Does this make
> sense to you?

Humm...  Do callbacks really need to have access to a struct output?
>From my side of the fence dealing with individual master/channels is
sufficient but other people may have different scenarios.  The end
result is the same and modifying the callback signature doesn't
represent a lot of work.

>
> Regards,
> --
> Alex