| 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
| ||
|
Message-ID: <20140110134426.GA7235@earth.universe>
Date: Fri, 10 Jan 2014 14:44:26 +0100
From: Sebastian Reichel <sre@...g0.de>
To: Pavel Machek <pavel@....cz>
Cc: Marcel Holtmann <marcel@...tmann.org>,
Pali Rohár <pali.rohar@...il.com>,
Ивайло Димитров
<freemangordon@....bg>, "Gustavo F. Padovan" <gustavo@...ovan.org>,
Johan Hedberg <johan.hedberg@...il.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
"linux-bluetooth@...r.kernel.org development"
<linux-bluetooth@...r.kernel.org>,
Ville Tervo <ville.tervo@...ia.com>
Subject: Re: [PATCH v4] Bluetooth: Add hci_h4p driver
Hi Pavel,
You have missed some things. See inline comments below.
On Fri, Jan 10, 2014 at 01:18:05PM +0100, Pavel Machek wrote:
> diff --git a/drivers/bluetooth/nokia_core.c b/drivers/bluetooth/nokia_core.c
> index 5c7acad..d6b0701 100644
> --- a/drivers/bluetooth/nokia_core.c
> v+++ b/drivers/bluetooth/nokia_core.c
> @@ -39,6 +39,7 @@
> #include <linux/gpio.h>
> #include <linux/timer.h>
> #include <linux/kthread.h>
> +#include <linux/io.h>
>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -1079,7 +1080,7 @@ static int hci_h4p_probe(struct platform_device *pdev)
> int err;
>
> dev_info(&pdev->dev, "Registering HCI H4P device\n");
> - info = kzalloc(sizeof(struct hci_h4p_info), GFP_KERNEL);
> + info = devm_kzalloc(&pdev->dev, sizeof(struct hci_h4p_info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
>
> @@ -1092,7 +1093,6 @@ static int hci_h4p_probe(struct platform_device *pdev)
>
> if (pdev->dev.platform_data == NULL) {
> dev_err(&pdev->dev, "Could not get Bluetooth config data\n");
> - kfree(info);
> return -ENODATA;
> }
>
> @@ -1113,67 +1113,59 @@ static int hci_h4p_probe(struct platform_device *pdev)
> complete_all(&info->test_completion);
>
> if (!info->reset_gpio_shared) {
> - err = gpio_request(info->reset_gpio, "bt_reset");
> + err = devm_gpio_request_one(&pdev->dev, info->reset_gpio,
> + GPIOF_OUT_INIT_LOW, "bt_reset");
> if (err < 0) {
> dev_err(&pdev->dev, "Cannot get GPIO line %d\n",
> info->reset_gpio);
> - goto cleanup_setup;
> + return err;
> }
> }
>
> - err = gpio_request(info->bt_wakeup_gpio, "bt_wakeup");
> + err = devm_gpio_request_one(&pdev->dev, info->bt_wakeup_gpio,
> + GPIOF_OUT_INIT_LOW, "bt_wakeup");
> +
> if (err < 0) {
> dev_err(info->dev, "Cannot get GPIO line 0x%d",
> info->bt_wakeup_gpio);
> - if (!info->reset_gpio_shared)
> - gpio_free(info->reset_gpio);
> - goto cleanup_setup;
> + return err;
> }
>
> - err = gpio_request(info->host_wakeup_gpio, "host_wakeup");
> + err = devm_gpio_request_one(&pdev->dev, info->host_wakeup_gpio,
> + GPIOF_DIR_IN, "host_wakeup");
> if (err < 0) {
> dev_err(info->dev, "Cannot get GPIO line %d",
> info->host_wakeup_gpio);
> - if (!info->reset_gpio_shared)
> - gpio_free(info->reset_gpio);
> - gpio_free(info->bt_wakeup_gpio);
> - goto cleanup_setup;
> + return err;
> }
>
> - gpio_direction_output(info->reset_gpio, 0);
> - gpio_direction_output(info->bt_wakeup_gpio, 0);
> - gpio_direction_input(info->host_wakeup_gpio);
> -
> info->irq = bt_plat_data->uart_irq;
> - info->uart_base = ioremap(bt_plat_data->uart_base, SZ_2K);
> - info->uart_iclk = clk_get(NULL, bt_plat_data->uart_iclk);
> - info->uart_fclk = clk_get(NULL, bt_plat_data->uart_fclk);
> + info->uart_base = devm_ioremap(&pdev->dev, bt_plat_data->uart_base, SZ_2K);
> + info->uart_iclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_iclk);
> + info->uart_fclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_fclk);
>
> err = request_irq(info->irq, hci_h4p_interrupt, IRQF_DISABLED, "hci_h4p",
> info);
This one should also use devm_request_irq(). Especially since you
removed the cleanup in the probe function already ;)
> if (err < 0) {
> dev_err(info->dev, "hci_h4p: unable to get IRQ %d\n", info->irq);
> - goto cleanup;
> + return err;
> }
>
> - err = request_irq(gpio_to_irq(info->host_wakeup_gpio),
> + err = devm_request_irq(&pdev->dev, gpio_to_irq(info->host_wakeup_gpio),
> hci_h4p_wakeup_interrupt, IRQF_TRIGGER_FALLING |
> IRQF_TRIGGER_RISING | IRQF_DISABLED,
> "hci_h4p_wkup", info);
> if (err < 0) {
> dev_err(info->dev, "hci_h4p: unable to get wakeup IRQ %d\n",
> gpio_to_irq(info->host_wakeup_gpio));
> - free_irq(info->irq, info);
> - goto cleanup;
> + return err;
> }
>
> err = irq_set_irq_wake(gpio_to_irq(info->host_wakeup_gpio), 1);
> if (err < 0) {
> dev_err(info->dev, "hci_h4p: unable to set wakeup for IRQ %d\n",
> gpio_to_irq(info->host_wakeup_gpio));
> - free_irq(info->irq, info);
> - free_irq(gpio_to_irq(info->host_wakeup_gpio), info);
> - goto cleanup;
> + return err;
> }
>
> init_timer_deferrable(&info->lazy_release);
> @@ -1182,7 +1174,7 @@ static int hci_h4p_probe(struct platform_device *pdev)
> hci_h4p_set_clk(info, &info->tx_clocks_en, 1);
> err = hci_h4p_reset_uart(info);
> if (err < 0)
> - goto cleanup_irq;
> + return err;
> gpio_set_value(info->reset_gpio, 0);
> hci_h4p_set_clk(info, &info->tx_clocks_en, 0);
>
> @@ -1190,23 +1182,10 @@ static int hci_h4p_probe(struct platform_device *pdev)
>
> if (hci_h4p_register_hdev(info) < 0) {
> dev_err(info->dev, "failed to register hci_h4p hci device\n");
> - goto cleanup_irq;
> + return -EINVAL;
> }
>
> return 0;
> -
> -cleanup_irq:
> - free_irq(info->irq, (void *)info);
> - free_irq(gpio_to_irq(info->host_wakeup_gpio), info);
> -cleanup:
> - gpio_set_value(info->reset_gpio, 0);
> - if (!info->reset_gpio_shared)
> - gpio_free(info->reset_gpio);
> - gpio_free(info->bt_wakeup_gpio);
> - gpio_free(info->host_wakeup_gpio);
> -cleanup_setup:
> - kfree(info);
> - return err;
> }
>
> static int hci_h4p_remove(struct platform_device *pdev)
> @@ -1217,15 +1196,11 @@ static int hci_h4p_remove(struct platform_device *pdev)
>
> hci_h4p_sysfs_remove_files(info->dev);
> hci_h4p_hci_close(info->hdev);
> - free_irq(gpio_to_irq(info->host_wakeup_gpio), info);
> hci_unregister_dev(info->hdev);
> hci_free_dev(info->hdev);
> - if (!info->reset_gpio_shared)
> - gpio_free(info->reset_gpio);
> gpio_free(info->bt_wakeup_gpio);
> gpio_free(info->host_wakeup_gpio);
those two gpios are also managed. No need to free them manually.
> free_irq(info->irq, (void *) info);
Once this irq is also requested using managed resources you can also
drop this line.
> - kfree(info);
>
> return 0;
> }
> [...]
-- Sebastian
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists