stm class: Adding master and channel parameter to ioctl function

Message ID 1427406371-6699-1-git-send-email-mathieu.poirier@linaro.org
State New
Headers show

Commit Message

Mathieu Poirier March 26, 2015, 9:46 p.m.
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@linaro.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@linaro.org>
---
 drivers/hwtracing/stm/core.c | 12 ++++++++++--
 include/linux/stm.h          |  5 +++--
 2 files changed, 13 insertions(+), 4 deletions(-)

Comments

Mathieu Poirier March 30, 2015, 2:33 p.m. | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch hide | download patch | download mbox

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,