On Monday 12 October 2009 07:19:52 Javier Martinez Canillas wrote:
> gpiochip_remove is prototiped with __must_check but bt8xxgpio is not checking this function result.
>
> This is a patch that checks the result value though I'm not sure if it is the correct semantics.
>
> Signed-off-by: Javier Martinez Canillas <martinez.javier@xxxxxxxxx>
I still think it's wrong for gpiochip_remove to fail at all. It should return void
and its internals should be fixed up to never fail.
What your patch does is just add more mess to the situation, IMO. It will leak
device state and kernel resources in case gpiochip_remove failed. I'm not sure
if that's really better than just ignoring the failure. It trades one bug for another.
In my opinion a deallocation/unregistering/removal function must never fail
and the subsystem must be designed from the ground up so that it won't fail.
It must not fail, because in 90% of the usecases we can't handle it.
Think of a module-exit handler, for example. There's no way it can fail. If that
handler calls some kind of an unregister function like gpiochip_remove that can fail,
there's really _nothing_ it can do about it. And that's actually the case here in
this specific driver.
> ---
> drivers/gpio/bt8xxgpio.c | 33 +++++++++++++++++++--------------
> 1 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/bt8xxgpio.c b/drivers/gpio/bt8xxgpio.c
> index 2559f22..b4aa126 100644
> --- a/drivers/gpio/bt8xxgpio.c
> +++ b/drivers/gpio/bt8xxgpio.c
> @@ -242,20 +242,25 @@ err_freebg:
> static void bt8xxgpio_remove(struct pci_dev *pdev)
> {
> struct bt8xxgpio *bg = pci_get_drvdata(pdev);
> -
> - gpiochip_remove(&bg->gpio);
> -
> - bgwrite(0, BT848_INT_MASK);
> - bgwrite(~0x0, BT848_INT_STAT);
> - bgwrite(0x0, BT848_GPIO_OUT_EN);
> -
> - iounmap(bg->mmio);
> - release_mem_region(pci_resource_start(pdev, 0),
> - pci_resource_len(pdev, 0));
> - pci_disable_device(pdev);
> -
> - pci_set_drvdata(pdev, NULL);
> - kfree(bg);
> + int ret;
> +
> + ret = gpiochip_remove(&bg->gpio);
> +
> + if (!ret) {
> + bgwrite(0, BT848_INT_MASK);
> + bgwrite(~0x0, BT848_INT_STAT);
> + bgwrite(0x0, BT848_GPIO_OUT_EN);
> +
> + iounmap(bg->mmio);
> + release_mem_region(pci_resource_start(pdev, 0),
> + pci_resource_len(pdev, 0));
> + pci_disable_device(pdev);
> +
> + pci_set_drvdata(pdev, NULL);
> + kfree(bg);
> + } else
> + dev_err(&pdev->dev, "Failed to remove the GPIO controller: %d\n",
> + ret);
> }
>
> #ifdef CONFIG_PM
--
Greetings, Michael.
--
To unsubscribe from this list: send an email with
"unsubscribe kernelnewbies" to ecartis@xxxxxxxxxxxx
Please read the FAQ at http://kernelnewbies.org/FAQ
