linux-kernel - Re: [PATCH 2/5] gpio: Congatec Board Controller gpio driver

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: <34ba62f8-dc90-4a8a-914d-30df35d2956a@bootlin.com>
Date: Mon, 19 Aug 2024 18:11:27 +0200
From: Thomas Richard <thomas.richard@...tlin.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Lee Jones <lee@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
 Andi Shyti <andi.shyti@...nel.org>, Wim Van Sebroeck
 <wim@...ux-watchdog.org>, Guenter Roeck <linux@...ck-us.net>,
 linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
 linux-i2c@...r.kernel.org, linux-watchdog@...r.kernel.org,
 thomas.petazzoni@...tlin.com, blake.vermeer@...sight.com
Subject: Re: [PATCH 2/5] gpio: Congatec Board Controller gpio driver

>> +static int cgbc_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
>> +       struct cgbc_device_data *cgbc = gpio->cgbc;
>> +       int ret;
>> +       u8 val;
>> +
> 
> Can you use scoped_guard() here and elsewhere?

Hi Bartosz,

Thanks for the review.

For the next iteration I added scoped_guard() in cgbc_gpio_get(), and
guard() in cgbc_gpio_set(), cgbc_gpio_direction_input(), and
cgbc_gpio_direction_output().

> 
>> +       mutex_lock(&gpio->lock);
>> +
>> +       ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_GET, (offset > 7) ? 1 : 0, 0, &val);
>> +
>> +       mutex_unlock(&gpio->lock);
>> +
>> +       offset %= 8;
>> +
>> +       if (ret)
>> +               return ret;

...

>> +static int cgbc_gpio_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct cgbc_device_data *cgbc = dev_get_drvdata(dev->parent);
>> +       struct cgbc_gpio_data *gpio;
>> +       struct gpio_chip *chip;
>> +       int ret;
>> +
>> +       gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
>> +       if (!gpio)
>> +               return -ENOMEM;
>> +
>> +       gpio->cgbc = cgbc;
>> +
>> +       platform_set_drvdata(pdev, gpio);
>> +
>> +       chip = &gpio->chip;
>> +       chip->label = dev_name(&pdev->dev);
>> +       chip->owner = THIS_MODULE;
>> +       chip->parent = dev;
>> +       chip->base = -1;
>> +       chip->direction_input = cgbc_gpio_direction_input;
>> +       chip->direction_output = cgbc_gpio_direction_output;
>> +       chip->get_direction = cgbc_gpio_get_direction;
>> +       chip->get = cgbc_gpio_get;
>> +       chip->set = cgbc_gpio_set;
>> +       chip->ngpio = CGBC_GPIO_NGPIO;
>> +
>> +       mutex_init(&gpio->lock);
> 
> Please use devm_mutex_init() so that it gets cleaned up at exit. It's
> not strictly necessary but helps with lock debugging.

Fixed in the next iteration.

Regards,

Thomas

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com