Hi,
> > + if (offset + length < offset ||
> > + offset + length > pci_resource_len(dev, bar)) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: virtio capability %u@%u "
> > + "out of range on bar %i length %lu\n",
> > + length, offset, bar,
> > + (unsigned long)pci_resource_len(dev, bar));
> > + return NULL;
> > + }
> > +
> > + if (limit && length > limit)
> > + length = limit;
> > +
>
> I'll have to review the above carefully. Hopefully next week.
> Any reason you didn't just move code out map_capability to a helper,
> without changes? Would have made review easier.
Doesn't work that easily as there are different things to check.
request_capability verifies the capability itself only, map_capability
has a bunch of additional range checks because it supports partial maps.
> I don't see reasons to request regions that we aren't going to
> claim ...
I had it that way first, simply calling request_mem_region from
map_capability. But then you get tons of notify entries in /proc/iomem,
and keeping track of the requested regions (for cleanup in
virtio_pci_modern_remove) also is more complicated than with a simple
set of 4 fixed regions.
> > + return request_mem_region(pci_resource_start(dev, bar) + offset,
> > + length, name);
>
> Hmm this seems wrong, resource can be IO, not just memory.
Right (not the case in the current qemu implementation, but still ...).
> > + vp_dev->res_common = request_capability(pci_dev, common, 0,
>
> Please make limit = sizeof(struct virtio_pci_common_cfg) and not 0.
>
> > + "virtio-pci-common");
> > + vp_dev->res_isr = request_capability(pci_dev, isr, 0,
> > + "virtio-pci-isr");
>
> And here, 1.
>
> > + vp_dev->res_notify = request_capability(pci_dev, notify, 0,
> > + "virtio-pci-notify");
> > + if (device) {
> > + vp_dev->res_device = request_capability(pci_dev, device,
> > + PAGE_SIZE,
> > + "virtio-pci-device");
> > + }
>
> Please drop {} around a single statement.
I'll fix them for the next version.
cheers,
Gerd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
