linux-kernel - Re: bluetooth: Add hci

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]

Message-ID: <20141219095000.GA24703@amd>
Date:	Fri, 19 Dec 2014 10:50:00 +0100
From:	Pavel Machek <pavel@....cz>
To:	Marcel Holtmann <marcel@...tmann.org>
Cc:	Pali Rohár <pali.rohar@...il.com>,
	Sebastian Reichel <sre@...ian.org>,
	Sebastian Reichel <sre@...g0.de>,
	kernel list <linux-kernel@...r.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	linux-omap <linux-omap@...r.kernel.org>,
	Tony Lindgren <tony@...mide.com>, khilman@...nel.org,
	Aaro Koskinen <aaro.koskinen@....fi>,
	Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>,
	linux-bluetooth@...r.kernel.org
Subject: Re: bluetooth: Add hci_h4p driver

Hi!

> And you want to set the QUIRK_INVALID_BADDR. At least you want to do that in the final submission.
> 

Ok, I found out that I can do it and result works, provided that I do
hciconfig hci0 up/down first.

I have trouble understanding... h4p_hci_open() seems to be called as
soon as I insmod the driver. Who does that? Is it some kind of udev
magic?

...
> > +#include "hci_h4p.h"
> > +
> > +#define BT_DBG(a...) do {} while(0)
> 
> Why is this needed? BT_DBG is hooked up with dynamic debug.
...

I did all the changes as requested. Thanks. I did't see harm in
include guards, but I removed them; it is not important.

> > +	if (info->rx_count == 0) {
> > +		/* H4+ devices should always send word aligned packets */
> > +		if (!(info->rx_skb->len % 2))
> > +			info->garbage_bytes++;
> > +		h4p_recv_frame(info, info->rx_skb);
> 
> The h4p_recv_frame should maybe done inline here since it only handles 2 exception packets. Also to make it easy, just leave the function if (info->rx_count).
> 
> This packet processing feels like a bit of spaghetti code. I think that is better handled in a proper function that goes through it and not with many tiny sub functions.
>

Well, CodingStyle says something about functions that should be kept
short... And when manually inlined, the inner function would have to
be made less readable, as it uses return to shortcut processing. 


To use __hci_cmd_sync()

> > +
> > +	SET_HCIDEV_DEV(hdev, info->dev);
> > +
> > +	if (hci_register_dev(hdev) >= 0)
> > +		return 0;
> > +
> > +	dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
> > +	hci_free_dev(info->hdev);
> > +	return -ENODEV;
> > +}
> 
> And normally this is folded into the probe handling and not in a
> separate function.

The probe function is too long, already...

> > +#include <linux/delay.h>
> > +#include <linux/clk.h>
> > +
> > +#include <linux/io.h>
> > +
> > +#include "hci_h4p.h"
> 
> Why is this a separate file? And if so, why not all UART specific details are in this file. Including bunch of the defines you have in the header.
> 

Well, you wanted less global functions, so I moved some as inlines to
headers. I guess I can merge nokia_core and nokia_uart if really wanted.

I did rest of requested changes.

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/