netdev - Re: [PATCH 10/12] bluetooth: 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: <20250213-fine-thankful-lobster-eed3e8@krzk-bin>
Date: Thu, 13 Feb 2025 10:24:13 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Alexis Lothoré <alexis.lothore@...tlin.com>
Cc: Marcel Holtmann <marcel@...tmann.org>, 
	Luiz Augusto von Dentz <luiz.dentz@...il.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Ajay Singh <ajay.kathat@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>, 
	Kalle Valo <kvalo@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Nicolas Ferre <nicolas.ferre@...rochip.com>, Alexandre Belloni <alexandre.belloni@...tlin.com>, 
	Marek Vasut <marex@...x.de>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
	linux-bluetooth@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 10/12] bluetooth: hci_wilc: add wilc hci driver

On Wed, Feb 12, 2025 at 04:46:29PM +0100, Alexis Lothoré wrote:
> +#include "linux/bitops.h"
> +#include "linux/byteorder/generic.h"
> +#include "linux/err.h"
> +#include "linux/gfp_types.h"
> +#include "net/bluetooth/bluetooth.h"
> +#include "net/bluetooth/hci.h"

Keep some order here. Why some are <> some "", why net is mixed with
linux...

> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/of.h>
> +#include <linux/serdev.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +#include <net/wilc.h>
> +
> +#include "hci_uart.h"
> +

...

> +static const struct hci_uart_proto wilc_bt_proto = {
> +	.id = HCI_UART_WILC,
> +	.name = "Microchip",
> +	.manufacturer = WILC_BT_UART_MANUFACTURER,
> +	.init_speed = WILC_UART_DEFAULT_BAUDRATE,
> +	.open = wilc_open,
> +	.close = wilc_close,
> +	.flush = wilc_flush,
> +	.recv = wilc_recv,
> +	.enqueue = wilc_enqueue,
> +	.dequeue = wilc_dequeue,
> +	.setup = wilc_setup,
> +	.set_baudrate = wilc_set_baudrate,
> +};
> +
> +static int wilc_bt_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct wilc_adapter *wilc_adapter;
> +	struct device_node *wlan_node;
> +	void *wlan = NULL;
> +	int ret;
> +
> +	wilc_adapter = kzalloc(sizeof(*wilc_adapter), GFP_KERNEL);

Why not devm?

> +	if (!wilc_adapter)
> +		return -ENOMEM;
> +
> +	wlan_node = of_parse_phandle(serdev->dev.of_node, "wlan", 0);
> +	if (!wlan_node) {
> +		BT_ERR("Can not run wilc bluetooth without wlan node");
> +		ret = -EINVAL;
> +		goto exit_free_adapter;
> +	}
> +
> +#if IS_ENABLED(CONFIG_WILC1000_SDIO)
> +	wlan = wilc_sdio_get_byphandle(wlan_node);
> +#endif
> +#if IS_ENABLED(CONFIG_WILC1000_SPI)
> +	if (!wlan || wlan == ERR_PTR(-EPROBE_DEFER))
> +		wlan = wilc_spi_get_byphandle(wlan_node);
> +#endif
> +	if (IS_ERR(wlan)) {
> +		pr_warn("Can not initialize bluetooth: %pe\n", wlan);

dev_warn or even dev_err_probe to handle deferral.


> +		ret = PTR_ERR(wlan);
> +		goto exit_put_wlan_node;
> +	}
> +
> +	of_node_put(wlan_node);
> +	wilc_adapter->wlan_priv = wlan;
> +	ret = wilc_bt_init(wlan);
> +	if (ret) {
> +		pr_err("Failed to initialize bluetooth firmware (%d)\n", ret);

dev_err_probe

> +		goto exit_put_wlan;
> +	}
> +
> +	wilc_adapter->dev = &serdev->dev;
> +	wilc_adapter->hu.serdev = serdev;
> +	wilc_adapter->flow_control = false;
> +	serdev_device_set_drvdata(serdev, wilc_adapter);
> +	ret = hci_uart_register_device(&wilc_adapter->hu, &wilc_bt_proto);
> +	if (ret) {
> +		dev_err(&serdev->dev, "Failed to register hci device");
> +		goto exit_deinit_bt;
> +	}
> +
> +	dev_info(&serdev->dev, "WILC hci interface registered");

Drop simple probe statuses. sysfs already provides this.


> +	return 0;
> +
> +exit_deinit_bt:
> +	wilc_bt_shutdown(wlan);
> +exit_put_wlan:
> +	wilc_put(wlan);
> +exit_put_wlan_node:
> +	of_node_put(wlan_node);
> +exit_free_adapter:
> +	kfree(wilc_adapter);
> +	return ret;
> +}
> +
> +static void wilc_bt_serdev_remove(struct serdev_device *serdev)
> +{
> +	struct wilc_adapter *wilc_adapter = serdev_device_get_drvdata(serdev);
> +
> +	hci_uart_unregister_device(&wilc_adapter->hu);
> +	wilc_bt_shutdown(wilc_adapter->wlan_priv);
> +	wilc_put(wilc_adapter->wlan_priv);
> +	kfree(wilc_adapter);
> +}
> +
> +static const struct of_device_id wilc_bt_of_match[] = {
> +	{ .compatible = "microchip,wilc3000-bt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, wilc_bt_of_match);
> +
> +static struct serdev_device_driver wilc_bt_serdev_driver = {
> +	.probe = wilc_bt_serdev_probe,
> +	.remove = wilc_bt_serdev_remove,
> +	.driver = {
> +		.name = "hci_uart_wilc",
> +		.of_match_table = of_match_ptr(wilc_bt_of_match),

Drop of_match_tr, you have warnings here.

Best regards,
Krzysztof