Date:   Fri, 8 Mar 2019 10:52:32 -0800
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Balakrishna Godavarthi <bgodavar@codeaurora.org>
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com,
        linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
        hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org,
        rjliao@codeaurora.org
Subject: Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86
 platforms.
Message-ID: <20190308185232.GB69116@google.com>
References: <20190307101722.25871-1-bgodavar@codeaurora.org>
 <20190307204224.GD138592@google.com>
 <6e0abe10b3ed44053e92ad6a21c75d93@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <6e0abe10b3ed44053e92ad6a21c75d93@codeaurora.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 08, 2019 at 10:43:14AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-03-08 02:12, Matthias Kaehlcke wrote:
> > Hi Balakrishna,
> > 
> > On Thu, Mar 07, 2019 at 03:47:22PM +0530, Balakrishna Godavarthi wrote:
> > > When using btattach to setup Rome over ldisc we observed a crash
> > > in qca_setup as it will try to access the serdev which is not
> > > available in the ldisc proto. This patch will fix the crash by
> > > support both the ldisc and serdev way in the qca hci_uart driver.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > 
> > Oh, I wasn't aware of the instantiation through ldisc and was actually
> > considering to *remove* some of the seemingly unnecessary serdev
> > checks.
> > 
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 47
> > > ++++++++++++++++++++++---------------
> > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 237aea34b69f..0a5c98d46864 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -963,7 +963,7 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >  {
> > >  	struct hci_uart *hu = hci_get_drvdata(hdev);
> > >  	struct qca_data *qca = hu->priv;
> > > -	struct qca_serdev *qcadev;
> > > +	struct qca_serdev *qcadev = NULL;
> > 
> > In many cases the only field that is accessed is qcadev->btsoc_type. I
> > think something like 'qca_get_soc_type(struct hci_dev *hdev / struct
> > hci_uart *hu)' would make things more readable.
> > 
> [Bala]: sure will update this in other patch once this change is landed as
> this has to
>         go in priority as we have crash coming.

That's not how things should work, especially for fairly trivial
changes. It requires reviewers to first spent time to review the patch
that adds clutter and later spend more time to review the one that
removes it. It's also easier to get a clean patch merged in the first
place, rather than a noisy one.

Anyway, here is my take at it: https://lore.kernel.org/patchwork/patch/1049014/

Please help with testing for ROME, unless you disagree with the
approach.

Thanks

Matthias