MIME-Version: 1.0
References: <20230313144028.3156825-1-neeraj.sanjaykale@nxp.com>
 <20230313144028.3156825-4-neeraj.sanjaykale@nxp.com> <ZBBUYDhrnn/udT+Z@corigine.com>
 <AM9PR04MB8603E3F3900DB13502CFCB8DE7BE9@AM9PR04MB8603.eurprd04.prod.outlook.com>
 <ZBCh98lGvhlMKQQp@corigine.com>
In-Reply-To: <ZBCh98lGvhlMKQQp@corigine.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 14 Mar 2023 12:29:55 -0700
Message-ID: <CABBYNZ+Zx8r-yJD9qq6B0oiDKTjzrvRa2Je0=C1hBjKvMyjmgA@mail.gmail.com>
Subject: Re: [PATCH v10 3/3] Bluetooth: NXP: Add protocol support for NXP
 Bluetooth chipsets
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Neeraj sanjay kale <neeraj.sanjaykale@nxp.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "marcel@holtmann.org" <marcel@holtmann.org>,
        "johan.hedberg@gmail.com" <johan.hedberg@gmail.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "jirislaby@kernel.org" <jirislaby@kernel.org>,
        "alok.a.tiwari@oracle.com" <alok.a.tiwari@oracle.com>,
        "hdanton@sina.com" <hdanton@sina.com>,
        "ilpo.jarvinen@linux.intel.com" <ilpo.jarvinen@linux.intel.com>,
        "leon@kernel.org" <leon@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        Amitkumar Karwar <amitkumar.karwar@nxp.com>,
        Rohit Fule <rohit.fule@nxp.com>,
        Sherry Sun <sherry.sun@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Tue, Mar 14, 2023 at 9:34=E2=80=AFAM Simon Horman <simon.horman@corigine=
.com> 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 kern=
el crash.
> > Keeping this change as it is.
> >
> > Please let me know if you have any further review comments on the v11 p=
atch.
>
> 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 kern=
el crash.
> > Keeping this change as it is.
> >
> > Please let me know if you have any further review comments on the v11 p=
atch.


--=20
Luiz Augusto von Dentz