Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 23 May 2022 18:16:40 +0200
From:   Johan Hovold <johan@kernel.org>
To:     frank zago <frank@zago.net>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Wolfram Sang <wsa@kernel.org>, linux-usb@vger.kernel.org,
        Lee Jones <lee.jones@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org
Subject: Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the
 CH341
Message-ID: <YouzaO6ogxYj40Bp@hovoldconsulting.com>
References: <20220401023306.79532-1-frank@zago.net>
 <20220401023306.79532-3-frank@zago.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220401023306.79532-3-frank@zago.net>
Precedence: bulk

On Thu, Mar 31, 2022 at 09:33:05PM -0500, frank zago wrote:
> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.
> 
> Signed-off-by: frank zago <frank@zago.net>
> ---

> +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 */
> +	union {
> +		u8 gpio_buf[SEG_SIZE];
> +		__le16 gpio_buf_status;
> +	};
> +
> +	struct urb *irq_urb;
> +	struct usb_anchor irq_urb_out;
> +	u8 irq_buf[CH341_USB_MAX_INTR_SIZE];
> +	struct irq_chip irq_chip;
> +
> +	struct ch341_device *ch341;
> +};

> +static void ch341_complete_intr_urb(struct urb *urb)
> +{
> +	struct ch341_gpio *dev = urb->context;
> +	int rc;
> +
> +	if (urb->status) {
> +		usb_unanchor_urb(dev->irq_urb);

URBs are unanchored by USB core on completion.

> +	} else {
> +		/*
> +		 * Data is 8 bytes. Byte 0 might be the length of
> +		 * significant data, which is 3 more bytes. Bytes 1
> +		 * and 2, and possibly 3, are the pin status. The byte
> +		 * order is different than for the GET_STATUS
> +		 * command. Byte 1 is GPIOs 8 to 15, and byte 2 is
> +		 * GPIOs 0 to 7.
> +		 */
> +
> +		handle_nested_irq(irq_find_mapping(dev->gpio.irq.domain,
> +						   CH341_GPIO_INT_LINE));
> +
> +		rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> +		if (rc)
> +			usb_unanchor_urb(dev->irq_urb);
> +	}
> +}

> +static void ch341_gpio_irq_enable(struct irq_data *data)
> +{
> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> +	int rc;
> +
> +	/*
> +	 * The URB might have just been unlinked in
> +	 * ch341_gpio_irq_disable, but the completion handler hasn't
> +	 * been called yet.
> +	 */
> +	if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
> +		usb_kill_anchored_urbs(&dev->irq_urb_out);
> +
> +	usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
> +	rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> +	if (rc)
> +		usb_unanchor_urb(dev->irq_urb);

This looks confused and broken.

usb_kill_anchored_urbs() can sleep so either calling it is broken or
using GFP_ATOMIC is unnecessary.

And isn't this function called multiple times when enabling more than
one irq?!

> +}
> +
> +static void ch341_gpio_irq_disable(struct irq_data *data)
> +{
> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> +
> +	usb_unlink_urb(dev->irq_urb);

Same here...

> +}
> +
> +static int ch341_gpio_remove(struct platform_device *pdev)
> +{
> +	struct ch341_gpio *dev = platform_get_drvdata(pdev);
> +
> +	usb_kill_anchored_urbs(&dev->irq_urb_out);

You only have one URB...

And what prevents it from being resubmitted here?

> +	gpiochip_remove(&dev->gpio);
> +	usb_free_urb(dev->irq_urb);
> +
> +	return 0;
> +}
> +
> +static int ch341_gpio_probe(struct platform_device *pdev)
> +{
> +	struct ch341_device *ch341 = dev_get_drvdata(pdev->dev.parent);
> +	struct gpio_irq_chip *girq;
> +	struct ch341_gpio *dev;
> +	struct gpio_chip *gpio;
> +	int rc;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (dev == NULL)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, dev);
> +	dev->ch341 = ch341;
> +	mutex_init(&dev->gpio_lock);
> +
> +	gpio = &dev->gpio;
> +	gpio->label = dev_name(&pdev->dev);
> +	gpio->parent = &pdev->dev;
> +	gpio->owner = THIS_MODULE;
> +	gpio->get_direction = ch341_gpio_get_direction;
> +	gpio->direction_input = ch341_gpio_direction_input;
> +	gpio->direction_output = ch341_gpio_direction_output;
> +	gpio->get = ch341_gpio_get;
> +	gpio->get_multiple = ch341_gpio_get_multiple;
> +	gpio->set = ch341_gpio_set;
> +	gpio->set_multiple = ch341_gpio_set_multiple;
> +	gpio->base = -1;
> +	gpio->ngpio = CH341_GPIO_NUM_PINS;
> +	gpio->can_sleep = true;
> +
> +	dev->irq_chip.name = dev_name(&pdev->dev);
> +	dev->irq_chip.irq_set_type = ch341_gpio_irq_set_type;
> +	dev->irq_chip.irq_enable = ch341_gpio_irq_enable;
> +	dev->irq_chip.irq_disable = ch341_gpio_irq_disable;
> +
> +	girq = &gpio->irq;
> +	girq->chip = &dev->irq_chip;
> +	girq->handler = handle_simple_irq;
> +	girq->default_type = IRQ_TYPE_NONE;
> +
> +	/* Create an URB for handling interrupt */
> +	dev->irq_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!dev->irq_urb)
> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Cannot allocate the int URB\n");
> +
> +	usb_fill_int_urb(dev->irq_urb, ch341->usb_dev,
> +			 usb_rcvintpipe(ch341->usb_dev, ch341->ep_intr),
> +			 dev->irq_buf, CH341_USB_MAX_INTR_SIZE,
> +			 ch341_complete_intr_urb, dev, ch341->ep_intr_interval);
> +
> +	init_usb_anchor(&dev->irq_urb_out);
> +
> +	rc = gpiochip_add_data(gpio, dev);
> +	if (rc) {
> +		rc = dev_err_probe(&pdev->dev, rc, "Could not add GPIO\n");
> +		goto release_urb;
> +	}
> +
> +	return 0;
> +
> +release_urb:
> +	usb_free_urb(dev->irq_urb);
> +
> +	return rc;
> +}

Johan