linux-kernel - Re: [PATCH v3] Add bt8xxgpio 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]

Date:	Thu, 10 Jul 2008 20:44:21 +0200
From:	Michael Buesch <mb@...sch.de>
To:	Jiri Slaby <jirislaby@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Stephen Rothwell <sfr@...b.auug.org.au>, mchehab@...radead.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Marcel Holtmann <marcel@...tmann.org>,
	David Brownell <david-b@...bell.net>
Subject: Re: [PATCH v3] Add bt8xxgpio driver

On Thursday 10 July 2008 20:15:38 Jiri Slaby wrote:
> > +static int bt8xxgpio_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
> > +{
> > +	struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio);
> > +	unsigned long flags;
> > +	u32 outen, data;
> > +
> > +	spin_lock_irqsave(&bg->lock, flags);
> 
> Why all those irq variants? I can't see interrupts anywhere. May gpio call this 
> from irq?

Yes. Call context is undefined.

> > +
> > +	data = bgread(BT848_GPIO_DATA);
> > +	data &= ~(1 << nr);
> > +	bgwrite(data, BT848_GPIO_DATA);
> > +
> > +	outen = bgread(BT848_GPIO_OUT_EN);
> > +	outen &= ~(1 << nr);
> > +	bgwrite(outen, BT848_GPIO_OUT_EN);
> 
> some flushing of posted values here?


> > +static void bt8xxgpio_gpio_set(struct gpio_chip *gpio,
> > +			    unsigned nr, int val)
> > +{
> > +	struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio);
> > +	unsigned long flags;
> > +	u32 data;
> > +
> > +	spin_lock_irqsave(&bg->lock, flags);
> > +
> > +	data = bgread(BT848_GPIO_DATA);
> > +	if (val)
> > +		data |= (1 << nr);
> > +	else
> > +		data &= ~(1 << nr);
> > +	bgwrite(data, BT848_GPIO_DATA);
> 
> and here and further in the code

Well, I'm not sure. This is pretty damn hotpath code. It can be used
for bitbanging busdata, for example. I do actually consider optimizing
that bgread() in the function above away, by caching it in memory.
(First need to do measurements on whether it improves stuff).

So well. What about flushing. In theory it might be needed on some
hardware. But unless somebody can show me some hardware, where this
actually breaks without flushing, I won't consider adding flushes
for performance reasons.

However, an mmiowb() might be added here right before the
spinlock to make the locking safe.

> > +	spin_unlock_irqrestore(&bg->lock, flags);
> > +}
> [...]
> > +static int bt8xxgpio_probe(struct pci_dev *dev,
> > +			const struct pci_device_id *pci_id)
> > +{
> > +	struct bt8xxgpio *bg;
> > +	int err;
> > +
> > +	bg = kzalloc(sizeof(*bg), GFP_KERNEL);
> > +	if (!bg)
> > +		return -ENOMEM;
> > +
> > +	bg->pdev = dev;
> > +	spin_lock_init(&bg->lock);
> > +
> > +	err = pci_enable_device(dev);
> > +	if (err) {
> > +		printk(KERN_ERR "bt8xxgpio: Can't enable device.\n");
> 
> dev_err() et al. all over here.
> 
> > +		goto err_freebg;
> > +	}
> > +	if (!request_mem_region(pci_resource_start(dev, 0),
> > +				pci_resource_len(dev, 0),
> > +				"bt8xxgpio")) {
> 
> pci_request_region();?

Hell, I shouldn't have derived this from the bttv driver. Almost all
style comments origin from that. ;)

> > +		printk(KERN_WARNING
> > +		       "bt8xxgpio: Can't request iomem (0x%llx).\n",
> > +		       (unsigned long long)pci_resource_start(dev, 0));
> 
> You don't need to print the value out, we can see it in lspci.

Also from bttv...

-- 
Greetings Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/