linux1394-devel Mailing List for IEEE 1394 for Linux (Page 10)
Brought to you by:
aeb,
bencollins
You can subscribe to this list here.
2000 |
Jan
|
Feb
|
Mar
(39) |
Apr
(154) |
May
(172) |
Jun
(237) |
Jul
(127) |
Aug
(135) |
Sep
(193) |
Oct
(175) |
Nov
(173) |
Dec
(148) |
---|---|---|---|---|---|---|---|---|---|---|---|---|
2001 |
Jan
(161) |
Feb
(225) |
Mar
(193) |
Apr
(158) |
May
(179) |
Jun
(292) |
Jul
(146) |
Aug
(134) |
Sep
(185) |
Oct
(190) |
Nov
(149) |
Dec
(161) |
2002 |
Jan
(186) |
Feb
(236) |
Mar
(254) |
Apr
(207) |
May
(189) |
Jun
(182) |
Jul
(202) |
Aug
(155) |
Sep
(149) |
Oct
(449) |
Nov
(191) |
Dec
(108) |
2003 |
Jan
(174) |
Feb
(242) |
Mar
(243) |
Apr
(255) |
May
(202) |
Jun
(290) |
Jul
(237) |
Aug
(178) |
Sep
(101) |
Oct
(153) |
Nov
(144) |
Dec
(95) |
2004 |
Jan
(162) |
Feb
(278) |
Mar
(282) |
Apr
(152) |
May
(127) |
Jun
(138) |
Jul
(94) |
Aug
(63) |
Sep
(64) |
Oct
(150) |
Nov
(102) |
Dec
(197) |
2005 |
Jan
(102) |
Feb
(172) |
Mar
(89) |
Apr
(158) |
May
(139) |
Jun
(160) |
Jul
(288) |
Aug
(89) |
Sep
(201) |
Oct
(92) |
Nov
(190) |
Dec
(139) |
2006 |
Jan
(121) |
Feb
(204) |
Mar
(133) |
Apr
(134) |
May
(91) |
Jun
(226) |
Jul
(122) |
Aug
(101) |
Sep
(144) |
Oct
(141) |
Nov
|
Dec
|
2023 |
Jan
(19) |
Feb
(1) |
Mar
(5) |
Apr
(5) |
May
(33) |
Jun
(17) |
Jul
|
Aug
|
Sep
(3) |
Oct
(1) |
Nov
(5) |
Dec
(40) |
2024 |
Jan
(26) |
Feb
(14) |
Mar
(26) |
Apr
(46) |
May
(17) |
Jun
(47) |
Jul
(23) |
Aug
(72) |
Sep
(33) |
Oct
|
Nov
|
Dec
|
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:45
|
In the helper function for logging in 1394 ohci driver includes the hard-coded variables for transaction code. They can be replaced with the enumerations in UAPI header. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 4666d941a2ae..85223a1c90a1 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -533,11 +533,17 @@ static void log_ar_at_event(struct fw_ohci *ohci, } switch (tcode) { - case 0x0: case 0x6: case 0x8: + case TCODE_WRITE_QUADLET_REQUEST: + case TCODE_READ_QUADLET_RESPONSE: + case TCODE_CYCLE_START: snprintf(specific, sizeof(specific), " = %08x", be32_to_cpu((__force __be32)header[3])); break; - case 0x1: case 0x5: case 0x7: case 0x9: case 0xb: + case TCODE_WRITE_BLOCK_REQUEST: + case TCODE_READ_BLOCK_REQUEST: + case TCODE_READ_BLOCK_RESPONSE: + case TCODE_LOCK_REQUEST: + case TCODE_LOCK_RESPONSE: snprintf(specific, sizeof(specific), " %x,%x", async_header_get_data_length(header), async_header_get_extended_tcode(header)); @@ -547,7 +553,7 @@ static void log_ar_at_event(struct fw_ohci *ohci, } switch (tcode) { - case 0xa: + case TCODE_STREAM_DATA: ohci_notice(ohci, "A%c %s, %s\n", dir, evts[evt], tcodes[tcode]); break; @@ -555,7 +561,11 @@ static void log_ar_at_event(struct fw_ohci *ohci, ohci_notice(ohci, "A%c %s, PHY %08x %08x\n", dir, evts[evt], header[1], header[2]); break; - case 0x0: case 0x1: case 0x4: case 0x5: case 0x9: + case TCODE_WRITE_QUADLET_REQUEST: + case TCODE_WRITE_BLOCK_REQUEST: + case TCODE_READ_QUADLET_REQUEST: + case TCODE_READ_BLOCK_REQUEST: + case TCODE_LOCK_REQUEST: ohci_notice(ohci, "A%c spd %x tl %02x, %04x -> %04x, %s, %s, %012llx%s\n", dir, speed, async_header_get_tlabel(header), -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:45
|
This commit declares the helper functions to check tcode to obsolete the functional macros. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-transaction.c | 4 ++-- drivers/firewire/core.h | 21 ++++++++++++++------- drivers/firewire/ohci.c | 6 +++--- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 01ce07f87452..24febc23c0c4 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -972,7 +972,7 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p) if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE) return; - if (TCODE_IS_LINK_INTERNAL(async_header_get_tcode(p->header))) { + if (tcode_is_link_internal(async_header_get_tcode(p->header))) { fw_cdev_handle_phy_packet(card, p); return; } @@ -1109,7 +1109,7 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request { int start; - if (!TCODE_IS_READ_REQUEST(tcode)) { + if (!tcode_is_read_request(tcode)) { fw_send_response(card, request, RCODE_TYPE_ERROR); return; } diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 95c10f3d2282..7c36d2628e37 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -225,13 +225,20 @@ static inline bool is_next_generation(int new_generation, int old_generation) #define TCODE_LINK_INTERNAL 0xe -#define TCODE_IS_READ_REQUEST(tcode) (((tcode) & ~1) == 4) -#define TCODE_IS_BLOCK_PACKET(tcode) (((tcode) & 1) != 0) -#define TCODE_IS_LINK_INTERNAL(tcode) ((tcode) == TCODE_LINK_INTERNAL) -#define TCODE_IS_REQUEST(tcode) (((tcode) & 2) == 0) -#define TCODE_IS_RESPONSE(tcode) (((tcode) & 2) != 0) -#define TCODE_HAS_REQUEST_DATA(tcode) (((tcode) & 12) != 4) -#define TCODE_HAS_RESPONSE_DATA(tcode) (((tcode) & 12) != 0) +static inline bool tcode_is_read_request(unsigned int tcode) +{ + return (tcode & ~1u) == 4u; +} + +static inline bool tcode_is_block_packet(unsigned int tcode) +{ + return (tcode & 1u) != 0u; +} + +static inline bool tcode_is_link_internal(unsigned int tcode) +{ + return (tcode == TCODE_LINK_INTERNAL); +} #define LOCAL_BUS 0xffc0 diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 85223a1c90a1..77f098fb9484 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1382,7 +1382,7 @@ static int at_context_queue_packet(struct context *ctx, (packet->header[0] & 0xffff0000)); header[2] = cpu_to_le32(packet->header[2]); - if (TCODE_IS_BLOCK_PACKET(tcode)) + if (tcode_is_block_packet(tcode)) header[3] = cpu_to_le32(packet->header[3]); else header[3] = (__force __le32) packet->header[3]; @@ -1568,7 +1568,7 @@ static void handle_local_rom(struct fw_ohci *ohci, int tcode, length, i; tcode = async_header_get_tcode(packet->header); - if (TCODE_IS_BLOCK_PACKET(tcode)) + if (tcode_is_block_packet(tcode)) length = async_header_get_data_length(packet->header); else length = 4; @@ -1577,7 +1577,7 @@ static void handle_local_rom(struct fw_ohci *ohci, if (i + length > CONFIG_ROM_SIZE) { fw_fill_response(&response, packet->header, RCODE_ADDRESS_ERROR, NULL, 0); - } else if (!TCODE_IS_READ_REQUEST(tcode)) { + } else if (!tcode_is_read_request(tcode)) { fw_fill_response(&response, packet->header, RCODE_TYPE_ERROR, NULL, 0); } else { -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:45
|
This commit replaces the local macros with the common inline functions to serialize the packer header for Asynchronous Streaming Packet. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-transaction.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 24febc23c0c4..52d8d483c178 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -31,9 +31,6 @@ #include "core.h" #include "packet-header-definitions.h" -#define HEADER_TCODE(tcode) ((tcode) << 4) -#define HEADER_DATA_LENGTH(length) ((length) << 16) - #define HEADER_DESTINATION_IS_BROADCAST(header) \ ((async_header_get_destination(header) & 0x3f) == 0x3f) @@ -215,10 +212,11 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, int ext_tcode; if (tcode == TCODE_STREAM_DATA) { - packet->header[0] = - HEADER_DATA_LENGTH(length) | - destination_id | - HEADER_TCODE(TCODE_STREAM_DATA); + // The value of destination_id argument should include tag, channel, and sy fields + // as isochronous packet header has. + packet->header[0] = destination_id; + isoc_header_set_data_length(packet->header, length); + isoc_header_set_tcode(packet->header, TCODE_STREAM_DATA); packet->header_length = 4; packet->payload = payload; packet->payload_length = length; -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:31
|
This commit uses common inline functions to serialize and deserialize header for asynchronous packet. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-transaction.c | 138 +++++++++++----------------- 1 file changed, 56 insertions(+), 82 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 130b95aca629..01ce07f87452 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -29,29 +29,13 @@ #include <asm/byteorder.h> #include "core.h" +#include "packet-header-definitions.h" -#define HEADER_PRI(pri) ((pri) << 0) #define HEADER_TCODE(tcode) ((tcode) << 4) -#define HEADER_RETRY(retry) ((retry) << 8) -#define HEADER_TLABEL(tlabel) ((tlabel) << 10) -#define HEADER_DESTINATION(destination) ((destination) << 16) -#define HEADER_SOURCE(source) ((source) << 16) -#define HEADER_RCODE(rcode) ((rcode) << 12) -#define HEADER_OFFSET_HIGH(offset_high) ((offset_high) << 0) #define HEADER_DATA_LENGTH(length) ((length) << 16) -#define HEADER_EXTENDED_TCODE(tcode) ((tcode) << 0) -#define HEADER_GET_TCODE(q) (((q) >> 4) & 0x0f) -#define HEADER_GET_TLABEL(q) (((q) >> 10) & 0x3f) -#define HEADER_GET_RCODE(q) (((q) >> 12) & 0x0f) -#define HEADER_GET_DESTINATION(q) (((q) >> 16) & 0xffff) -#define HEADER_GET_SOURCE(q) (((q) >> 16) & 0xffff) -#define HEADER_GET_OFFSET_HIGH(q) (((q) >> 0) & 0xffff) -#define HEADER_GET_DATA_LENGTH(q) (((q) >> 16) & 0xffff) -#define HEADER_GET_EXTENDED_TCODE(q) (((q) >> 0) & 0xffff) - -#define HEADER_DESTINATION_IS_BROADCAST(q) \ - (((q) & HEADER_DESTINATION(0x3f)) == HEADER_DESTINATION(0x3f)) +#define HEADER_DESTINATION_IS_BROADCAST(header) \ + ((async_header_get_destination(header) & 0x3f) == 0x3f) #define PHY_PACKET_CONFIG 0x0 #define PHY_PACKET_LINK_ON 0x1 @@ -248,28 +232,24 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, } else ext_tcode = 0; - packet->header[0] = - HEADER_RETRY(RETRY_X) | - HEADER_TLABEL(tlabel) | - HEADER_TCODE(tcode) | - HEADER_DESTINATION(destination_id); - packet->header[1] = - HEADER_OFFSET_HIGH(offset >> 32) | HEADER_SOURCE(source_id); - packet->header[2] = - offset; + async_header_set_retry(packet->header, RETRY_X); + async_header_set_tlabel(packet->header, tlabel); + async_header_set_tcode(packet->header, tcode); + async_header_set_destination(packet->header, destination_id); + async_header_set_source(packet->header, source_id); + async_header_set_offset(packet->header, offset); switch (tcode) { case TCODE_WRITE_QUADLET_REQUEST: - packet->header[3] = *(u32 *)payload; + async_header_set_quadlet_data(packet->header, *(u32 *)payload); packet->header_length = 16; packet->payload_length = 0; break; case TCODE_LOCK_REQUEST: case TCODE_WRITE_BLOCK_REQUEST: - packet->header[3] = - HEADER_DATA_LENGTH(length) | - HEADER_EXTENDED_TCODE(ext_tcode); + async_header_set_data_length(packet->header, length); + async_header_set_extended_tcode(packet->header, ext_tcode); packet->header_length = 16; packet->payload = payload; packet->payload_length = length; @@ -281,9 +261,8 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, break; case TCODE_READ_BLOCK_REQUEST: - packet->header[3] = - HEADER_DATA_LENGTH(length) | - HEADER_EXTENDED_TCODE(ext_tcode); + async_header_set_data_length(packet->header, length); + async_header_set_extended_tcode(packet->header, ext_tcode); packet->header_length = 16; packet->payload_length = 0; break; @@ -655,7 +634,7 @@ EXPORT_SYMBOL(fw_core_remove_address_handler); struct fw_request { struct kref kref; struct fw_packet response; - u32 request_header[4]; + u32 request_header[ASYNC_HEADER_QUADLET_COUNT]; int ack; u32 timestamp; u32 length; @@ -695,7 +674,7 @@ int fw_get_response_length(struct fw_request *r) { int tcode, ext_tcode, data_length; - tcode = HEADER_GET_TCODE(r->request_header[0]); + tcode = async_header_get_tcode(r->request_header); switch (tcode) { case TCODE_WRITE_QUADLET_REQUEST: @@ -706,12 +685,12 @@ int fw_get_response_length(struct fw_request *r) return 4; case TCODE_READ_BLOCK_REQUEST: - data_length = HEADER_GET_DATA_LENGTH(r->request_header[3]); + data_length = async_header_get_data_length(r->request_header); return data_length; case TCODE_LOCK_REQUEST: - ext_tcode = HEADER_GET_EXTENDED_TCODE(r->request_header[3]); - data_length = HEADER_GET_DATA_LENGTH(r->request_header[3]); + ext_tcode = async_header_get_extended_tcode(r->request_header); + data_length = async_header_get_data_length(r->request_header); switch (ext_tcode) { case EXTCODE_FETCH_ADD: case EXTCODE_LITTLE_ADD: @@ -731,46 +710,42 @@ void fw_fill_response(struct fw_packet *response, u32 *request_header, { int tcode, tlabel, extended_tcode, source, destination; - tcode = HEADER_GET_TCODE(request_header[0]); - tlabel = HEADER_GET_TLABEL(request_header[0]); - source = HEADER_GET_DESTINATION(request_header[0]); - destination = HEADER_GET_SOURCE(request_header[1]); - extended_tcode = HEADER_GET_EXTENDED_TCODE(request_header[3]); - - response->header[0] = - HEADER_RETRY(RETRY_1) | - HEADER_TLABEL(tlabel) | - HEADER_DESTINATION(destination); - response->header[1] = - HEADER_SOURCE(source) | - HEADER_RCODE(rcode); - response->header[2] = 0; + tcode = async_header_get_tcode(request_header); + tlabel = async_header_get_tlabel(request_header); + source = async_header_get_source(request_header); + destination = async_header_get_destination(request_header); + extended_tcode = async_header_get_extended_tcode(request_header); + + async_header_set_retry(response->header, RETRY_1); + async_header_set_tlabel(response->header, tlabel); + async_header_set_destination(response->header, destination); + async_header_set_source(response->header, source); + async_header_set_rcode(response->header, rcode); + response->header[2] = 0; // The field is reserved. switch (tcode) { case TCODE_WRITE_QUADLET_REQUEST: case TCODE_WRITE_BLOCK_REQUEST: - response->header[0] |= HEADER_TCODE(TCODE_WRITE_RESPONSE); + async_header_set_tcode(response->header, TCODE_WRITE_RESPONSE); response->header_length = 12; response->payload_length = 0; break; case TCODE_READ_QUADLET_REQUEST: - response->header[0] |= - HEADER_TCODE(TCODE_READ_QUADLET_RESPONSE); + async_header_set_tcode(response->header, TCODE_READ_QUADLET_RESPONSE); if (payload != NULL) - response->header[3] = *(u32 *)payload; + async_header_set_quadlet_data(response->header, *(u32 *)payload); else - response->header[3] = 0; + async_header_set_quadlet_data(response->header, 0); response->header_length = 16; response->payload_length = 0; break; case TCODE_READ_BLOCK_REQUEST: case TCODE_LOCK_REQUEST: - response->header[0] |= HEADER_TCODE(tcode + 2); - response->header[3] = - HEADER_DATA_LENGTH(length) | - HEADER_EXTENDED_TCODE(extended_tcode); + async_header_set_tcode(response->header, tcode + 2); + async_header_set_data_length(response->header, length); + async_header_set_extended_tcode(response->header, extended_tcode); response->header_length = 16; response->payload = payload; response->payload_length = length; @@ -807,7 +782,7 @@ static struct fw_request *allocate_request(struct fw_card *card, u32 *data, length; int request_tcode; - request_tcode = HEADER_GET_TCODE(p->header[0]); + request_tcode = async_header_get_tcode(p->header); switch (request_tcode) { case TCODE_WRITE_QUADLET_REQUEST: data = &p->header[3]; @@ -817,7 +792,7 @@ static struct fw_request *allocate_request(struct fw_card *card, case TCODE_WRITE_BLOCK_REQUEST: case TCODE_LOCK_REQUEST: data = p->payload; - length = HEADER_GET_DATA_LENGTH(p->header[3]); + length = async_header_get_data_length(p->header); break; case TCODE_READ_QUADLET_REQUEST: @@ -827,7 +802,7 @@ static struct fw_request *allocate_request(struct fw_card *card, case TCODE_READ_BLOCK_REQUEST: data = NULL; - length = HEADER_GET_DATA_LENGTH(p->header[3]); + length = async_header_get_data_length(p->header); break; default: @@ -872,7 +847,7 @@ void fw_send_response(struct fw_card *card, { /* unified transaction or broadcast transaction: don't respond */ if (request->ack != ACK_PENDING || - HEADER_DESTINATION_IS_BROADCAST(request->request_header[0])) { + HEADER_DESTINATION_IS_BROADCAST(request->request_header)) { fw_request_put(request); return; } @@ -926,11 +901,11 @@ static void handle_exclusive_region_request(struct fw_card *card, struct fw_address_handler *handler; int tcode, destination, source; - destination = HEADER_GET_DESTINATION(p->header[0]); - source = HEADER_GET_SOURCE(p->header[1]); - tcode = HEADER_GET_TCODE(p->header[0]); + destination = async_header_get_destination(p->header); + source = async_header_get_source(p->header); + tcode = async_header_get_tcode(p->header); if (tcode == TCODE_LOCK_REQUEST) - tcode = 0x10 + HEADER_GET_EXTENDED_TCODE(p->header[3]); + tcode = 0x10 + async_header_get_extended_tcode(p->header); rcu_read_lock(); handler = lookup_enclosing_address_handler(&address_handler_list, @@ -963,9 +938,9 @@ static void handle_fcp_region_request(struct fw_card *card, return; } - tcode = HEADER_GET_TCODE(p->header[0]); - destination = HEADER_GET_DESTINATION(p->header[0]); - source = HEADER_GET_SOURCE(p->header[1]); + tcode = async_header_get_tcode(p->header); + destination = async_header_get_destination(p->header); + source = async_header_get_source(p->header); if (tcode != TCODE_WRITE_QUADLET_REQUEST && tcode != TCODE_WRITE_BLOCK_REQUEST) { @@ -997,7 +972,7 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p) if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE) return; - if (TCODE_IS_LINK_INTERNAL(HEADER_GET_TCODE(p->header[0]))) { + if (TCODE_IS_LINK_INTERNAL(async_header_get_tcode(p->header))) { fw_cdev_handle_phy_packet(card, p); return; } @@ -1008,8 +983,7 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p) return; } - offset = ((u64)HEADER_GET_OFFSET_HIGH(p->header[1]) << 32) | - p->header[2]; + offset = async_header_get_offset(p->header); if (!is_in_fcp_region(offset, request->length)) handle_exclusive_region_request(card, p, request, offset); @@ -1027,10 +1001,10 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) size_t data_length; int tcode, tlabel, source, rcode; - tcode = HEADER_GET_TCODE(p->header[0]); - tlabel = HEADER_GET_TLABEL(p->header[0]); - source = HEADER_GET_SOURCE(p->header[1]); - rcode = HEADER_GET_RCODE(p->header[1]); + tcode = async_header_get_tcode(p->header); + tlabel = async_header_get_tlabel(p->header); + source = async_header_get_source(p->header); + rcode = async_header_get_rcode(p->header); spin_lock_irqsave(&card->lock, flags); list_for_each_entry(iter, &card->transaction_list, link) { @@ -1073,7 +1047,7 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) case TCODE_READ_BLOCK_RESPONSE: case TCODE_LOCK_RESPONSE: data = p->payload; - data_length = HEADER_GET_DATA_LENGTH(p->header[3]); + data_length = async_header_get_data_length(p->header); break; default: -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:31
|
Hi, In a view of IEEE 1394 bus, the main function of kernel core is to provide transaction service to the bus. It is helpful to have some mechanisms to trace any action of the service. This series of changes adds some tracepoints events for the purpose. It adds the following tracepoints events via firewire subsystem: * For outbound transactions (e.g. initiated by user process) * async_request_outbound_initiate * async_request_outbound_complete * async_response_inbound * For inbound transactions (e.g. initiated by the other nodes in the bus) * async_request_inbound * async_response_outbound_initiate * async_response_outbound_complete When probing these tracepoints events, the content of 'struct fw_packet' passed between the core function and 1394 OHCI driver is recorded with the fields of header and packet data. For example of the outbound transaction: async_request_outbound_initiate: \ transaction=0xffffb7e382373718 scode=2 generation=6 dst_id=0xffc0 \ tlabel=59 retry=1 tcode=1 priority=0 src_id=0xffc1 \ offset=0xecc000000000 \ data={0x6000000,0x1000000,0x40000100,0x3000000,0x1000000,0x0} async_request_outbound_complete: \ transaction=0xffffb7e382373718 scode=2 generation=6 ack=2 \ timestamp=0x2296 async_response_inbound: \ transaction=0xffffb7e382373718 scode=2 timestamp=0x2297 dst_id=0xffc1 \ tlabel=59 retry=1 tcode=2 priority=0 src_id=0xffc0 rcode=0 data={} To provide the parsed fields of header, the series adds some helper incline functions for this purpose, then refactors the existent code in both core and 1394 OHCI driver with sufficient tests. Takashi Sakamoto (13): firewire: core: add common inline functions to serialize/deserialize asynchronous packet header firewire: core: replace local macros with common inline functions for asynchronous packet header firewire: ohci: replace local macros with common inline functions for asynchronous packet header firewire: ohci: replace hard-coded values with inline functions for asynchronous packet header firewire: ohci: replace hard-coded values with common macros firewire: core: obsolete tcode check macros with inline functions firewire: core: add common macro to serialize/deserialize isochronous packet header firewire: core: replace local macros with common inline functions for isochronous packet header firewire: core: add support for Linux kernel tracepoints firewire: core: add tracepoints events for asynchronous outbound request firewire: core: add tracepoints event for asynchronous inbound response firewire: core: add tracepoint event for asynchronous inbound request firewire: core: add tracepoints events for asynchronous outbound response drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 16 + drivers/firewire/Makefile | 8 +- drivers/firewire/core-transaction.c | 239 ++++---- drivers/firewire/core.h | 21 +- drivers/firewire/ohci.c | 78 +-- drivers/firewire/packet-header-definitions.h | 234 ++++++++ drivers/firewire/packet-serdes-test.c | 582 +++++++++++++++++++ drivers/firewire/trace.c | 5 + drivers/firewire/trace.h | 265 +++++++++ 10 files changed, 1280 insertions(+), 169 deletions(-) create mode 100644 drivers/firewire/packet-header-definitions.h create mode 100644 drivers/firewire/packet-serdes-test.c create mode 100644 drivers/firewire/trace.c create mode 100644 drivers/firewire/trace.h -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:31
|
This commit replaces the hard-coded values with the common inline functions to serialize and deserialize the header of asynchronous packet. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 5254cf5c2e58..4666d941a2ae 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -517,14 +517,14 @@ static const char *tcodes[] = { static void log_ar_at_event(struct fw_ohci *ohci, char dir, int speed, u32 *header, int evt) { - int tcode = header[0] >> 4 & 0xf; + int tcode = async_header_get_tcode(header); char specific[12]; if (likely(!(param_debug & OHCI_PARAM_DEBUG_AT_AR))) return; if (unlikely(evt >= ARRAY_SIZE(evts))) - evt = 0x1f; + evt = 0x1f; if (evt == OHCI1394_evt_bus_reset) { ohci_notice(ohci, "A%c evt_bus_reset, generation %d\n", @@ -539,7 +539,8 @@ static void log_ar_at_event(struct fw_ohci *ohci, break; case 0x1: case 0x5: case 0x7: case 0x9: case 0xb: snprintf(specific, sizeof(specific), " %x,%x", - header[3] >> 16, header[3] & 0xffff); + async_header_get_data_length(header), + async_header_get_extended_tcode(header)); break; default: specific[0] = '\0'; @@ -556,17 +557,17 @@ static void log_ar_at_event(struct fw_ohci *ohci, break; case 0x0: case 0x1: case 0x4: case 0x5: case 0x9: ohci_notice(ohci, - "A%c spd %x tl %02x, %04x -> %04x, %s, %s, %04x%08x%s\n", - dir, speed, header[0] >> 10 & 0x3f, - header[1] >> 16, header[0] >> 16, evts[evt], - tcodes[tcode], header[1] & 0xffff, header[2], specific); + "A%c spd %x tl %02x, %04x -> %04x, %s, %s, %012llx%s\n", + dir, speed, async_header_get_tlabel(header), + async_header_get_source(header), async_header_get_destination(header), + evts[evt], tcodes[tcode], async_header_get_offset(header), specific); break; default: ohci_notice(ohci, "A%c spd %x tl %02x, %04x -> %04x, %s, %s%s\n", - dir, speed, header[0] >> 10 & 0x3f, - header[1] >> 16, header[0] >> 16, evts[evt], - tcodes[tcode], specific); + dir, speed, async_header_get_tlabel(header), + async_header_get_source(header), async_header_get_destination(header), + evts[evt], tcodes[tcode], specific); } } @@ -854,7 +855,7 @@ static __le32 *handle_ar_packet(struct ar_context *ctx, __le32 *buffer) p.header[1] = cond_le32_to_cpu(buffer[1]); p.header[2] = cond_le32_to_cpu(buffer[2]); - tcode = (p.header[0] >> 4) & 0x0f; + tcode = async_header_get_tcode(p.header); switch (tcode) { case TCODE_WRITE_QUADLET_REQUEST: case TCODE_READ_QUADLET_RESPONSE: @@ -875,7 +876,7 @@ static __le32 *handle_ar_packet(struct ar_context *ctx, __le32 *buffer) case TCODE_LOCK_RESPONSE: p.header[3] = cond_le32_to_cpu(buffer[3]); p.header_length = 16; - p.payload_length = p.header[3] >> 16; + p.payload_length = async_header_get_data_length(p.header); if (p.payload_length > MAX_ASYNC_PAYLOAD) { ar_context_abort(ctx, "invalid packet length"); return NULL; @@ -912,8 +913,7 @@ static __le32 *handle_ar_packet(struct ar_context *ctx, __le32 *buffer) * Several controllers, notably from NEC and VIA, forget to * write ack_complete status at PHY packet reception. */ - if (evt == OHCI1394_evt_no_status && - (p.header[0] & 0xff) == (OHCI1394_phy_tcode << 4)) + if (evt == OHCI1394_evt_no_status && async_header_get_tcode(p.header) == OHCI1394_phy_tcode) p.ack = ACK_COMPLETE; /* @@ -1354,7 +1354,7 @@ static int at_context_queue_packet(struct context *ctx, * accordingly. */ - tcode = (packet->header[0] >> 4) & 0x0f; + tcode = async_header_get_tcode(packet->header); header = (__le32 *) &d[1]; switch (tcode) { case TCODE_WRITE_QUADLET_REQUEST: -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 09:23:31
|
In both core and 1394 OHCI driver, some hard-coded values and macros are used to serialize/deserialize the header for asynchronous packets. It is inconvenient to reuse them. This commit adds some helper inline functions with their tests for the purpose. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/.kunitconfig | 1 + drivers/firewire/Kconfig | 16 + drivers/firewire/Makefile | 3 + drivers/firewire/packet-header-definitions.h | 168 ++++++ drivers/firewire/packet-serdes-test.c | 538 +++++++++++++++++++ 5 files changed, 726 insertions(+) create mode 100644 drivers/firewire/packet-header-definitions.h create mode 100644 drivers/firewire/packet-serdes-test.c diff --git a/drivers/firewire/.kunitconfig b/drivers/firewire/.kunitconfig index 76444a2d5e12..60d9e7c35417 100644 --- a/drivers/firewire/.kunitconfig +++ b/drivers/firewire/.kunitconfig @@ -3,3 +3,4 @@ CONFIG_PCI=y CONFIG_FIREWIRE=y CONFIG_FIREWIRE_KUNIT_UAPI_TEST=y CONFIG_FIREWIRE_KUNIT_DEVICE_ATTRIBUTE_TEST=y +CONFIG_FIREWIRE_KUNIT_PACKET_SERDES_TEST=y diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig index 552a39df8cbd..869598b20e3a 100644 --- a/drivers/firewire/Kconfig +++ b/drivers/firewire/Kconfig @@ -50,6 +50,22 @@ config FIREWIRE_KUNIT_DEVICE_ATTRIBUTE_TEST For more information on KUnit and unit tests in general, refer to the KUnit documentation in Documentation/dev-tools/kunit/. +config FIREWIRE_KUNIT_PACKET_SERDES_TEST + tristate "KUnit tests for packet serialization/deserialization" if !KUNIT_ALL_TESTS + depends on FIREWIRE && KUNIT + default KUNIT_ALL_TESTS + help + This builds the KUnit tests for packet serialization and + deserialization. + + 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 b24b2879ac34..bbde29a0fba6 100644 --- a/drivers/firewire/Makefile +++ b/drivers/firewire/Makefile @@ -17,4 +17,7 @@ obj-$(CONFIG_FIREWIRE_NOSY) += nosy.o obj-$(CONFIG_PROVIDE_OHCI1394_DMA_INIT) += init_ohci1394_dma.o firewire-uapi-test-objs += uapi-test.o +firewire-packet-serdes-test-objs += packet-serdes-test.o + obj-$(CONFIG_FIREWIRE_KUNIT_UAPI_TEST) += firewire-uapi-test.o +obj-$(CONFIG_FIREWIRE_KUNIT_PACKET_SERDES_TEST) += firewire-packet-serdes-test.o diff --git a/drivers/firewire/packet-header-definitions.h b/drivers/firewire/packet-header-definitions.h new file mode 100644 index 000000000000..83e550427706 --- /dev/null +++ b/drivers/firewire/packet-header-definitions.h @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// +// packet-header-definitions.h - The definitions of header fields for IEEE 1394 packet. +// +// Copyright (c) 2024 Takashi Sakamoto + +#ifndef _FIREWIRE_PACKET_HEADER_DEFINITIONS_H +#define _FIREWIRE_PACKET_HEADER_DEFINITIONS_H + +#define ASYNC_HEADER_QUADLET_COUNT 4 + +#define ASYNC_HEADER_Q0_DESTINATION_SHIFT 16 +#define ASYNC_HEADER_Q0_DESTINATION_MASK 0xffff0000 +#define ASYNC_HEADER_Q0_TLABEL_SHIFT 10 +#define ASYNC_HEADER_Q0_TLABEL_MASK 0x0000fc00 +#define ASYNC_HEADER_Q0_RETRY_SHIFT 8 +#define ASYNC_HEADER_Q0_RETRY_MASK 0x00000300 +#define ASYNC_HEADER_Q0_TCODE_SHIFT 4 +#define ASYNC_HEADER_Q0_TCODE_MASK 0x000000f0 +#define ASYNC_HEADER_Q0_PRIORITY_SHIFT 0 +#define ASYNC_HEADER_Q0_PRIORITY_MASK 0x0000000f +#define ASYNC_HEADER_Q1_SOURCE_SHIFT 16 +#define ASYNC_HEADER_Q1_SOURCE_MASK 0xffff0000 +#define ASYNC_HEADER_Q1_RCODE_SHIFT 12 +#define ASYNC_HEADER_Q1_RCODE_MASK 0x0000f000 +#define ASYNC_HEADER_Q1_RCODE_SHIFT 12 +#define ASYNC_HEADER_Q1_RCODE_MASK 0x0000f000 +#define ASYNC_HEADER_Q1_OFFSET_HIGH_SHIFT 0 +#define ASYNC_HEADER_Q1_OFFSET_HIGH_MASK 0x0000ffff +#define ASYNC_HEADER_Q3_DATA_LENGTH_SHIFT 16 +#define ASYNC_HEADER_Q3_DATA_LENGTH_MASK 0xffff0000 +#define ASYNC_HEADER_Q3_EXTENDED_TCODE_SHIFT 0 +#define ASYNC_HEADER_Q3_EXTENDED_TCODE_MASK 0x0000ffff + +static inline unsigned int async_header_get_destination(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[0] & ASYNC_HEADER_Q0_DESTINATION_MASK) >> ASYNC_HEADER_Q0_DESTINATION_SHIFT; +} + +static inline unsigned int async_header_get_tlabel(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[0] & ASYNC_HEADER_Q0_TLABEL_MASK) >> ASYNC_HEADER_Q0_TLABEL_SHIFT; +} + +static inline unsigned int async_header_get_retry(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[0] & ASYNC_HEADER_Q0_RETRY_MASK) >> ASYNC_HEADER_Q0_RETRY_SHIFT; +} + +static inline unsigned int async_header_get_tcode(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[0] & ASYNC_HEADER_Q0_TCODE_MASK) >> ASYNC_HEADER_Q0_TCODE_SHIFT; +} + +static inline unsigned int async_header_get_priority(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[0] & ASYNC_HEADER_Q0_PRIORITY_MASK) >> ASYNC_HEADER_Q0_PRIORITY_SHIFT; +} + +static inline unsigned int async_header_get_source(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[1] & ASYNC_HEADER_Q1_SOURCE_MASK) >> ASYNC_HEADER_Q1_SOURCE_SHIFT; +} + +static inline unsigned int async_header_get_rcode(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[1] & ASYNC_HEADER_Q1_RCODE_MASK) >> ASYNC_HEADER_Q1_RCODE_SHIFT; +} + +static inline u64 async_header_get_offset(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + u32 hi = (header[1] & ASYNC_HEADER_Q1_OFFSET_HIGH_MASK) >> ASYNC_HEADER_Q1_OFFSET_HIGH_SHIFT; + return (((u64)hi) << 32) | ((u64)header[2]); +} + +static inline u32 async_header_get_quadlet_data(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return header[3]; +} + +static inline unsigned int async_header_get_data_length(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[3] & ASYNC_HEADER_Q3_DATA_LENGTH_MASK) >> ASYNC_HEADER_Q3_DATA_LENGTH_SHIFT; +} + +static inline unsigned int async_header_get_extended_tcode(const u32 header[ASYNC_HEADER_QUADLET_COUNT]) +{ + return (header[3] & ASYNC_HEADER_Q3_EXTENDED_TCODE_MASK) >> ASYNC_HEADER_Q3_EXTENDED_TCODE_SHIFT; +} + +static inline void async_header_set_destination(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int destination) +{ + header[0] &= ~ASYNC_HEADER_Q0_DESTINATION_MASK; + header[0] |= (((u32)destination) << ASYNC_HEADER_Q0_DESTINATION_SHIFT) & ASYNC_HEADER_Q0_DESTINATION_MASK; +} + +static inline void async_header_set_tlabel(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int tlabel) +{ + header[0] &= ~ASYNC_HEADER_Q0_TLABEL_MASK; + header[0] |= (((u32)tlabel) << ASYNC_HEADER_Q0_TLABEL_SHIFT) & ASYNC_HEADER_Q0_TLABEL_MASK; +} + +static inline void async_header_set_retry(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int retry) +{ + header[0] &= ~ASYNC_HEADER_Q0_RETRY_MASK; + header[0] |= (((u32)retry) << ASYNC_HEADER_Q0_RETRY_SHIFT) & ASYNC_HEADER_Q0_RETRY_MASK; +} + +static inline void async_header_set_tcode(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int tcode) +{ + header[0] &= ~ASYNC_HEADER_Q0_TCODE_MASK; + header[0] |= (((u32)tcode) << ASYNC_HEADER_Q0_TCODE_SHIFT) & ASYNC_HEADER_Q0_TCODE_MASK; +} + +static inline void async_header_set_priority(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int priority) +{ + header[0] &= ~ASYNC_HEADER_Q0_PRIORITY_MASK; + header[0] |= (((u32)priority) << ASYNC_HEADER_Q0_PRIORITY_SHIFT) & ASYNC_HEADER_Q0_PRIORITY_MASK; +} + + +static inline void async_header_set_source(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int source) +{ + header[1] &= ~ASYNC_HEADER_Q1_SOURCE_MASK; + header[1] |= (((u32)source) << ASYNC_HEADER_Q1_SOURCE_SHIFT) & ASYNC_HEADER_Q1_SOURCE_MASK; +} + +static inline void async_header_set_rcode(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int rcode) +{ + header[1] &= ~ASYNC_HEADER_Q1_RCODE_MASK; + header[1] |= (((u32)rcode) << ASYNC_HEADER_Q1_RCODE_SHIFT) & ASYNC_HEADER_Q1_RCODE_MASK; +} + +static inline void async_header_set_offset(u32 header[ASYNC_HEADER_QUADLET_COUNT], u64 offset) +{ + u32 hi = (u32)(offset >> 32); + header[1] &= ~ASYNC_HEADER_Q1_OFFSET_HIGH_MASK; + header[1] |= (hi << ASYNC_HEADER_Q1_OFFSET_HIGH_SHIFT) & ASYNC_HEADER_Q1_OFFSET_HIGH_MASK; + header[2] = (u32)(offset & 0x00000000ffffffff); +} + +static inline void async_header_set_quadlet_data(u32 header[ASYNC_HEADER_QUADLET_COUNT], u32 quadlet_data) +{ + header[3] = quadlet_data; +} + +static inline void async_header_set_data_length(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int data_length) +{ + header[3] &= ~ASYNC_HEADER_Q3_DATA_LENGTH_MASK; + header[3] |= (((u32)data_length) << ASYNC_HEADER_Q3_DATA_LENGTH_SHIFT) & ASYNC_HEADER_Q3_DATA_LENGTH_MASK; +} + +static inline void async_header_set_extended_tcode(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int extended_tcode) +{ + header[3] &= ~ASYNC_HEADER_Q3_EXTENDED_TCODE_MASK; + header[3] |= (((u32)extended_tcode) << ASYNC_HEADER_Q3_EXTENDED_TCODE_SHIFT) & ASYNC_HEADER_Q3_EXTENDED_TCODE_MASK; +} + +#endif // _FIREWIRE_PACKET_HEADER_DEFINITIONS_H diff --git a/drivers/firewire/packet-serdes-test.c b/drivers/firewire/packet-serdes-test.c new file mode 100644 index 000000000000..299e9f908463 --- /dev/null +++ b/drivers/firewire/packet-serdes-test.c @@ -0,0 +1,538 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// +// packet-serdes-test.c - An application of Kunit to check serialization/deserialization of packets +// defined by IEEE 1394. +// +// Copyright (c) 2024 Takashi Sakamoto + +#include <kunit/test.h> + +#include <linux/firewire-constants.h> + +#include "packet-header-definitions.h" + +static void serialize_async_header_common(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id) +{ + async_header_set_destination(header, dst_id); + async_header_set_tlabel(header, tlabel); + async_header_set_retry(header, retry); + async_header_set_tcode(header, tcode); + async_header_set_priority(header, priority); + async_header_set_source(header, src_id); +} + +static void serialize_async_header_request(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id, u64 offset) +{ + serialize_async_header_common(header, dst_id, tlabel, retry, tcode, priority, src_id); + async_header_set_offset(header, offset); +} + +static void serialize_async_header_quadlet_request(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id, + u64 offset) +{ + serialize_async_header_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset); +} + +static void serialize_async_header_block_request(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id, + u64 offset, unsigned int data_length, + unsigned int extended_tcode) +{ + serialize_async_header_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset); + async_header_set_data_length(header, data_length); + async_header_set_extended_tcode(header, extended_tcode); +} + +static void serialize_async_header_response(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id, + unsigned int rcode) +{ + serialize_async_header_common(header, dst_id, tlabel, retry, tcode, priority, src_id); + async_header_set_rcode(header, rcode); +} + +static void serialize_async_header_quadlet_response(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id, + unsigned int rcode) +{ + serialize_async_header_response(header, dst_id, tlabel, retry, tcode, priority, src_id, + rcode); +} + +static void serialize_async_header_block_response(u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int dst_id, unsigned int tlabel, + unsigned int retry, unsigned int tcode, + unsigned int priority, unsigned int src_id, + unsigned int rcode, unsigned int data_length, + unsigned int extended_tcode) +{ + serialize_async_header_response(header, dst_id, tlabel, retry, tcode, priority, src_id, + rcode); + async_header_set_data_length(header, data_length); + async_header_set_extended_tcode(header, extended_tcode); +} + +static void deserialize_async_header_common(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id) +{ + *dst_id = async_header_get_destination(header); + *tlabel = async_header_get_tlabel(header); + *retry = async_header_get_retry(header); + *tcode = async_header_get_tcode(header); + *priority = async_header_get_priority(header); + *src_id = async_header_get_source(header); +} + +static void deserialize_async_header_request(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id, + u64 *offset) +{ + deserialize_async_header_common(header, dst_id, tlabel, retry, tcode, priority, src_id); + *offset = async_header_get_offset(header); +} + +static void deserialize_async_header_quadlet_request(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id, + u64 *offset) +{ + deserialize_async_header_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset); +} + +static void deserialize_async_header_block_request(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id, + u64 *offset, + unsigned int *data_length, + unsigned int *extended_tcode) +{ + deserialize_async_header_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset); + *data_length = async_header_get_data_length(header); + *extended_tcode = async_header_get_extended_tcode(header); +} + +static void deserialize_async_header_response(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id, + unsigned int *rcode) +{ + deserialize_async_header_common(header, dst_id, tlabel, retry, tcode, priority, src_id); + *rcode = async_header_get_rcode(header); +} + +static void deserialize_async_header_quadlet_response(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id, + unsigned int *rcode) +{ + deserialize_async_header_response(header, dst_id, tlabel, retry, tcode, priority, src_id, rcode); +} + +static void deserialize_async_header_block_response(const u32 header[ASYNC_HEADER_QUADLET_COUNT], + unsigned int *dst_id, unsigned int *tlabel, + unsigned int *retry, unsigned int *tcode, + unsigned int *priority, unsigned int *src_id, + unsigned int *rcode, unsigned int *data_length, + unsigned int *extended_tcode) +{ + deserialize_async_header_response(header, dst_id, tlabel, retry, tcode, priority, src_id, rcode); + *data_length = async_header_get_data_length(header); + *extended_tcode = async_header_get_extended_tcode(header); +} + +static void test_async_header_write_quadlet_request(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc05100, + 0xffc1ffff, + 0xf0000234, + 0x1f0000c0, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + u64 offset; + u32 quadlet_data; + + deserialize_async_header_quadlet_request(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &offset); + quadlet_data = async_header_get_quadlet_data(expected); + + KUNIT_EXPECT_EQ(test, 0xffc0, dst_id); + KUNIT_EXPECT_EQ(test, 0x14, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_WRITE_QUADLET_REQUEST, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc1, src_id); + KUNIT_EXPECT_EQ(test, 0xfffff0000234, offset); + KUNIT_EXPECT_EQ(test, 0x1f0000c0, quadlet_data); + + serialize_async_header_quadlet_request(header, dst_id, tlabel, retry, tcode, priority, + src_id, offset); + async_header_set_quadlet_data(header, quadlet_data); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_write_block_request(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc06510, + 0xffc1ecc0, + 0x00000000, + 0x00180000, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + u64 offset; + unsigned int data_length; + unsigned int extended_tcode; + + deserialize_async_header_block_request(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &offset, &data_length, + &extended_tcode); + + KUNIT_EXPECT_EQ(test, 0xffc0, dst_id); + KUNIT_EXPECT_EQ(test, 0x19, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_WRITE_BLOCK_REQUEST, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc1, src_id); + KUNIT_EXPECT_EQ(test, 0xecc000000000, offset); + KUNIT_EXPECT_EQ(test, 0x0018, data_length); + KUNIT_EXPECT_EQ(test, 0x0000, extended_tcode); + + serialize_async_header_block_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset, data_length, extended_tcode); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_write_response(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc15120, + 0xffc00000, + 0x00000000, + 0x00000000, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + unsigned int rcode; + + deserialize_async_header_quadlet_response(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &rcode); + + KUNIT_EXPECT_EQ(test, 0xffc1, dst_id); + KUNIT_EXPECT_EQ(test, 0x14, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_WRITE_RESPONSE, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc0, src_id); + KUNIT_EXPECT_EQ(test, RCODE_COMPLETE, rcode); + + serialize_async_header_quadlet_response(header, dst_id, tlabel, retry, tcode, priority, + src_id, rcode); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected) - sizeof(expected[0])); +} + +static void test_async_header_read_quadlet_request(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc0f140, + 0xffc1ffff, + 0xf0000984, + 0x00000000, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + u64 offset; + + deserialize_async_header_quadlet_request(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &offset); + + KUNIT_EXPECT_EQ(test, 0xffc0, dst_id); + KUNIT_EXPECT_EQ(test, 0x3c, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_READ_QUADLET_REQUEST, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc1, src_id); + KUNIT_EXPECT_EQ(test, 0xfffff0000984, offset); + + serialize_async_header_quadlet_request(header, dst_id, tlabel, retry, tcode, priority, + src_id, offset); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_read_quadlet_response(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc1f160, + 0xffc00000, + 0x00000000, + 0x00000180, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + unsigned int rcode; + u32 quadlet_data; + + deserialize_async_header_quadlet_response(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &rcode); + quadlet_data = async_header_get_quadlet_data(expected); + + KUNIT_EXPECT_EQ(test, 0xffc1, dst_id); + KUNIT_EXPECT_EQ(test, 0x3c, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_READ_QUADLET_RESPONSE, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc0, src_id); + KUNIT_EXPECT_EQ(test, RCODE_COMPLETE, rcode); + KUNIT_EXPECT_EQ(test, 0x00000180, quadlet_data); + + serialize_async_header_quadlet_response(header, dst_id, tlabel, retry, tcode, priority, + src_id, rcode); + async_header_set_quadlet_data(header, quadlet_data); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_read_block_request(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc0e150, + 0xffc1ffff, + 0xf0000400, + 0x00200000, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + u64 offset; + unsigned int data_length; + unsigned int extended_tcode; + + deserialize_async_header_block_request(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &offset, &data_length, + &extended_tcode); + + KUNIT_EXPECT_EQ(test, 0xffc0, dst_id); + KUNIT_EXPECT_EQ(test, 0x38, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_READ_BLOCK_REQUEST, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc1, src_id); + KUNIT_EXPECT_EQ(test, 0xfffff0000400, offset); + KUNIT_EXPECT_EQ(test, 0x0020, data_length); + KUNIT_EXPECT_EQ(test, 0x0000, extended_tcode); + + serialize_async_header_block_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset, data_length, extended_tcode); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_read_block_response(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc1e170, + 0xffc00000, + 0x00000000, + 0x00200000, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + unsigned int rcode; + unsigned int data_length; + unsigned int extended_tcode; + + deserialize_async_header_block_response(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &rcode, &data_length, + &extended_tcode); + + KUNIT_EXPECT_EQ(test, 0xffc1, dst_id); + KUNIT_EXPECT_EQ(test, 0x38, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_READ_BLOCK_RESPONSE, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc0, src_id); + KUNIT_EXPECT_EQ(test, RCODE_COMPLETE, rcode); + KUNIT_EXPECT_EQ(test, 0x0020, data_length); + KUNIT_EXPECT_EQ(test, 0x0000, extended_tcode); + + serialize_async_header_block_response(header, dst_id, tlabel, retry, tcode, priority, + src_id, rcode, data_length, extended_tcode); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_lock_request(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc02d90, + 0xffc1ffff, + 0xf0000984, + 0x00080002, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + u64 offset; + unsigned int data_length; + unsigned int extended_tcode; + + deserialize_async_header_block_request(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &offset, &data_length, + &extended_tcode); + + KUNIT_EXPECT_EQ(test, 0xffc0, dst_id); + KUNIT_EXPECT_EQ(test, 0x0b, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_LOCK_REQUEST, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc1, src_id); + KUNIT_EXPECT_EQ(test, 0xfffff0000984, offset); + KUNIT_EXPECT_EQ(test, 0x0008, data_length); + KUNIT_EXPECT_EQ(test, EXTCODE_COMPARE_SWAP, extended_tcode); + + serialize_async_header_block_request(header, dst_id, tlabel, retry, tcode, priority, src_id, + offset, data_length, extended_tcode); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + +static void test_async_header_lock_response(struct kunit *test) +{ + static const u32 expected[ASYNC_HEADER_QUADLET_COUNT] = { + 0xffc12db0, + 0xffc00000, + 0x00000000, + 0x00040002, + }; + u32 header[ASYNC_HEADER_QUADLET_COUNT] = {0, 0, 0, 0}; + + unsigned int dst_id; + unsigned int tlabel; + unsigned int retry; + unsigned int tcode; + unsigned int priority; + unsigned int src_id; + unsigned int rcode; + unsigned int data_length; + unsigned int extended_tcode; + + deserialize_async_header_block_response(expected, &dst_id, &tlabel, &retry, &tcode, + &priority, &src_id, &rcode, &data_length, + &extended_tcode); + + KUNIT_EXPECT_EQ(test, 0xffc1, dst_id); + KUNIT_EXPECT_EQ(test, 0x0b, tlabel); + KUNIT_EXPECT_EQ(test, 0x01, retry); + KUNIT_EXPECT_EQ(test, TCODE_LOCK_RESPONSE, tcode); + KUNIT_EXPECT_EQ(test, 0x00, priority); + KUNIT_EXPECT_EQ(test, 0xffc0, src_id); + KUNIT_EXPECT_EQ(test, RCODE_COMPLETE, rcode); + KUNIT_EXPECT_EQ(test, 0x0004, data_length); + KUNIT_EXPECT_EQ(test, EXTCODE_COMPARE_SWAP, extended_tcode); + + serialize_async_header_block_response(header, dst_id, tlabel, retry, tcode, priority, + src_id, rcode, data_length, extended_tcode); + + KUNIT_EXPECT_MEMEQ(test, header, expected, sizeof(expected)); +} + + +static struct kunit_case packet_serdes_test_cases[] = { + KUNIT_CASE(test_async_header_write_quadlet_request), + KUNIT_CASE(test_async_header_write_block_request), + KUNIT_CASE(test_async_header_write_response), + KUNIT_CASE(test_async_header_read_quadlet_request), + KUNIT_CASE(test_async_header_read_quadlet_response), + KUNIT_CASE(test_async_header_read_block_request), + KUNIT_CASE(test_async_header_read_block_response), + KUNIT_CASE(test_async_header_lock_request), + KUNIT_CASE(test_async_header_lock_response), + {} +}; + +static struct kunit_suite packet_serdes_test_suite = { + .name = "firewire-packet-serdes", + .test_cases = packet_serdes_test_cases, +}; +kunit_test_suite(packet_serdes_test_suite); + +MODULE_LICENSE("GPL"); -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-04-18 08:45:03
|
Hi, On Wed, Apr 17, 2024 at 11:46:47AM -0400, Thanassis Avgerinos wrote: > Hi Takashi, > > Apologies for the delay, and thank you for the super fast response! Please > find attached a possible patch for this issue. It should be > self-explanatory but you may have a better suggestion on how to handle it. > Please let me know and I'm happy to make changes as needed. > > After this is patched, would it be possible to issue a CVE for it? Would > you be the best person to help me with this? > > Best, > Thanassis Thanks for the patch to fix the TODO, however would I ask you to send it again with your `Sign-off-by:` tag? It is mandatory in usual kernel development process, and I can not apply any modification about it, with the respect to copyright, sorry. I have a nitpick about the return value at the case that the user process provides the insufficient size of buffer against the data stored in kernel space. In the case, the typical UNIX-like kernel returns zero instead of -EINVAL. We should follow to the convention if we have no specific reason, in my opinion. > From f7ee97fabe1519225ba30fd9454344d0a75f4d94 Mon Sep 17 00:00:00 2001 > From: Thanassis Avgerinos <tha...@gm...> > Date: Wed, 17 Apr 2024 11:30:02 -0400 > Subject: [PATCH] firewire: nosy: ensure user_length is taken into account when > fetching packet contents > > Ensure that packet_buffer_get respects the user_length provided. If > the length of the head packet exceeds the user_length, packet_buffer_get > will now return -EINVAL to signify to the user that a larger data > buffer is required. Helps prevent user space overflows. > --- > drivers/firewire/nosy.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/firewire/nosy.c b/drivers/firewire/nosy.c > index b0d671db178a..b1dccf3ba8f9 100644 > --- a/drivers/firewire/nosy.c > +++ b/drivers/firewire/nosy.c > @@ -148,10 +148,12 @@ packet_buffer_get(struct client *client, char __user *data, size_t user_length) > if (atomic_read(&buffer->size) == 0) > return -ENODEV; > > - /* FIXME: Check length <= user_length. */ > + length = buffer->head->length; > + > + if (length > user_length) > + return -EINVAL; > > end = buffer->data + buffer->capacity; > - length = buffer->head->length; > > if (&buffer->head->data[length] < end) { > if (copy_to_user(data, buffer->head->data, length)) > -- > 2.23.0 Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-04 12:03:50
|
Hi, Thanks for the patch. The replacement of tasklet with workqueue is one of my TODO list, and the change would be helpful. On Wed, Apr 03, 2024 at 02:45:58PM +0000, Allen Pais wrote: > The only generic interface to execute asynchronously in the BH context is > tasklet; however, it's marked deprecated and has some design flaws. To > replace tasklets, BH workqueue support was recently added. A BH workqueue > behaves similarly to regular workqueues except that the queued work items > are executed in the BH context. > > This patch converts drivers/firewire/* from tasklet to BH workqueue. > > Based on the work done by Tejun Heo <tj...@ke...> > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git disable_work-v1 > > Changes are tested by: @recallmenot > (https://github.com/allenpais/for-6.9-bh-conversions/issues/1) > > Signed-off-by: Allen Pais <all...@gm...> > --- > drivers/firewire/ohci.c | 54 ++++++++++++++++++++--------------------- > 1 file changed, 26 insertions(+), 28 deletions(-) However, the changes look to be too early, since some kernel APIs are referred from the change but locate just in Heo's tree. Thus, any application of the patch brings build failure, like: ``` drivers/firewire/ohci.c: In function ‘at_context_flush’: drivers/firewire/ohci.c:1463:9: error: implicit declaration of function ‘disable_work_sync’; did you mean ‘disable_irq_nosync’? [-Werror=implicit-function-declaration] 1463 | disable_work_sync(&ctx->work); | ^~~~~~~~~~~~~~~~~ | disable_irq_nosync drivers/firewire/ohci.c:1468:9: error: implicit declaration of function ‘enable_and_queue_work’ [-Werror=implicit-function-declaration] 1468 | enable_and_queue_work(system_bh_wq, &ctx->work); | ^~~~~~~~~~~~~~~~~~~~~ ``` In my humble opinion, the change proposal should be posted after merging Heo's work, to prevent developers and users from being puzzled. Furthermore, any kind of report for the performance test is preferable. Especially, in FireWire subsystem, 1394 OHCI IT/IR contexts can be processed by both tasklet and process (e.g. ioctl), thus the exclusive control of workqueue for the contexts is important between them. I wish it is done successfully by the new pair of enabling/disabling workqueue API, and need more information about it. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-02 14:33:09
|
Hi Thanassis, Thanks for your contacting to me about the issue. On Tue, Apr 2, 2024, at 22:56, Thanassis Avgerinos wrote: > Dear Sakamoto-san, > > I apologize for the email - I am sure you are very busy. Thank you for > all the work to help maintain firewire in the Linux trunk. > > I will be brief to not waste your time. I am contacting you to report a > possible security issue in firewire/nosy.c:L151 which reads: > > ``` > /* FIXME: Check length <= user_length. */ > > end = buffer->data + buffer->capacity; > length = buffer->head->length; > > if (&buffer->head->data[length] < end) { > if (copy_to_user(data, buffer->head->data, length)) > return -EFAULT; > ``` > > Note that the user-supplied length is not respected at all when reading > bytes from the wire which can lead to a number of issues (including > security ones such as overflows). > > I believe this is potentially serious but also quite fixable. Would you > accept a patch? Should I contact the security team instead? > > Thank you for your time. I'm pleased to review your fix for the issue;) I don't mind that you post it just to me, since any mailing list demands you to subscribe it in advance when posting, and it is a bit cumbersome just to send patches. Additionally, I don't mind the patch format as long as it includes enough fractions to fix the issue. In the case, I'll revise it for the complete patches, alternatively. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-01 23:55:31
|
On Sun, Mar 31, 2024 at 10:50:33PM +0900, Takashi Sakamoto wrote: > Hi, > > The pair of pci_enable_msi() and pci_disable_msi() is deprecated > nowadays, while it is still used in 1394 OHCI driver in FireWire > subsystem. This series of changes is to use modern API sets for the > driver. > > > Takashi Sakamoto (4): > Revert "firewire: ohci: use devres for requested IRQ" > firewire: ohci: replace request_irq() with request_threaded_irq() > firewire: ohci: obsolete usage of deprecated API for MSI > firewire: ohci: use pci_irq_vector() to retrieve allocated interrupt > line > > drivers/firewire/ohci.c | 35 +++++++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 10 deletions(-) Applied to for-next branch for v6.10. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-01 12:18:19
|
Hi Adam, On Mon, Mar 25, 2024 at 09:58:28AM +0900, Takashi Sakamoto wrote: > Hi, > > On Wed, Mar 20, 2024 at 02:15:19AM -0700, Adam Goldman wrote: > > In the FireWire OHCI interrupt handler, if a bus reset interrupt has > > occurred, mask bus reset interrupts until bus_reset_work has serviced and > > cleared the interrupt. > > > > Normally, we always leave bus reset interrupts masked. We infer the bus > > reset from the self-ID interrupt that happens shortly thereafter. A > > scenario where we unmask bus reset interrupts was introduced in 2008 in > > a007bb857e0b26f5d8b73c2ff90782d9c0972620: If > > OHCI_PARAM_DEBUG_BUSRESETS (8) is set in the debug parameter bitmask, we > > will unmask bus reset interrupts so we can log them. > > > > irq_handler logs the bus reset interrupt. However, we can't clear the bus > > reset event flag in irq_handler, because we won't service the event until > > later. irq_handler exits with the event flag still set. If the > > corresponding interrupt is still unmasked, the first bus reset will > > usually freeze the system due to irq_handler being called again each > > time it exits. This freeze can be reproduced by loading firewire_ohci > > with "modprobe firewire_ohci debug=-1" (to enable all debugging output). > > Apparently there are also some cases where bus_reset_work will get called > > soon enough to clear the event, and operation will continue normally. > > > > This freeze was first reported a few months after a007bb85 was committed, > > but until now it was never fixed. The debug level could safely be set > > to -1 through sysfs after the module was loaded, but this would be > > ineffectual in logging bus reset interrupts since they were only > > unmasked during initialization. > > > > irq_handler will now leave the event flag set but mask bus reset > > interrupts, so irq_handler won't be called again and there will be no > > freeze. If OHCI_PARAM_DEBUG_BUSRESETS is enabled, bus_reset_work will > > unmask the interrupt after servicing the event, so future interrupts > > will be caught as desired. > > > > As a side effect to this change, OHCI_PARAM_DEBUG_BUSRESETS can now be > > enabled through sysfs in addition to during initial module loading. > > However, when enabled through sysfs, logging of bus reset interrupts will > > be effective only starting with the second bus reset, after > > bus_reset_work has executed. > > > > Signed-off-by: Adam Goldman <ad...@po...> > > --- > > > > --- linux-6.8-rc1.orig/drivers/firewire/ohci.c 2024-01-21 14:11:32.000000000 -0800 > > +++ linux-6.8-rc1/drivers/firewire/ohci.c 2024-03-12 01:15:10.000000000 -0700 > > @@ -2060,6 +2060,8 @@ static void bus_reset_work(struct work_s > > > > ohci->generation = generation; > > reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset); > > + if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS) > > + reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset); > > > > if (ohci->quirks & QUIRK_RESET_PACKET) > > ohci->request_generation = generation; > > @@ -2125,12 +2127,14 @@ static irqreturn_t irq_handler(int irq, > > return IRQ_NONE; > > > > /* > > - * busReset and postedWriteErr must not be cleared yet > > + * busReset and postedWriteErr events must not be cleared yet > > * (OHCI 1.1 clauses 7.2.3.2 and 13.2.8.1) > > */ > > reg_write(ohci, OHCI1394_IntEventClear, > > event & ~(OHCI1394_busReset | OHCI1394_postedWriteErr)); > > log_irqs(ohci, event); > > + if (event & OHCI1394_busReset) > > + reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset); > > > > if (event & OHCI1394_selfIDComplete) > > queue_work(selfid_workqueue, &ohci->bus_reset_work); > > Thanks for the patch. I pushed topic branch[1] for it, since I'm > considering about whether to send it to stable and longterm releases. > > I had realized that the debug=8 for firewire-ohci module provides tons > of logs triggering by the irq handler, since the irq for bus reset is > not unmasked, so I rely on selfID events when debugging bus-reset. I have > few objections to the change. > > My concern is how much invasive it is. The unmasking is kept until > bus_reset_work() is executed in the workqueue. When considering about > the delay of workqueue (since it is a kind of schedulable task), many > irq events for bus reset is potentially skipped from the logging. Of > course, it is the aim of change. > > Let me take more time to evaluate the change, but I'm willing to send it > to upstream until -rc3 or -rc4, at least, if receiving no objections > from the others. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/log/?h=v6.9-firewire-mask-bus-reset-event-during-handling As long as I tested, the patch is enough good to suppress the spurious IRQ event. I'll send it to mainline in this weekend. I sent an additional patch[1] to handle the bus-reset event at the first time. I'd like you to review and test it as well, especially under your environment in which 1394:1995 and 1394a phys exist. [1] https://lore.kernel.org/lkml/202...@sa.../ Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-01 12:12:20
|
In the former commit, the spurious interrupt events are suppressed as possible, when masking the expected interrupts events for bus reset. The change was written to be less intrusive, thus it does not work at the first event of bus reset. However, it has few trouble to make it work at the first event. This commit is to mask the interrupt events as a default for the purpose. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 38d19410a2be..4e86205f81bf 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -2060,8 +2060,7 @@ static void bus_reset_work(struct work_struct *work) ohci->generation = generation; reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset); - if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS) - reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset); + reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset); if (ohci->quirks & QUIRK_RESET_PACKET) ohci->request_generation = generation; @@ -2133,6 +2132,7 @@ static irqreturn_t irq_handler(int irq, void *data) reg_write(ohci, OHCI1394_IntEventClear, event & ~(OHCI1394_busReset | OHCI1394_postedWriteErr)); log_irqs(ohci, event); + // The flag is masked again at bus_reset_work() scheduled by selfID event. if (event & OHCI1394_busReset) reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset); @@ -2472,9 +2472,8 @@ static int ohci_enable(struct fw_card *card, OHCI1394_cycleInconsistent | OHCI1394_unrecoverableError | OHCI1394_cycleTooLong | - OHCI1394_masterIntEnable; - if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS) - irqs |= OHCI1394_busReset; + OHCI1394_masterIntEnable | + OHCI1394_busReset; reg_write(ohci, OHCI1394_IntMaskSet, irqs); reg_write(ohci, OHCI1394_HCControlSet, -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-03-31 13:51:04
|
The pci_irq_vector() is available to retrieve the allocated interrupt line instead of the direct access to the member of device structure. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index c14e6efaba36..f3b610f86aac 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3623,7 +3623,7 @@ static int pci_probe(struct pci_dev *dev, struct fw_ohci *ohci; u32 bus_options, max_receive, link_speed, version; u64 guid; - int i, flags, err; + int i, flags, irq, err; size_t size; if (dev->vendor == PCI_VENDOR_ID_PINNACLE_SYSTEMS) { @@ -3754,12 +3754,17 @@ static int pci_probe(struct pci_dev *dev, err = pci_alloc_irq_vectors(dev, 1, 1, flags); if (err < 0) return err; + irq = pci_irq_vector(dev, 0); + if (irq < 0) { + err = irq; + goto fail_msi; + } - err = request_threaded_irq(dev->irq, irq_handler, NULL, + err = request_threaded_irq(irq, irq_handler, NULL, 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); + ohci_err(ohci, "failed to allocate interrupt %d\n", irq); goto fail_msi; } @@ -3779,7 +3784,7 @@ static int pci_probe(struct pci_dev *dev, return 0; fail_irq: - free_irq(dev->irq, ohci); + free_irq(irq, ohci); fail_msi: pci_free_irq_vectors(dev); @@ -3789,6 +3794,7 @@ static int pci_probe(struct pci_dev *dev, static void pci_remove(struct pci_dev *dev) { struct fw_ohci *ohci = pci_get_drvdata(dev); + int irq; /* * If the removal is happening from the suspend state, LPS won't be @@ -3808,7 +3814,9 @@ static void pci_remove(struct pci_dev *dev) software_reset(ohci); - free_irq(dev->irq, ohci); + irq = pci_irq_vector(dev, 0); + if (irq >= 0) + free_irq(irq, ohci); pci_free_irq_vectors(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-03-31 13:51:03
|
The usage of the pair of pci_enable_msi() and pci_disable_msi() is deprecated. This commit uses the preferred pair of API for the purpose. The call of pci_alloc_irq_vectors() can have a subeffect to change the return value of pci_dev_msi_enabled(). Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index ec02708c5f05..c14e6efaba36 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3623,7 +3623,7 @@ static int pci_probe(struct pci_dev *dev, struct fw_ohci *ohci; u32 bus_options, max_receive, link_speed, version; u64 guid; - int i, err; + int i, flags, err; size_t size; if (dev->vendor == PCI_VENDOR_ID_PINNACLE_SYSTEMS) { @@ -3748,8 +3748,13 @@ static int pci_probe(struct pci_dev *dev, guid = ((u64) reg_read(ohci, OHCI1394_GUIDHi) << 32) | reg_read(ohci, OHCI1394_GUIDLo); + flags = PCI_IRQ_INTX; if (!(ohci->quirks & QUIRK_NO_MSI)) - pci_enable_msi(dev); + flags |= PCI_IRQ_MSI; + err = pci_alloc_irq_vectors(dev, 1, 1, flags); + if (err < 0) + return err; + err = request_threaded_irq(dev->irq, irq_handler, NULL, pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED, ohci_driver_name, ohci); @@ -3776,7 +3781,7 @@ static int pci_probe(struct pci_dev *dev, fail_irq: free_irq(dev->irq, ohci); fail_msi: - pci_disable_msi(dev); + pci_free_irq_vectors(dev); return err; } @@ -3804,7 +3809,7 @@ static void pci_remove(struct pci_dev *dev) software_reset(ohci); free_irq(dev->irq, ohci); - pci_disable_msi(dev); + pci_free_irq_vectors(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); } -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-03-31 13:50:59
|
This reverts commit 5a95f1ded28691e69f7d6718c5dcbc149613d431. As long as allocating any device interrupt vector for MSI, it is inconvenient to utilize managed device resources for IRQ requesting. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 7bc71f4be64a..f18019c5d220 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3750,16 +3750,17 @@ static int pci_probe(struct pci_dev *dev, if (!(ohci->quirks & QUIRK_NO_MSI)) pci_enable_msi(dev); - 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) { + if (request_irq(dev->irq, irq_handler, + pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED, + ohci_driver_name, ohci)) { 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_msi; + goto fail_irq; version = reg_read(ohci, OHCI1394_Version) & 0x00ff00ff; ohci_notice(ohci, @@ -3772,8 +3773,9 @@ static int pci_probe(struct pci_dev *dev, return 0; + fail_irq: + free_irq(dev->irq, ohci); fail_msi: - devm_free_irq(&dev->dev, dev->irq, ohci); pci_disable_msi(dev); return err; @@ -3801,7 +3803,7 @@ static void pci_remove(struct pci_dev *dev) software_reset(ohci); - devm_free_irq(&dev->dev, dev->irq, ohci); + free_irq(dev->irq, ohci); pci_disable_msi(dev); dev_notice(&dev->dev, "removing fw-ohci device\n"); -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-03-31 13:50:58
|
Hi, The pair of pci_enable_msi() and pci_disable_msi() is deprecated nowadays, while it is still used in 1394 OHCI driver in FireWire subsystem. This series of changes is to use modern API sets for the driver. Takashi Sakamoto (4): Revert "firewire: ohci: use devres for requested IRQ" firewire: ohci: replace request_irq() with request_threaded_irq() firewire: ohci: obsolete usage of deprecated API for MSI firewire: ohci: use pci_irq_vector() to retrieve allocated interrupt line drivers/firewire/ohci.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-03-31 13:50:58
|
Nowadays request_irq() is a wrapper of request_threaded_irq(). The IRQ handler of 1394 ohci driver has never been optimized yet, while it is a good preparation for the future work to replace the latter. This commit replaces the former. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index f18019c5d220..ec02708c5f05 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -3750,11 +3750,11 @@ 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 = request_threaded_irq(dev->irq, irq_handler, NULL, + 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; } -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-03-29 08:12:37
|
Hi Lukas, On Fri, Mar 29, 2024 at 05:41:16AM +0100, Lukas Wunner wrote: > On Thu, Mar 28, 2024 at 02:42:01PM -0600, Alex Williamson wrote: > > On Wed, 27 Mar 2024 10:01:19 -0500 Bjorn Helgaas <he...@ke...> wrote: > > The original patch proposed for this gave me the impression that this > > was a device used on various old Mac systems, not likely applicable to > > a general purpose plug-in card. > > I'm still using one of those "old Mac systems" as my daily driver. > > Just checked the ACPI tables and there's an FPEN method below the > FRWR device which toggles GPIO 48 on the PCH. Checked the schematics > as well and GPIO 48 is marked FW_PWR_EN. The GPIO controls load > switches which cut power to the FW643 chip when nothing is connected. > > Also, FW_PWR_EN feeds into an SLG4AP016V chip where it seems to > internally gate FW_CLKREQ_L. > > I'm guessing the driver may need to call the FPEN ACPI method after > issuing a SBR to force the chip on (or perhaps first off, then on) > and thereby re-enable Clock Request. > > It's a pity the ohci.c driver doesn't seem to support runtime PM. > That would allow cutting power to the chip when nothing is connected > and thus increase battery life. The ACPI tables indicate that the > platform sends a notification when something is plugged in, so all > the necessary ingredients are there but we're not taking advantage > of them. > > Thanks, > > Lukas Yup. In both PCI drivers and unit drivers belonging to Linux FireWire subsystem, any type of runtime PM is not supported. If I integrate 1394 OHCI driver, I should implement all of the above in any callback of runtime PM, or the part of the above is already supported by any driver in parent PCI layer? Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-29 08:05:32
|
Hi, On Thu, Mar 28, 2024 at 06:35:29PM +0000, edmund.raile wrote: > So instead I ran this: > ``` > su -c 'echo 1 > /sys/devices/pci0000\:00/0000\:00\:1c.1/0000\:03\:00.0/reset' > ``` > Playback stopped immediately and could not be resumed. > > Then I received this trace: > > INFO: task alsa-sink-Firef:4110 blocked for more than 245 seconds. > Tainted: G W OE 6.6.10-1-MANJARO #1 > task:alsa-sink-Firef state:D stack:0 pid:4110 ppid:2657 flags:0x00000002 > Call Trace: > <TASK> > __schedule+0x3e7/0x1410 > ? tlb_batch_pages_flush+0x3d/0x70 > schedule+0x5e/0xd0 > schedule_timeout+0x151/0x160 > wait_for_completion+0x8a/0x160 > fw_run_transaction+0xe5/0x120 [firewire_core d9ff4eaf1ffb23a203d413e851f405323b49fec7] > ? __pfx_split_transaction_timeout_callback+0x10/0x10 [firewire_core d9ff4eaf1ffb23a203d413e851f405323b49fec7] > ? __pfx_transmit_complete_callback+0x10/0x10 [firewire_core d9ff4eaf1ffb23a203d413e851f405323b49fec7] > ? __pfx_transaction_callback+0x10/0x10 [firewire_core d9ff4eaf1ffb23a203d413e851f405323b49fec7] > snd_fw_transaction+0x70/0x110 [snd_firewire_lib 30b43a591db389bbc6be51459cb243ba1fe1e662] > ff800_finish_session+0x43/0x70 [snd_fireface 5f7f3f556960f4838886792be8e9c18aa5089b0a] > snd_ff_stream_stop_duplex+0x39/0x70 [snd_fireface 5f7f3f556960f4838886792be8e9c18aa5089b0a] > pcm_hw_free+0x3c/0x50 [snd_fireface 5f7f3f556960f4838886792be8e9c18aa5089b0a] > snd_pcm_common_ioctl+0xe28/0x12b0 [snd_pcm 24933227879438b755ef98bc4844113025f38cdf] > ? __seccomp_filter+0x32c/0x510 > ? __vm_munmap+0xbb/0x150 > snd_pcm_ioctl+0x2e/0x50 [snd_pcm 24933227879438b755ef98bc4844113025f38cdf] > __x64_sys_ioctl+0x94/0xd0 > do_syscall_64+0x5d/0x90 > ? syscall_exit_to_user_mode+0x2b/0x40 > ? do_syscall_64+0x6c/0x90 > ? do_syscall_64+0x6c/0x90 > entry_SYSCALL_64_after_hwframe+0x6e/0xd8 Please mind that current software stack to operate your device does not support this kind of operation, as I've already sent to you several times. Users should cancel any type of communication on IEEE 1394 bus, then unplug devices from the bus (or power them off), finally operate suspending. By the way, it is apart from PCI subsystem. Your change is now going to be reverted for v6.9. Regards Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-29 08:04:19
|
On Thu, Mar 28, 2024 at 04:23:02PM -0500, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhe...@go...> > > This reverts commit 29a43dc130ce65d365a8ea9e1cc4bc51005a353e. > > 29a43dc130ce ("PCI: Mark LSI FW643 to avoid bus reset") by Edmund was based > on the assumption that the LSI / Agere FW643 has a defect such that it > can't recover after a Secondary Bus Reset (SBR). > > But Takashi Sakamoto reported that SBR works fine on this same FW643 device > in an AMD Ryzen 5 2400G system, so apparently there is some other aspect of > Edmund's system that accounts for the issue. > > The down side of 29a43dc130ce is that when the FW643 is assigned to a VM, > avoiding the SBR means we leak data out of the VM. > > Revert 29a43dc130ce until we figure out a better solution. In the > meantime, we can use the sysfs "reset_method" interface to restrict the > available reset methods. > > Fixes: 29a43dc130ce ("PCI: Mark LSI FW643 to avoid bus reset") > Reported-by: Takashi Sakamoto <o-t...@sa...> > Link: https://lore.kernel.org/r/202...@sa... > Signed-off-by: Bjorn Helgaas <bhe...@go...> > --- > drivers/pci/quirks.c | 8 -------- > 1 file changed, 8 deletions(-) Reviewed-by: Takashi Sakamoto <o-t...@sa...> Thank you for the revert patch, and I apologize for the delay in catching up with the change. Please feel free to add me or the linux1394-devel list to CC of these kind of changes. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-26 13:19:16
|
Hi Bjorn, Thanks for your reply. On Mon, Mar 25, 2024 at 09:41:49AM -0500, Bjorn Helgaas wrote: > So even without this patch, you are able to pass the FW643 to a VM > with VFIO, and you don't see any issues caused by VFIO resetting the > device? Absolutely yes, at least in my VM, for recent years to maintain Linux FireWire subsystem and ALSA firewire stack. > Can you collect the output of: > > $ find /sys/devices -name reset_method | xargs grep . ``` $ sudo find /sys/devices -name reset_method | xargs grep . /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:09.0/0000:09:00.0/reset_method:bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:09.0/reset_method:pm /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:02.0/0000:06:00.0/reset_method:pm bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:02.0/reset_method:pm /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:05.0/0000:07:00.0/reset_method:bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:05.0/reset_method:pm /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:0a.0/0000:0a:00.0/reset_method:bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:0a.0/reset_method:pm /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:08.0/0000:08:00.0/reset_method:bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:08.0/0000:08:00.3/reset_method:pm /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:08.0/reset_method:pm /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:01.0/reset_method:pm bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:01.0/0000:05:00.0/reset_method:device_specific flr bus /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/reset_method:pm bus /sys/devices/pci0000:00/0000:00:01.2/reset_method:pm /sys/devices/pci0000:00/0000:00:08.1/0000:0c:00.0/reset_method:flr bus /sys/devices/pci0000:00/0000:00:08.1/0000:0c:00.3/reset_method:pm /sys/devices/pci0000:00/0000:00:08.1/0000:0c:00.1/reset_method:flr pm /sys/devices/pci0000:00/0000:00:08.1/reset_method:pm /sys/devices/pci0000:00/0000:00:08.1/0000:0c:00.4/reset_method:pm /sys/devices/pci0000:00/0000:00:01.1/0000:01:00.0/0000:02:00.0/reset_method:pm bus /sys/devices/pci0000:00/0000:00:01.1/0000:01:00.0/reset_method:bus /sys/devices/pci0000:00/0000:00:01.1/reset_method:pm /sys/devices/pci0000:00/0000:00:01.6/0000:0b:00.0/reset_method:flr bus /sys/devices/pci0000:00/0000:00:01.6/reset_method:pm ``` If you need each PCI bus bridge information, I can provide it to you. but I can promise they are typical hardware in AMD CPU or chipset for Zen generation and nothing special. > You should be able to manually reset the device with something like > this (I don't know your topology, so you might have to replace "1d.6" > with the bridge leading to 06:00.0): > > # sudo echo 1 > # /sys/devices/pci0000:00/0000:00:1d.6/0000:06:00.0/reset ``` $ echo 1 > sudo tee -a /sys/devices/pci0000:00/0000:00:01.2/0000:03:00.0/0000:04:02.0/0000:06:00.0/reset (nothing happens) $ journalctl -k -n10 (nothing specific) ``` Would I ask you any point to check after the reset operation? > I don't *know* of a reason why a Secondary Bus Reset would work > correctly on your hardware but not on a Mac, but there could be > something weird going on. Note that the hardware provided by Apple for the past decade has no IEEE 1394 interface, thus the patch author seems to use any kind of bus extension to connect the issued 1394 OHCI hardware. I guess: * Apple Thunderbolt Display * https://lore.kernel.org/linux-pci/137...@li.../ * Apple Thunderbolt-OHCI1394 adapter * I know FW643 is used for the product. * Some kind of eGPU box > Does the patch cause a problem for you? (Other than the fact that the > device leaks state between VMs?) It takes a bit time for me to set up my system with self-compiled v6.9-rc1 kernel. However the leak between VMs is really inconvenient to me by itself. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-26 12:39:09
|
Hi, On Tue, Mar 26, 2024 at 05:18:32AM -0700, Adam Goldman wrote: > Hi Takashi, > > On Mon, Mar 25, 2024 at 09:41:34AM +0900, Takashi Sakamoto wrote: > > Now we have two debug parameters per module for the slightly-similar > > purpose. In my opinion, it is a pretty cumbersome to enable them when > > checking bus-reset behaviour. I think it is time to investigate the other > > way. > > > > Linux Kernel Tracepoints[2] is one of options. Roughly describing, the > > tracepoints mechanism allows users to deliver structured data from kernel > > space to user space via ring-buffer when enabling it by either sysfs or > > kernel command-line parameters. Linux kernel also has a command-line > > parameter to redirect the human-readable formatted data to kernel log[3]. > > I think it is suitable in the case. > > > > It requires many work to replace the existent debug parameter of > > firewire-ohci, while it is a good start to work just for bus-reset debug. > > The data structure layout should be pre-defined in each subsystem, thus we > > need to decide it. In my opinion, it would be like: > > > > ``` > > struct bus_reset_event { > > enum reason { > > Initiate, > > Schedule, > > Postpone, > > Detect, > > }, > > // We can put any other data if prefering. > > } > > ``` > > Maybe these should be four separate trace events? > > > Would I ask your opinion about my idea? > > It seems that tracepoints are the modern way to make debugging logs, so > if we want to modernize the FireWire driver, we should replace the > existent logging with tracepoints. Thanks for your positive comment. I pushed my work-in-progress patches to the following specific topic branch:: https://github.com/takaswie/linux-firewire-dkms/tree/topic/backport-to-v6.8/tracepoints You can see some patches onto your commits: * 145da78e firewire: ohci: obsolete OHCI_PARAM_DEBUG_BUSRESETS from debug parameter with tracepoints event * 3bdad35d firewire: core: obsolete debug parameter with tracepoints event * 30f489af firewire: ohci: support bus_reset tracepoints event * 4937d9c8 firewire: core: support bus_reset tracepoints event * 0da26087 firewire: core: add support for Linux kernel tracepoints * 961cba18 firewire: core: option to log bus reset initiation * b3124560 firewire: ohci: mask bus reset interrupts between ISR and bottom half In the above, I added 'bus_reset' events in 'firewire' tracepoints subsystem. The structure is something like: ``` struct bus_reset { enum fw_trace_bus_reset_issue issue; bool short_reset; }; ``` The issue enumerations are in 'drivers/firewire/core.h': ``` enum fw_trace_bus_reset_issue { FW_TRACE_BUS_RESET_ISSUE_INITIATE = 0, FW_TRACE_BUS_RESET_ISSUE_SCHEDULE, FW_TRACE_BUS_RESET_ISSUE_POSTPONE, FW_TRACE_BUS_RESET_ISSUE_DETECT, }; ``` You can see the above event is trigerred by two kernel modules: * firewire-core * firewire-ohci When merging the above changes and build/load the kernel modules, we can see 'firewire:bus_reset' event in Linux Kernel tracepoints system, like: ``` $ ls /sys/kernel/debug/tracing/events/firewire/bus_reset ``` I currently consider about a pair of events for OHCI interrupts and PHY operation, instead of the above event. I'm happy if receiving your opinion about it or the other ideas. Regards Takashi Sakamoto |
From: Adam G. <ad...@po...> - 2024-03-26 12:18:56
|
Hi Takashi, On Mon, Mar 25, 2024 at 09:41:34AM +0900, Takashi Sakamoto wrote: > Now we have two debug parameters per module for the slightly-similar > purpose. In my opinion, it is a pretty cumbersome to enable them when > checking bus-reset behaviour. I think it is time to investigate the other > way. > > Linux Kernel Tracepoints[2] is one of options. Roughly describing, the > tracepoints mechanism allows users to deliver structured data from kernel > space to user space via ring-buffer when enabling it by either sysfs or > kernel command-line parameters. Linux kernel also has a command-line > parameter to redirect the human-readable formatted data to kernel log[3]. > I think it is suitable in the case. > > It requires many work to replace the existent debug parameter of > firewire-ohci, while it is a good start to work just for bus-reset debug. > The data structure layout should be pre-defined in each subsystem, thus we > need to decide it. In my opinion, it would be like: > > ``` > struct bus_reset_event { > enum reason { > Initiate, > Schedule, > Postpone, > Detect, > }, > // We can put any other data if prefering. > } > ``` Maybe these should be four separate trace events? > Would I ask your opinion about my idea? It seems that tracepoints are the modern way to make debugging logs, so if we want to modernize the FireWire driver, we should replace the existent logging with tracepoints. -- Adam |
From: Takashi S. <o-t...@sa...> - 2024-03-26 04:19:53
|
Hi, On Mon, Mar 25, 2024 at 07:56:10PM -0600, Gustavo A. R. Silva wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for > array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > Signed-off-by: Gustavo A. R. Silva <gus...@ke...> > --- > include/linux/firewire.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Applied to for-next branch, since it demands no code changes to the other subsystem. Thanks Takashi Sakamoto |