lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 8 Mar 2019 10:52:32 -0800
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Balakrishna Godavarthi <bgodavar@...eaurora.org>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        hemantg@...eaurora.org, linux-arm-msm@...r.kernel.org,
        rjliao@...eaurora.org
Subject: Re: [PATCH v1] Bluetooth: hci_qca: Enable the ldisc for ROME for x86
 platforms.

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@...eaurora.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ