Re: [QUEUED v20180920 05/16] stm class: Add a helper for writing data packets
Mathieu Poirier writes:
>> +static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
>> + unsigned int channel, const char *buf, size_t count)
>> +{
>> +ssize_t sz;
>> +
>> +sz = stm_data_write(data, master, channel, true, buf, count);
>> +if (sz > 0)
>> +data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
>> + buf);
>
> The original code the payload of a flag packet was '0' while in this patch
> changes it to be anything. Some external tooling could be very confused.
I think the original intention was so: the 'size' field of ->packet()
refers to how many bytes from the given 'payload' the callback should
use. IOW, with size 0, buf may point to anything (still a valid pointer
though). But that should have been documented better from the beginning,
so you're completely right. I'll make a note to myself to go over the
API bits and sort out stuff like that.
Thanks,
--
Alex
Re: [QUEUED v20180920 05/16] stm class: Add a helper for writing data packets
Mathieu Poirier writes:
>> +static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
>> + unsigned int channel, const char *buf, size_t count)
>> +{
>> +ssize_t sz;
>> +
>> +sz = stm_data_write(data, master, channel, true, buf, count);
>> +if (sz > 0)
>> +data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
>> + buf);
>
> The original code the payload of a flag packet was '0' while in this patch
> changes it to be anything. Some external tooling could be very confused.
I think the original intention was so: the 'size' field of ->packet()
refers to how many bytes from the given 'payload' the callback should
use. IOW, with size 0, buf may point to anything (still a valid pointer
though). But that should have been documented better from the beginning,
so you're completely right. I'll make a note to myself to go over the
API bits and sort out stuff like that.
Thanks,
--
Alex
Re: [QUEUED v20180920 05/16] stm class: Add a helper for writing data packets
On Thu, Sep 20, 2018 at 03:45:42PM +0300, Alexander Shishkin wrote:
> Add a helper to write a sequence of bytes as STP data packets. This
> is used by protocol drivers to output their metadata, as well as the
> actual data payload.
>
> Signed-off-by: Alexander Shishkin
> ---
> drivers/hwtracing/stm/core.c | 50 ++--
> drivers/hwtracing/stm/stm.h | 3 +++
> 2 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index c869a30661ac..9ed2f06deb47 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -564,27 +564,51 @@ stm_assign_first_policy(struct stm_device *stm, struct
> stm_output *output,
> return err;
> }
>
> -static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> - unsigned int channel, const char *buf, size_t count)
> +/**
> + * stm_data_write() - send the given payload as data packets
> + * @data:stm driver's data
> + * @m: STP master
> + * @c: STP channel
> + * @ts_first:timestamp the first packet
> + * @buf: data payload buffer
> + * @count: data payload size
> + */
> +ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
> +unsigned int c, bool ts_first, const void *buf,
> +size_t count)
> {
> - unsigned int flags = STP_PACKET_TIMESTAMPED;
> - const unsigned char *p = buf, nil = 0;
> - size_t pos;
> + unsigned int flags = ts_first ? STP_PACKET_TIMESTAMPED : 0;
> ssize_t sz;
> + size_t pos;
>
> - for (pos = 0, p = buf; count > pos; pos += sz, p += sz) {
> + for (pos = 0, sz = 0; pos < count; pos += sz) {
> sz = min_t(unsigned int, count - pos, 8);
> - sz = data->packet(data, master, channel, STP_PACKET_DATA, flags,
> - sz, p);
> - flags = 0;
> -
> - if (sz < 0)
> + sz = data->packet(data, m, c, STP_PACKET_DATA, flags, sz,
> + &((u8 *)buf)[pos]);
> + if (sz <= 0)
> break;
> +
> + if (ts_first) {
> + flags = 0;
> + ts_first = false;
> + }
> }
>
> - data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0, );
> + return sz < 0 ? sz : pos;
> +}
> +EXPORT_SYMBOL_GPL(stm_data_write);
> +
> +static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> + unsigned int channel, const char *buf, size_t count)
> +{
> + ssize_t sz;
> +
> + sz = stm_data_write(data, master, channel, true, buf, count);
> + if (sz > 0)
> + data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
> + buf);
The original code the payload of a flag packet was '0' while in this patch
changes it to be anything. Some external tooling could be very confused.
>
> - return pos;
> + return sz;
> }
>
> static ssize_t stm_char_write(struct file *file, const char __user *buf,
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index 921ebd9fd3bd..753d83acb7ae 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -111,5 +111,8 @@ int stm_lookup_protocol(const char *name,
> const struct stm_protocol_driver **pdrv,
> const struct config_item_type **type);
> void stm_put_protocol(const struct stm_protocol_driver *pdrv);
> +ssize_t stm_data_write(struct stm_data *data, unsigned int m,
> +unsigned int c, bool ts_first, const void *buf,
> +size_t count);
>
> #endif /* _STM_STM_H_ */
> --
> 2.18.0
>
Re: [QUEUED v20180920 05/16] stm class: Add a helper for writing data packets
On Thu, Sep 20, 2018 at 03:45:42PM +0300, Alexander Shishkin wrote:
> Add a helper to write a sequence of bytes as STP data packets. This
> is used by protocol drivers to output their metadata, as well as the
> actual data payload.
>
> Signed-off-by: Alexander Shishkin
> ---
> drivers/hwtracing/stm/core.c | 50 ++--
> drivers/hwtracing/stm/stm.h | 3 +++
> 2 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index c869a30661ac..9ed2f06deb47 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -564,27 +564,51 @@ stm_assign_first_policy(struct stm_device *stm, struct
> stm_output *output,
> return err;
> }
>
> -static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> - unsigned int channel, const char *buf, size_t count)
> +/**
> + * stm_data_write() - send the given payload as data packets
> + * @data:stm driver's data
> + * @m: STP master
> + * @c: STP channel
> + * @ts_first:timestamp the first packet
> + * @buf: data payload buffer
> + * @count: data payload size
> + */
> +ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
> +unsigned int c, bool ts_first, const void *buf,
> +size_t count)
> {
> - unsigned int flags = STP_PACKET_TIMESTAMPED;
> - const unsigned char *p = buf, nil = 0;
> - size_t pos;
> + unsigned int flags = ts_first ? STP_PACKET_TIMESTAMPED : 0;
> ssize_t sz;
> + size_t pos;
>
> - for (pos = 0, p = buf; count > pos; pos += sz, p += sz) {
> + for (pos = 0, sz = 0; pos < count; pos += sz) {
> sz = min_t(unsigned int, count - pos, 8);
> - sz = data->packet(data, master, channel, STP_PACKET_DATA, flags,
> - sz, p);
> - flags = 0;
> -
> - if (sz < 0)
> + sz = data->packet(data, m, c, STP_PACKET_DATA, flags, sz,
> + &((u8 *)buf)[pos]);
> + if (sz <= 0)
> break;
> +
> + if (ts_first) {
> + flags = 0;
> + ts_first = false;
> + }
> }
>
> - data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0, );
> + return sz < 0 ? sz : pos;
> +}
> +EXPORT_SYMBOL_GPL(stm_data_write);
> +
> +static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> + unsigned int channel, const char *buf, size_t count)
> +{
> + ssize_t sz;
> +
> + sz = stm_data_write(data, master, channel, true, buf, count);
> + if (sz > 0)
> + data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
> + buf);
The original code the payload of a flag packet was '0' while in this patch
changes it to be anything. Some external tooling could be very confused.
>
> - return pos;
> + return sz;
> }
>
> static ssize_t stm_char_write(struct file *file, const char __user *buf,
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index 921ebd9fd3bd..753d83acb7ae 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -111,5 +111,8 @@ int stm_lookup_protocol(const char *name,
> const struct stm_protocol_driver **pdrv,
> const struct config_item_type **type);
> void stm_put_protocol(const struct stm_protocol_driver *pdrv);
> +ssize_t stm_data_write(struct stm_data *data, unsigned int m,
> +unsigned int c, bool ts_first, const void *buf,
> +size_t count);
>
> #endif /* _STM_STM_H_ */
> --
> 2.18.0
>
[QUEUED v20180920 05/16] stm class: Add a helper for writing data packets
Add a helper to write a sequence of bytes as STP data packets. This
is used by protocol drivers to output their metadata, as well as the
actual data payload.
Signed-off-by: Alexander Shishkin
---
drivers/hwtracing/stm/core.c | 50 ++--
drivers/hwtracing/stm/stm.h | 3 +++
2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index c869a30661ac..9ed2f06deb47 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -564,27 +564,51 @@ stm_assign_first_policy(struct stm_device *stm, struct
stm_output *output,
return err;
}
-static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
- unsigned int channel, const char *buf, size_t count)
+/**
+ * stm_data_write() - send the given payload as data packets
+ * @data: stm driver's data
+ * @m: STP master
+ * @c: STP channel
+ * @ts_first: timestamp the first packet
+ * @buf: data payload buffer
+ * @count: data payload size
+ */
+ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
+ unsigned int c, bool ts_first, const void *buf,
+ size_t count)
{
- unsigned int flags = STP_PACKET_TIMESTAMPED;
- const unsigned char *p = buf, nil = 0;
- size_t pos;
+ unsigned int flags = ts_first ? STP_PACKET_TIMESTAMPED : 0;
ssize_t sz;
+ size_t pos;
- for (pos = 0, p = buf; count > pos; pos += sz, p += sz) {
+ for (pos = 0, sz = 0; pos < count; pos += sz) {
sz = min_t(unsigned int, count - pos, 8);
- sz = data->packet(data, master, channel, STP_PACKET_DATA, flags,
- sz, p);
- flags = 0;
-
- if (sz < 0)
+ sz = data->packet(data, m, c, STP_PACKET_DATA, flags, sz,
+ &((u8 *)buf)[pos]);
+ if (sz <= 0)
break;
+
+ if (ts_first) {
+ flags = 0;
+ ts_first = false;
+ }
}
- data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0, );
+ return sz < 0 ? sz : pos;
+}
+EXPORT_SYMBOL_GPL(stm_data_write);
+
+static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
+ unsigned int channel, const char *buf, size_t count)
+{
+ ssize_t sz;
+
+ sz = stm_data_write(data, master, channel, true, buf, count);
+ if (sz > 0)
+ data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
+buf);
- return pos;
+ return sz;
}
static ssize_t stm_char_write(struct file *file, const char __user *buf,
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index 921ebd9fd3bd..753d83acb7ae 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -111,5 +111,8 @@ int stm_lookup_protocol(const char *name,
const struct stm_protocol_driver **pdrv,
const struct config_item_type **type);
void stm_put_protocol(const struct stm_protocol_driver *pdrv);
+ssize_t stm_data_write(struct stm_data *data, unsigned int m,
+ unsigned int c, bool ts_first, const void *buf,
+ size_t count);
#endif /* _STM_STM_H_ */
--
2.18.0
[QUEUED v20180920 05/16] stm class: Add a helper for writing data packets
Add a helper to write a sequence of bytes as STP data packets. This
is used by protocol drivers to output their metadata, as well as the
actual data payload.
Signed-off-by: Alexander Shishkin
---
drivers/hwtracing/stm/core.c | 50 ++--
drivers/hwtracing/stm/stm.h | 3 +++
2 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index c869a30661ac..9ed2f06deb47 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -564,27 +564,51 @@ stm_assign_first_policy(struct stm_device *stm, struct
stm_output *output,
return err;
}
-static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
- unsigned int channel, const char *buf, size_t count)
+/**
+ * stm_data_write() - send the given payload as data packets
+ * @data: stm driver's data
+ * @m: STP master
+ * @c: STP channel
+ * @ts_first: timestamp the first packet
+ * @buf: data payload buffer
+ * @count: data payload size
+ */
+ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
+ unsigned int c, bool ts_first, const void *buf,
+ size_t count)
{
- unsigned int flags = STP_PACKET_TIMESTAMPED;
- const unsigned char *p = buf, nil = 0;
- size_t pos;
+ unsigned int flags = ts_first ? STP_PACKET_TIMESTAMPED : 0;
ssize_t sz;
+ size_t pos;
- for (pos = 0, p = buf; count > pos; pos += sz, p += sz) {
+ for (pos = 0, sz = 0; pos < count; pos += sz) {
sz = min_t(unsigned int, count - pos, 8);
- sz = data->packet(data, master, channel, STP_PACKET_DATA, flags,
- sz, p);
- flags = 0;
-
- if (sz < 0)
+ sz = data->packet(data, m, c, STP_PACKET_DATA, flags, sz,
+ &((u8 *)buf)[pos]);
+ if (sz <= 0)
break;
+
+ if (ts_first) {
+ flags = 0;
+ ts_first = false;
+ }
}
- data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0, );
+ return sz < 0 ? sz : pos;
+}
+EXPORT_SYMBOL_GPL(stm_data_write);
+
+static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
+ unsigned int channel, const char *buf, size_t count)
+{
+ ssize_t sz;
+
+ sz = stm_data_write(data, master, channel, true, buf, count);
+ if (sz > 0)
+ data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
+buf);
- return pos;
+ return sz;
}
static ssize_t stm_char_write(struct file *file, const char __user *buf,
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index 921ebd9fd3bd..753d83acb7ae 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -111,5 +111,8 @@ int stm_lookup_protocol(const char *name,
const struct stm_protocol_driver **pdrv,
const struct config_item_type **type);
void stm_put_protocol(const struct stm_protocol_driver *pdrv);
+ssize_t stm_data_write(struct stm_data *data, unsigned int m,
+ unsigned int c, bool ts_first, const void *buf,
+ size_t count);
#endif /* _STM_STM_H_ */
--
2.18.0
