Hi, On Tue, Mar 14, 2023 at 9:34 AM Simon Horman <simon.horman@xxxxxxxxxxxx> wrote: > > On Tue, Mar 14, 2023 at 03:40:34PM +0000, Neeraj sanjay kale wrote: > > Hi Simon > > > > Thank you for reviewing the patch. I have a comment below: > > > > > > > > > +send_skb: > > > > + /* Prepend skb with frame type */ > > > > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > > > + skb_queue_tail(&nxpdev->txq, skb); > > > > + > > > > + btnxpuart_tx_wakeup(nxpdev); > > > > +ret: > > > > + return 0; > > > > + > > > > +free_skb: > > > > + kfree_skb(skb); > > > > + goto ret; > > > > > > nit: I think it would be nicer to simply return 0 here. > > > And remove the ret label entirely. > > > > > > > +} > > > > > We need to return from this function without clearing the skbs, unless "goto free_skb" is called. > > If I remove the ret label and return after kfree_skb() it causes a kernel crash. > > Keeping this change as it is. > > > > Please let me know if you have any further review comments on the v11 patch. > > I'll look over v11. > > But for the record, I meant something like this: > > send_skb: > /* Prepend skb with frame type */ > memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > skb_queue_tail(&nxpdev->txq, skb); > > btnxpuart_tx_wakeup(nxpdev); > return 0; +1, perhaps it wouldn't be a bad idea to have the code above in a separate function e.g. btnxpuart_queue_skb since this code might be common. > free_skb: > kfree_skb(skb); > return 0; > } > > > We need to return from this function without clearing the skbs, unless "goto free_skb" is called. > > If I remove the ret label and return after kfree_skb() it causes a kernel crash. > > Keeping this change as it is. > > > > Please let me know if you have any further review comments on the v11 patch. -- Luiz Augusto von Dentz
