On Thu, Jul 09, 2009 at 07:19:45PM -0700, Chris Wright wrote:
> * Michael S. Tsirkin (mst@xxxxxxxxxx) 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
