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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 09, 2009 at 07:19:45PM -0700, Chris Wright wrote:
> * 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"?

Hmm, I'd like to keep it short. the struct is private in file after all.
Longer name will not let me init variables of this type on the same
line.  generic_dev is enough to make grep/find in file happy?

> > +	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).

I still expect it to be noticeable ...

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

Good point.

> > +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?

Nope, why should it? If the device deasserts interrupt, the bridge
does not care about the reason.

> > +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?

Good point. Although I am not sure why might irq be 0,
but from code this seems possible.

> > +	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.

The reason I put it here, is because with userspace driver the device
might be left in a very strange state if you kill the driver abruptly.
With uio I don't currently get notification on open (which would be
ideal) so user will have to unbind/rebind to reset the device.
And btw, an unfortunate side affect that I didn't realise
is that this actually is restricted to PCI express devices.

So, what I think I will do is add a patch in pci-sysfs.c with a sysfs
entry that resets the device. Should be pretty useful generally.
Makes sense?

> thanks,
> -chris