From dc7c51638f4660cc6c72b5c8151970341b6d9587 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:39 +0900 Subject: [PATCH 01/25] firewire: add KUnit test to check layout of UAPI structures In future commits, some new structure will be added to express new type of event. They are exposed to user space as the part of UAPI. It is likely to get trouble in ioctl compatibility layer for 32 bit binaries in 64 bit host machine since the layout of structure could differ depending on System V ABI for these architectures. Actually the subsystem already got such trouble at v2.6.27. It is preferable to decide the layout of structure carefully so that the layer is free from such trouble. This commit utilizes KUnit framework to check the layout of structure for the purpose. A test is added for the existent issue. Cc: kunit-dev@googlegroups.com Link: https://lore.kernel.org/r/20230529113406.986289-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/.kunitconfig | 4 ++++ drivers/firewire/Kconfig | 16 +++++++++++++++ drivers/firewire/Makefile | 3 +++ drivers/firewire/uapi-test.c | 38 +++++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 drivers/firewire/.kunitconfig create mode 100644 drivers/firewire/uapi-test.c diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig new file mode 100644 index 000000000000..1599e069395f --- /dev/null +++ b/drivers/firewire/.kunitconfig @@ -0,0 +1,4 @@ +CONFIG_KUNIT=y +CONFIG_PCI=y +CONFIG_FIREWIRE=y +CONFIG_FIREWIRE_KUNIT_UAPI_TEST=y diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig index ec00a6f70da8..0a6596b027db 100644 --- a/drivers/firewire/Kconfig +++ b/drivers/firewire/Kconfig @@ -18,6 +18,22 @@ config FIREWIRE To compile this driver as a module, say M here: the module will be called firewire-core. +config FIREWIRE_KUNIT_UAPI_TEST + tristate "KUnit tests for layout of structure in UAPI" if !KUNIT_ALL_TESTS + depends on FIREWIRE && KUNIT + default KUNIT_ALL_TESTS + help + This builds the KUnit tests whether structures exposed to user + space have expected layout. + + KUnit tests run during boot and output the results to the debug + log in TAP format (https://testanything.org/). Only useful for + kernel devs running KUnit test harness and are not for inclusion + into a production build. + + For more information on KUnit and unit tests in general, refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + config FIREWIRE_OHCI tristate "OHCI-1394 controllers" depends on PCI && FIREWIRE && MMU diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile index e58c8c794778..b24b2879ac34 100644 --- a/drivers/firewire/Makefile +++ b/drivers/firewire/Makefile @@ -15,3 +15,6 @@ obj-$(CONFIG_FIREWIRE_SBP2) += firewire-sbp2.o obj-$(CONFIG_FIREWIRE_NET) += firewire-net.o obj-$(CONFIG_FIREWIRE_NOSY) += nosy.o obj-$(CONFIG_PROVIDE_OHCI1394_DMA_INIT) += init_ohci1394_dma.o + +firewire-uapi-test-objs += uapi-test.o +obj-$(CONFIG_FIREWIRE_KUNIT_UAPI_TEST) += firewire-uapi-test.o diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c new file mode 100644 index 000000000000..4dc633b91336 --- /dev/null +++ b/drivers/firewire/uapi-test.c @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// uapi_test.c - An application of Kunit to check layout of structures exposed to user space for +// FireWire subsystem. +// +// Copyright (c) 2023 Takashi Sakamoto + +#include +#include + +// Known issue added at v2.6.27 kernel. +static void structure_layout_event_response(struct kunit *test) +{ +#if defined(CONFIG_X86_32) + // 4 bytes alignment for aggregate type including 8 bytes storage types. + KUNIT_EXPECT_EQ(test, 20, sizeof(struct fw_cdev_event_response)); +#else + // 8 bytes alignment for aggregate type including 8 bytes storage types. + KUNIT_EXPECT_EQ(test, 24, sizeof(struct fw_cdev_event_response)); +#endif + + KUNIT_EXPECT_EQ(test, 0, offsetof(struct fw_cdev_event_response, closure)); + KUNIT_EXPECT_EQ(test, 8, offsetof(struct fw_cdev_event_response, type)); + KUNIT_EXPECT_EQ(test, 12, offsetof(struct fw_cdev_event_response, rcode)); + KUNIT_EXPECT_EQ(test, 16, offsetof(struct fw_cdev_event_response, length)); + KUNIT_EXPECT_EQ(test, 20, offsetof(struct fw_cdev_event_response, data)); +} + +static struct kunit_case structure_layout_test_cases[] = { + KUNIT_CASE(structure_layout_event_response), + {} +}; + +static struct kunit_suite structure_layout_test_suite = { + .name = "firewire-uapi-structure-layout", + .test_cases = structure_layout_test_cases, +}; +kunit_test_suite(structure_layout_test_suite); From 6add87e9764dd308006b078cbdbf36d5a611cc9b Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:39 +0900 Subject: [PATCH 02/25] 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. Link: https://lore.kernel.org/r/20230529113406.986289-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- 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 */ /** From 7c22d4a92bb26f2357b27446138c8be54f88caed Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:39 +0900 Subject: [PATCH 03/25] firewire: cdev: add new event to notify request subaction with time stamp This commit adds new event to notify event of request 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: kunit-dev@googlegroups.com Link: https://lore.kernel.org/r/20230529113406.986289-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/uapi-test.c | 20 +++++++++++ include/uapi/linux/firewire-cdev.h | 53 ++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c index 4dc633b91336..dd95899de316 100644 --- a/drivers/firewire/uapi-test.c +++ b/drivers/firewire/uapi-test.c @@ -26,8 +26,28 @@ static void structure_layout_event_response(struct kunit *test) KUNIT_EXPECT_EQ(test, 20, offsetof(struct fw_cdev_event_response, data)); } +// Added at v6.5. +static void structure_layout_event_request3(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, 56, sizeof(struct fw_cdev_event_request3)); + + KUNIT_EXPECT_EQ(test, 0, offsetof(struct fw_cdev_event_request3, closure)); + KUNIT_EXPECT_EQ(test, 8, offsetof(struct fw_cdev_event_request3, type)); + KUNIT_EXPECT_EQ(test, 12, offsetof(struct fw_cdev_event_request3, tcode)); + KUNIT_EXPECT_EQ(test, 16, offsetof(struct fw_cdev_event_request3, offset)); + KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_request3, source_node_id)); + KUNIT_EXPECT_EQ(test, 28, offsetof(struct fw_cdev_event_request3, destination_node_id)); + KUNIT_EXPECT_EQ(test, 32, offsetof(struct fw_cdev_event_request3, card)); + KUNIT_EXPECT_EQ(test, 36, offsetof(struct fw_cdev_event_request3, generation)); + KUNIT_EXPECT_EQ(test, 40, offsetof(struct fw_cdev_event_request3, handle)); + KUNIT_EXPECT_EQ(test, 44, offsetof(struct fw_cdev_event_request3, length)); + KUNIT_EXPECT_EQ(test, 48, offsetof(struct fw_cdev_event_request3, tstamp)); + KUNIT_EXPECT_EQ(test, 56, offsetof(struct fw_cdev_event_request3, data)); +} + static struct kunit_case structure_layout_test_cases[] = { KUNIT_CASE(structure_layout_event_response), + KUNIT_CASE(structure_layout_event_request3), {} }; diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h index 76441eb551e5..7767cd53a013 100644 --- a/include/uapi/linux/firewire-cdev.h +++ b/include/uapi/linux/firewire-cdev.h @@ -46,6 +46,9 @@ #define FW_CDEV_EVENT_PHY_PACKET_RECEIVED 0x08 #define FW_CDEV_EVENT_ISO_INTERRUPT_MULTICHANNEL 0x09 +/* available since kernel version 6.5 */ +#define FW_CDEV_EVENT_REQUEST3 0x0a + /** * struct fw_cdev_event_common - Common part of all fw_cdev_event_* types * @closure: For arbitrary use by userspace @@ -159,6 +162,38 @@ struct fw_cdev_event_request { * @length: Data length, i.e. the request's payload size in bytes * @data: Incoming data, if any * + * This event is sent instead of &fw_cdev_event_request3 if the kernel or the client implements + * ABI version <= 5. It has the lack of time stamp field comparing to &fw_cdev_event_request3. + */ +struct fw_cdev_event_request2 { + __u64 closure; + __u32 type; + __u32 tcode; + __u64 offset; + __u32 source_node_id; + __u32 destination_node_id; + __u32 card; + __u32 generation; + __u32 handle; + __u32 length; + __u32 data[]; +}; + +/** + * struct fw_cdev_event_request3 - Sent on incoming request to an address region + * @closure: See &fw_cdev_event_common; set by %FW_CDEV_IOC_ALLOCATE ioctl + * @type: See &fw_cdev_event_common; always %FW_CDEV_EVENT_REQUEST2 + * @tcode: Transaction code of the incoming request + * @offset: The offset into the 48-bit per-node address space + * @source_node_id: Sender node ID + * @destination_node_id: Destination node ID + * @card: The index of the card from which the request came + * @generation: Bus generation in which the request is valid + * @handle: Reference to the kernel-side pending request + * @length: Data length, i.e. the request's payload size in bytes + * @tstamp: The time stamp of isochronous cycle at which the request arrived. + * @data: Incoming data, if any + * * This event is sent when the stack receives an incoming request to an address * region registered using the %FW_CDEV_IOC_ALLOCATE ioctl. The request is * guaranteed to be completely contained in the specified region. Userspace is @@ -191,10 +226,14 @@ struct fw_cdev_event_request { * sent. * * If the client subsequently needs to initiate requests to the sender node of - * an &fw_cdev_event_request2, it needs to use a device file with matching + * an &fw_cdev_event_request3, it needs to use a device file with matching * card index, node ID, and generation for outbound requests. + * + * @tstamp is isochronous cycle at which the request arrived. It is 16 bit integer value and the + * higher 3 bits expresses three low order bits of second field in the format of CYCLE_TIME + * register and the rest 13 bits expresses cycle field. */ -struct fw_cdev_event_request2 { +struct fw_cdev_event_request3 { __u64 closure; __u32 type; __u32 tcode; @@ -205,6 +244,12 @@ struct fw_cdev_event_request2 { __u32 generation; __u32 handle; __u32 length; + __u32 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[]; }; @@ -375,6 +420,8 @@ struct fw_cdev_event_phy_packet { * %FW_CDEV_EVENT_PHY_PACKET_SENT or * %FW_CDEV_EVENT_PHY_PACKET_RECEIVED * + * @request3: Valid if @common.type == %FW_CDEV_EVENT_REQUEST3 + * * Convenience union for userspace use. Events could be read(2) into an * appropriately aligned char buffer and then cast to this union for further * processing. Note that for a request, response or iso_interrupt event, @@ -393,6 +440,7 @@ union fw_cdev_event { struct fw_cdev_event_iso_interrupt_mc iso_interrupt_mc; /* added in 2.6.36 */ 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 */ }; /* available since kernel version 2.6.22 */ @@ -458,6 +506,7 @@ union fw_cdev_event { * avoid dropping data * - added %FW_CDEV_IOC_FLUSH_ISO * 6 (6.5) - added some event for subactions of asynchronous transaction with time stamp + * - %FW_CDEV_EVENT_REQUEST3 */ /** From 865efffb2d11402bc6f96c7e390b89384e9d209d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 04/25] firewire: cdev: implement new event to notify request subaction with time stamp In 1394 OHCI, the trailer quadlet of descriptor in Asynchronous Receive (AR) request context has timeStamp field, in which the 1394 OHCI controller record the isochronous cycle when the packet arrived. Current implementation of 1394 OHCI controller driver stores the value of field to internal structure as time stamp, while the implementation of FireWire character device doesn't have a field for the time stamp, thus it is not available in user space. The time stamp is convenient to some kind of application in which data from several sources are compared in isochronous cycle unit. This commit implement the new event, fw_cdev_event_request3, with an additional field, tstamp. Link: https://lore.kernel.org/r/20230529113406.986289-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 88c8b5fac5e5..5a9446d30447 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -178,6 +178,7 @@ struct inbound_transaction_event { union { struct fw_cdev_event_request request; struct fw_cdev_event_request2 request2; + struct fw_cdev_event_request3 with_tstamp; } req; }; @@ -709,7 +710,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request, req->handle = r->resource.handle; req->closure = handler->closure; event_size0 = sizeof(*req); - } else { + } else if (handler->client->version < FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP) { struct fw_cdev_event_request2 *req = &e->req.request2; req->type = FW_CDEV_EVENT_REQUEST2; @@ -723,6 +724,21 @@ static void handle_request(struct fw_card *card, struct fw_request *request, req->handle = r->resource.handle; req->closure = handler->closure; event_size0 = sizeof(*req); + } else { + struct fw_cdev_event_request3 *req = &e->req.with_tstamp; + + req->type = FW_CDEV_EVENT_REQUEST3; + req->tcode = tcode; + req->offset = offset; + req->source_node_id = source; + req->destination_node_id = destination; + req->card = card->index; + req->generation = generation; + req->length = length; + req->handle = r->resource.handle; + req->closure = handler->closure; + req->tstamp = fw_request_get_timestamp(request); + event_size0 = sizeof(*req); } queue_event(handler->client, &e->event, From dcadfd7f7c74ef9ee415e072a19bdf6c085159eb Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 05/25] firewire: core: use union for callback of transaction completion In 1394 OHCI, the OUTPUT_LAST descriptor of Asynchronous Transmit (AT) request context has timeStamp field, in which 1394 OHCI controller record the isochronous cycle when the packet was sent for the request subaction. Additionally, for the case of split transaction in IEEE 1394, Asynchronous Receive (AT) request context is used for response subaction to finish the transaction. The trailer quadlet of descriptor in the context has timeStamp field, in which 1394 OHCI controller records the isochronous cycle when the packet arrived. Current implementation of 1394 OHCI controller driver stores values of both fields to internal structure as time stamp, while Linux FireWire subsystem provides no way to access to it. When using asynchronous transaction service provided by the subsystem, callback function is passed to kernel API. The prototype of callback function has the lack of argument for the values. This commit adds a new callback function for the purpose. It has an additional argument to point to the constant array with two elements. For backward compatibility to kernel space, a new union is also adds to wrap two different prototype of callback function. The fw_transaction structure has the union as a member and a boolean flag to express which function callback is available. The core function is changed to handle the two cases; with or without time stamp. For the error path to process transaction, the isochronous cycle is computed by current value of CYCLE_TIMER register in 1394 OHCI controller. Especially for the case of timeout of split transaction, the expected isochronous cycle is computed. Link: https://lore.kernel.org/r/20230529113406.986289-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 58 +++++++++++++++++++++++------ drivers/firewire/core.h | 7 ++++ drivers/firewire/ohci.c | 17 ++++++++- include/linux/firewire.h | 13 ++++++- 4 files changed, 80 insertions(+), 15 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index a9f70c96323e..a20f97fdd06c 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -70,8 +70,8 @@ static int try_cancel_split_timeout(struct fw_transaction *t) return 1; } -static int close_transaction(struct fw_transaction *transaction, - struct fw_card *card, int rcode) +static int close_transaction(struct fw_transaction *transaction, struct fw_card *card, int rcode, + u32 response_tstamp) { struct fw_transaction *t = NULL, *iter; unsigned long flags; @@ -92,7 +92,12 @@ static int close_transaction(struct fw_transaction *transaction, spin_unlock_irqrestore(&card->lock, flags); if (t) { - t->callback(card, rcode, NULL, 0, t->callback_data); + if (!t->with_tstamp) { + t->callback.without_tstamp(card, rcode, NULL, 0, t->callback_data); + } else { + t->callback.with_tstamp(card, rcode, t->packet.timestamp, response_tstamp, + NULL, 0, t->callback_data); + } return 0; } @@ -107,6 +112,8 @@ static int close_transaction(struct fw_transaction *transaction, int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction) { + u32 tstamp; + /* * Cancel the packet transmission if it's still queued. That * will call the packet transmission callback which cancels @@ -121,7 +128,17 @@ int fw_cancel_transaction(struct fw_card *card, * if the transaction is still pending and remove it in that case. */ - return close_transaction(transaction, card, RCODE_CANCELLED); + if (transaction->packet.ack == 0) { + // The timestamp is reused since it was just read now. + tstamp = transaction->packet.timestamp; + } else { + u32 curr_cycle_time = 0; + + (void)fw_card_read_cycle_time(card, &curr_cycle_time); + tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + } + + return close_transaction(transaction, card, RCODE_CANCELLED, tstamp); } EXPORT_SYMBOL(fw_cancel_transaction); @@ -140,7 +157,12 @@ static void split_transaction_timeout_callback(struct timer_list *timer) card->tlabel_mask &= ~(1ULL << t->tlabel); spin_unlock_irqrestore(&card->lock, flags); - t->callback(card, RCODE_CANCELLED, NULL, 0, t->callback_data); + if (!t->with_tstamp) { + t->callback.without_tstamp(card, RCODE_CANCELLED, NULL, 0, t->callback_data); + } else { + t->callback.with_tstamp(card, RCODE_CANCELLED, t->packet.timestamp, + t->split_timeout_cycle, NULL, 0, t->callback_data); + } } static void start_split_transaction_timeout(struct fw_transaction *t, @@ -162,6 +184,8 @@ static void start_split_transaction_timeout(struct fw_transaction *t, spin_unlock_irqrestore(&card->lock, flags); } +static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_timestamp); + static void transmit_complete_callback(struct fw_packet *packet, struct fw_card *card, int status) { @@ -170,28 +194,32 @@ static void transmit_complete_callback(struct fw_packet *packet, switch (status) { case ACK_COMPLETE: - close_transaction(t, card, RCODE_COMPLETE); + close_transaction(t, card, RCODE_COMPLETE, packet->timestamp); break; case ACK_PENDING: + { + t->split_timeout_cycle = + compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff; start_split_transaction_timeout(t, card); break; + } case ACK_BUSY_X: case ACK_BUSY_A: case ACK_BUSY_B: - close_transaction(t, card, RCODE_BUSY); + close_transaction(t, card, RCODE_BUSY, packet->timestamp); break; case ACK_DATA_ERROR: - close_transaction(t, card, RCODE_DATA_ERROR); + close_transaction(t, card, RCODE_DATA_ERROR, packet->timestamp); break; case ACK_TYPE_ERROR: - close_transaction(t, card, RCODE_TYPE_ERROR); + close_transaction(t, card, RCODE_TYPE_ERROR, packet->timestamp); break; default: /* * In this case the ack is really a juju specific * rcode, so just forward that to the callback. */ - close_transaction(t, card, status); + close_transaction(t, card, status, packet->timestamp); break; } } @@ -363,7 +391,8 @@ void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, t->is_split_transaction = false; timer_setup(&t->split_timeout_timer, split_transaction_timeout_callback, 0); - t->callback = callback; + t->callback.without_tstamp = callback; + t->with_tstamp = false; t->callback_data = callback_data; fw_fill_request(&t->packet, tcode, t->tlabel, @@ -1047,7 +1076,12 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) */ card->driver->cancel_packet(card, &t->packet); - t->callback(card, rcode, data, data_length, t->callback_data); + if (!t->with_tstamp) { + t->callback.without_tstamp(card, rcode, data, data_length, t->callback_data); + } else { + t->callback.with_tstamp(card, rcode, t->packet.timestamp, p->timestamp, data, + data_length, t->callback_data); + } } EXPORT_SYMBOL(fw_core_handle_response); diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index eafa4eaae737..2a05f411328f 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -247,6 +247,13 @@ void fw_fill_response(struct fw_packet *response, u32 *request_header, void fw_request_get(struct fw_request *request); void fw_request_put(struct fw_request *request); +// Convert the value of IEEE 1394 CYCLE_TIME register to the format of timeStamp field in +// descriptors of 1394 OHCI. +static inline u32 cycle_time_to_ohci_tstamp(u32 tstamp) +{ + return (tstamp & 0x0ffff000) >> 12; +} + #define FW_PHY_CONFIG_NO_NODE_ID -1 #define FW_PHY_CONFIG_CURRENT_GAP_COUNT -1 void fw_send_phy_config(struct fw_card *card, diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 17c9d825188b..06386c3b7f03 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1623,6 +1623,8 @@ static void handle_local_request(struct context *ctx, struct fw_packet *packet) } } +static u32 get_cycle_time(struct fw_ohci *ohci); + static void at_context_transmit(struct context *ctx, struct fw_packet *packet) { unsigned long flags; @@ -1633,6 +1635,10 @@ static void at_context_transmit(struct context *ctx, struct fw_packet *packet) if (HEADER_GET_DESTINATION(packet->header[0]) == ctx->ohci->node_id && ctx->ohci->generation == packet->generation) { spin_unlock_irqrestore(&ctx->ohci->lock, flags); + + // Timestamping on behalf of the hardware. + packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ctx->ohci)); + handle_local_request(ctx, packet); return; } @@ -1640,9 +1646,12 @@ static void at_context_transmit(struct context *ctx, struct fw_packet *packet) ret = at_context_queue_packet(ctx, packet); spin_unlock_irqrestore(&ctx->ohci->lock, flags); - if (ret < 0) - packet->callback(packet, &ctx->ohci->card, packet->ack); + if (ret < 0) { + // Timestamping on behalf of the hardware. + packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ctx->ohci)); + packet->callback(packet, &ctx->ohci->card, packet->ack); + } } static void detect_dead_context(struct fw_ohci *ohci, @@ -2557,6 +2566,10 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) log_ar_at_event(ohci, 'T', packet->speed, packet->header, 0x20); driver_data->packet = NULL; packet->ack = RCODE_CANCELLED; + + // Timestamping on behalf of the hardware. + packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci)); + packet->callback(packet, &ohci->card, packet->ack); ret = 0; out: diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 1716c01c4e54..d61693341da1 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -261,6 +261,15 @@ typedef void (*fw_packet_callback_t)(struct fw_packet *packet, typedef void (*fw_transaction_callback_t)(struct fw_card *card, int rcode, void *data, size_t length, void *callback_data); +typedef void (*fw_transaction_callback_with_tstamp_t)(struct fw_card *card, int rcode, + u32 request_tstamp, u32 response_tstamp, void *data, + size_t length, void *callback_data); + +union fw_transaction_callback { + fw_transaction_callback_t without_tstamp; + fw_transaction_callback_with_tstamp_t with_tstamp; +}; + /* * This callback handles an inbound request subaction. It is called in * RCU read-side context, therefore must not sleep. @@ -312,6 +321,7 @@ struct fw_transaction { struct fw_card *card; bool is_split_transaction; struct timer_list split_timeout_timer; + u32 split_timeout_cycle; struct fw_packet packet; @@ -319,7 +329,8 @@ struct fw_transaction { * The data passed to the callback is valid only during the * callback. */ - fw_transaction_callback_t callback; + union fw_transaction_callback callback; + bool with_tstamp; void *callback_data; }; From 39ce342c3a4b763d774c531323d6573af389f332 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 06/25] firewire: core: implement variations to send request and wait for response with time stamp In the previous commit, the core function of Linux FireWire subsystem was changed for two cases to operate asynchronous transaction with or without time stamp. This commit changes kernel API for the two cases. Current kernel API, fw_send_request(), is changed to be static inline function to call __fw_send_request(), which receives two argument for union and flag of callback function. The new kernel API, fw_send_request_with_tstamp() is also added as static inline function, too. When calling, the two arguments are copied to internal structure, then used in softIRQ context. Link: https://lore.kernel.org/r/20230529113406.986289-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-transaction.c | 41 +++++++++++------ include/linux/firewire.h | 69 +++++++++++++++++++++++++++-- 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index a20f97fdd06c..130b95aca629 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -316,7 +316,8 @@ static int allocate_tlabel(struct fw_card *card) } /** - * fw_send_request() - submit a request packet for transmission + * __fw_send_request() - submit a request packet for transmission to generate callback for response + * subaction with or without time stamp. * @card: interface to send the request at * @t: transaction instance to which the request belongs * @tcode: transaction code @@ -326,7 +327,9 @@ static int allocate_tlabel(struct fw_card *card) * @offset: 48bit wide offset into destination's address space * @payload: data payload for the request subaction * @length: length of the payload, in bytes - * @callback: function to be called when the transaction is completed + * @callback: union of two functions whether to receive time stamp or not for response + * subaction. + * @with_tstamp: Whether to receive time stamp or not for response subaction. * @callback_data: data to be passed to the transaction completion callback * * Submit a request packet into the asynchronous request transmission queue. @@ -363,10 +366,10 @@ static int allocate_tlabel(struct fw_card *card) * transaction completion and hence execution of @callback may happen even * before fw_send_request() returns. */ -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, - fw_transaction_callback_t callback, void *callback_data) +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, union fw_transaction_callback callback, + bool with_tstamp, void *callback_data) { unsigned long flags; int tlabel; @@ -381,7 +384,19 @@ void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, tlabel = allocate_tlabel(card); if (tlabel < 0) { spin_unlock_irqrestore(&card->lock, flags); - callback(card, RCODE_SEND_ERROR, NULL, 0, callback_data); + if (!with_tstamp) { + callback.without_tstamp(card, RCODE_SEND_ERROR, NULL, 0, callback_data); + } else { + // Timestamping on behalf of hardware. + u32 curr_cycle_time = 0; + u32 tstamp; + + (void)fw_card_read_cycle_time(card, &curr_cycle_time); + tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time); + + callback.with_tstamp(card, RCODE_SEND_ERROR, tstamp, tstamp, NULL, 0, + callback_data); + } return; } @@ -389,14 +404,12 @@ void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, t->tlabel = tlabel; t->card = card; t->is_split_transaction = false; - timer_setup(&t->split_timeout_timer, - split_transaction_timeout_callback, 0); - t->callback.without_tstamp = callback; - t->with_tstamp = false; + timer_setup(&t->split_timeout_timer, split_transaction_timeout_callback, 0); + t->callback = callback; + t->with_tstamp = with_tstamp; t->callback_data = callback_data; - fw_fill_request(&t->packet, tcode, t->tlabel, - destination_id, card->node_id, generation, + fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, card->node_id, generation, speed, offset, payload, length); t->packet.callback = transmit_complete_callback; @@ -406,7 +419,7 @@ void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode, card->driver->send_request(card, &t->packet); } -EXPORT_SYMBOL(fw_send_request); +EXPORT_SYMBOL_GPL(__fw_send_request); struct transaction_callback_data { struct completion done; diff --git a/include/linux/firewire.h b/include/linux/firewire.h index d61693341da1..a7fd23d0010d 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -356,10 +356,71 @@ 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, - fw_transaction_callback_t callback, void *callback_data); + +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, union fw_transaction_callback callback, + bool with_tstamp, void *callback_data); + +/** + * fw_send_request() - submit a request packet for transmission to generate callback for response + * subaction without time stamp. + * @card: interface to send the request at + * @t: transaction instance to which the request belongs + * @tcode: transaction code + * @destination_id: destination node ID, consisting of bus_ID and phy_ID + * @generation: bus generation in which request and response are valid + * @speed: transmission speed + * @offset: 48bit wide offset into destination's address space + * @payload: data payload for the request subaction + * @length: length of the payload, in bytes + * @callback: function to be called when the transaction is completed + * @callback_data: data to be passed to the transaction completion callback + * + * A variation of __fw_send_request() to generate callback for response subaction without time + * stamp. + */ +static inline 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, + fw_transaction_callback_t callback, void *callback_data) +{ + union fw_transaction_callback cb = { + .without_tstamp = callback, + }; + __fw_send_request(card, t, tcode, destination_id, generation, speed, offset, payload, + length, cb, false, callback_data); +} + +/** + * fw_send_request_with_tstamp() - submit a request packet for transmission to generate callback for + * response with time stamp. + * @card: interface to send the request at + * @t: transaction instance to which the request belongs + * @tcode: transaction code + * @destination_id: destination node ID, consisting of bus_ID and phy_ID + * @generation: bus generation in which request and response are valid + * @speed: transmission speed + * @offset: 48bit wide offset into destination's address space + * @payload: data payload for the request subaction + * @length: length of the payload, in bytes + * @callback: function to be called when the transaction is completed + * @callback_data: data to be passed to the transaction completion callback + * + * A variation of __fw_send_request() to generate callback for response subaction with time stamp. + */ +static inline void fw_send_request_with_tstamp(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, fw_transaction_callback_with_tstamp_t callback, + void *callback_data) +{ + union fw_transaction_callback cb = { + .with_tstamp = callback, + }; + __fw_send_request(card, t, tcode, destination_id, generation, speed, offset, payload, + length, cb, true, callback_data); +} + int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction); int fw_run_transaction(struct fw_card *card, int tcode, int destination_id, From 147e9d3af34a92ff567c58b0e89099d26787faba Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 07/25] firewire: cdev: code refactoring to operate event of response This commit is a preparation to handle time stamp of asynchronous transaction for user space application. Link: https://lore.kernel.org/r/20230529113406.986289-8-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-cdev.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 5a9446d30447..315ebc8c545d 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -170,7 +170,9 @@ struct outbound_transaction_event { struct event event; struct client *client; struct outbound_transaction_resource r; - struct fw_cdev_event_response response; + union { + struct fw_cdev_event_response without_tstamp; + } rsp; }; struct inbound_transaction_event { @@ -540,7 +542,7 @@ static void complete_transaction(struct fw_card *card, int rcode, void *payload, size_t length, void *data) { struct outbound_transaction_event *e = data; - struct fw_cdev_event_response *rsp = &e->response; + struct fw_cdev_event_response *rsp = &e->rsp.without_tstamp; struct client *client = e->client; unsigned long flags; @@ -581,6 +583,8 @@ 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; if (request->tcode != TCODE_STREAM_DATA && @@ -594,14 +598,14 @@ static int init_request(struct client *client, e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL); if (e == NULL) return -ENOMEM; - e->client = client; - e->response.length = request->length; - e->response.closure = request->closure; - if (request->data && - copy_from_user(e->response.data, - u64_to_uptr(request->data), request->length)) { + rsp = &e->rsp.without_tstamp; + 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; goto failed; } @@ -611,10 +615,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, e->response.data, - request->length, complete_transaction, e); + fw_send_request(client->device->card, &e->r.transaction, request->tcode, destination_id, + request->generation, speed, request->offset, payload, request->length, + complete_transaction, e); return 0; failed: From fc2b52cf2e0e48938b65e20fae7099e54ef74c76 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 08/25] 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: kunit-dev@googlegroups.com Link: https://lore.kernel.org/r/20230529113406.986289-9-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- 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..6c04ed08e4a5 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. + * @response_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; From d8527cab6c311da34193b7c04f4d363fc2d72458 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 09/25] 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. Link: https://lore.kernel.org/r/20230529113406.986289-10-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- 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; - /* - * 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); + 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); + + 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: From 1ef147710b54d47f4108c802d8ee6f3d27fe922d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 10/25] 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. Link: https://lore.kernel.org/r/20230529113406.986289-11-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- 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); From e27b3939128a1d99a4c84f35e4f3897dae73ccd0 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 11/25] 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: kunit-dev@googlegroups.com Link: https://lore.kernel.org/r/20230529113406.986289-12-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/uapi-test.c | 14 ++++++ include/uapi/linux/firewire-cdev.h | 69 +++++++++++++++++++++++++----- 2 files changed, 72 insertions(+), 11 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 6c04ed08e4a5..99e823935427 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,14 +425,12 @@ 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 + * @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. * - * 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_RECEIVED, @length is 8 and @data - * consists of the two PHY packet quadlets, in host byte order. + * 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; @@ -440,6 +440,47 @@ struct fw_cdev_event_phy_packet { __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_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_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_packet2 { + __u64 closure; + __u32 type; + __u32 rcode; + __u32 length; + __u32 tstamp; + __u32 data[]; +}; + /** * union fw_cdev_event - Convenience union of fw_cdev_event_* types * @common: Valid for all types @@ -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. */ From fe971f9163b67b5338dfe4a0e4ce1cfa1b6cd325 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 08:12:40 +0900 Subject: [PATCH 12/25] 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. Link: https://lore.kernel.org/r/20230529113406.986289-13-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- 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); From e003498ec055eef3847e5433273df1e151b9af4b Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 30 May 2023 19:25:32 +0900 Subject: [PATCH 13/25] firewire: fix build failure due to missing module license The added KUnit test has no MODULE_LICENSE even if built for tristate. It brings build failure in linux-next integration. This commit releases the test under GPL and fixes the bug. Reported-by: Stephen Rothwell Closes: https://lore.kernel.org/lkml/20230530122450.1603af75@canb.auug.org.au/ Fixes: dc7c51638f46 ("firewire: add KUnit test to check layout of UAPI structures") Link: https://lore.kernel.org/r/20230530102532.56386-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/uapi-test.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c index c6ebf02e3d45..2fcbede4fab1 100644 --- a/drivers/firewire/uapi-test.c +++ b/drivers/firewire/uapi-test.c @@ -85,3 +85,5 @@ static struct kunit_suite structure_layout_test_suite = { .test_cases = structure_layout_test_cases, }; kunit_test_suite(structure_layout_test_suite); + +MODULE_LICENSE("GPL"); From 0258d889a7ee28310dbe3f5d4d039b48a1d080ad Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Thu, 1 Jun 2023 23:49:37 +0900 Subject: [PATCH 14/25] firewire: fix warnings to generate UAPI documentation Any target to generate UAPI documentation reports warnings to missing annotation for padding member in structures added recently. This commit suppresses the warnings. Reported-by: Stephen Rothwell Closes: https://lore.kernel.org/lkml/20230531135306.43613a59@canb.auug.org.au/ Fixes: 7c22d4a92bb2 ("firewire: cdev: add new event to notify request subaction with time stamp") Fixes: fc2b52cf2e0e ("firewire: cdev: add new event to notify response subaction with time stamp") Link: https://lore.kernel.org/r/20230601144937.121179-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- include/uapi/linux/firewire-cdev.h | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h index 99e823935427..1f2c9469f921 100644 --- a/include/uapi/linux/firewire-cdev.h +++ b/include/uapi/linux/firewire-cdev.h @@ -130,6 +130,9 @@ struct fw_cdev_event_response { * @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. * @response_tstamp: The time stamp of isochronous cycle at which the response was sent. + * @padding: 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. * @data: Payload data, if any * * This event is sent when the stack receives a response to an outgoing request @@ -155,10 +158,6 @@ struct fw_cdev_event_response2 { __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[]; }; @@ -231,6 +230,9 @@ struct fw_cdev_event_request2 { * @handle: Reference to the kernel-side pending request * @length: Data length, i.e. the request's payload size in bytes * @tstamp: The time stamp of isochronous cycle at which the request arrived. + * @padding: 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. * @data: Incoming data, if any * * This event is sent when the stack receives an incoming request to an address @@ -284,10 +286,6 @@ struct fw_cdev_event_request3 { __u32 handle; __u32 length; __u32 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[]; }; From f86319c02c3c5253ddebc21e81271c83421aaeb9 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:43 +0900 Subject: [PATCH 15/25] firewire: ohci: use devres for memory object of ohci structure The managed device resource (devres) framework is convenient to maintain lifetime of allocated memory object for device. This commit utilizes the framework for the object of ohci structure. The extra operation for power management is required in Apple PowerMac based machines, thus release callback is assigned to the object to call the operation. Link: https://lore.kernel.org/r/20230604054451.161076-2-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 06386c3b7f03..2b02cebcb0ae 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3557,6 +3557,15 @@ static inline void pmac_ohci_on(struct pci_dev *dev) {} static inline void pmac_ohci_off(struct pci_dev *dev) {} #endif /* CONFIG_PPC_PMAC */ +static void release_ohci(struct device *dev, void *data) +{ + struct pci_dev *pdev = to_pci_dev(dev); + + pmac_ohci_off(pdev); + + dev_notice(dev, "removed fw-ohci device\n"); +} + static int pci_probe(struct pci_dev *dev, const struct pci_device_id *ent) { @@ -3571,25 +3580,22 @@ static int pci_probe(struct pci_dev *dev, return -ENOSYS; } - ohci = kzalloc(sizeof(*ohci), GFP_KERNEL); - if (ohci == NULL) { - err = -ENOMEM; - goto fail; - } - + ohci = devres_alloc(release_ohci, sizeof(*ohci), GFP_KERNEL); + if (ohci == NULL) + return -ENOMEM; fw_card_initialize(&ohci->card, &ohci_driver, &dev->dev); - + pci_set_drvdata(dev, ohci); pmac_ohci_on(dev); + devres_add(&dev->dev, ohci); err = pci_enable_device(dev); if (err) { dev_err(&dev->dev, "failed to enable OHCI hardware\n"); - goto fail_free; + return err; } pci_set_master(dev); pci_write_config_dword(dev, OHCI1394_PCI_HCI_Control, 0); - pci_set_drvdata(dev, ohci); spin_lock_init(&ohci->lock); mutex_init(&ohci->phy_reg_mutex); @@ -3748,10 +3754,7 @@ static int pci_probe(struct pci_dev *dev, pci_release_region(dev, 0); fail_disable: pci_disable_device(dev); - fail_free: - kfree(ohci); - pmac_ohci_off(dev); - fail: + return err; } @@ -3796,10 +3799,8 @@ static void pci_remove(struct pci_dev *dev) pci_iounmap(dev, ohci->registers); pci_release_region(dev, 0); pci_disable_device(dev); - kfree(ohci); - pmac_ohci_off(dev); - dev_notice(&dev->dev, "removed fw-ohci device\n"); + dev_notice(&dev->dev, "removing fw-ohci device\n"); } #ifdef CONFIG_PM From 14f6ca5b6ae759046fdc461633043bb05ff6de58 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:44 +0900 Subject: [PATCH 16/25] firewire: ohci: use devres for PCI-related resources The PCI framework supports managed device resource to maintain the lifetime of PCI specific resources. This commit allows 1394 OHCI driver to utilize it. Link: https://lore.kernel.org/r/20230604054451.161076-3-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 2b02cebcb0ae..f3d0882a876c 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3588,7 +3588,7 @@ static int pci_probe(struct pci_dev *dev, pmac_ohci_on(dev); devres_add(&dev->dev, ohci); - err = pci_enable_device(dev); + err = pcim_enable_device(dev); if (err) { dev_err(&dev->dev, "failed to enable OHCI hardware\n"); return err; @@ -3605,14 +3605,13 @@ static int pci_probe(struct pci_dev *dev, if (!(pci_resource_flags(dev, 0) & IORESOURCE_MEM) || pci_resource_len(dev, 0) < OHCI1394_REGISTER_SIZE) { ohci_err(ohci, "invalid MMIO resource\n"); - err = -ENXIO; - goto fail_disable; + return -ENXIO; } err = pci_request_region(dev, 0, ohci_driver_name); if (err) { ohci_err(ohci, "MMIO resource unavailable\n"); - goto fail_disable; + return err; } ohci->registers = pci_iomap(dev, 0, OHCI1394_REGISTER_SIZE); @@ -3752,8 +3751,6 @@ static int pci_probe(struct pci_dev *dev, pci_iounmap(dev, ohci->registers); fail_iomem: pci_release_region(dev, 0); - fail_disable: - pci_disable_device(dev); return err; } @@ -3798,7 +3795,6 @@ static void pci_remove(struct pci_dev *dev) pci_disable_msi(dev); pci_iounmap(dev, ohci->registers); pci_release_region(dev, 0); - pci_disable_device(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); } From 086a0afbe9bbcdf7e12cdc7e317a57f49fa35207 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:45 +0900 Subject: [PATCH 17/25] firewire: ohci: use devres for MMIO region mapping The PCI framework has the convenient helper function to check and map MMIO region with managed device resource. This commit elaborates 1394 OHCI driver to use the function. Link: https://lore.kernel.org/r/20230604054451.161076-4-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index f3d0882a876c..26c64b60144d 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3608,18 +3608,12 @@ static int pci_probe(struct pci_dev *dev, return -ENXIO; } - err = pci_request_region(dev, 0, ohci_driver_name); + err = pcim_iomap_regions(dev, 1 << 0, ohci_driver_name); if (err) { - ohci_err(ohci, "MMIO resource unavailable\n"); - return err; - } - - ohci->registers = pci_iomap(dev, 0, OHCI1394_REGISTER_SIZE); - if (ohci->registers == NULL) { - ohci_err(ohci, "failed to remap registers\n"); - err = -ENXIO; - goto fail_iomem; + ohci_err(ohci, "request and map MMIO resource unavailable\n"); + return -ENXIO; } + ohci->registers = pcim_iomap_table(dev)[0]; for (i = 0; i < ARRAY_SIZE(ohci_quirks); i++) if ((ohci_quirks[i].vendor == dev->vendor) && @@ -3644,10 +3638,8 @@ static int pci_probe(struct pci_dev *dev, PAGE_SIZE, &ohci->misc_buffer_bus, GFP_KERNEL); - if (!ohci->misc_buffer) { - err = -ENOMEM; - goto fail_iounmap; - } + if (!ohci->misc_buffer) + return -ENOMEM; err = ar_context_init(&ohci->ar_request_ctx, ohci, 0, OHCI1394_AsReqRcvContextControlSet); @@ -3747,10 +3739,6 @@ static int pci_probe(struct pci_dev *dev, fail_misc_buf: dma_free_coherent(ohci->card.device, PAGE_SIZE, ohci->misc_buffer, ohci->misc_buffer_bus); - fail_iounmap: - pci_iounmap(dev, ohci->registers); - fail_iomem: - pci_release_region(dev, 0); return err; } @@ -3793,8 +3781,6 @@ static void pci_remove(struct pci_dev *dev) kfree(ohci->it_context_list); kfree(ohci->ir_context_list); pci_disable_msi(dev); - pci_iounmap(dev, ohci->registers); - pci_release_region(dev, 0); dev_notice(&dev->dev, "removing fw-ohci device\n"); } From 8320442b264a8d49b08f2730d63a39b2e0c146f8 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:46 +0900 Subject: [PATCH 18/25] firewire: ohci: use devres for misc DMA buffer The 1394 OHCI driver allocates a DMA coherent buffer for multi-purposes. The buffer is split into three region for specific purposes; i.e. 1/4 for context descriptors of AR request and response as well as 1/2 for self ID handling. This commit uses managed device resource to maintain the lifetime of buffer. Link: https://lore.kernel.org/r/20230604054451.161076-5-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 26c64b60144d..3b31d90781fe 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3634,17 +3634,15 @@ static int pci_probe(struct pci_dev *dev, */ BUILD_BUG_ON(AR_BUFFERS * sizeof(struct descriptor) > PAGE_SIZE/4); BUILD_BUG_ON(SELF_ID_BUF_SIZE > PAGE_SIZE/2); - ohci->misc_buffer = dma_alloc_coherent(ohci->card.device, - PAGE_SIZE, - &ohci->misc_buffer_bus, - GFP_KERNEL); + ohci->misc_buffer = dmam_alloc_coherent(&dev->dev, PAGE_SIZE, &ohci->misc_buffer_bus, + GFP_KERNEL); if (!ohci->misc_buffer) return -ENOMEM; err = ar_context_init(&ohci->ar_request_ctx, ohci, 0, OHCI1394_AsReqRcvContextControlSet); if (err < 0) - goto fail_misc_buf; + return err; err = ar_context_init(&ohci->ar_response_ctx, ohci, PAGE_SIZE/4, OHCI1394_AsRspRcvContextControlSet); @@ -3736,9 +3734,6 @@ static int pci_probe(struct pci_dev *dev, ar_context_release(&ohci->ar_response_ctx); fail_arreq_ctx: ar_context_release(&ohci->ar_request_ctx); - fail_misc_buf: - dma_free_coherent(ohci->card.device, PAGE_SIZE, - ohci->misc_buffer, ohci->misc_buffer_bus); return err; } @@ -3774,8 +3769,6 @@ static void pci_remove(struct pci_dev *dev) ohci->config_rom, ohci->config_rom_bus); ar_context_release(&ohci->ar_request_ctx); ar_context_release(&ohci->ar_response_ctx); - dma_free_coherent(ohci->card.device, PAGE_SIZE, - ohci->misc_buffer, ohci->misc_buffer_bus); context_release(&ohci->at_request_ctx); context_release(&ohci->at_response_ctx); kfree(ohci->it_context_list); From 5a95f1ded28691e69f7d6718c5dcbc149613d431 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:47 +0900 Subject: [PATCH 19/25] firewire: ohci: use devres for requested IRQ The 1394 OHCI controller register handler to single interrupt number. This commit uses managed device resource to maintain the lifetime of requested IRQ. Link: https://lore.kernel.org/r/20230604054451.161076-6-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 3b31d90781fe..cb6b43e3f67e 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3697,17 +3697,16 @@ static int pci_probe(struct pci_dev *dev, if (!(ohci->quirks & QUIRK_NO_MSI)) pci_enable_msi(dev); - if (request_irq(dev->irq, irq_handler, - pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED, - ohci_driver_name, ohci)) { + err = devm_request_irq(&dev->dev, dev->irq, irq_handler, + pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED, ohci_driver_name, ohci); + if (err < 0) { ohci_err(ohci, "failed to allocate interrupt %d\n", dev->irq); - err = -EIO; goto fail_msi; } err = fw_card_add(&ohci->card, max_receive, link_speed, guid); if (err) - goto fail_irq; + goto fail_msi; version = reg_read(ohci, OHCI1394_Version) & 0x00ff00ff; ohci_notice(ohci, @@ -3720,8 +3719,6 @@ static int pci_probe(struct pci_dev *dev, return 0; - fail_irq: - free_irq(dev->irq, ohci); fail_msi: pci_disable_msi(dev); fail_contexts: @@ -3759,7 +3756,6 @@ static void pci_remove(struct pci_dev *dev) */ software_reset(ohci); - free_irq(dev->irq, ohci); if (ohci->next_config_rom && ohci->next_config_rom != ohci->config_rom) dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, From 30d97fd7e30b6ba8ecef15c43797f90dad56ddab Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:48 +0900 Subject: [PATCH 20/25] firewire: ohci: use devres for list of isochronous contexts The 1394 OHCI driver allocates the list of isochronous contexts as much as the hardware supports. This commit utilizes managed device resource to maintain the lifetime of list. Link: https://lore.kernel.org/r/20230604054451.161076-7-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index cb6b43e3f67e..086505bd1729 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3666,7 +3666,11 @@ static int pci_probe(struct pci_dev *dev, ohci->ir_context_mask = ohci->ir_context_support; ohci->n_ir = hweight32(ohci->ir_context_mask); size = sizeof(struct iso_context) * ohci->n_ir; - ohci->ir_context_list = kzalloc(size, GFP_KERNEL); + ohci->ir_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); + if (!ohci->ir_context_list) { + err = -ENOMEM; + goto fail_atresp_ctx; + } reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, ~0); ohci->it_context_support = reg_read(ohci, OHCI1394_IsoXmitIntMaskSet); @@ -3679,11 +3683,10 @@ static int pci_probe(struct pci_dev *dev, ohci->it_context_mask = ohci->it_context_support; ohci->n_it = hweight32(ohci->it_context_mask); size = sizeof(struct iso_context) * ohci->n_it; - ohci->it_context_list = kzalloc(size, GFP_KERNEL); - - if (ohci->it_context_list == NULL || ohci->ir_context_list == NULL) { + ohci->it_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); + if (!ohci->it_context_list) { err = -ENOMEM; - goto fail_contexts; + goto fail_atresp_ctx; } ohci->self_id = ohci->misc_buffer + PAGE_SIZE/2; @@ -3721,9 +3724,7 @@ static int pci_probe(struct pci_dev *dev, fail_msi: pci_disable_msi(dev); - fail_contexts: - kfree(ohci->ir_context_list); - kfree(ohci->it_context_list); + fail_atresp_ctx: context_release(&ohci->at_response_ctx); fail_atreq_ctx: context_release(&ohci->at_request_ctx); @@ -3767,8 +3768,6 @@ static void pci_remove(struct pci_dev *dev) ar_context_release(&ohci->ar_response_ctx); context_release(&ohci->at_request_ctx); context_release(&ohci->at_response_ctx); - kfree(ohci->it_context_list); - kfree(ohci->ir_context_list); pci_disable_msi(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); From aa71e28d757244166c9ff50e4bc85cd83f2956fe Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:49 +0900 Subject: [PATCH 21/25] firewire: ohci: use devres for IT, IR, AT/receive, and AT/request contexts The 1394 OHCI driver allocates DMA coherent buffer for descriptors of IT, IR, AT receive, and AT request contexts by the same way. This commit utilizes managed device resource to maintain the lifetime of buffers. Link: https://lore.kernel.org/r/20230604054451.161076-8-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 086505bd1729..21eb13ea4dad 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1105,8 +1105,7 @@ static int context_add_buffer(struct context *ctx) if (ctx->total_allocation >= 16*1024*1024) return -ENOMEM; - desc = dma_alloc_coherent(ctx->ohci->card.device, PAGE_SIZE, - &bus_addr, GFP_ATOMIC); + desc = dmam_alloc_coherent(ctx->ohci->card.device, PAGE_SIZE, &bus_addr, GFP_ATOMIC); if (!desc) return -ENOMEM; @@ -1165,10 +1164,10 @@ static void context_release(struct context *ctx) struct fw_card *card = &ctx->ohci->card; struct descriptor_buffer *desc, *tmp; - list_for_each_entry_safe(desc, tmp, &ctx->buffer_list, list) - dma_free_coherent(card->device, PAGE_SIZE, desc, - desc->buffer_bus - - ((void *)&desc->buffer - (void *)desc)); + list_for_each_entry_safe(desc, tmp, &ctx->buffer_list, list) { + dmam_free_coherent(card->device, PAGE_SIZE, desc, + desc->buffer_bus - ((void *)&desc->buffer - (void *)desc)); + } } /* Must be called with ohci->lock held */ @@ -3657,7 +3656,7 @@ static int pci_probe(struct pci_dev *dev, err = context_init(&ohci->at_response_ctx, ohci, OHCI1394_AsRspTrContextControlSet, handle_at_packet); if (err < 0) - goto fail_atreq_ctx; + goto fail_arrsp_ctx; reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0); ohci->ir_context_channels = ~0ULL; @@ -3669,7 +3668,7 @@ static int pci_probe(struct pci_dev *dev, ohci->ir_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); if (!ohci->ir_context_list) { err = -ENOMEM; - goto fail_atresp_ctx; + goto fail_arrsp_ctx; } reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, ~0); @@ -3686,7 +3685,7 @@ static int pci_probe(struct pci_dev *dev, ohci->it_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); if (!ohci->it_context_list) { err = -ENOMEM; - goto fail_atresp_ctx; + goto fail_arrsp_ctx; } ohci->self_id = ohci->misc_buffer + PAGE_SIZE/2; @@ -3724,10 +3723,6 @@ static int pci_probe(struct pci_dev *dev, fail_msi: pci_disable_msi(dev); - fail_atresp_ctx: - context_release(&ohci->at_response_ctx); - fail_atreq_ctx: - context_release(&ohci->at_request_ctx); fail_arrsp_ctx: ar_context_release(&ohci->ar_response_ctx); fail_arreq_ctx: @@ -3766,8 +3761,6 @@ static void pci_remove(struct pci_dev *dev) ohci->config_rom, ohci->config_rom_bus); ar_context_release(&ohci->ar_request_ctx); ar_context_release(&ohci->ar_response_ctx); - context_release(&ohci->at_request_ctx); - context_release(&ohci->at_response_ctx); pci_disable_msi(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); From aeaf6aa8647182892f6bd63c5ae9faf4e73cbad0 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:50 +0900 Subject: [PATCH 22/25] firewire: ohci: use devres for content of configuration ROM The 1394 OHCI driver allocates DMA coherent buffer to transfer content of configuration ROM. This commit utilizes managed device resource to maintain the lifetime of buffer. Link: https://lore.kernel.org/r/20230604054451.161076-9-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 21eb13ea4dad..298667963538 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -2052,8 +2052,7 @@ static void bus_reset_work(struct work_struct *work) spin_unlock_irq(&ohci->lock); if (free_rom) - dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, - free_rom, free_rom_bus); + dmam_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, free_rom, free_rom_bus); log_selfids(ohci, generation, self_id_count); @@ -2385,10 +2384,8 @@ static int ohci_enable(struct fw_card *card, */ if (config_rom) { - ohci->next_config_rom = - dma_alloc_coherent(ohci->card.device, CONFIG_ROM_SIZE, - &ohci->next_config_rom_bus, - GFP_KERNEL); + ohci->next_config_rom = dmam_alloc_coherent(ohci->card.device, CONFIG_ROM_SIZE, + &ohci->next_config_rom_bus, GFP_KERNEL); if (ohci->next_config_rom == NULL) return -ENOMEM; @@ -2480,9 +2477,8 @@ static int ohci_set_config_rom(struct fw_card *card, * ohci->next_config_rom to NULL (see bus_reset_work). */ - next_config_rom = - dma_alloc_coherent(ohci->card.device, CONFIG_ROM_SIZE, - &next_config_rom_bus, GFP_KERNEL); + next_config_rom = dmam_alloc_coherent(ohci->card.device, CONFIG_ROM_SIZE, + &next_config_rom_bus, GFP_KERNEL); if (next_config_rom == NULL) return -ENOMEM; @@ -2515,9 +2511,10 @@ static int ohci_set_config_rom(struct fw_card *card, spin_unlock_irq(&ohci->lock); /* If we didn't use the DMA allocation, delete it. */ - if (next_config_rom != NULL) - dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, - next_config_rom, next_config_rom_bus); + if (next_config_rom != NULL) { + dmam_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, next_config_rom, + next_config_rom_bus); + } /* * Now initiate a bus reset to have the changes take @@ -3753,12 +3750,6 @@ static void pci_remove(struct pci_dev *dev) software_reset(ohci); - if (ohci->next_config_rom && ohci->next_config_rom != ohci->config_rom) - dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, - ohci->next_config_rom, ohci->next_config_rom_bus); - if (ohci->config_rom) - dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE, - ohci->config_rom, ohci->config_rom_bus); ar_context_release(&ohci->ar_request_ctx); ar_context_release(&ohci->ar_response_ctx); pci_disable_msi(dev); From 5716e58aecdd4f7225c2e46ce903a839fc54f22f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 14:44:51 +0900 Subject: [PATCH 23/25] firewire: ohci: release buffer for AR req/resp contexts when managed resource is released The 1394 OHCI driver allocates several non-coherent DMA buffers for AR request and response contexts. The buffers are mapped to kernel virtual address (VMA) so that the first page locates after the last page. Even when large payload of packet is handled crossing the boundary of buffers, the driver operates continuously on VMA. No kernel API is provided for this kind of mapping, while it is possible to release the buffer when PCI device is going to be released. This commit moves the call of release helper function to the callback function of release resources. Link: https://lore.kernel.org/r/20230604054451.161076-10-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/ohci.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 298667963538..7e88fd489741 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -677,6 +677,9 @@ static void ar_context_release(struct ar_context *ctx) struct device *dev = ctx->ohci->card.device; unsigned int i; + if (!ctx->buffer) + return; + vunmap(ctx->buffer); for (i = 0; i < AR_BUFFERS; i++) { @@ -3556,9 +3559,13 @@ static inline void pmac_ohci_off(struct pci_dev *dev) {} static void release_ohci(struct device *dev, void *data) { struct pci_dev *pdev = to_pci_dev(dev); + struct fw_ohci *ohci = pci_get_drvdata(pdev); pmac_ohci_off(pdev); + ar_context_release(&ohci->ar_response_ctx); + ar_context_release(&ohci->ar_request_ctx); + dev_notice(dev, "removed fw-ohci device\n"); } @@ -3643,17 +3650,17 @@ static int pci_probe(struct pci_dev *dev, err = ar_context_init(&ohci->ar_response_ctx, ohci, PAGE_SIZE/4, OHCI1394_AsRspRcvContextControlSet); if (err < 0) - goto fail_arreq_ctx; + return err; err = context_init(&ohci->at_request_ctx, ohci, OHCI1394_AsReqTrContextControlSet, handle_at_packet); if (err < 0) - goto fail_arrsp_ctx; + return err; err = context_init(&ohci->at_response_ctx, ohci, OHCI1394_AsRspTrContextControlSet, handle_at_packet); if (err < 0) - goto fail_arrsp_ctx; + return err; reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0); ohci->ir_context_channels = ~0ULL; @@ -3663,10 +3670,8 @@ static int pci_probe(struct pci_dev *dev, ohci->n_ir = hweight32(ohci->ir_context_mask); size = sizeof(struct iso_context) * ohci->n_ir; ohci->ir_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); - if (!ohci->ir_context_list) { - err = -ENOMEM; - goto fail_arrsp_ctx; - } + if (!ohci->ir_context_list) + return -ENOMEM; reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, ~0); ohci->it_context_support = reg_read(ohci, OHCI1394_IsoXmitIntMaskSet); @@ -3680,10 +3685,8 @@ static int pci_probe(struct pci_dev *dev, ohci->n_it = hweight32(ohci->it_context_mask); size = sizeof(struct iso_context) * ohci->n_it; ohci->it_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL); - if (!ohci->it_context_list) { - err = -ENOMEM; - goto fail_arrsp_ctx; - } + if (!ohci->it_context_list) + return -ENOMEM; ohci->self_id = ohci->misc_buffer + PAGE_SIZE/2; ohci->self_id_bus = ohci->misc_buffer_bus + PAGE_SIZE/2; @@ -3720,10 +3723,6 @@ static int pci_probe(struct pci_dev *dev, fail_msi: pci_disable_msi(dev); - fail_arrsp_ctx: - ar_context_release(&ohci->ar_response_ctx); - fail_arreq_ctx: - ar_context_release(&ohci->ar_request_ctx); return err; } @@ -3750,8 +3749,6 @@ static void pci_remove(struct pci_dev *dev) software_reset(ohci); - ar_context_release(&ohci->ar_request_ctx); - ar_context_release(&ohci->ar_response_ctx); pci_disable_msi(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); From 06f45435d985d60d7d2fe2424fbb9909d177a63d Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Sun, 4 Jun 2023 16:02:55 +0900 Subject: [PATCH 24/25] firewire: core: obsolete usage of GFP_ATOMIC at building node tree The flag of GFP_ATOMIC is given to the call of kmalloc when building node tree, but the call is not atomic context. The call of fw_core_handle_bus_reset() and fw_core_remove_card() builds the tree, while they are done in specific workqueue or pci remove callback. This commit obsolete the usage of GFP_ATOMIC. Link: https://lore.kernel.org/r/20230604070255.172700-1-o-takashi@sakamocchi.jp Signed-off-by: Takashi Sakamoto --- drivers/firewire/core-device.c | 2 +- drivers/firewire/core-topology.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index aa597cda0d88..a3104e35412c 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -1211,7 +1211,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event) * without actually having a link. */ create: - device = kzalloc(sizeof(*device), GFP_ATOMIC); + device = kzalloc(sizeof(*device), GFP_KERNEL); if (device == NULL) break; diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index f40c81534381..88466b663482 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -101,7 +101,7 @@ static struct fw_node *fw_node_create(u32 sid, int port_count, int color) { struct fw_node *node; - node = kzalloc(struct_size(node, ports, port_count), GFP_ATOMIC); + node = kzalloc(struct_size(node, ports, port_count), GFP_KERNEL); if (node == NULL) return NULL; From 3ff256751a2853e1ffaa36958ff933ccc98c6cb5 Mon Sep 17 00:00:00 2001 From: Zhang Shurong Date: Fri, 23 Jun 2023 13:39:35 +0800 Subject: [PATCH 25/25] firewire: net: fix use after free in fwnet_finish_incoming_packet() The netif_rx() function frees the skb so we can't dereference it to save the skb->len. Signed-off-by: Zhang Shurong Link: https://lore.kernel.org/r/tencent_3B3D24B66ED66A6BB73CC0E63C6A14E45109@qq.com Signed-off-by: Takashi Sakamoto --- drivers/firewire/net.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c index 538bd677c254..7a4d1a478e33 100644 --- a/drivers/firewire/net.c +++ b/drivers/firewire/net.c @@ -479,7 +479,7 @@ static int fwnet_finish_incoming_packet(struct net_device *net, struct sk_buff *skb, u16 source_node_id, bool is_broadcast, u16 ether_type) { - int status; + int status, len; switch (ether_type) { case ETH_P_ARP: @@ -533,13 +533,15 @@ static int fwnet_finish_incoming_packet(struct net_device *net, } skb->protocol = protocol; } + + len = skb->len; status = netif_rx(skb); if (status == NET_RX_DROP) { net->stats.rx_errors++; net->stats.rx_dropped++; } else { net->stats.rx_packets++; - net->stats.rx_bytes += skb->len; + net->stats.rx_bytes += len; } return 0;