Re: [PATCH v10 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

[Simon Horman <simon.horman@corigine.com>]

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;

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.