2023-05-29 11:37:10

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 00/12] firewire: deliver hardware time stamp for asynchronous transaction

Cc: [email protected]

Hi,

This patchset is revised version of the previous one[1], I realized that
it included wrong version number of Linux kernel. The new series
includes changes just for it.


Copied from the initial cover letter:

1394 OHCI hardware supports hardware time stamp for asynchronous
communication at 8,000 Hz resolution (= isochronous cycle), while
current implementation of FireWire subsystem does not deliver the time
stamp to both unit driver and user space application when operating the
asynchronous communication. It is inconvenient to a kind of application
which attempts to synchronize data from multiple sources by the (coarse)
time stamp.

This patchset changes the subsystem so that the unit driver and the user
space application to receive the time stamp, therefore it affects kernel
service for asynchronous transaction, kernel API for unit driver, and UAPI
for user space application.

[1] https://lore.kernel.org/lkml/[email protected]/

Takashi Sakamoto (12):
firewire: add KUnit test to check layout of UAPI structures
firewire: cdev: add new version of ABI to notify time stamp at
request/response subaction of transaction
firewire: cdev: add new event to notify request subaction with time
stamp
firewire: cdev: implement new event to notify request subaction with
time stamp
firewire: core: use union for callback of transaction completion
firewire: core: implement variations to send request and wait for
response with time stamp
firewire: cdev: code refactoring to operate event of response
firewire: cdev: add new event to notify response subaction with time
stamp
firewire: cdev: implement new event to notify response subaction with
time stamp
firewire: cdev: code refactoring to dispatch event for phy packet
firewire: cdev: add new event to notify phy packet with time stamp
firewire: cdev: implement new event relevant to phy packet with time
stamp

drivers/firewire/.kunitconfig | 4 +
drivers/firewire/Kconfig | 16 ++
drivers/firewire/Makefile | 3 +
drivers/firewire/core-cdev.c | 252 +++++++++++++++++++++-------
drivers/firewire/core-transaction.c | 93 +++++++---
drivers/firewire/core.h | 7 +
drivers/firewire/ohci.c | 17 +-
drivers/firewire/uapi-test.c | 87 ++++++++++
include/linux/firewire.h | 82 ++++++++-
include/uapi/linux/firewire-cdev.h | 180 +++++++++++++++++---
10 files changed, 625 insertions(+), 116 deletions(-)
create mode 100644 drivers/firewire/.kunitconfig
create mode 100644 drivers/firewire/uapi-test.c

--
2.39.2



2023-05-29 11:37:21

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 08/12] firewire: cdev: add new event to notify response subaction with time stamp

This commit adds new event to notify event of response subaction with
time stamp field.

Current compiler implementation of System V ABI selects one of structure
members which has the maximum alignment size in the structure to decide
the size of structure. In the case of fw_cdev_event_request3 structure,
it is closure member which has 8 byte storage. The size of alignment for
the type of 8 byte storage differs depending on architectures; 4 byte for
i386 architecture and 8 byte for the others including x32 architecture.
It is inconvenient to device driver developer to use structure layout
which varies between architectures since the developer takes care of ioctl
compat layer. This commit adds 32 bit member for padding to keep the
size of structure as multiples of 8.

Cc: [email protected]
Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/uapi-test.c | 15 ++++++++
include/uapi/linux/firewire-cdev.h | 59 +++++++++++++++++++++++++-----
2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c
index dd95899de316..640e5c05415a 100644
--- a/drivers/firewire/uapi-test.c
+++ b/drivers/firewire/uapi-test.c
@@ -45,9 +45,24 @@ static void structure_layout_event_request3(struct kunit *test)
KUNIT_EXPECT_EQ(test, 56, offsetof(struct fw_cdev_event_request3, data));
}

+// Added at v6.5.
+static void structure_layout_event_response2(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, 32, sizeof(struct fw_cdev_event_response2));
+
+ KUNIT_EXPECT_EQ(test, 0, offsetof(struct fw_cdev_event_response2, closure));
+ KUNIT_EXPECT_EQ(test, 8, offsetof(struct fw_cdev_event_response2, type));
+ KUNIT_EXPECT_EQ(test, 12, offsetof(struct fw_cdev_event_response2, rcode));
+ KUNIT_EXPECT_EQ(test, 16, offsetof(struct fw_cdev_event_response2, length));
+ KUNIT_EXPECT_EQ(test, 20, offsetof(struct fw_cdev_event_response2, request_tstamp));
+ KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_response2, response_tstamp));
+ KUNIT_EXPECT_EQ(test, 32, offsetof(struct fw_cdev_event_response2, data));
+}
+
static struct kunit_case structure_layout_test_cases[] = {
KUNIT_CASE(structure_layout_event_response),
KUNIT_CASE(structure_layout_event_request3),
+ KUNIT_CASE(structure_layout_event_response2),
{}
};

diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h
index 7767cd53a013..13892016c266 100644
--- a/include/uapi/linux/firewire-cdev.h
+++ b/include/uapi/linux/firewire-cdev.h
@@ -48,6 +48,7 @@

/* available since kernel version 6.5 */
#define FW_CDEV_EVENT_REQUEST3 0x0a
+#define FW_CDEV_EVENT_RESPONSE2 0x0b

/**
* struct fw_cdev_event_common - Common part of all fw_cdev_event_* types
@@ -106,6 +107,29 @@ struct fw_cdev_event_bus_reset {
* @length: Data length, i.e. the response's payload size in bytes
* @data: Payload data, if any
*
+ * This event is sent instead of &fw_cdev_event_response if the kernel or the client implements
+ * ABI version <= 5. It has the lack of time stamp field comparing to &fw_cdev_event_response2.
+ */
+struct fw_cdev_event_response {
+ __u64 closure;
+ __u32 type;
+ __u32 rcode;
+ __u32 length;
+ __u32 data[];
+};
+
+/**
+ * struct fw_cdev_event_response2 - Sent when a response packet was received
+ * @closure: See &fw_cdev_event_common; set by %FW_CDEV_IOC_SEND_REQUEST
+ * or %FW_CDEV_IOC_SEND_BROADCAST_REQUEST
+ * or %FW_CDEV_IOC_SEND_STREAM_PACKET ioctl
+ * @type: See &fw_cdev_event_common; always %FW_CDEV_EVENT_RESPONSE
+ * @rcode: Response code returned by the remote node
+ * @length: Data length, i.e. the response's payload size in bytes
+ * @request_tstamp: The time stamp of isochronous cycle at which the request was sent.
+ * @request_tstamp: The time stamp of isochronous cycle at which the response was sent.
+ * @data: Payload data, if any
+ *
* This event is sent when the stack receives a response to an outgoing request
* sent by %FW_CDEV_IOC_SEND_REQUEST ioctl. The payload data for responses
* carrying data (read and lock responses) follows immediately and can be
@@ -115,12 +139,25 @@ struct fw_cdev_event_bus_reset {
* involve response packets. This includes unified write transactions,
* broadcast write transactions, and transmission of asynchronous stream
* packets. @rcode indicates success or failure of such transmissions.
+ *
+ * The value of @request_tstamp expresses the isochronous cycle at which the request was sent to
+ * initiate the transaction. The value of @response_tstamp expresses the isochronous cycle at which
+ * the response arrived to complete the transaction. Each value is unsigned 16 bit integer
+ * containing three low order bits of second field and all 13 bits of cycle field in format of
+ * CYCLE_TIMER register.
*/
-struct fw_cdev_event_response {
+struct fw_cdev_event_response2 {
__u64 closure;
__u32 type;
__u32 rcode;
__u32 length;
+ __u32 request_tstamp;
+ __u32 response_tstamp;
+ /*
+ * Padding to keep the size of structure as multiples of 8 in various architectures since
+ * 4 byte alignment is used for 8 byte of object type in System V ABI for i386 architecture.
+ */
+ __u32 padding;
__u32 data[];
};

@@ -421,6 +458,7 @@ struct fw_cdev_event_phy_packet {
* %FW_CDEV_EVENT_PHY_PACKET_RECEIVED
*
* @request3: Valid if @common.type == %FW_CDEV_EVENT_REQUEST3
+ * @response2: Valid if @common.type == %FW_CDEV_EVENT_RESPONSE2
*
* Convenience union for userspace use. Events could be read(2) into an
* appropriately aligned char buffer and then cast to this union for further
@@ -441,6 +479,7 @@ union fw_cdev_event {
struct fw_cdev_event_iso_resource iso_resource; /* added in 2.6.30 */
struct fw_cdev_event_phy_packet phy_packet; /* added in 2.6.36 */
struct fw_cdev_event_request3 request3; /* added in 6.5 */
+ struct fw_cdev_event_response2 response2; /* added in 6.5 */
};

/* available since kernel version 2.6.22 */
@@ -507,6 +546,7 @@ union fw_cdev_event {
* - added %FW_CDEV_IOC_FLUSH_ISO
* 6 (6.5) - added some event for subactions of asynchronous transaction with time stamp
* - %FW_CDEV_EVENT_REQUEST3
+ * - %FW_CDEV_EVENT_RESPONSE2
*/

/**
@@ -552,11 +592,11 @@ struct fw_cdev_get_info {
* @data: Userspace pointer to payload
* @generation: The bus generation where packet is valid
*
- * Send a request to the device. This ioctl implements all outgoing requests.
- * Both quadlet and block request specify the payload as a pointer to the data
- * in the @data field. Once the transaction completes, the kernel writes an
- * &fw_cdev_event_response event back. The @closure field is passed back to
- * user space in the response event.
+ * Send a request to the device. This ioctl implements all outgoing requests. Both quadlet and
+ * block request specify the payload as a pointer to the data in the @data field. Once the
+ * transaction completes, the kernel writes either &fw_cdev_event_response event or
+ * &fw_cdev_event_response event back. The @closure field is passed back to user space in the
+ * response event.
*/
struct fw_cdev_send_request {
__u32 tcode;
@@ -1039,10 +1079,9 @@ struct fw_cdev_allocate_iso_resource {
* @generation: The bus generation where packet is valid
* @speed: Speed to transmit at
*
- * The %FW_CDEV_IOC_SEND_STREAM_PACKET ioctl sends an asynchronous stream packet
- * to every device which is listening to the specified channel. The kernel
- * writes an &fw_cdev_event_response event which indicates success or failure of
- * the transmission.
+ * The %FW_CDEV_IOC_SEND_STREAM_PACKET ioctl sends an asynchronous stream packet to every device
+ * which is listening to the specified channel. The kernel writes either &fw_cdev_event_response
+ * event or &fw_cdev_event_response2 event which indicates success or failure of the transmission.
*/
struct fw_cdev_send_stream_packet {
__u32 length;
--
2.39.2


2023-05-29 11:58:28

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 09/12] firewire: cdev: implement new event to notify response subaction with time stamp

The callback function now receives an argument for time stamps relevant
to asynchronous transaction. This commit implements a new event to
notify response subaction with the time stamps for user space.

Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/core-cdev.c | 96 ++++++++++++++++++++++++------------
1 file changed, 65 insertions(+), 31 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 315ebc8c545d..8b24abdd51b8 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -172,6 +172,7 @@ struct outbound_transaction_event {
struct outbound_transaction_resource r;
union {
struct fw_cdev_event_response without_tstamp;
+ struct fw_cdev_event_response2 with_tstamp;
} rsp;
};

@@ -538,41 +539,64 @@ static void release_transaction(struct client *client,
{
}

-static void complete_transaction(struct fw_card *card, int rcode,
- void *payload, size_t length, void *data)
+static void complete_transaction(struct fw_card *card, int rcode, u32 request_tstamp,
+ u32 response_tstamp, void *payload, size_t length, void *data)
{
struct outbound_transaction_event *e = data;
- struct fw_cdev_event_response *rsp = &e->rsp.without_tstamp;
struct client *client = e->client;
unsigned long flags;

- if (length < rsp->length)
- rsp->length = length;
- if (rcode == RCODE_COMPLETE)
- memcpy(rsp->data, payload, rsp->length);
-
spin_lock_irqsave(&client->lock, flags);
idr_remove(&client->resource_idr, e->r.resource.handle);
if (client->in_shutdown)
wake_up(&client->tx_flush_wait);
spin_unlock_irqrestore(&client->lock, flags);

- rsp->type = FW_CDEV_EVENT_RESPONSE;
- rsp->rcode = rcode;
+ switch (e->rsp.without_tstamp.type) {
+ case FW_CDEV_EVENT_RESPONSE:
+ {
+ struct fw_cdev_event_response *rsp = &e->rsp.without_tstamp;
+
+ if (length < rsp->length)
+ rsp->length = length;
+ if (rcode == RCODE_COMPLETE)
+ memcpy(rsp->data, payload, rsp->length);
+
+ rsp->rcode = rcode;
+
+ // In the case that sizeof(*rsp) doesn't align with the position of the
+ // data, and the read is short, preserve an extra copy of the data
+ // to stay compatible with a pre-2.6.27 bug. Since the bug is harmless
+ // for short reads and some apps depended on it, this is both safe
+ // and prudent for compatibility.
+ if (rsp->length <= sizeof(*rsp) - offsetof(typeof(*rsp), data))
+ queue_event(client, &e->event, rsp, sizeof(*rsp), rsp->data, rsp->length);
+ else
+ queue_event(client, &e->event, rsp, sizeof(*rsp) + rsp->length, NULL, 0);

- /*
- * In the case that sizeof(*rsp) doesn't align with the position of the
- * data, and the read is short, preserve an extra copy of the data
- * to stay compatible with a pre-2.6.27 bug. Since the bug is harmless
- * for short reads and some apps depended on it, this is both safe
- * and prudent for compatibility.
- */
- if (rsp->length <= sizeof(*rsp) - offsetof(typeof(*rsp), data))
- queue_event(client, &e->event, rsp, sizeof(*rsp),
- rsp->data, rsp->length);
- else
- queue_event(client, &e->event, rsp, sizeof(*rsp) + rsp->length,
- NULL, 0);
+ break;
+ }
+ case FW_CDEV_EVENT_RESPONSE2:
+ {
+ struct fw_cdev_event_response2 *rsp = &e->rsp.with_tstamp;
+
+ if (length < rsp->length)
+ rsp->length = length;
+ if (rcode == RCODE_COMPLETE)
+ memcpy(rsp->data, payload, rsp->length);
+
+ rsp->rcode = rcode;
+ rsp->request_tstamp = request_tstamp;
+ rsp->response_tstamp = response_tstamp;
+
+ queue_event(client, &e->event, rsp, sizeof(*rsp) + rsp->length, NULL, 0);
+
+ break;
+ default:
+ WARN_ON(1);
+ break;
+ }
+ }

/* Drop the idr's reference */
client_put(client);
@@ -583,7 +607,6 @@ static int init_request(struct client *client,
int destination_id, int speed)
{
struct outbound_transaction_event *e;
- struct fw_cdev_event_response *rsp;
void *payload;
int ret;

@@ -600,10 +623,21 @@ static int init_request(struct client *client,
return -ENOMEM;
e->client = client;

- rsp = &e->rsp.without_tstamp;
- rsp->length = request->length;
- rsp->closure = request->closure;
- payload = rsp->data;
+ if (client->version < FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP) {
+ struct fw_cdev_event_response *rsp = &e->rsp.without_tstamp;
+
+ rsp->type = FW_CDEV_EVENT_RESPONSE;
+ rsp->length = request->length;
+ rsp->closure = request->closure;
+ payload = rsp->data;
+ } else {
+ struct fw_cdev_event_response2 *rsp = &e->rsp.with_tstamp;
+
+ rsp->type = FW_CDEV_EVENT_RESPONSE2;
+ rsp->length = request->length;
+ rsp->closure = request->closure;
+ payload = rsp->data;
+ }

if (request->data && copy_from_user(payload, u64_to_uptr(request->data), request->length)) {
ret = -EFAULT;
@@ -615,9 +649,9 @@ static int init_request(struct client *client,
if (ret < 0)
goto failed;

- fw_send_request(client->device->card, &e->r.transaction, request->tcode, destination_id,
- request->generation, speed, request->offset, payload, request->length,
- complete_transaction, e);
+ fw_send_request_with_tstamp(client->device->card, &e->r.transaction, request->tcode,
+ destination_id, request->generation, speed, request->offset,
+ payload, request->length, complete_transaction, e);
return 0;

failed:
--
2.39.2


2023-05-29 12:00:18

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 11/12] firewire: cdev: add new event to notify phy packet with time stamp

This commit adds new event to notify event of phy packet with time stamp
field.

Unlike the fw_cdev_event_request3 and fw_cdev_event_response2, the size
of new structure, fw_cdev_event_phy_packet2, is multiples of 8, thus
padding is not required to keep the same size between System V ABI for
different architectures.

It is noticeable that for the case of ping request 1394 OHCI controller
does not record the isochronous cycle at which the packet was sent for
the request subaction. Instead, it records round-trip count measured by
hardware at 42.195 MHz resolution.

Cc: [email protected]
Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/uapi-test.c | 14 +++++++
include/uapi/linux/firewire-cdev.h | 67 +++++++++++++++++++++++++-----
2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c
index 640e5c05415a..c6ebf02e3d45 100644
--- a/drivers/firewire/uapi-test.c
+++ b/drivers/firewire/uapi-test.c
@@ -59,10 +59,24 @@ static void structure_layout_event_response2(struct kunit *test)
KUNIT_EXPECT_EQ(test, 32, offsetof(struct fw_cdev_event_response2, data));
}

+// Added at v6.5.
+static void structure_layout_event_phy_packet2(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, 24, sizeof(struct fw_cdev_event_phy_packet2));
+
+ KUNIT_EXPECT_EQ(test, 0, offsetof(struct fw_cdev_event_phy_packet2, closure));
+ KUNIT_EXPECT_EQ(test, 8, offsetof(struct fw_cdev_event_phy_packet2, type));
+ KUNIT_EXPECT_EQ(test, 12, offsetof(struct fw_cdev_event_phy_packet2, rcode));
+ KUNIT_EXPECT_EQ(test, 16, offsetof(struct fw_cdev_event_phy_packet2, length));
+ KUNIT_EXPECT_EQ(test, 20, offsetof(struct fw_cdev_event_phy_packet2, tstamp));
+ KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_phy_packet2, data));
+}
+
static struct kunit_case structure_layout_test_cases[] = {
KUNIT_CASE(structure_layout_event_response),
KUNIT_CASE(structure_layout_event_request3),
KUNIT_CASE(structure_layout_event_response2),
+ KUNIT_CASE(structure_layout_event_phy_packet2),
{}
};

diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h
index 13892016c266..d72705b49687 100644
--- a/include/uapi/linux/firewire-cdev.h
+++ b/include/uapi/linux/firewire-cdev.h
@@ -49,6 +49,8 @@
/* available since kernel version 6.5 */
#define FW_CDEV_EVENT_REQUEST3 0x0a
#define FW_CDEV_EVENT_RESPONSE2 0x0b
+#define FW_CDEV_EVENT_PHY_PACKET_SENT2 0x0c
+#define FW_CDEV_EVENT_PHY_PACKET_RECEIVED2 0x0d

/**
* struct fw_cdev_event_common - Common part of all fw_cdev_event_* types
@@ -423,20 +425,59 @@ struct fw_cdev_event_iso_resource {
* @type: %FW_CDEV_EVENT_PHY_PACKET_SENT or %..._RECEIVED
* @rcode: %RCODE_..., indicates success or failure of transmission
* @length: Data length in bytes
+ * @data: Incoming data for %FW_CDEV_IOC_RECEIVE_PHY_PACKETS. For %FW_CDEV_IOC_SEND_PHY_PACKET
+ * the field has the same data in the request, thus the length of 8 bytes.
+ *
+ * This event is sent instead of &fw_cdev_event_phy_packet2 if the kernel or
+ * the client implements ABI version <= 5. It has the lack of time stamp field comparing to
+ * &fw_cdev_event_phy_packet2.
+ */
+struct fw_cdev_event_phy_packet {
+ __u64 closure;
+ __u32 type;
+ __u32 rcode;
+ __u32 length;
+ __u32 data[];
+};
+
+/**
+ * struct fw_cdev_event_phy_packet2 - A PHY packet was transmitted or received with time stamp.
+ * @closure: See &fw_cdev_event_common; set by %FW_CDEV_IOC_SEND_PHY_PACKET
+ * or %FW_CDEV_IOC_RECEIVE_PHY_PACKETS ioctl
+ * @type: %FW_CDEV_EVENT_PHY_PACKET_SENT2 or %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2
+ * @rcode: %RCODE_..., indicates success or failure of transmission
+ * @length: Data length in bytes
+ * @tstamp: For %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2, the time stamp of isochronous cycle at
+ * which the packet arrived. For %FW_CDEV_EVENT_PHY_PACKET_SENT2 and non-ping packet,
+ * the time stamp of isochronous cycle at which the packet was sent. For ping packet,
+ * the tick count for round-trip time measured by 1394 OHCI controller.
+ * The time stamp of isochronous cycle at which either the response was sent for
+ * %FW_CDEV_EVENT_PHY_PACKET_SENT2 or the request arrived for
+ * %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2.
* @data: Incoming data
*
- * If @type is %FW_CDEV_EVENT_PHY_PACKET_SENT, @length is 0 and @data empty,
- * except in case of a ping packet: Then, @length is 4, and @data[0] is the
- * ping time in 49.152MHz clocks if @rcode is %RCODE_COMPLETE.
+ * If @type is %FW_CDEV_EVENT_PHY_PACKET_SENT2, @length is 8 and @data consists of the two PHY
+ * packet quadlets to be sent, in host byte order,
*
- * If @type is %FW_CDEV_EVENT_PHY_PACKET_RECEIVED, @length is 8 and @data
- * consists of the two PHY packet quadlets, in host byte order.
+ * If @type is %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2, @length is 8 and @data consists of the two PHY
+ * packet quadlets, in host byte order.
+ *
+ * For %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2, the @tstamp is the isochronous cycle at which the
+ * packet arrived. It is 16 bit integer value and the higher 3 bits expresses three low order bits
+ * of second field and the rest 13 bits expresses cycle field in the format of CYCLE_TIME register.
+ *
+ * For %FW_CDEV_EVENT_PHY_PACKET_SENT2, the @tstamp has different meanings whether to sent the
+ * packet for ping or not. If it's not for ping, the @tstamp is the isochronous cycle at which the
+ * packet was sent, and use the same format as the case of %FW_CDEV_EVENT_PHY_PACKET_SENT2. If it's
+ * for ping, the @tstamp is for round-trip time measured by 1394 OHCI controller with 42.195 MHz
+ * resolution.
*/
-struct fw_cdev_event_phy_packet {
+struct fw_cdev_event_phy_packet2 {
__u64 closure;
__u32 type;
__u32 rcode;
__u32 length;
+ __u32 tstamp;
__u32 data[];
};

@@ -459,6 +500,8 @@ struct fw_cdev_event_phy_packet {
*
* @request3: Valid if @common.type == %FW_CDEV_EVENT_REQUEST3
* @response2: Valid if @common.type == %FW_CDEV_EVENT_RESPONSE2
+ * @phy_packet2: Valid if @common.type == %FW_CDEV_EVENT_PHY_PACKET_SENT2 or
+ * %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2
*
* Convenience union for userspace use. Events could be read(2) into an
* appropriately aligned char buffer and then cast to this union for further
@@ -480,6 +523,7 @@ union fw_cdev_event {
struct fw_cdev_event_phy_packet phy_packet; /* added in 2.6.36 */
struct fw_cdev_event_request3 request3; /* added in 6.5 */
struct fw_cdev_event_response2 response2; /* added in 6.5 */
+ struct fw_cdev_event_phy_packet2 phy_packet2; /* added in 6.5 */
};

/* available since kernel version 2.6.22 */
@@ -547,6 +591,8 @@ union fw_cdev_event {
* 6 (6.5) - added some event for subactions of asynchronous transaction with time stamp
* - %FW_CDEV_EVENT_REQUEST3
* - %FW_CDEV_EVENT_RESPONSE2
+ * - %FW_CDEV_EVENT_PHY_PACKET_SENT2
+ * - %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2
*/

/**
@@ -1100,8 +1146,8 @@ struct fw_cdev_send_stream_packet {
* @data: First and second quadlet of the PHY packet
* @generation: The bus generation where packet is valid
*
- * The %FW_CDEV_IOC_SEND_PHY_PACKET ioctl sends a PHY packet to all nodes
- * on the same card as this device. After transmission, an
+ * The %FW_CDEV_IOC_SEND_PHY_PACKET ioctl sends a PHY packet to all nodes on the same card as this
+ * device. After transmission, either %FW_CDEV_EVENT_PHY_PACKET_SENT event or
* %FW_CDEV_EVENT_PHY_PACKET_SENT event is generated.
*
* The payload @data\[\] shall be specified in host byte order. Usually,
@@ -1120,8 +1166,9 @@ struct fw_cdev_send_phy_packet {
* struct fw_cdev_receive_phy_packets - start reception of PHY packets
* @closure: Passed back to userspace in phy packet events
*
- * This ioctl activates issuing of %FW_CDEV_EVENT_PHY_PACKET_RECEIVED due to
- * incoming PHY packets from any node on the same bus as the device.
+ * This ioctl activates issuing of either %FW_CDEV_EVENT_PHY_PACKET_RECEIVED or
+ * %FW_CDEV_EVENT_PHY_PACKET_RECEIVED2 due to incoming PHY packets from any node on the same bus
+ * as the device.
*
* The ioctl is only permitted on device files which represent a local node.
*/
--
2.39.2


2023-05-29 12:02:50

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 10/12] firewire: cdev: code refactoring to dispatch event for phy packet

In 1394 OHCI, both Asynchronous Transmit (AT) and Asynchronous Receive
(AR) contexts are used to deliver the phy packet of IEEE 1394. The time
stamp is available as well as the usual asynchronous transaction.

This commit is a preparation for future commit to handle the time stamp.

Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/core-cdev.c | 78 +++++++++++++++++++++++-------------
1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 8b24abdd51b8..2220de3c945e 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -204,12 +204,16 @@ struct outbound_phy_packet_event {
struct event event;
struct client *client;
struct fw_packet p;
- struct fw_cdev_event_phy_packet phy_packet;
+ union {
+ struct fw_cdev_event_phy_packet without_tstamp;
+ } phy_packet;
};

struct inbound_phy_packet_event {
struct event event;
- struct fw_cdev_event_phy_packet phy_packet;
+ union {
+ struct fw_cdev_event_phy_packet without_tstamp;
+ } phy_packet;
};

#ifdef CONFIG_COMPAT
@@ -1549,26 +1553,41 @@ static void outbound_phy_packet_callback(struct fw_packet *packet,
{
struct outbound_phy_packet_event *e =
container_of(packet, struct outbound_phy_packet_event, p);
- struct client *e_client;
+ struct client *e_client = e->client;
+ u32 rcode;
+ struct fw_cdev_event_phy_packet *pp;

switch (status) {
- /* expected: */
- case ACK_COMPLETE: e->phy_packet.rcode = RCODE_COMPLETE; break;
- /* should never happen with PHY packets: */
- case ACK_PENDING: e->phy_packet.rcode = RCODE_COMPLETE; break;
+ // expected:
+ case ACK_COMPLETE:
+ rcode = RCODE_COMPLETE;
+ break;
+ // should never happen with PHY packets:
+ case ACK_PENDING:
+ rcode = RCODE_COMPLETE;
+ break;
case ACK_BUSY_X:
case ACK_BUSY_A:
- case ACK_BUSY_B: e->phy_packet.rcode = RCODE_BUSY; break;
- case ACK_DATA_ERROR: e->phy_packet.rcode = RCODE_DATA_ERROR; break;
- case ACK_TYPE_ERROR: e->phy_packet.rcode = RCODE_TYPE_ERROR; break;
- /* stale generation; cancelled; on certain controllers: no ack */
- default: e->phy_packet.rcode = status; break;
+ case ACK_BUSY_B:
+ rcode = RCODE_BUSY;
+ break;
+ case ACK_DATA_ERROR:
+ rcode = RCODE_DATA_ERROR;
+ break;
+ case ACK_TYPE_ERROR:
+ rcode = RCODE_TYPE_ERROR;
+ break;
+ // stale generation; cancelled; on certain controllers: no ack
+ default:
+ rcode = status;
+ break;
}
- e->phy_packet.data[0] = packet->timestamp;

- e_client = e->client;
- queue_event(e->client, &e->event, &e->phy_packet,
- sizeof(e->phy_packet) + e->phy_packet.length, NULL, 0);
+ pp = &e->phy_packet.without_tstamp;
+ pp->rcode = rcode;
+ pp->data[0] = packet->timestamp;
+ queue_event(e->client, &e->event, &e->phy_packet, sizeof(*pp) + pp->length, NULL, 0);
+
client_put(e_client);
}

@@ -1577,6 +1596,7 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
struct fw_cdev_send_phy_packet *a = &arg->send_phy_packet;
struct fw_card *card = client->device->card;
struct outbound_phy_packet_event *e;
+ struct fw_cdev_event_phy_packet *pp;

/* Access policy: Allow this ioctl only on local nodes' device files. */
if (!client->device->is_local)
@@ -1595,10 +1615,12 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
e->p.header[2] = a->data[1];
e->p.header_length = 12;
e->p.callback = outbound_phy_packet_callback;
- e->phy_packet.closure = a->closure;
- e->phy_packet.type = FW_CDEV_EVENT_PHY_PACKET_SENT;
+
+ pp = &e->phy_packet.without_tstamp;
+ pp->closure = a->closure;
+ pp->type = FW_CDEV_EVENT_PHY_PACKET_SENT;
if (is_ping_packet(a->data))
- e->phy_packet.length = 4;
+ pp->length = 4;

card->driver->send_request(card, &e->p);

@@ -1633,18 +1655,20 @@ void fw_cdev_handle_phy_packet(struct fw_card *card, struct fw_packet *p)
spin_lock_irqsave(&card->lock, flags);

list_for_each_entry(client, &card->phy_receiver_list, phy_receiver_link) {
+ struct fw_cdev_event_phy_packet *pp;
+
e = kmalloc(sizeof(*e) + 8, GFP_ATOMIC);
if (e == NULL)
break;

- e->phy_packet.closure = client->phy_receiver_closure;
- e->phy_packet.type = FW_CDEV_EVENT_PHY_PACKET_RECEIVED;
- e->phy_packet.rcode = RCODE_COMPLETE;
- e->phy_packet.length = 8;
- e->phy_packet.data[0] = p->header[1];
- e->phy_packet.data[1] = p->header[2];
- queue_event(client, &e->event,
- &e->phy_packet, sizeof(e->phy_packet) + 8, NULL, 0);
+ pp = &e->phy_packet.without_tstamp;
+ pp->closure = client->phy_receiver_closure;
+ pp->type = FW_CDEV_EVENT_PHY_PACKET_RECEIVED;
+ pp->rcode = RCODE_COMPLETE;
+ pp->length = 8;
+ pp->data[0] = p->header[1];
+ pp->data[1] = p->header[2];
+ queue_event(client, &e->event, &e->phy_packet, sizeof(*pp) + 8, NULL, 0);
}

spin_unlock_irqrestore(&card->lock, flags);
--
2.39.2


2023-05-29 12:03:12

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 12/12] firewire: cdev: implement new event relevant to phy packet with time stamp

In 1394 OHCI, the OUTPUT_LAST descriptor of Asynchronous Transmit (AT)
context has timeStamp field, in which 1394 OHCI controller record the
isochronous cycle when the packet was sent for the request subaction.
Additionally, the trailing quadlet of Asynchronous Receive (AR) context
has timeStamp field as well in which 1394 OHCI controller record the
isochronous cycle when the packet arrived. The time stamps are also
available for the cases to send and receive phy packet.

This commit implements new events with time stamp field for user space.

Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/core-cdev.c | 90 +++++++++++++++++++++++++++---------
1 file changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 2220de3c945e..6274b86eb943 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -206,6 +206,7 @@ struct outbound_phy_packet_event {
struct fw_packet p;
union {
struct fw_cdev_event_phy_packet without_tstamp;
+ struct fw_cdev_event_phy_packet2 with_tstamp;
} phy_packet;
};

@@ -213,6 +214,7 @@ struct inbound_phy_packet_event {
struct event event;
union {
struct fw_cdev_event_phy_packet without_tstamp;
+ struct fw_cdev_event_phy_packet2 with_tstamp;
} phy_packet;
};

@@ -1555,7 +1557,6 @@ static void outbound_phy_packet_callback(struct fw_packet *packet,
container_of(packet, struct outbound_phy_packet_event, p);
struct client *e_client = e->client;
u32 rcode;
- struct fw_cdev_event_phy_packet *pp;

switch (status) {
// expected:
@@ -1583,10 +1584,31 @@ static void outbound_phy_packet_callback(struct fw_packet *packet,
break;
}

- pp = &e->phy_packet.without_tstamp;
- pp->rcode = rcode;
- pp->data[0] = packet->timestamp;
- queue_event(e->client, &e->event, &e->phy_packet, sizeof(*pp) + pp->length, NULL, 0);
+ switch (e->phy_packet.without_tstamp.type) {
+ case FW_CDEV_EVENT_PHY_PACKET_SENT:
+ {
+ struct fw_cdev_event_phy_packet *pp = &e->phy_packet.without_tstamp;
+
+ pp->rcode = rcode;
+ pp->data[0] = packet->timestamp;
+ queue_event(e->client, &e->event, &e->phy_packet, sizeof(*pp) + pp->length,
+ NULL, 0);
+ break;
+ }
+ case FW_CDEV_EVENT_PHY_PACKET_SENT2:
+ {
+ struct fw_cdev_event_phy_packet2 *pp = &e->phy_packet.with_tstamp;
+
+ pp->rcode = rcode;
+ pp->tstamp = packet->timestamp;
+ queue_event(e->client, &e->event, &e->phy_packet, sizeof(*pp) + pp->length,
+ NULL, 0);
+ break;
+ }
+ default:
+ WARN_ON(1);
+ break;
+ }

client_put(e_client);
}
@@ -1596,13 +1618,12 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
struct fw_cdev_send_phy_packet *a = &arg->send_phy_packet;
struct fw_card *card = client->device->card;
struct outbound_phy_packet_event *e;
- struct fw_cdev_event_phy_packet *pp;

/* Access policy: Allow this ioctl only on local nodes' device files. */
if (!client->device->is_local)
return -ENOSYS;

- e = kzalloc(sizeof(*e) + 4, GFP_KERNEL);
+ e = kzalloc(sizeof(*e) + sizeof(a->data), GFP_KERNEL);
if (e == NULL)
return -ENOMEM;

@@ -1616,11 +1637,23 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
e->p.header_length = 12;
e->p.callback = outbound_phy_packet_callback;

- pp = &e->phy_packet.without_tstamp;
- pp->closure = a->closure;
- pp->type = FW_CDEV_EVENT_PHY_PACKET_SENT;
- if (is_ping_packet(a->data))
- pp->length = 4;
+ if (client->version < FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP) {
+ struct fw_cdev_event_phy_packet *pp = &e->phy_packet.without_tstamp;
+
+ pp->closure = a->closure;
+ pp->type = FW_CDEV_EVENT_PHY_PACKET_SENT;
+ if (is_ping_packet(a->data))
+ pp->length = 4;
+ } else {
+ struct fw_cdev_event_phy_packet2 *pp = &e->phy_packet.with_tstamp;
+
+ pp->closure = a->closure;
+ pp->type = FW_CDEV_EVENT_PHY_PACKET_SENT2;
+ // Keep the data field so that application can match the response event to the
+ // request.
+ pp->length = sizeof(a->data);
+ memcpy(pp->data, a->data, sizeof(a->data));
+ }

card->driver->send_request(card, &e->p);

@@ -1655,20 +1688,33 @@ void fw_cdev_handle_phy_packet(struct fw_card *card, struct fw_packet *p)
spin_lock_irqsave(&card->lock, flags);

list_for_each_entry(client, &card->phy_receiver_list, phy_receiver_link) {
- struct fw_cdev_event_phy_packet *pp;
-
e = kmalloc(sizeof(*e) + 8, GFP_ATOMIC);
if (e == NULL)
break;

- pp = &e->phy_packet.without_tstamp;
- pp->closure = client->phy_receiver_closure;
- pp->type = FW_CDEV_EVENT_PHY_PACKET_RECEIVED;
- pp->rcode = RCODE_COMPLETE;
- pp->length = 8;
- pp->data[0] = p->header[1];
- pp->data[1] = p->header[2];
- queue_event(client, &e->event, &e->phy_packet, sizeof(*pp) + 8, NULL, 0);
+ if (client->version < FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP) {
+ struct fw_cdev_event_phy_packet *pp = &e->phy_packet.without_tstamp;
+
+ pp->closure = client->phy_receiver_closure;
+ pp->type = FW_CDEV_EVENT_PHY_PACKET_RECEIVED;
+ pp->rcode = RCODE_COMPLETE;
+ pp->length = 8;
+ pp->data[0] = p->header[1];
+ pp->data[1] = p->header[2];
+ queue_event(client, &e->event, &e->phy_packet, sizeof(*pp) + 8, NULL, 0);
+ } else {
+ struct fw_cdev_event_phy_packet2 *pp = &e->phy_packet.with_tstamp;
+
+ pp = &e->phy_packet.with_tstamp;
+ pp->closure = client->phy_receiver_closure;
+ pp->type = FW_CDEV_EVENT_PHY_PACKET_RECEIVED2;
+ pp->rcode = RCODE_COMPLETE;
+ pp->length = 8;
+ pp->tstamp = p->timestamp;
+ pp->data[0] = p->header[1];
+ pp->data[1] = p->header[2];
+ queue_event(client, &e->event, &e->phy_packet, sizeof(*pp) + 8, NULL, 0);
+ }
}

spin_unlock_irqrestore(&card->lock, flags);
--
2.39.2


2023-05-29 12:03:32

by Takashi Sakamoto

[permalink] [raw]
Subject: [PATCH v3 02/12] firewire: cdev: add new version of ABI to notify time stamp at request/response subaction of transaction

This commit adds new version of ABI for future new events with time stamp
for request/response subaction of asynchronous transaction to user
space.

Signed-off-by: Takashi Sakamoto <[email protected]>
---
drivers/firewire/core-cdev.c | 1 +
include/uapi/linux/firewire-cdev.h | 1 +
2 files changed, 2 insertions(+)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 2c16ee8fd842..88c8b5fac5e5 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -43,6 +43,7 @@
#define FW_CDEV_VERSION_EVENT_REQUEST2 4
#define FW_CDEV_VERSION_ALLOCATE_REGION_END 4
#define FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW 5
+#define FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP 6

struct client {
u32 version;
diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h
index 92be3ea3c6e0..76441eb551e5 100644
--- a/include/uapi/linux/firewire-cdev.h
+++ b/include/uapi/linux/firewire-cdev.h
@@ -457,6 +457,7 @@ union fw_cdev_event {
* 5 (3.4) - send %FW_CDEV_EVENT_ISO_INTERRUPT events when needed to
* avoid dropping data
* - added %FW_CDEV_IOC_FLUSH_ISO
+ * 6 (6.5) - added some event for subactions of asynchronous transaction with time stamp
*/

/**
--
2.39.2


2023-05-29 23:52:57

by Takashi Sakamoto

[permalink] [raw]
Subject: Re: [PATCH v3 00/12] firewire: deliver hardware time stamp for asynchronous transaction

On Mon, May 29, 2023 at 08:33:54PM +0900, Takashi Sakamoto wrote:
> Cc: [email protected]
>
> Hi,
>
> This patchset is revised version of the previous one[1], I realized that
> it included wrong version number of Linux kernel. The new series
> includes changes just for it.
>
>
> Copied from the initial cover letter:
>
> 1394 OHCI hardware supports hardware time stamp for asynchronous
> communication at 8,000 Hz resolution (= isochronous cycle), while
> current implementation of FireWire subsystem does not deliver the time
> stamp to both unit driver and user space application when operating the
> asynchronous communication. It is inconvenient to a kind of application
> which attempts to synchronize data from multiple sources by the (coarse)
> time stamp.
>
> This patchset changes the subsystem so that the unit driver and the user
> space application to receive the time stamp, therefore it affects kernel
> service for asynchronous transaction, kernel API for unit driver, and UAPI
> for user space application.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Takashi Sakamoto (12):
> firewire: add KUnit test to check layout of UAPI structures
> firewire: cdev: add new version of ABI to notify time stamp at
> request/response subaction of transaction
> firewire: cdev: add new event to notify request subaction with time
> stamp
> firewire: cdev: implement new event to notify request subaction with
> time stamp
> firewire: core: use union for callback of transaction completion
> firewire: core: implement variations to send request and wait for
> response with time stamp
> firewire: cdev: code refactoring to operate event of response
> firewire: cdev: add new event to notify response subaction with time
> stamp
> firewire: cdev: implement new event to notify response subaction with
> time stamp
> firewire: cdev: code refactoring to dispatch event for phy packet
> firewire: cdev: add new event to notify phy packet with time stamp
> firewire: cdev: implement new event relevant to phy packet with time
> stamp
>
> drivers/firewire/.kunitconfig | 4 +
> drivers/firewire/Kconfig | 16 ++
> drivers/firewire/Makefile | 3 +
> drivers/firewire/core-cdev.c | 252 +++++++++++++++++++++-------
> drivers/firewire/core-transaction.c | 93 +++++++---
> drivers/firewire/core.h | 7 +
> drivers/firewire/ohci.c | 17 +-
> drivers/firewire/uapi-test.c | 87 ++++++++++
> include/linux/firewire.h | 82 ++++++++-
> include/uapi/linux/firewire-cdev.h | 180 +++++++++++++++++---
> 10 files changed, 625 insertions(+), 116 deletions(-)
> create mode 100644 drivers/firewire/.kunitconfig
> create mode 100644 drivers/firewire/uapi-test.c

Applied to for-next branch.

* https://git.kernel.org/ieee1394/linux1394/c/fe971f9163b6


Thanks

Takashi Sakamoto