Re: [PATCH corrected RFC] uio: add generic driver for PCI 2.3 devices

[Chris Wright <chrisw@sous-sol.org>]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> +struct generic_dev {

I know I commented on this one on an earlier, private version, and naming
is not my strength... maybe "struct uio_generic_pci_dev" or "struct
uio_generic_pci"?

> +	struct uio_info info;
> +	struct pci_dev *pdev;
> +	spinlock_t lock; /* guards command register accesses */
> +};
> +
> +/* Read/modify/write command register to disable interrupts.
> + * Note: we could cache the value and optimize the read if there was a way to
> + * get notified of user changes to command register through sysfs.
> + * */

For the irqcontrol case, I don't think RMW is a problem (coming from
userspace it's already a slower path).

For the irqhandler case, you can grab the full dword to get Command+Status
(since you needed status anyway, and config reads are dword).

> +static void irqtoggle(struct generic_dev *dev, int irq_on)
> +{
> +	struct pci_dev *pdev = dev->pdev;
> +	unsigned long flags;
> +	u16 orig, new;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	pci_block_user_cfg_access(pdev);
> +	pci_read_config_word(pdev, PCI_COMMAND, &orig);
> +	new = irq_on ? orig & ~PCI_COMMAND_INTX_DISABLE :
> +		orig | PCI_COMMAND_INTX_DISABLE;
> +	if (new != orig)
> +		pci_write_config_word(dev->pdev, PCI_COMMAND, new);
> +	pci_unblock_user_cfg_access(dev);
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +}
> +
> +/* irqcontrol is use by userspace to enable/disable interrupts. */
> +static int irqcontrol(struct uio_info *info, s32 irq_on)
> +{
> +	struct generic_dev *dev = container_of(info, struct generic_dev, info);
> +	irqtoggle(dev, irq_on);
> +	return 0;
> +}
> +
> +static irqreturn_t irqhandler(int irq, struct uio_info *info)
> +{
> +	struct generic_dev *dev = container_of(info, struct generic_dev, info);
> +	irqreturn_t ret = IRQ_NONE;
> +	u16 status;
> +
> +	/* Check interrupt status register to see whether our device
> +	 * triggered the interrupt. */
> +	pci_read_config_word(dev->pdev, PCI_STATUS, &status);
> +	if (!(status & PCI_STATUS_INTERRUPT))
> +		goto done;
> +
> +	/* We triggered the interrupt, disable it. */
> +	irqtoggle(dev, 0);
> +	/* UIO core will signal the user process. */
> +	ret = IRQ_HANDLED;
> +done:
> +	return ret;
> +}
> +
> +/* Verify that the device supports Interrupt Disable bit in command register,
> + * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
> + * in PCI 2.2. */

Wonder if this should also restrict from things like bridges?

> +static int __devinit verify_pci_2_3(struct pci_dev *pdev)
> +{
> +	u16 orig, new;
> +	int err = 0;
> +
> +	pci_block_user_cfg_access(pdev);
> +	pci_read_config_word(pdev, PCI_COMMAND, &orig);
> +	pci_write_config_word(pdev, PCI_COMMAND,
> +			      orig ^ PCI_COMMAND_INTX_DISABLE);
> +	pci_read_config_word(pdev, PCI_COMMAND, &new);
> +	/* There's no way to protect against
> +	 * hardware bugs or detect them reliably, but as long as we know
> +	 * what the value should be, let's go ahead and check it. */
> +	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
> +		err = -EBUSY;
> +		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
> +			"driver or HW bug?\n", orig, new);
> +		goto err;
> +	}
> +	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
> +		dev_warn(&pdev->dev, "Device does not support "
> +			 "disabling interrupts: unable to bind.\n");
> +		err = -ENODEV;
> +		goto err;
> +	}
> +	/* Now restore the original value. */
> +	pci_write_config_word(pdev, PCI_COMMAND, orig);
> +err:
> +	pci_unblock_user_cfg_access(pdev);
> +	return err;
> +}
> +
> +static int __devinit probe(struct pci_dev *pdev,
> +			   const struct pci_device_id *id)
> +{
> +	struct generic_dev *dev;
> +	int err;
> +
> +	err = pci_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
> +			 __func__, err);
> +		return err;
> +	}
> +
> +	err = verify_pci_2_3(pdev);
> +	if (err)
> +		goto err_verify;
> +
> +	err = pci_request_regions(pdev, "uio_pci_generic");
> +	if (err) {
> +		dev_err(&pdev->dev, "%s: pci_request_regions failed: %d\n",
> +			 __func__, err);
> +		goto err_verify;
> +	}
> +
> +	dev = kzalloc(sizeof(struct generic_dev), GFP_KERNEL);
> +	if (!dev) {
> +		err = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	dev->info.name = "uio_pci_generic";
> +	dev->info.version = "0.01";
> +	dev->info.irq = pdev->irq;

May need to verify pdev->irq in verify too?

> +	dev->info.irq_flags = IRQF_SHARED;
> +	dev->info.handler = irqhandler;
> +	dev->info.irqcontrol = irqcontrol;
> +	dev->pdev = pdev;
> +	spin_lock_init(&dev->lock);
> +
> +	pci_reset_function(pdev);

I think this could be a bit much, since it will fall back to secondary
bus reset.

thanks,
-chris