* [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution
@ 2025-10-21 20:53 Francesco Valla
2025-10-21 21:20 ` Calvin Owens
0 siblings, 1 reply; 7+ messages in thread
From: Francesco Valla @ 2025-10-21 20:53 UTC (permalink / raw)
To: Calvin Owens, Marcel Holtmann, Luiz Augusto von Dentz,
Paul Menzel
Cc: linux-bluetooth, linux-kernel
Hello,
while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612
Bluetooth chipset from NXP, I encountered an erratic bug during initialization.
While the firmware download always completed without errors, subsequent HCI
communication would fail most of the time with:
Frame reassembly failed (-84)
After some debug, I found the culprit to be this patch that was integrated as
part of the current (v6.18) cycle:
93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1]
The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now
used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes
that the private drvdata for the input struct hci_dev is a pointer to a
struct hci_uart, but that's not the case for the btnxpuart driver. In this
case, the information about padding and alignment are pretty random and
depend on the content of the data that was incorrectly casted as a
struct hci_uart.
The bug should impact also the other platforms that were touched by the
same patch.
For the time being, I'd then propose to revert the commit.
Thank you
Regards,
Francesco Valla
[1] https://lore.kernel.org/linux-bluetooth/be8edf7f8ba8dea6c61272b02fb20a4ac7e1c5a5.1756179634.git.calvin@wbinvd.org/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution 2025-10-21 20:53 [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution Francesco Valla @ 2025-10-21 21:20 ` Calvin Owens 2025-10-21 21:29 ` Calvin Owens 0 siblings, 1 reply; 7+ messages in thread From: Calvin Owens @ 2025-10-21 21:20 UTC (permalink / raw) To: Francesco Valla Cc: Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, linux-kernel On Tuesday 10/21 at 22:53 +0200, Francesco Valla wrote: > Hello, > > while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612 > Bluetooth chipset from NXP, I encountered an erratic bug during initialization. > > While the firmware download always completed without errors, subsequent HCI > communication would fail most of the time with: > > Frame reassembly failed (-84) > > After some debug, I found the culprit to be this patch that was integrated as > part of the current (v6.18) cycle: > > 93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1] > > The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now > used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes > that the private drvdata for the input struct hci_dev is a pointer to a > struct hci_uart, but that's not the case for the btnxpuart driver. In this > case, the information about padding and alignment are pretty random and > depend on the content of the data that was incorrectly casted as a > struct hci_uart. > > The bug should impact also the other platforms that were touched by the > same patch. Hi Francesco, Thanks for investigating, this makes sense to me. Funny enough, I specifically tested this on btnxpuart and saw no problems. I suppose some kconfig difference or some other innocuous patch moved structure fields around such that it triggered for you? Not that it really matters... > For the time being, I'd then propose to revert the commit. Adding back all the duplicate code is not the right way forward, IMHO. There must be some way to "mask" the problematic behavior for the drivers which stash the different structure in drvdata, right? Any thoughts from anybody else? I should have time to spin something up tomorrow, if nobody beats me to it. Thanks, Calvin > Thank you > > Regards, > Francesco Valla > > [1] https://lore.kernel.org/linux-bluetooth/be8edf7f8ba8dea6c61272b02fb20a4ac7e1c5a5.1756179634.git.calvin@wbinvd.org/ > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution 2025-10-21 21:20 ` Calvin Owens @ 2025-10-21 21:29 ` Calvin Owens 2025-10-21 22:07 ` Francesco Valla 0 siblings, 1 reply; 7+ messages in thread From: Calvin Owens @ 2025-10-21 21:29 UTC (permalink / raw) To: Francesco Valla Cc: Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, linux-kernel On Tuesday 10/21 at 14:20 -0700, Calvin Owens wrote: > On Tuesday 10/21 at 22:53 +0200, Francesco Valla wrote: > > Hello, > > > > while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612 > > Bluetooth chipset from NXP, I encountered an erratic bug during initialization. > > > > While the firmware download always completed without errors, subsequent HCI > > communication would fail most of the time with: > > > > Frame reassembly failed (-84) > > > > After some debug, I found the culprit to be this patch that was integrated as > > part of the current (v6.18) cycle: > > > > 93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1] > > > > The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now > > used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes > > that the private drvdata for the input struct hci_dev is a pointer to a > > struct hci_uart, but that's not the case for the btnxpuart driver. In this > > case, the information about padding and alignment are pretty random and > > depend on the content of the data that was incorrectly casted as a > > struct hci_uart. > > > > The bug should impact also the other platforms that were touched by the > > same patch. > > Hi Francesco, > > Thanks for investigating, this makes sense to me. > > Funny enough, I specifically tested this on btnxpuart and saw no > problems. I suppose some kconfig difference or some other innocuous > patch moved structure fields around such that it triggered for you? > Not that it really matters... > > > For the time being, I'd then propose to revert the commit. > > Adding back all the duplicate code is not the right way forward, IMHO. > There must be some way to "mask" the problematic behavior for the > drivers which stash the different structure in drvdata, right? Actually, the right approach is probably to tweak these drivers to do what the Intel driver does: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bluetooth/hci_intel.c#n869 static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb) { struct hci_uart *hu = hci_get_drvdata(hdev); struct intel_data *intel = hu->priv; I'll spin that up unless I hear better from anyone else :) > Any thoughts from anybody else? I should have time to spin something up > tomorrow, if nobody beats me to it. > > Thanks, > Calvin > > > Thank you > > > > Regards, > > Francesco Valla > > > > [1] https://lore.kernel.org/linux-bluetooth/be8edf7f8ba8dea6c61272b02fb20a4ac7e1c5a5.1756179634.git.calvin@wbinvd.org/ > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution 2025-10-21 21:29 ` Calvin Owens @ 2025-10-21 22:07 ` Francesco Valla 2025-10-22 16:12 ` Calvin Owens 0 siblings, 1 reply; 7+ messages in thread From: Francesco Valla @ 2025-10-21 22:07 UTC (permalink / raw) To: Calvin Owens Cc: Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, linux-kernel On Tuesday, 21 October 2025 at 23:29:59 Calvin Owens <calvin@wbinvd.org> wrote: > On Tuesday 10/21 at 14:20 -0700, Calvin Owens wrote: > > On Tuesday 10/21 at 22:53 +0200, Francesco Valla wrote: > > > Hello, > > > > > > while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612 > > > Bluetooth chipset from NXP, I encountered an erratic bug during initialization. > > > > > > While the firmware download always completed without errors, subsequent HCI > > > communication would fail most of the time with: > > > > > > Frame reassembly failed (-84) > > > > > > After some debug, I found the culprit to be this patch that was integrated as > > > part of the current (v6.18) cycle: > > > > > > 93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1] > > > > > > The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now > > > used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes > > > that the private drvdata for the input struct hci_dev is a pointer to a > > > struct hci_uart, but that's not the case for the btnxpuart driver. In this > > > case, the information about padding and alignment are pretty random and > > > depend on the content of the data that was incorrectly casted as a > > > struct hci_uart. > > > > > > The bug should impact also the other platforms that were touched by the > > > same patch. > > > > Hi Francesco, > > > > Thanks for investigating, this makes sense to me. > > > > Funny enough, I specifically tested this on btnxpuart and saw no > > problems. I suppose some kconfig difference or some other innocuous > > patch moved structure fields around such that it triggered for you? > > Not that it really matters... > > > > > For the time being, I'd then propose to revert the commit. > > > > Adding back all the duplicate code is not the right way forward, IMHO. > > There must be some way to "mask" the problematic behavior for the > > drivers which stash the different structure in drvdata, right? > > Actually, the right approach is probably to tweak these drivers to do > what the Intel driver does: > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bluetooth/hci_intel.c#n869 > > static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb) > { > struct hci_uart *hu = hci_get_drvdata(hdev); > struct intel_data *intel = hu->priv; > > I'll spin that up unless I hear better from anyone else :) > Hi, thanks for the quick response! That was my first thought, but the Intel driver actually _uses_ the hci_uart structure, while btnxpuart and such would only piggy-back on it to be able to use h4_recv_buf() (and struct hci_uart is huge!). One possible solution would be to define an "inner" __h4_recv_buf() function that accepts alignment and padding as arguments, and use that directly on drivers that don't use struct hci_uart (PoC attached - I don't like the __h4_recv_buf name but I don't really know how it should be called). Regards, Francesco --- diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index d5153fed0518..02511ef1a841 100644 --- a/drivers/bluetooth/btnxpuart.c +++ b/drivers/bluetooth/btnxpuart.c @@ -1756,8 +1756,9 @@ static size_t btnxpuart_receive_buf(struct serdev_device *serdev, ps_start_timer(nxpdev); - nxpdev->rx_skb = h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count, - nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts)); + nxpdev->rx_skb = __h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count, + nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts), + 0, NULL); if (IS_ERR(nxpdev->rx_skb)) { int err = PTR_ERR(nxpdev->rx_skb); /* Safe to ignore out-of-sync bootloader signatures */ diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c index 9070e31a68bf..c83c266ba506 100644 --- a/drivers/bluetooth/hci_h4.c +++ b/drivers/bluetooth/hci_h4.c @@ -151,27 +151,32 @@ int __exit h4_deinit(void) return hci_uart_unregister_proto(&h4p); } -struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, - const unsigned char *buffer, int count, - const struct h4_recv_pkt *pkts, int pkts_count) +struct sk_buff *__h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, + const unsigned char *buffer, int count, + const struct h4_recv_pkt *pkts, int pkts_count, + u8 alignment, u8 *padding) { - struct hci_uart *hu = hci_get_drvdata(hdev); - u8 alignment = hu->alignment ? hu->alignment : 1; - /* Check for error from previous call */ if (IS_ERR(skb)) skb = NULL; + if (alignment == 0) + alignment = 1; + + WARN_ON_ONCE(alignment > 1 && !padding); + while (count) { int i, len; /* remove padding bytes from buffer */ - for (; hu->padding && count > 0; hu->padding--) { - count--; - buffer++; + if (padding) { + for (; *padding && count > 0; *padding = *padding - 1) { + count--; + buffer++; + } + if (!count) + break; } - if (!count) - break; if (!skb) { for (i = 0; i < pkts_count; i++) { @@ -252,16 +257,20 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, } if (!dlen) { - hu->padding = (skb->len + 1) % alignment; - hu->padding = (alignment - hu->padding) % alignment; + if (padding) { + *padding = (skb->len + 1) % alignment; + *padding = (alignment - *padding) % alignment; + } /* No more data, complete frame */ (&pkts[i])->recv(hdev, skb); skb = NULL; } } else { - hu->padding = (skb->len + 1) % alignment; - hu->padding = (alignment - hu->padding) % alignment; + if (padding) { + *padding = (skb->len + 1) % alignment; + *padding = (alignment - *padding) % alignment; + } /* Complete frame */ (&pkts[i])->recv(hdev, skb); @@ -271,4 +280,16 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, return skb; } +EXPORT_SYMBOL_GPL(__h4_recv_buf); + +struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, + const unsigned char *buffer, int count, + const struct h4_recv_pkt *pkts, int pkts_count) +{ + struct hci_uart *hu = hci_get_drvdata(hdev); + u8 alignment = hu->alignment ? hu->alignment : 1; + + return __h4_recv_buf(hdev, skb, buffer, count, pkts, pkts_count, + alignment, &hu->padding); +} EXPORT_SYMBOL_GPL(h4_recv_buf); diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index cbbe79b241ce..0b61ee953fa4 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h @@ -162,6 +162,11 @@ struct h4_recv_pkt { int h4_init(void); int h4_deinit(void); +struct sk_buff *__h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, + const unsigned char *buffer, int count, + const struct h4_recv_pkt *pkts, int pkts_count, + u8 alignment, u8 *padding); + struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb, const unsigned char *buffer, int count, const struct h4_recv_pkt *pkts, int pkts_count); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution 2025-10-21 22:07 ` Francesco Valla @ 2025-10-22 16:12 ` Calvin Owens 2025-10-22 20:35 ` Francesco Valla 0 siblings, 1 reply; 7+ messages in thread From: Calvin Owens @ 2025-10-22 16:12 UTC (permalink / raw) To: Francesco Valla Cc: Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, linux-kernel On Wednesday 10/22 at 00:07 +0200, Francesco Valla wrote: > On Tuesday, 21 October 2025 at 23:29:59 Calvin Owens <calvin@wbinvd.org> wrote: > > On Tuesday 10/21 at 14:20 -0700, Calvin Owens wrote: > > > On Tuesday 10/21 at 22:53 +0200, Francesco Valla wrote: > > > > Hello, > > > > > > > > while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612 > > > > Bluetooth chipset from NXP, I encountered an erratic bug during initialization. > > > > > > > > While the firmware download always completed without errors, subsequent HCI > > > > communication would fail most of the time with: > > > > > > > > Frame reassembly failed (-84) > > > > > > > > After some debug, I found the culprit to be this patch that was integrated as > > > > part of the current (v6.18) cycle: > > > > > > > > 93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1] > > > > > > > > The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now > > > > used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes > > > > that the private drvdata for the input struct hci_dev is a pointer to a > > > > struct hci_uart, but that's not the case for the btnxpuart driver. In this > > > > case, the information about padding and alignment are pretty random and > > > > depend on the content of the data that was incorrectly casted as a > > > > struct hci_uart. > > > > > > > > The bug should impact also the other platforms that were touched by the > > > > same patch. > > > > > > Hi Francesco, > > > > > > Thanks for investigating, this makes sense to me. > > > > > > Funny enough, I specifically tested this on btnxpuart and saw no > > > problems. I suppose some kconfig difference or some other innocuous > > > patch moved structure fields around such that it triggered for you? > > > Not that it really matters... > > > > > > > For the time being, I'd then propose to revert the commit. > > > > > > Adding back all the duplicate code is not the right way forward, IMHO. > > > There must be some way to "mask" the problematic behavior for the > > > drivers which stash the different structure in drvdata, right? > > > > Actually, the right approach is probably to tweak these drivers to do > > what the Intel driver does: > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bluetooth/hci_intel.c#n869 > > > > static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb) > > { > > struct hci_uart *hu = hci_get_drvdata(hdev); > > struct intel_data *intel = hu->priv; > > > > I'll spin that up unless I hear better from anyone else :) > > > > Hi, thanks for the quick response! > > That was my first thought, but the Intel driver actually _uses_ the hci_uart > structure, while btnxpuart and such would only piggy-back on it to be able to > use h4_recv_buf() (and struct hci_uart is huge!). Why is that a problem? Certainly, nobody is going to mind the extra bytes with that monstrosity hanging around :) > One possible solution would be to define an "inner" __h4_recv_buf() function > that accepts alignment and padding as arguments, and use that directly on > drivers that don't use struct hci_uart (PoC attached - I don't like the > __h4_recv_buf name but I don't really know how it should be called). I don't feel super strongly about it, but IMHO the whole thing is easier to understand if we just put the data the core function expects where it expects it. I haven't had enough coffee yet, but I think zero-initializing hu is sufficient... Something like this, only compile tested: ---8<--- From: Calvin Owens <calvin@wbinvd.org> Subject: [PATCH] Working bugfix for bt cleanup, only for btnxpuart for now Signed-off-by: Calvin Owens <calvin@wbinvd.org> --- drivers/bluetooth/btnxpuart.c | 74 +++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 33 deletions(-) diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index d5153fed0518..cf464515c855 100644 --- a/drivers/bluetooth/btnxpuart.c +++ b/drivers/bluetooth/btnxpuart.c @@ -212,6 +212,7 @@ struct btnxpuart_dev { struct ps_data psdata; struct btnxpuart_data *nxp_data; struct reset_control *pdn; + struct hci_uart hu; }; #define NXP_V1_FW_REQ_PKT 0xa5 @@ -363,6 +364,12 @@ union nxp_set_bd_addr_payload { static u8 crc8_table[CRC8_TABLE_SIZE]; +static struct btnxpuart_dev *hci_get_nxpdev(struct hci_dev *hdev) +{ + struct hci_uart *hu = hci_get_drvdata(hdev); + return hu->priv; +} + /* Default configurations */ #define DEFAULT_H2C_WAKEUP_MODE WAKEUP_METHOD_BREAK #define DEFAULT_PS_MODE PS_MODE_ENABLE @@ -373,7 +380,7 @@ static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode, void *param, bool resp) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; struct sk_buff *skb = NULL; @@ -426,7 +433,7 @@ static void ps_cancel_timer(struct btnxpuart_dev *nxpdev) static void ps_control(struct hci_dev *hdev, u8 ps_state) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; int status = 0; @@ -483,7 +490,7 @@ static void ps_timeout_func(struct timer_list *t) { struct ps_data *data = timer_container_of(data, t, ps_timer); struct hci_dev *hdev = data->hdev; - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) { ps_start_timer(nxpdev); @@ -502,7 +509,7 @@ static irqreturn_t ps_host_wakeup_irq_handler(int irq, void *priv) } static int ps_setup(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct serdev_device *serdev = nxpdev->serdev; struct ps_data *psdata = &nxpdev->psdata; int ret; @@ -597,7 +604,7 @@ static void ps_cleanup(struct btnxpuart_dev *nxpdev) static int send_ps_cmd(struct hci_dev *hdev, void *data) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; struct psmode_cmd_payload pcmd; struct sk_buff *skb; @@ -636,7 +643,7 @@ static int send_ps_cmd(struct hci_dev *hdev, void *data) static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; struct wakeup_cmd_payload pcmd; struct sk_buff *skb; @@ -682,7 +689,7 @@ static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data) static void ps_init(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; u8 default_h2c_wakeup_mode = DEFAULT_H2C_WAKEUP_MODE; @@ -732,7 +739,7 @@ static void ps_init(struct hci_dev *hdev) /* NXP Firmware Download Feature */ static int nxp_download_firmware(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); int err = 0; nxpdev->fw_dnld_v1_offset = 0; @@ -782,7 +789,7 @@ static int nxp_download_firmware(struct hci_dev *hdev) static void nxp_send_ack(u8 ack, struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); u8 ack_nak[2]; int len = 1; @@ -796,7 +803,7 @@ static void nxp_send_ack(u8 ack, struct hci_dev *hdev) static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct nxp_bootloader_cmd nxp_cmd5; struct uart_config uart_config; u32 clkdivaddr = CLKDIVADDR - nxpdev->boot_reg_offset; @@ -846,7 +853,7 @@ static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len) static bool nxp_fw_change_timeout(struct hci_dev *hdev, u16 req_len) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct nxp_bootloader_cmd nxp_cmd7; if (req_len != sizeof(nxp_cmd7)) @@ -899,7 +906,7 @@ static bool process_boot_signature(struct btnxpuart_dev *nxpdev) static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name, const char *fw_name_old) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); const char *fw_name_dt; int err = 0; @@ -931,7 +938,7 @@ static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name, /* for legacy chipsets with V1 bootloader */ static int nxp_recv_chip_ver_v1(struct hci_dev *hdev, struct sk_buff *skb) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct v1_start_ind *req; __u16 chip_id; @@ -956,7 +963,7 @@ static int nxp_recv_chip_ver_v1(struct hci_dev *hdev, struct sk_buff *skb) static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct btnxpuart_data *nxp_data = nxpdev->nxp_data; struct v1_data_req *req; __u16 len; @@ -1065,7 +1072,7 @@ static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb) static char *nxp_get_fw_name_from_chipid(struct hci_dev *hdev, u16 chipid, u8 loader_ver) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); char *fw_name = NULL; switch (chipid) { @@ -1139,7 +1146,7 @@ static char *nxp_get_old_fw_name_from_chipid(struct hci_dev *hdev, u16 chipid, static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff *skb) { struct v3_start_ind *req = skb_pull_data(skb, sizeof(*req)); - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); const char *fw_name; const char *fw_name_old; u16 chip_id; @@ -1163,7 +1170,7 @@ static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff *skb) static void nxp_handle_fw_download_error(struct hci_dev *hdev, struct v3_data_req *req) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); __u32 offset = __le32_to_cpu(req->offset); __u16 err = __le16_to_cpu(req->error); union nxp_v3_rx_timeout_nak_u timeout_nak_buf; @@ -1191,7 +1198,7 @@ static void nxp_handle_fw_download_error(struct hci_dev *hdev, struct v3_data_re static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct v3_data_req *req; __u16 len = 0; __u16 err = 0; @@ -1277,7 +1284,7 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb) static int nxp_set_baudrate_cmd(struct hci_dev *hdev, void *data) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); __le32 new_baudrate = __cpu_to_le32(nxpdev->new_baudrate); struct ps_data *psdata = &nxpdev->psdata; struct sk_buff *skb; @@ -1362,7 +1369,7 @@ static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb) struct hci_acl_hdr *acl_hdr = (struct hci_acl_hdr *)skb_pull_data(skb, sizeof(*acl_hdr)); struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data; - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); __u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num); __u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len); int err; @@ -1439,7 +1446,7 @@ static int nxp_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr) /* NXP protocol */ static int nxp_setup(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct serdev_device *serdev = nxpdev->serdev; char device_string[30]; char event_string[50]; @@ -1475,7 +1482,7 @@ static int nxp_setup(struct hci_dev *hdev) static int nxp_post_init(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; if (nxpdev->current_baudrate != nxpdev->secondary_baudrate) { @@ -1491,7 +1498,7 @@ static int nxp_post_init(struct hci_dev *hdev) static void nxp_hw_err(struct hci_dev *hdev, u8 code) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); switch (code) { case BTNXPUART_IR_HW_ERR: @@ -1505,7 +1512,7 @@ static void nxp_hw_err(struct hci_dev *hdev, u8 code) static int nxp_shutdown(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct sk_buff *skb; u8 pcmd = 0; @@ -1529,7 +1536,7 @@ static int nxp_shutdown(struct hci_dev *hdev) static bool nxp_wakeup(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; if (psdata->c2h_wakeupmode != BT_HOST_WAKEUP_METHOD_NONE) @@ -1540,7 +1547,7 @@ static bool nxp_wakeup(struct hci_dev *hdev) static void nxp_reset(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); if (!ind_reset_in_progress(nxpdev) && !fw_dump_in_progress(nxpdev)) { bt_dev_dbg(hdev, "CMD Timeout detected. Resetting."); @@ -1550,7 +1557,7 @@ static void nxp_reset(struct hci_dev *hdev) static int btnxpuart_queue_skb(struct hci_dev *hdev, struct sk_buff *skb) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); /* Prepend skb with frame type */ memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); @@ -1561,7 +1568,7 @@ static int btnxpuart_queue_skb(struct hci_dev *hdev, struct sk_buff *skb) static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct ps_data *psdata = &nxpdev->psdata; struct hci_command_hdr *hdr; struct psmode_cmd_payload ps_parm; @@ -1693,7 +1700,7 @@ static void btnxpuart_tx_work(struct work_struct *work) static int btnxpuart_open(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); int err = 0; err = serdev_device_open(nxpdev->serdev); @@ -1708,7 +1715,7 @@ static int btnxpuart_open(struct hci_dev *hdev) static int btnxpuart_close(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); serdev_device_close(nxpdev->serdev); skb_queue_purge(&nxpdev->txq); @@ -1722,7 +1729,7 @@ static int btnxpuart_close(struct hci_dev *hdev) static int btnxpuart_flush(struct hci_dev *hdev) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); /* Flush any pending characters */ serdev_device_write_flush(nxpdev->serdev); @@ -1784,7 +1791,7 @@ static const struct serdev_device_ops btnxpuart_client_ops = { static void nxp_coredump_notify(struct hci_dev *hdev, int state) { - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); struct serdev_device *serdev = nxpdev->serdev; char device_string[30]; char event_string[50]; @@ -1877,7 +1884,8 @@ static int nxp_serdev_probe(struct serdev_device *serdev) nxpdev->hdev = hdev; hdev->bus = HCI_UART; - hci_set_drvdata(hdev, nxpdev); + hci_set_drvdata(hdev, &nxpdev->hu); + nxpdev->hu.priv = nxpdev; hdev->manufacturer = MANUFACTURER_NXP; hdev->open = btnxpuart_open; -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution 2025-10-22 16:12 ` Calvin Owens @ 2025-10-22 20:35 ` Francesco Valla 2025-10-23 18:38 ` Calvin Owens 0 siblings, 1 reply; 7+ messages in thread From: Francesco Valla @ 2025-10-22 20:35 UTC (permalink / raw) To: Calvin Owens Cc: Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, linux-kernel On Wednesday, 22 October 2025 at 18:12:23 Calvin Owens <calvin@wbinvd.org> wrote: > On Wednesday 10/22 at 00:07 +0200, Francesco Valla wrote: > > On Tuesday, 21 October 2025 at 23:29:59 Calvin Owens <calvin@wbinvd.org> wrote: > > > On Tuesday 10/21 at 14:20 -0700, Calvin Owens wrote: > > > > On Tuesday 10/21 at 22:53 +0200, Francesco Valla wrote: > > > > > Hello, > > > > > > > > > > while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612 > > > > > Bluetooth chipset from NXP, I encountered an erratic bug during initialization. > > > > > > > > > > While the firmware download always completed without errors, subsequent HCI > > > > > communication would fail most of the time with: > > > > > > > > > > Frame reassembly failed (-84) > > > > > > > > > > After some debug, I found the culprit to be this patch that was integrated as > > > > > part of the current (v6.18) cycle: > > > > > > > > > > 93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1] > > > > > > > > > > The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now > > > > > used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes > > > > > that the private drvdata for the input struct hci_dev is a pointer to a > > > > > struct hci_uart, but that's not the case for the btnxpuart driver. In this > > > > > case, the information about padding and alignment are pretty random and > > > > > depend on the content of the data that was incorrectly casted as a > > > > > struct hci_uart. > > > > > > > > > > The bug should impact also the other platforms that were touched by the > > > > > same patch. > > > > > > > > Hi Francesco, > > > > > > > > Thanks for investigating, this makes sense to me. > > > > > > > > Funny enough, I specifically tested this on btnxpuart and saw no > > > > problems. I suppose some kconfig difference or some other innocuous > > > > patch moved structure fields around such that it triggered for you? > > > > Not that it really matters... > > > > > > > > > For the time being, I'd then propose to revert the commit. > > > > > > > > Adding back all the duplicate code is not the right way forward, IMHO. > > > > There must be some way to "mask" the problematic behavior for the > > > > drivers which stash the different structure in drvdata, right? > > > > > > Actually, the right approach is probably to tweak these drivers to do > > > what the Intel driver does: > > > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bluetooth/hci_intel.c#n869 > > > > > > static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb) > > > { > > > struct hci_uart *hu = hci_get_drvdata(hdev); > > > struct intel_data *intel = hu->priv; > > > > > > I'll spin that up unless I hear better from anyone else :) > > > > > > > Hi, thanks for the quick response! > > > > That was my first thought, but the Intel driver actually _uses_ the hci_uart > > structure, while btnxpuart and such would only piggy-back on it to be able to > > use h4_recv_buf() (and struct hci_uart is huge!). > > Why is that a problem? Certainly, nobody is going to mind the extra > bytes with that monstrosity hanging around :) > You have a point there. > > One possible solution would be to define an "inner" __h4_recv_buf() function > > that accepts alignment and padding as arguments, and use that directly on > > drivers that don't use struct hci_uart (PoC attached - I don't like the > > __h4_recv_buf name but I don't really know how it should be called). > > I don't feel super strongly about it, but IMHO the whole thing is easier > to understand if we just put the data the core function expects where it > expects it. I haven't had enough coffee yet, but I think > zero-initializing hu is sufficient... > > Something like this, only compile tested: > ---8<--- > From: Calvin Owens <calvin@wbinvd.org> > Subject: [PATCH] Working bugfix for bt cleanup, only for btnxpuart for now > > Signed-off-by: Calvin Owens <calvin@wbinvd.org> > --- > drivers/bluetooth/btnxpuart.c | 74 +++++++++++++++++++---------------- > 1 file changed, 41 insertions(+), 33 deletions(-) > > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c > index d5153fed0518..cf464515c855 100644 > --- a/drivers/bluetooth/btnxpuart.c > +++ b/drivers/bluetooth/btnxpuart.c > @@ -212,6 +212,7 @@ struct btnxpuart_dev { > struct ps_data psdata; > struct btnxpuart_data *nxp_data; > struct reset_control *pdn; > + struct hci_uart hu; > }; > > #define NXP_V1_FW_REQ_PKT 0xa5 > @@ -363,6 +364,12 @@ union nxp_set_bd_addr_payload { > > static u8 crc8_table[CRC8_TABLE_SIZE]; > > +static struct btnxpuart_dev *hci_get_nxpdev(struct hci_dev *hdev) > +{ > + struct hci_uart *hu = hci_get_drvdata(hdev); > + return hu->priv; > +} > + > /* Default configurations */ > #define DEFAULT_H2C_WAKEUP_MODE WAKEUP_METHOD_BREAK > #define DEFAULT_PS_MODE PS_MODE_ENABLE > @@ -373,7 +380,7 @@ static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode, > void *param, > bool resp) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > struct sk_buff *skb = NULL; > > @@ -426,7 +433,7 @@ static void ps_cancel_timer(struct btnxpuart_dev *nxpdev) > > static void ps_control(struct hci_dev *hdev, u8 ps_state) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > int status = 0; > > @@ -483,7 +490,7 @@ static void ps_timeout_func(struct timer_list *t) > { > struct ps_data *data = timer_container_of(data, t, ps_timer); > struct hci_dev *hdev = data->hdev; > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > > if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) { > ps_start_timer(nxpdev); > @@ -502,7 +509,7 @@ static irqreturn_t ps_host_wakeup_irq_handler(int irq, void *priv) > } > static int ps_setup(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct serdev_device *serdev = nxpdev->serdev; > struct ps_data *psdata = &nxpdev->psdata; > int ret; > @@ -597,7 +604,7 @@ static void ps_cleanup(struct btnxpuart_dev *nxpdev) > > static int send_ps_cmd(struct hci_dev *hdev, void *data) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > struct psmode_cmd_payload pcmd; > struct sk_buff *skb; > @@ -636,7 +643,7 @@ static int send_ps_cmd(struct hci_dev *hdev, void *data) > > static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > struct wakeup_cmd_payload pcmd; > struct sk_buff *skb; > @@ -682,7 +689,7 @@ static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data) > > static void ps_init(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > u8 default_h2c_wakeup_mode = DEFAULT_H2C_WAKEUP_MODE; > > @@ -732,7 +739,7 @@ static void ps_init(struct hci_dev *hdev) > /* NXP Firmware Download Feature */ > static int nxp_download_firmware(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > int err = 0; > > nxpdev->fw_dnld_v1_offset = 0; > @@ -782,7 +789,7 @@ static int nxp_download_firmware(struct hci_dev *hdev) > > static void nxp_send_ack(u8 ack, struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > u8 ack_nak[2]; > int len = 1; > > @@ -796,7 +803,7 @@ static void nxp_send_ack(u8 ack, struct hci_dev *hdev) > > static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct nxp_bootloader_cmd nxp_cmd5; > struct uart_config uart_config; > u32 clkdivaddr = CLKDIVADDR - nxpdev->boot_reg_offset; > @@ -846,7 +853,7 @@ static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len) > > static bool nxp_fw_change_timeout(struct hci_dev *hdev, u16 req_len) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct nxp_bootloader_cmd nxp_cmd7; > > if (req_len != sizeof(nxp_cmd7)) > @@ -899,7 +906,7 @@ static bool process_boot_signature(struct btnxpuart_dev *nxpdev) > static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name, > const char *fw_name_old) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > const char *fw_name_dt; > int err = 0; > > @@ -931,7 +938,7 @@ static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name, > /* for legacy chipsets with V1 bootloader */ > static int nxp_recv_chip_ver_v1(struct hci_dev *hdev, struct sk_buff *skb) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct v1_start_ind *req; > __u16 chip_id; > > @@ -956,7 +963,7 @@ static int nxp_recv_chip_ver_v1(struct hci_dev *hdev, struct sk_buff *skb) > > static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct btnxpuart_data *nxp_data = nxpdev->nxp_data; > struct v1_data_req *req; > __u16 len; > @@ -1065,7 +1072,7 @@ static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb) > static char *nxp_get_fw_name_from_chipid(struct hci_dev *hdev, u16 chipid, > u8 loader_ver) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > char *fw_name = NULL; > > switch (chipid) { > @@ -1139,7 +1146,7 @@ static char *nxp_get_old_fw_name_from_chipid(struct hci_dev *hdev, u16 chipid, > static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff *skb) > { > struct v3_start_ind *req = skb_pull_data(skb, sizeof(*req)); > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > const char *fw_name; > const char *fw_name_old; > u16 chip_id; > @@ -1163,7 +1170,7 @@ static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff *skb) > > static void nxp_handle_fw_download_error(struct hci_dev *hdev, struct v3_data_req *req) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > __u32 offset = __le32_to_cpu(req->offset); > __u16 err = __le16_to_cpu(req->error); > union nxp_v3_rx_timeout_nak_u timeout_nak_buf; > @@ -1191,7 +1198,7 @@ static void nxp_handle_fw_download_error(struct hci_dev *hdev, struct v3_data_re > > static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct v3_data_req *req; > __u16 len = 0; > __u16 err = 0; > @@ -1277,7 +1284,7 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb) > > static int nxp_set_baudrate_cmd(struct hci_dev *hdev, void *data) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > __le32 new_baudrate = __cpu_to_le32(nxpdev->new_baudrate); > struct ps_data *psdata = &nxpdev->psdata; > struct sk_buff *skb; > @@ -1362,7 +1369,7 @@ static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb) > struct hci_acl_hdr *acl_hdr = (struct hci_acl_hdr *)skb_pull_data(skb, > sizeof(*acl_hdr)); > struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data; > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > __u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num); > __u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len); > int err; > @@ -1439,7 +1446,7 @@ static int nxp_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr) > /* NXP protocol */ > static int nxp_setup(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct serdev_device *serdev = nxpdev->serdev; > char device_string[30]; > char event_string[50]; > @@ -1475,7 +1482,7 @@ static int nxp_setup(struct hci_dev *hdev) > > static int nxp_post_init(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > > if (nxpdev->current_baudrate != nxpdev->secondary_baudrate) { > @@ -1491,7 +1498,7 @@ static int nxp_post_init(struct hci_dev *hdev) > > static void nxp_hw_err(struct hci_dev *hdev, u8 code) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > > switch (code) { > case BTNXPUART_IR_HW_ERR: > @@ -1505,7 +1512,7 @@ static void nxp_hw_err(struct hci_dev *hdev, u8 code) > > static int nxp_shutdown(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct sk_buff *skb; > u8 pcmd = 0; > > @@ -1529,7 +1536,7 @@ static int nxp_shutdown(struct hci_dev *hdev) > > static bool nxp_wakeup(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > > if (psdata->c2h_wakeupmode != BT_HOST_WAKEUP_METHOD_NONE) > @@ -1540,7 +1547,7 @@ static bool nxp_wakeup(struct hci_dev *hdev) > > static void nxp_reset(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > > if (!ind_reset_in_progress(nxpdev) && !fw_dump_in_progress(nxpdev)) { > bt_dev_dbg(hdev, "CMD Timeout detected. Resetting."); > @@ -1550,7 +1557,7 @@ static void nxp_reset(struct hci_dev *hdev) > > static int btnxpuart_queue_skb(struct hci_dev *hdev, struct sk_buff *skb) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > > /* Prepend skb with frame type */ > memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > @@ -1561,7 +1568,7 @@ static int btnxpuart_queue_skb(struct hci_dev *hdev, struct sk_buff *skb) > > static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct ps_data *psdata = &nxpdev->psdata; > struct hci_command_hdr *hdr; > struct psmode_cmd_payload ps_parm; > @@ -1693,7 +1700,7 @@ static void btnxpuart_tx_work(struct work_struct *work) > > static int btnxpuart_open(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > int err = 0; > > err = serdev_device_open(nxpdev->serdev); > @@ -1708,7 +1715,7 @@ static int btnxpuart_open(struct hci_dev *hdev) > > static int btnxpuart_close(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > > serdev_device_close(nxpdev->serdev); > skb_queue_purge(&nxpdev->txq); > @@ -1722,7 +1729,7 @@ static int btnxpuart_close(struct hci_dev *hdev) > > static int btnxpuart_flush(struct hci_dev *hdev) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > > /* Flush any pending characters */ > serdev_device_write_flush(nxpdev->serdev); > @@ -1784,7 +1791,7 @@ static const struct serdev_device_ops btnxpuart_client_ops = { > > static void nxp_coredump_notify(struct hci_dev *hdev, int state) > { > - struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_dev *nxpdev = hci_get_nxpdev(hdev); > struct serdev_device *serdev = nxpdev->serdev; > char device_string[30]; > char event_string[50]; > @@ -1877,7 +1884,8 @@ static int nxp_serdev_probe(struct serdev_device *serdev) > nxpdev->hdev = hdev; > > hdev->bus = HCI_UART; > - hci_set_drvdata(hdev, nxpdev); > + hci_set_drvdata(hdev, &nxpdev->hu); > + nxpdev->hu.priv = nxpdev; > > hdev->manufacturer = MANUFACTURER_NXP; > hdev->open = btnxpuart_open; > I tested this on my i.MX93 FRDM and I confirm it's working. Can't say that I like it, but... Regards, Francesco ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution 2025-10-22 20:35 ` Francesco Valla @ 2025-10-23 18:38 ` Calvin Owens 0 siblings, 0 replies; 7+ messages in thread From: Calvin Owens @ 2025-10-23 18:38 UTC (permalink / raw) To: Francesco Valla Cc: Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel, linux-bluetooth, linux-kernel On Wednesday 10/22 at 22:35 +0200, Francesco Valla wrote: > On Wednesday, 22 October 2025 at 18:12:23 Calvin Owens <calvin@wbinvd.org> wrote: > > <snip> > > I tested this on my i.MX93 FRDM and I confirm it's working. Can't say that I > like it, but... Yeah, on second thought I agree, the cross-driver dependency on drvdata is gross. But I think I found a better way, I'll send a patch in a sec. Thanks again for looking into this. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-10-23 18:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-21 20:53 [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution Francesco Valla 2025-10-21 21:20 ` Calvin Owens 2025-10-21 21:29 ` Calvin Owens 2025-10-21 22:07 ` Francesco Valla 2025-10-22 16:12 ` Calvin Owens 2025-10-22 20:35 ` Francesco Valla 2025-10-23 18:38 ` Calvin Owens
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox