| 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: <CAHp75VdfEheEPWBDb+3FUwmwGx_4NR8o+SMwTwjgPr7oGGM5-A@mail.gmail.com>
Date: Mon, 21 Mar 2022 19:06:10 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: frank zago <frank@...o.net>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Wolfram Sang <wsa@...nel.org>, Johan Hovold <johan@...nel.org>,
USB <linux-usb@...r.kernel.org>,
Lee Jones <lee.jones@...aro.org>,
Linus Walleij <linus.walleij@...aro.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
linux-i2c <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] gpio: ch341: add MFD cell driver for the CH341
On Mon, Mar 21, 2022 at 4:13 PM frank zago <frank@...o.net> wrote:
>
> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.
We use terminology of output-only and input-only. Is it what you are
telling us? If it's something else, you have to elaborate much better
on what's going on with these GPIO lines.
...
> +config GPIO_CH341
> + tristate "CH341 USB adapter in GPIO/I2C/SPI mode"
How is this driver related to either SPI or I²C modes?
> + depends on MFD_CH341
Can't be compile tested?
> + help
> + If you say yes to this option, GPIO support will be included for the
> + WCH CH341, a USB to I2C/SPI/GPIO interface.
> +
> + This driver can also be built as a module. If so, the module
> + will be called gpio-ch341.
...
> +/* Notes.
Keep the proper (not network) style for multi-line comments.
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/gpio.h>
> +#include <linux/mfd/ch341.h>
If I got your intention with groups of headers, I would see rather
...ordered headers...
blank line
#include <linux/gpio.h>
But more importantly that gpio.h is the wrong header and must be
replaced with the appropriate one from the include/gpio/ folder.
Also you have missed headers, like types.h.
...
> +#define CH341_PARA_CMD_STS 0xA0 /* Get pins status */
> +#define CH341_CMD_UIO_STREAM 0xAB /* UIO stream command */
> +
> +#define CH341_CMD_UIO_STM_OUT 0x80 /* UIO interface OUT command (D0~D5) */
> +#define CH341_CMD_UIO_STM_DIR 0x40 /* UIO interface DIR command (D0~D5) */
> +#define CH341_CMD_UIO_STM_END 0x20 /* UIO interface END command */
What does UIO mean here? If it is Userspace I/O in terms of Linux
kernel, it's no-go we want this. Otherwise needs to be explained
somewhere.
...
> +struct ch341_gpio {
> + struct gpio_chip gpio;
> + struct mutex gpio_lock;
> + u16 gpio_dir; /* 1 bit per pin, 0=IN, 1=OUT. */
> + u16 gpio_last_read; /* last GPIO values read */
> + u16 gpio_last_written; /* last GPIO values written */
> + u8 gpio_buf[SEG_SIZE];
> +
> + struct {
> + char name[32];
> + bool enabled;
> + struct irq_chip irq;
> + int num;
> + struct urb *urb;
> + struct usb_anchor urb_out;
> + u8 buf[CH341_USB_MAX_INTR_SIZE];
> + } gpio_irq;
We have a specific GPIO IRQ chip structure, what is the purpose of
semi-duplication of it?
> +
> + struct ch341_device *ch341;
> +};
...
> +static void ch341_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> + struct ch341_gpio *dev = gpiochip_get_data(chip);
> +
> + seq_printf(s, "pin config : %04x (0=IN, 1=OUT)\n", dev->gpio_dir);
> + seq_printf(s, "last read : %04x\n", dev->gpio_last_read);
> + seq_printf(s, "last written: %04x\n", dev->gpio_last_written);
Multi-line debug output is quite non-standard among GPIO drivers.
> +}
> +{
> + struct ch341_device *ch341 = dev->ch341;
> + int actual;
> + int rc;
> +
> + mutex_lock(&ch341->usb_lock);
> +
> + rc = usb_bulk_msg(ch341->usb_dev,
> + usb_sndbulkpipe(ch341->usb_dev, ch341->ep_out),
> + dev->gpio_buf, out_len,
> + &actual, DEFAULT_TIMEOUT);
> + if (rc < 0)
> + goto done;
> +
> + if (in_len == 0) {
> + rc = actual;
> + goto done;
> + }
You may do it better. See below.
> + rc = usb_bulk_msg(ch341->usb_dev,
> + usb_rcvbulkpipe(ch341->usb_dev, ch341->ep_in),
> + dev->gpio_buf, SEG_SIZE, &actual, DEFAULT_TIMEOUT);
> +
> + if (rc == 0)
> + rc = actual;
> +done:
out_unlock: sounds better.
> + mutex_unlock(&ch341->usb_lock);
> + return rc;
if (rc < 0)
return rc;
return actual;
> +}
...
> + int result;
rc / result / etc... Please, become consistent in naming the return
code variable.
...
> + if (result == 6)
> + dev->gpio_last_read = le16_to_cpu(*(__le16 *)dev->gpio_buf);
So, it means you have the wrong type of gpio_but. Also you missed the
pointer versions of leXX_to_cpu() helpers.
...
> + return (result != 6) ? result : 0;
Besides redundant parentheses, this can be optimized. I will leave it
for your homework (the hint is given at the top part of the review).
...
> + return (dev->gpio_last_read & BIT(offset)) ? 1 : 0;
!! can be used. But it's up to you and maintainers, the compiler will
do its job anyway.
...
> + dev->gpio_last_written &= ~*mask;
> + dev->gpio_last_written |= (*bits & *mask);
Can be done in one line as it's a well established pattern in Linux
kernel for drivers.
...
> + return (dev->gpio_dir & BIT(offset)) ? 0 : 1;
! will do the job.
...
> + if (!(pin_can_output & mask))
> + return -EINVAL;
I don't remember if we have a valid mask for this case.
...
> + if (!urb->status) {
Will be much better to simply do:
if (urb_status) {
...
return;
}
> + } else {
> + usb_unanchor_urb(dev->gpio_irq.urb);
> + }
...
> + if (data->irq != dev->gpio_irq.num || type != IRQ_TYPE_EDGE_RISING)
> + return -EINVAL;
Usually we lock the handler type here while in ->probe() we assign a
bad handler by default in order to filter out spurious interrupts.
...
> + dev->gpio_irq.enabled = true;
What is the purpose of this flag? Note there is a patch to add a
specific flag to the descriptor to do exactly this.
...
> +/* Convert the GPIO index to the IRQ number */
> +static int ch341_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct ch341_gpio *dev = gpiochip_get_data(chip);
> +
> + if (offset != CH341_GPIO_INT_LINE)
> + return -ENXIO;
> +
> + return dev->gpio_irq.num;
In the new code we will have the special field that limits the GPIO
IRQ lines (can be different to the ngpio).
> +}
...
> + snprintf(dev->gpio_irq.name, sizeof(dev->gpio_irq.name),
> + "ch341-%s-gpio", dev_name(&ch341->usb_dev->dev));
> + dev->gpio_irq.name[sizeof(dev->gpio_irq.name) - 1] = 0;
This is redundant. Have you read the manual page on snprintf()?
...
> + rc = devm_irq_alloc_desc(&pdev->dev, 0);
> + if (rc < 0) {
> + dev_err(&pdev->dev, "Cannot allocate an IRQ desc");
> + return rc;
return dev_err_probe();
> + }
> +
> + dev->gpio_irq.num = rc;
> + dev->gpio_irq.enabled = false;
> +
> + irq_set_chip_data(dev->gpio_irq.num, dev);
> + irq_set_chip_and_handler(dev->gpio_irq.num, &dev->gpio_irq.irq,
> + handle_simple_irq);
Oh là là. Can you use the latest and greatest approach of
instantiating the GPIO IRQ chip?
...
> + dev_err(&pdev->dev, "Cannot allocate the int URB");
> + return -ENOMEM;
return dev_err_probe();
...
> + rc = gpiochip_add_data(gpio, dev);
Why not devm?
> + if (rc) {
> + dev_err(&pdev->dev, "Could not add GPIO\n");
> + goto release_urb;
return dev_err_probe();
> + }
...
> +static struct platform_driver ch341_gpio_driver = {
> + .driver.name = "ch341-gpio",
> + .probe = ch341_gpio_probe,
> + .remove = ch341_gpio_remove,
> +};
> +
Redundant blank line.
> +module_platform_driver(ch341_gpio_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists