[PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[Takashi Sakamoto]

Hi,

In 1394 OHCI specification, each descriptor of IR/IT/AR/AT DMA context
has timeStamp field. The value of timeStamp field express the time in
which the controller accept packet. The resolution of value is isochronous
cycle count (8,000 Hz) with second up to 7.

I have a plan to use the value of timeStamp field for ALSA firewire stack
so that userspace ALSA PCM/Rawmidi applications can get converted timestamp
(ktime) for PCM frame/MIDI message. The timestamp can ideally express
finer granularity than the time to invoke IRQ handler (and co).

Current implementation of Linux FireWire subsystem delivers the value of
timeStamp field to unit driver for IR/IT/AT DMA context, but not for AR
DMA context. Additionally, the way to refer to Isochronous Cycle Timer
Register in MMIO region of 1394 OHCI controller is transaction to local
node. It includes overhead of transaction and it's preferable to add
less-overhead way available in any type of IRQ context.

This patchset adds two functions exposed in kernel space:

 * fw_card_read_cycle_time()
    * allow unit driver to access to CYCLE_TIME register in MMIO region
      without initiate transaction
 * fw_request_get_timestamp()
    * allow unit driver to get timestamp of request packet inner request
      handler

I note that Hector Martin found kernel null pointer dereference during
process to remove PCI card and has posted a patch:

 * https://lore.kernel.org/lkml/20211027113130.8802-1-marcan@marcan.st/

His patch is included in the series with my comment for relevant commit
20802224298c ("firewire: core: add forgotten dummy driver methods, remove
unused ones"). The patch is required since unit driver can refer to dummy
driver between removal callback of PCI subsystem and removal callback of
FireWire subsystem.

Hector Martin (1):
  firewire: Add dummy read_csr/write_csr functions

Takashi Sakamoto (2):
  firewire: add kernel API to access CYCLE_TIME register
  firewire: add kernel API to access packet structure in request
    structure for AR context

 drivers/firewire/core-card.c        | 39 +++++++++++++++++++++++++++++
 drivers/firewire/core-cdev.c        |  6 +++--
 drivers/firewire/core-transaction.c | 18 +++++++++++++
 include/linux/firewire.h            |  3 +++
 4 files changed, 64 insertions(+), 2 deletions(-)

-- 
2.32.0

[PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[Takashi Sakamoto]

Hi,

In 1394 OHCI specification, each descriptor of IR/IT/AR/AT DMA context
has timeStamp field. The value of timeStamp field express the time in
which the controller accept packet. The resolution of value is isochronous
cycle count (8,000 Hz) with second up to 7.

I have a plan to use the value of timeStamp field for ALSA firewire stack
so that userspace ALSA PCM/Rawmidi applications can get converted timestamp
(ktime) for PCM frame/MIDI message. The timestamp can ideally express
finer granularity than the time to invoke IRQ handler (and co).

Current implementation of Linux FireWire subsystem delivers the value of
timeStamp field to unit driver for IR/IT/AT DMA context, but not for AR
DMA context. Additionally, the way to refer to Isochronous Cycle Timer
Register in MMIO region of 1394 OHCI controller is transaction to local
node. It includes overhead of transaction and it's preferable to add
less-overhead way available in any type of IRQ context.

This patchset adds two functions exposed in kernel space:

 * fw_card_read_cycle_time()
    * allow unit driver to access to CYCLE_TIME register in MMIO region
      without initiate transaction
 * fw_request_get_timestamp()
    * allow unit driver to get timestamp of request packet inner request
      handler

I note that Hector Martin found kernel null pointer dereference during
process to remove PCI card and has posted a patch:

 * https://lore.kernel.org/lkml/20211027113130.8802-1-marcan@marcan.st/

His patch is included in the series with my comment for relevant commit
20802224298c ("firewire: core: add forgotten dummy driver methods, remove
unused ones"). The patch is required since unit driver can refer to dummy
driver between removal callback of PCI subsystem and removal callback of
FireWire subsystem.

Hector Martin (1):
  firewire: Add dummy read_csr/write_csr functions

Takashi Sakamoto (2):
  firewire: add kernel API to access CYCLE_TIME register
  firewire: add kernel API to access packet structure in request
    structure for AR context

 drivers/firewire/core-card.c        | 39 +++++++++++++++++++++++++++++
 drivers/firewire/core-cdev.c        |  6 +++--
 drivers/firewire/core-transaction.c | 18 +++++++++++++
 include/linux/firewire.h            |  3 +++
 4 files changed, 64 insertions(+), 2 deletions(-)

-- 
2.32.0

[PATCH 1/3] firewire: Add dummy read_csr/write_csr functions

[Takashi Sakamoto]

From: Hector Martin <marcan@marcan.st>

(Hector Martin wrote)
This fixes segfaults when a card gets yanked off of the PCIe bus while
busy, e.g. with a userspace app trying to get the cycle time:

[8638860.994310] Call Trace:
[8638860.994313]  ioctl_get_cycle_timer2+0x4f/0xd0 [firewire_core]
[8638860.994323]  fw_device_op_ioctl+0xae/0x150 [firewire_core]
[8638860.994328]  __x64_sys_ioctl+0x7d/0xb0
[8638860.994332]  do_syscall_64+0x45/0x80
[8638860.994337]  entry_SYSCALL_64_after_hwframe+0x44/0xae

(Takashi Sakamoto wrote)
As long as reading commit 20802224298c ("firewire: core: add forgotten
dummy driver methods, remove unused ones"), three functions are not
implemeted in dummy driver for reason; .read_csr, .write_csr, and
.set_config_rom.

In core of Linux FireWire subsystem, the callback of .set_config_rom is
under acquisition of mutual exclusive for local list of card. The
acquision is also done in process for removal of card, therefore it's
safe for missing implementation of .set_config_rom.

On the other hand, no lock primitive accompanies any call of .read_csr and
.write_csr. For userspace client, check of node shutdown is done in the
beginning of dispatch of ioctl request, while node shifts to shutdown
state in workqueue context enough after card shifts to dummy driver. It's
probable that these two functions are called for the dummy driver by the
code of userspace client. In-kernel unit driver has similar situation.
It's better to add implementation of the two functions for dummy driver.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 54be88167c60..d994da6cf465 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -616,6 +616,15 @@ static struct fw_iso_context *dummy_allocate_iso_context(struct fw_card *card,
 	return ERR_PTR(-ENODEV);
 }
 
+static u32 dummy_read_csr(struct fw_card *card, int csr_offset)
+{
+	return 0;
+}
+
+static void dummy_write_csr(struct fw_card *card, int csr_offset, u32 value)
+{
+}
+
 static int dummy_start_iso(struct fw_iso_context *ctx,
 			   s32 cycle, u32 sync, u32 tags)
 {
@@ -649,6 +658,8 @@ static const struct fw_card_driver dummy_driver_template = {
 	.send_response		= dummy_send_response,
 	.cancel_packet		= dummy_cancel_packet,
 	.enable_phys_dma	= dummy_enable_phys_dma,
+	.read_csr		= dummy_read_csr,
+	.write_csr		= dummy_write_csr,
 	.allocate_iso_context	= dummy_allocate_iso_context,
 	.start_iso		= dummy_start_iso,
 	.set_iso_channels	= dummy_set_iso_channels,
-- 
2.32.0

[PATCH 1/3] firewire: Add dummy read_csr/write_csr functions

[Takashi Sakamoto]

From: Hector Martin <marcan@marcan.st>

(Hector Martin wrote)
This fixes segfaults when a card gets yanked off of the PCIe bus while
busy, e.g. with a userspace app trying to get the cycle time:

[8638860.994310] Call Trace:
[8638860.994313]  ioctl_get_cycle_timer2+0x4f/0xd0 [firewire_core]
[8638860.994323]  fw_device_op_ioctl+0xae/0x150 [firewire_core]
[8638860.994328]  __x64_sys_ioctl+0x7d/0xb0
[8638860.994332]  do_syscall_64+0x45/0x80
[8638860.994337]  entry_SYSCALL_64_after_hwframe+0x44/0xae

(Takashi Sakamoto wrote)
As long as reading commit 20802224298c ("firewire: core: add forgotten
dummy driver methods, remove unused ones"), three functions are not
implemeted in dummy driver for reason; .read_csr, .write_csr, and
.set_config_rom.

In core of Linux FireWire subsystem, the callback of .set_config_rom is
under acquisition of mutual exclusive for local list of card. The
acquision is also done in process for removal of card, therefore it's
safe for missing implementation of .set_config_rom.

On the other hand, no lock primitive accompanies any call of .read_csr and
.write_csr. For userspace client, check of node shutdown is done in the
beginning of dispatch of ioctl request, while node shifts to shutdown
state in workqueue context enough after card shifts to dummy driver. It's
probable that these two functions are called for the dummy driver by the
code of userspace client. In-kernel unit driver has similar situation.
It's better to add implementation of the two functions for dummy driver.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 54be88167c60..d994da6cf465 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -616,6 +616,15 @@ static struct fw_iso_context *dummy_allocate_iso_context(struct fw_card *card,
 	return ERR_PTR(-ENODEV);
 }
 
+static u32 dummy_read_csr(struct fw_card *card, int csr_offset)
+{
+	return 0;
+}
+
+static void dummy_write_csr(struct fw_card *card, int csr_offset, u32 value)
+{
+}
+
 static int dummy_start_iso(struct fw_iso_context *ctx,
 			   s32 cycle, u32 sync, u32 tags)
 {
@@ -649,6 +658,8 @@ static const struct fw_card_driver dummy_driver_template = {
 	.send_response		= dummy_send_response,
 	.cancel_packet		= dummy_cancel_packet,
 	.enable_phys_dma	= dummy_enable_phys_dma,
+	.read_csr		= dummy_read_csr,
+	.write_csr		= dummy_write_csr,
 	.allocate_iso_context	= dummy_allocate_iso_context,
 	.start_iso		= dummy_start_iso,
 	.set_iso_channels	= dummy_set_iso_channels,
-- 
2.32.0

[PATCH 2/3] firewire: add kernel API to access CYCLE_TIME register

[Takashi Sakamoto]

1394 OHCI specification defined Isochronous Cycle Timer Register to get
value of CYCLE_TIME register defined by IEEE 1394 for CSR architecture
defined by ISO/IEC 13213. Unit driver can calculate packet time by
compute with the value of CYCLE_TIME and timeStamp field in descriptor
of each isochronous and asynchronous context. The resolution of CYCLE_TIME
is 49.576 MHz, while the one of timeStamp is 8,000 Hz.

Current implementation of Linux FireWire subsystem allows the driver to
get the value of CYCLE_TIMER CSR register by transaction service. The
transaction service has overhead in regard of access to MMIO register.

This commit adds kernel API for unit driver to access the register
directly.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c | 28 ++++++++++++++++++++++++++++
 drivers/firewire/core-cdev.c |  6 ++++--
 include/linux/firewire.h     |  2 ++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index d994da6cf465..cd09de61bc4f 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -702,3 +702,31 @@ void fw_core_remove_card(struct fw_card *card)
 	WARN_ON(!list_empty(&card->transaction_list));
 }
 EXPORT_SYMBOL(fw_core_remove_card);
+
+/**
+ * fw_card_read_cycle_time: read from Isochronous Cycle Timer Register of 1394 OHCI in MMIO region
+ *			    for controller card.
+ * @card: The instance of card for 1394 OHCI controller.
+ * @cycle_time: The mutual reference to value of cycle time for the read operation.
+ *
+ * Read value from Isochronous Cycle Timer Register of 1394 OHCI in MMIO region for the given
+ * controller card. This function accesses the region without any lock primitives or IRQ mask.
+ * When returning successfully, the content of @value argument has value aligned to host endianness,
+ * formetted by CYCLE_TIME CSR Register of IEEE 1394 std.
+ *
+ * Context: Any context.
+ * Return:
+ * * 0 - Read successfully.
+ * * -ENODEV - The controller is unavailable due to being removed or unbound.
+ */
+int fw_card_read_cycle_time(struct fw_card *card, u32 *cycle_time)
+{
+	if (card->driver->read_csr == dummy_read_csr)
+		return -ENODEV;
+
+	// It's possible to switch to dummy driver between the above and the below. This is the best
+	// effort to return -ENODEV.
+	*cycle_time = card->driver->read_csr(card, CSR_CYCLE_TIME);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fw_card_read_cycle_time);
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 9f89c17730b1..8e9670036e5c 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1216,7 +1216,9 @@ static int ioctl_get_cycle_timer2(struct client *client, union ioctl_arg *arg)
 
 	local_irq_disable();
 
-	cycle_time = card->driver->read_csr(card, CSR_CYCLE_TIME);
+	ret = fw_card_read_cycle_time(card, &cycle_time);
+	if (ret < 0)
+		goto end;
 
 	switch (a->clk_id) {
 	case CLOCK_REALTIME:      ktime_get_real_ts64(&ts);	break;
@@ -1225,7 +1227,7 @@ static int ioctl_get_cycle_timer2(struct client *client, union ioctl_arg *arg)
 	default:
 		ret = -EINVAL;
 	}
-
+end:
 	local_irq_enable();
 
 	a->tv_sec      = ts.tv_sec;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 07967a450eaa..2f467c52bdec 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -150,6 +150,8 @@ static inline void fw_card_put(struct fw_card *card)
 	kref_put(&card->kref, fw_card_release);
 }
 
+int fw_card_read_cycle_time(struct fw_card *card, u32 *cycle_time);
+
 struct fw_attribute_group {
 	struct attribute_group *groups[2];
 	struct attribute_group group;
-- 
2.32.0

[PATCH 2/3] firewire: add kernel API to access CYCLE_TIME register

[Takashi Sakamoto]

1394 OHCI specification defined Isochronous Cycle Timer Register to get
value of CYCLE_TIME register defined by IEEE 1394 for CSR architecture
defined by ISO/IEC 13213. Unit driver can calculate packet time by
compute with the value of CYCLE_TIME and timeStamp field in descriptor
of each isochronous and asynchronous context. The resolution of CYCLE_TIME
is 49.576 MHz, while the one of timeStamp is 8,000 Hz.

Current implementation of Linux FireWire subsystem allows the driver to
get the value of CYCLE_TIMER CSR register by transaction service. The
transaction service has overhead in regard of access to MMIO register.

This commit adds kernel API for unit driver to access the register
directly.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c | 28 ++++++++++++++++++++++++++++
 drivers/firewire/core-cdev.c |  6 ++++--
 include/linux/firewire.h     |  2 ++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index d994da6cf465..cd09de61bc4f 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -702,3 +702,31 @@ void fw_core_remove_card(struct fw_card *card)
 	WARN_ON(!list_empty(&card->transaction_list));
 }
 EXPORT_SYMBOL(fw_core_remove_card);
+
+/**
+ * fw_card_read_cycle_time: read from Isochronous Cycle Timer Register of 1394 OHCI in MMIO region
+ *			    for controller card.
+ * @card: The instance of card for 1394 OHCI controller.
+ * @cycle_time: The mutual reference to value of cycle time for the read operation.
+ *
+ * Read value from Isochronous Cycle Timer Register of 1394 OHCI in MMIO region for the given
+ * controller card. This function accesses the region without any lock primitives or IRQ mask.
+ * When returning successfully, the content of @value argument has value aligned to host endianness,
+ * formetted by CYCLE_TIME CSR Register of IEEE 1394 std.
+ *
+ * Context: Any context.
+ * Return:
+ * * 0 - Read successfully.
+ * * -ENODEV - The controller is unavailable due to being removed or unbound.
+ */
+int fw_card_read_cycle_time(struct fw_card *card, u32 *cycle_time)
+{
+	if (card->driver->read_csr == dummy_read_csr)
+		return -ENODEV;
+
+	// It's possible to switch to dummy driver between the above and the below. This is the best
+	// effort to return -ENODEV.
+	*cycle_time = card->driver->read_csr(card, CSR_CYCLE_TIME);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fw_card_read_cycle_time);
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 9f89c17730b1..8e9670036e5c 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1216,7 +1216,9 @@ static int ioctl_get_cycle_timer2(struct client *client, union ioctl_arg *arg)
 
 	local_irq_disable();
 
-	cycle_time = card->driver->read_csr(card, CSR_CYCLE_TIME);
+	ret = fw_card_read_cycle_time(card, &cycle_time);
+	if (ret < 0)
+		goto end;
 
 	switch (a->clk_id) {
 	case CLOCK_REALTIME:      ktime_get_real_ts64(&ts);	break;
@@ -1225,7 +1227,7 @@ static int ioctl_get_cycle_timer2(struct client *client, union ioctl_arg *arg)
 	default:
 		ret = -EINVAL;
 	}
-
+end:
 	local_irq_enable();
 
 	a->tv_sec      = ts.tv_sec;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 07967a450eaa..2f467c52bdec 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -150,6 +150,8 @@ static inline void fw_card_put(struct fw_card *card)
 	kref_put(&card->kref, fw_card_release);
 }
 
+int fw_card_read_cycle_time(struct fw_card *card, u32 *cycle_time);
+
 struct fw_attribute_group {
 	struct attribute_group *groups[2];
 	struct attribute_group group;
-- 
2.32.0

[PATCH 3/3] firewire: add kernel API to access packet structure in request structure for AR context

[Takashi Sakamoto]

In 1394 OHCI specification, descriptor of Asynchronous Receive DMA context
has timeStamp field in its trailer quadlet. The field is written by
the host controller for the time to receive asynchronous request
subaction in isochronous cycle time.

In Linux FireWire subsystem, the value of field is stored to fw_packet
structure and copied to fw_request structure as the part. The fw_request
structure is hidden from unit driver and passed as opaque pointer when
calling registered handler. It's inconvenient to the unit driver which
needs timestamp of packet.

This commit adds kernel API to pick up timestamp from opaque pointer to
fw_request structure.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 18 ++++++++++++++++++
 include/linux/firewire.h            |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index ac487c96bb71..e12a0a4c33f7 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -619,6 +619,7 @@ struct fw_request {
 	struct fw_packet response;
 	u32 request_header[4];
 	int ack;
+	u32 timestamp;
 	u32 length;
 	u32 data[];
 };
@@ -788,6 +789,7 @@ static struct fw_request *allocate_request(struct fw_card *card,
 	request->response.ack = 0;
 	request->response.callback = free_response_callback;
 	request->ack = p->ack;
+	request->timestamp = p->timestamp;
 	request->length = length;
 	if (data)
 		memcpy(request->data, data, length);
@@ -832,6 +834,22 @@ int fw_get_request_speed(struct fw_request *request)
 }
 EXPORT_SYMBOL(fw_get_request_speed);
 
+/**
+ * fw_request_get_timestamp: Get timestamp of the request.
+ * @request: The opaque pointer to request structure.
+ *
+ * Get timestamp when 1394 OHCI controller receives the asynchronous request subaction. The
+ * timestamp consists of the low order 3 bits of second field and the full 13 bits of count
+ * field of isochronous cycle time register.
+ *
+ * Returns: timestamp of the request.
+ */
+u32 fw_request_get_timestamp(const struct fw_request *request)
+{
+	return request->timestamp;
+}
+EXPORT_SYMBOL_GPL(fw_request_get_timestamp);
+
 static void handle_exclusive_region_request(struct fw_card *card,
 					    struct fw_packet *p,
 					    struct fw_request *request,
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 2f467c52bdec..980019053e54 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -354,6 +354,7 @@ void fw_core_remove_address_handler(struct fw_address_handler *handler);
 void fw_send_response(struct fw_card *card,
 		      struct fw_request *request, int rcode);
 int fw_get_request_speed(struct fw_request *request);
+u32 fw_request_get_timestamp(const struct fw_request *request);
 void fw_send_request(struct fw_card *card, struct fw_transaction *t,
 		     int tcode, int destination_id, int generation, int speed,
 		     unsigned long long offset, void *payload, size_t length,
-- 
2.32.0

[PATCH 3/3] firewire: add kernel API to access packet structure in request structure for AR context

[Takashi Sakamoto]

In 1394 OHCI specification, descriptor of Asynchronous Receive DMA context
has timeStamp field in its trailer quadlet. The field is written by
the host controller for the time to receive asynchronous request
subaction in isochronous cycle time.

In Linux FireWire subsystem, the value of field is stored to fw_packet
structure and copied to fw_request structure as the part. The fw_request
structure is hidden from unit driver and passed as opaque pointer when
calling registered handler. It's inconvenient to the unit driver which
needs timestamp of packet.

This commit adds kernel API to pick up timestamp from opaque pointer to
fw_request structure.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 18 ++++++++++++++++++
 include/linux/firewire.h            |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index ac487c96bb71..e12a0a4c33f7 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -619,6 +619,7 @@ struct fw_request {
 	struct fw_packet response;
 	u32 request_header[4];
 	int ack;
+	u32 timestamp;
 	u32 length;
 	u32 data[];
 };
@@ -788,6 +789,7 @@ static struct fw_request *allocate_request(struct fw_card *card,
 	request->response.ack = 0;
 	request->response.callback = free_response_callback;
 	request->ack = p->ack;
+	request->timestamp = p->timestamp;
 	request->length = length;
 	if (data)
 		memcpy(request->data, data, length);
@@ -832,6 +834,22 @@ int fw_get_request_speed(struct fw_request *request)
 }
 EXPORT_SYMBOL(fw_get_request_speed);
 
+/**
+ * fw_request_get_timestamp: Get timestamp of the request.
+ * @request: The opaque pointer to request structure.
+ *
+ * Get timestamp when 1394 OHCI controller receives the asynchronous request subaction. The
+ * timestamp consists of the low order 3 bits of second field and the full 13 bits of count
+ * field of isochronous cycle time register.
+ *
+ * Returns: timestamp of the request.
+ */
+u32 fw_request_get_timestamp(const struct fw_request *request)
+{
+	return request->timestamp;
+}
+EXPORT_SYMBOL_GPL(fw_request_get_timestamp);
+
 static void handle_exclusive_region_request(struct fw_card *card,
 					    struct fw_packet *p,
 					    struct fw_request *request,
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 2f467c52bdec..980019053e54 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -354,6 +354,7 @@ void fw_core_remove_address_handler(struct fw_address_handler *handler);
 void fw_send_response(struct fw_card *card,
 		      struct fw_request *request, int rcode);
 int fw_get_request_speed(struct fw_request *request);
+u32 fw_request_get_timestamp(const struct fw_request *request);
 void fw_send_request(struct fw_card *card, struct fw_transaction *t,
 		     int tcode, int destination_id, int generation, int speed,
 		     unsigned long long offset, void *payload, size_t length,
-- 
2.32.0

Re: [PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[Takashi Sakamoto]

Hi Stefan,

Thank you for your long effort to maintain Linux FireWire subsystem. I'd
like to use the timestamp function for my integration in ALSA firewire
stack planned at next version of Linux kernel. I'm glad if getting to
your help for upstreaming.

On Thu, Dec 02, 2021 at 08:34:54PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> In 1394 OHCI specification, each descriptor of IR/IT/AR/AT DMA context
> has timeStamp field. The value of timeStamp field express the time in
> which the controller accept packet. The resolution of value is isochronous
> cycle count (8,000 Hz) with second up to 7.
> 
> I have a plan to use the value of timeStamp field for ALSA firewire stack
> so that userspace ALSA PCM/Rawmidi applications can get converted timestamp
> (ktime) for PCM frame/MIDI message. The timestamp can ideally express
> finer granularity than the time to invoke IRQ handler (and co).
> 
> Current implementation of Linux FireWire subsystem delivers the value of
> timeStamp field to unit driver for IR/IT/AT DMA context, but not for AR
> DMA context. Additionally, the way to refer to Isochronous Cycle Timer
> Register in MMIO region of 1394 OHCI controller is transaction to local
> node. It includes overhead of transaction and it's preferable to add
> less-overhead way available in any type of IRQ context.
> 
> This patchset adds two functions exposed in kernel space:
> 
>  * fw_card_read_cycle_time()
>     * allow unit driver to access to CYCLE_TIME register in MMIO region
>       without initiate transaction
>  * fw_request_get_timestamp()
>     * allow unit driver to get timestamp of request packet inner request
>       handler
> 
> I note that Hector Martin found kernel null pointer dereference during
> process to remove PCI card and has posted a patch:
> 
>  * https://lore.kernel.org/lkml/20211027113130.8802-1-marcan@marcan.st/
> 
> His patch is included in the series with my comment for relevant commit
> 20802224298c ("firewire: core: add forgotten dummy driver methods, remove
> unused ones"). The patch is required since unit driver can refer to dummy
> driver between removal callback of PCI subsystem and removal callback of
> FireWire subsystem.
> 
> Hector Martin (1):
>   firewire: Add dummy read_csr/write_csr functions
> 
> Takashi Sakamoto (2):
>   firewire: add kernel API to access CYCLE_TIME register
>   firewire: add kernel API to access packet structure in request
>     structure for AR context
> 
>  drivers/firewire/core-card.c        | 39 +++++++++++++++++++++++++++++
>  drivers/firewire/core-cdev.c        |  6 +++--
>  drivers/firewire/core-transaction.c | 18 +++++++++++++
>  include/linux/firewire.h            |  3 +++
>  4 files changed, 64 insertions(+), 2 deletions(-)
> 
> -- 
> 2.32.0


Sincerely yours

Takashi Sakamoto

Re: [PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[Takashi Sakamoto]

Hi Stefan,

Wishing you a happy new year.

We are in the last week for release of v5.16 kernel, and soon merge
window for v5.17 kernel will be opened if thing goes well. I wish any
action for the review process to merge these patches into upstream.


Thanks

Takashi Sakamoto

On Tue, Dec 21, 2021 at 07:54:42PM +0900, Takashi Sakamoto wrote:
> Hi Stefan,
> 
> Thank you for your long effort to maintain Linux FireWire subsystem. I'd
> like to use the timestamp function for my integration in ALSA firewire
> stack planned at next version of Linux kernel. I'm glad if getting to
> your help for upstreaming.
> 
> On Thu, Dec 02, 2021 at 08:34:54PM +0900, Takashi Sakamoto wrote:
> > Hi,
> > 
> > In 1394 OHCI specification, each descriptor of IR/IT/AR/AT DMA context
> > has timeStamp field. The value of timeStamp field express the time in
> > which the controller accept packet. The resolution of value is isochronous
> > cycle count (8,000 Hz) with second up to 7.
> > 
> > I have a plan to use the value of timeStamp field for ALSA firewire stack
> > so that userspace ALSA PCM/Rawmidi applications can get converted timestamp
> > (ktime) for PCM frame/MIDI message. The timestamp can ideally express
> > finer granularity than the time to invoke IRQ handler (and co).
> > 
> > Current implementation of Linux FireWire subsystem delivers the value of
> > timeStamp field to unit driver for IR/IT/AT DMA context, but not for AR
> > DMA context. Additionally, the way to refer to Isochronous Cycle Timer
> > Register in MMIO region of 1394 OHCI controller is transaction to local
> > node. It includes overhead of transaction and it's preferable to add
> > less-overhead way available in any type of IRQ context.
> > 
> > This patchset adds two functions exposed in kernel space:
> > 
> >  * fw_card_read_cycle_time()
> >     * allow unit driver to access to CYCLE_TIME register in MMIO region
> >       without initiate transaction
> >  * fw_request_get_timestamp()
> >     * allow unit driver to get timestamp of request packet inner request
> >       handler
> > 
> > I note that Hector Martin found kernel null pointer dereference during
> > process to remove PCI card and has posted a patch:
> > 
> >  * https://lore.kernel.org/lkml/20211027113130.8802-1-marcan@marcan.st/
> > 
> > His patch is included in the series with my comment for relevant commit
> > 20802224298c ("firewire: core: add forgotten dummy driver methods, remove
> > unused ones"). The patch is required since unit driver can refer to dummy
> > driver between removal callback of PCI subsystem and removal callback of
> > FireWire subsystem.
> > 
> > Hector Martin (1):
> >   firewire: Add dummy read_csr/write_csr functions
> > 
> > Takashi Sakamoto (2):
> >   firewire: add kernel API to access CYCLE_TIME register
> >   firewire: add kernel API to access packet structure in request
> >     structure for AR context
> > 
> >  drivers/firewire/core-card.c        | 39 +++++++++++++++++++++++++++++
> >  drivers/firewire/core-cdev.c        |  6 +++--
> >  drivers/firewire/core-transaction.c | 18 +++++++++++++
> >  include/linux/firewire.h            |  3 +++
> >  4 files changed, 64 insertions(+), 2 deletions(-)
> > 
> > -- 
> > 2.32.0
> 
> 
> Sincerely yours
> 
> Takashi Sakamoto

Re: [PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[Takashi Sakamoto]

Hi Stefan,

I'm sorry to post messages several times for the patchset if you are
still busy. But I'm still waiting for any reaction.

I note that Linus have announced merge window for v5.17 kernel.
 * https://lore.kernel.org/lkml/CAHk-=wgUkBrUVhjixy4wvrUhPbW-DTgtQubJWVOoLW=O0wRKMA@mail.gmail.com/T/#u

I'm glad if seeing your action for pull request as a response to the
window.


Kind Regards

Takashi Sakamoto

On Fri, Jan 07, 2022 at 08:01:18PM +0900, Takashi Sakamoto wrote:
> Hi Stefan,
> 
> Wishing you a happy new year.
> 
> We are in the last week for release of v5.16 kernel, and soon merge
> window for v5.17 kernel will be opened if thing goes well. I wish any
> action for the review process to merge these patches into upstream.
> 
> 
> Thanks
> 
> Takashi Sakamoto
> 
> On Tue, Dec 21, 2021 at 07:54:42PM +0900, Takashi Sakamoto wrote:
> > Hi Stefan,
> > 
> > Thank you for your long effort to maintain Linux FireWire subsystem. I'd
> > like to use the timestamp function for my integration in ALSA firewire
> > stack planned at next version of Linux kernel. I'm glad if getting to
> > your help for upstreaming.
> > 
> > On Thu, Dec 02, 2021 at 08:34:54PM +0900, Takashi Sakamoto wrote:
> > > Hi,
> > > 
> > > In 1394 OHCI specification, each descriptor of IR/IT/AR/AT DMA context
> > > has timeStamp field. The value of timeStamp field express the time in
> > > which the controller accept packet. The resolution of value is isochronous
> > > cycle count (8,000 Hz) with second up to 7.
> > > 
> > > I have a plan to use the value of timeStamp field for ALSA firewire stack
> > > so that userspace ALSA PCM/Rawmidi applications can get converted timestamp
> > > (ktime) for PCM frame/MIDI message. The timestamp can ideally express
> > > finer granularity than the time to invoke IRQ handler (and co).
> > > 
> > > Current implementation of Linux FireWire subsystem delivers the value of
> > > timeStamp field to unit driver for IR/IT/AT DMA context, but not for AR
> > > DMA context. Additionally, the way to refer to Isochronous Cycle Timer
> > > Register in MMIO region of 1394 OHCI controller is transaction to local
> > > node. It includes overhead of transaction and it's preferable to add
> > > less-overhead way available in any type of IRQ context.
> > > 
> > > This patchset adds two functions exposed in kernel space:
> > > 
> > >  * fw_card_read_cycle_time()
> > >     * allow unit driver to access to CYCLE_TIME register in MMIO region
> > >       without initiate transaction
> > >  * fw_request_get_timestamp()
> > >     * allow unit driver to get timestamp of request packet inner request
> > >       handler
> > > 
> > > I note that Hector Martin found kernel null pointer dereference during
> > > process to remove PCI card and has posted a patch:
> > > 
> > >  * https://lore.kernel.org/lkml/20211027113130.8802-1-marcan@marcan.st/
> > > 
> > > His patch is included in the series with my comment for relevant commit
> > > 20802224298c ("firewire: core: add forgotten dummy driver methods, remove
> > > unused ones"). The patch is required since unit driver can refer to dummy
> > > driver between removal callback of PCI subsystem and removal callback of
> > > FireWire subsystem.
> > > 
> > > Hector Martin (1):
> > >   firewire: Add dummy read_csr/write_csr functions
> > > 
> > > Takashi Sakamoto (2):
> > >   firewire: add kernel API to access CYCLE_TIME register
> > >   firewire: add kernel API to access packet structure in request
> > >     structure for AR context
> > > 
> > >  drivers/firewire/core-card.c        | 39 +++++++++++++++++++++++++++++
> > >  drivers/firewire/core-cdev.c        |  6 +++--
> > >  drivers/firewire/core-transaction.c | 18 +++++++++++++
> > >  include/linux/firewire.h            |  3 +++
> > >  4 files changed, 64 insertions(+), 2 deletions(-)
> > > 
> > > -- 
> > > 2.32.0
> > 
> > 
> > Sincerely yours
> > 
> > Takashi Sakamoto

Re: [PATCH 0/3] firewire: assist unit driver to compute packet timestamp

[Hector Martin]

On 2022/01/12 12:51, Takashi Sakamoto wrote:
> Hi Stefan,
> 
> I'm sorry to post messages several times for the patchset if you are
> still busy. But I'm still waiting for any reaction.
> 
> I note that Linus have announced merge window for v5.17 kernel.
>  * https://lore.kernel.org/lkml/CAHk-=wgUkBrUVhjixy4wvrUhPbW-DTgtQubJWVOoLW=O0wRKMA@mail.gmail.com/T/#u
> 
> I'm glad if seeing your action for pull request as a response to the
> window.

Hi Tahashi,

Just FYI, I think a lot of maintainers have been off or doing less work
over December/the holidays; I also have some things that didn't make it
into subsystem trees for 5.17. So I'm guessing this patchset will have
to wait for 5.18. AIUI most maintainers don't merge things into
subsystem trees after the upstream merge window opens.

I've also been meaning to test your Firewire improvements again (and
also with Pipewire), but I've been pretty busy lately... hopefully I'll
get a chance soon. When I tried the first round of improvements that got
merged (the sequence replay stuff) I noticed it fixed the glitchy audio
problem, but the minimum stable period size with JACK+ALSA was still
higher than with JACK+FFADO, and Pipewire also had even higher
latencies. So I'm back using FFADO but I hope I can switch to ALSA soon :)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub