by Simon Horman

[permalink] [raw]

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

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.

2023-03-14 19:31:00

by Luiz Augusto von Dentz

[permalink] [raw]

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

Hi,

On Tue, Mar 14, 2023 at 9:34 AM Simon Horman <[email protected]> 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