[PATCH v3 0/3] firewire: assist unit driver to compute packet time stamp
Hi,
Current implementation of Linux FireWire subsystem doesn't allow unit driver to operate content of packet in IR context according to time stamp. Additionally it doesn't allow unit driver to read current value of CYCLE_TIME register in OHCI 1394 controller. It brings disadvantages to drivers in Linux sound subsystem in regards of handling time for sampled data such as PCM frames and MIDI messages.
This rerolled patchset is first step to improve the situation.
Changes in v3: * Rebase v2 patchset to v5.18-rc1 Changes in v2: * Rebase v1 patchset to v5.16 release * https://lore.kernel.org/lkml/20220212022131.199855-1-o-takashi@sakamocchi.jp... V1: * https://lore.kernel.org/lkml/20211202113457.24011-1-o-takashi@sakamocchi.jp/
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(-)
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,
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;
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,
On Tue, 05 Apr 2022 09:22:18 +0200, Takashi Sakamoto wrote:
Hi,
Current implementation of Linux FireWire subsystem doesn't allow unit driver to operate content of packet in IR context according to time stamp. Additionally it doesn't allow unit driver to read current value of CYCLE_TIME register in OHCI 1394 controller. It brings disadvantages to drivers in Linux sound subsystem in regards of handling time for sampled data such as PCM frames and MIDI messages.
This rerolled patchset is first step to improve the situation.
Changes in v3:
- Rebase v2 patchset to v5.18-rc1
Changes in v2:
- Rebase v1 patchset to v5.16 release
- https://lore.kernel.org/lkml/20220212022131.199855-1-o-takashi@sakamocchi.jp...
V1:
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
Thanks, applied all three patches now to for-next branch.
Takashi
On Tue, Apr 05, 2022 at 06:23:35PM +0200, Takashi Iwai wrote:
On Tue, 05 Apr 2022 09:22:18 +0200, Takashi Sakamoto wrote:
Hi,
Current implementation of Linux FireWire subsystem doesn't allow unit driver to operate content of packet in IR context according to time stamp. Additionally it doesn't allow unit driver to read current value of CYCLE_TIME register in OHCI 1394 controller. It brings disadvantages to drivers in Linux sound subsystem in regards of handling time for sampled data such as PCM frames and MIDI messages.
This rerolled patchset is first step to improve the situation.
Changes in v3:
- Rebase v2 patchset to v5.18-rc1
Changes in v2:
- Rebase v1 patchset to v5.16 release
- https://lore.kernel.org/lkml/20220212022131.199855-1-o-takashi@sakamocchi.jp...
V1:
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
Thanks, applied all three patches now to for-next branch.
Although thanks for your applying them into your tree, I apologize to trouble you if you overlook that the included changes is just for Linux FireWire subsystem. It's my fault to send them only to Linux sound subsystem, but the changes are required to my work in sound drivers...
If you are willing to include patches to Linux FireWire subsystem for your pull-request to Linus, I can prepare respined patches for it since I have the list of patches posted to LKML as bug fixes for Linux FireWire subsystem.
I need any help to solve current situation of Linux FireWire subsystem that bug fixes and new changes are hardly merged. Of course, IEEE 1394 bus is already outdated and legacy, but I know that some users still work with it. If your path is available for it, it's the easiest and the most convenient way for upstreaming, I think.
Thanks
Takashi Sakamoto
On Thu, 07 Apr 2022 02:10:21 +0200, Takashi Sakamoto wrote:
On Tue, Apr 05, 2022 at 06:23:35PM +0200, Takashi Iwai wrote:
On Tue, 05 Apr 2022 09:22:18 +0200, Takashi Sakamoto wrote:
Hi,
Current implementation of Linux FireWire subsystem doesn't allow unit driver to operate content of packet in IR context according to time stamp. Additionally it doesn't allow unit driver to read current value of CYCLE_TIME register in OHCI 1394 controller. It brings disadvantages to drivers in Linux sound subsystem in regards of handling time for sampled data such as PCM frames and MIDI messages.
This rerolled patchset is first step to improve the situation.
Changes in v3:
- Rebase v2 patchset to v5.18-rc1
Changes in v2:
- Rebase v1 patchset to v5.16 release
- https://lore.kernel.org/lkml/20220212022131.199855-1-o-takashi@sakamocchi.jp...
V1:
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
Thanks, applied all three patches now to for-next branch.
Although thanks for your applying them into your tree, I apologize to trouble you if you overlook that the included changes is just for Linux FireWire subsystem. It's my fault to send them only to Linux sound subsystem, but the changes are required to my work in sound drivers...
If you are willing to include patches to Linux FireWire subsystem for your pull-request to Linus, I can prepare respined patches for it since I have the list of patches posted to LKML as bug fixes for Linux FireWire subsystem.
I need any help to solve current situation of Linux FireWire subsystem that bug fixes and new changes are hardly merged. Of course, IEEE 1394 bus is already outdated and legacy, but I know that some users still work with it. If your path is available for it, it's the easiest and the most convenient way for upstreaming, I think.
Ah OK, it's fine for me in either way. I can keep up those changes in my tree, or go through others. I leave the decision Firewire subsystem people. Just let me know.
thanks,
Takashi
On Thu, Apr 07, 2022 at 08:02:10AM +0200, Takashi Iwai wrote:
On Thu, 07 Apr 2022 02:10:21 +0200, Takashi Sakamoto wrote:
On Tue, Apr 05, 2022 at 06:23:35PM +0200, Takashi Iwai wrote:
On Tue, 05 Apr 2022 09:22:18 +0200, Takashi Sakamoto wrote:
Hi,
Current implementation of Linux FireWire subsystem doesn't allow unit driver to operate content of packet in IR context according to time stamp. Additionally it doesn't allow unit driver to read current value of CYCLE_TIME register in OHCI 1394 controller. It brings disadvantages to drivers in Linux sound subsystem in regards of handling time for sampled data such as PCM frames and MIDI messages.
This rerolled patchset is first step to improve the situation.
Changes in v3:
- Rebase v2 patchset to v5.18-rc1
Changes in v2:
- Rebase v1 patchset to v5.16 release
- https://lore.kernel.org/lkml/20220212022131.199855-1-o-takashi@sakamocchi.jp...
V1:
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
Thanks, applied all three patches now to for-next branch.
Although thanks for your applying them into your tree, I apologize to trouble you if you overlook that the included changes is just for Linux FireWire subsystem. It's my fault to send them only to Linux sound subsystem, but the changes are required to my work in sound drivers...
If you are willing to include patches to Linux FireWire subsystem for your pull-request to Linus, I can prepare respined patches for it since I have the list of patches posted to LKML as bug fixes for Linux FireWire subsystem.
I need any help to solve current situation of Linux FireWire subsystem that bug fixes and new changes are hardly merged. Of course, IEEE 1394 bus is already outdated and legacy, but I know that some users still work with it. If your path is available for it, it's the easiest and the most convenient way for upstreaming, I think.
Ah OK, it's fine for me in either way. I can keep up those changes in my tree, or go through others. I leave the decision Firewire subsystem people. Just let me know.
That's great. I think we can see few objections.
In this weekend, I'll send two respined patchset. One is for bug fixes for your for-linus and living stables. Another is for your linux-next.
Thanks for your kindness.
Takashi Sakamoto
participants (2)
-
Takashi Iwai
-
Takashi Sakamoto