Hi Kiran, On Tue, Apr 1, 2025 at 10:04 PM Kiran K <kiran.k@xxxxxxxxx> wrote: > > Reuse the skb buffer provided by the PCIe driver to pass it onto the > stack, instead of copying it to a new skb. > > Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport") > Signed-off-by: Kiran K <kiran.k@xxxxxxxxx> > --- > drivers/bluetooth/btintel_pcie.c | 58 ++++++++++++-------------------- > 1 file changed, 22 insertions(+), 36 deletions(-) > > diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c > index e0b50513403f..ebc36fd33de8 100644 > --- a/drivers/bluetooth/btintel_pcie.c > +++ b/drivers/bluetooth/btintel_pcie.c > @@ -947,8 +947,10 @@ static int btintel_pcie_recv_event(struct hci_dev *hdev, struct sk_buff *skb) > /* This is a debug event that comes from IML and OP image when it > * starts execution. There is no need pass this event to stack. Is this logged/sent to monitor at least? > */ > - if (skb->data[2] == 0x97) > + if (skb->data[2] == 0x97) { > + kfree_skb(skb); > return 0; > + } > } > > return hci_recv_frame(hdev, skb); > @@ -964,7 +966,6 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, > u8 pkt_type; > u16 plen; > u32 pcie_pkt_type; > - struct sk_buff *new_skb; > void *pdata; > struct hci_dev *hdev = data->hdev; > > @@ -974,8 +975,7 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, > pdata = skb_pull_data(skb, BTINTEL_PCIE_HCI_TYPE_LEN); > if (!pdata) { > bt_dev_err(hdev, "Corrupted packet received"); > - ret = -EILSEQ; > - goto exit_error; > + goto no_or_short_data; > } > > pcie_pkt_type = get_unaligned_le32(pdata); > @@ -987,8 +987,7 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, > pkt_type = HCI_ACLDATA_PKT; > } else { > bt_dev_err(hdev, "ACL packet is too short"); > - ret = -EILSEQ; > - goto exit_error; > + goto no_or_short_data; > } > break; > > @@ -998,8 +997,7 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, > pkt_type = HCI_SCODATA_PKT; > } else { > bt_dev_err(hdev, "SCO packet is too short"); > - ret = -EILSEQ; > - goto exit_error; > + goto no_or_short_data; > } > break; > > @@ -1009,8 +1007,7 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, > pkt_type = HCI_EVENT_PKT; > } else { > bt_dev_err(hdev, "Event packet is too short"); > - ret = -EILSEQ; > - goto exit_error; > + goto no_or_short_data; > } > break; > > @@ -1020,45 +1017,40 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, > pkt_type = HCI_ISODATA_PKT; > } else { > bt_dev_err(hdev, "ISO packet is too short"); > - ret = -EILSEQ; > - goto exit_error; > + goto no_or_short_data; > } > break; > > default: > bt_dev_err(hdev, "Invalid packet type received: 0x%4.4x", > pcie_pkt_type); > - ret = -EINVAL; > - goto exit_error; > + goto no_or_short_data; > } > > if (skb->len < plen) { > bt_dev_err(hdev, "Received corrupted packet. type: 0x%2.2x", > pkt_type); > - ret = -EILSEQ; > - goto exit_error; > + goto no_or_short_data; > } > > bt_dev_dbg(hdev, "pkt_type: 0x%2.2x len: %u", pkt_type, plen); > > - new_skb = bt_skb_alloc(plen, GFP_ATOMIC); > - if (!new_skb) { > - bt_dev_err(hdev, "Failed to allocate memory for skb of len: %u", > - skb->len); > - ret = -ENOMEM; > - goto exit_error; > - } > - > - hci_skb_pkt_type(new_skb) = pkt_type; > - skb_put_data(new_skb, skb->data, plen); > + hci_skb_pkt_type(skb) = pkt_type; > hdev->stat.byte_rx += plen; > + skb_trim(skb, plen); > > if (pcie_pkt_type == BTINTEL_PCIE_HCI_EVT_PKT) > - ret = btintel_pcie_recv_event(hdev, new_skb); > + ret = btintel_pcie_recv_event(hdev, skb); > else > - ret = hci_recv_frame(hdev, new_skb); > + ret = hci_recv_frame(hdev, skb); > > -exit_error: > +goto out; > + > +no_or_short_data: > + ret = -EILSEQ; > + kfree_skb(skb); Don't really like where this is going, having a goto label to cleanup is fine, having multiple and the having to figure out which one does what is rather confusing and doesn't help that much, anyway if we got a negative ret you can probably just make it drop the skb on the if (ret) statement. > +out: > if (ret) > hdev->stat.err_rx++; > > @@ -1192,8 +1184,6 @@ static void btintel_pcie_rx_work(struct work_struct *work) > struct btintel_pcie_data *data = container_of(work, > struct btintel_pcie_data, rx_work); > struct sk_buff *skb; > - int err; > - struct hci_dev *hdev = data->hdev; > > if (test_bit(BTINTEL_PCIE_HWEXP_INPROGRESS, &data->flags)) { > /* Unlike usb products, controller will not send hardware > @@ -1214,11 +1204,7 @@ static void btintel_pcie_rx_work(struct work_struct *work) > > /* Process the sk_buf in queue and send to the HCI layer */ > while ((skb = skb_dequeue(&data->rx_skb_q))) { > - err = btintel_pcie_recv_frame(data, skb); > - if (err) > - bt_dev_err(hdev, "Failed to send received frame: %d", > - err); > - kfree_skb(skb); > + btintel_pcie_recv_frame(data, skb); > } > } > > -- > 2.43.0 > > -- Luiz Augusto von Dentz