From: Tom Lyon <pugs@lyon-about.com>
To: Joerg Roedel <joro@8bytes.org>
Cc: mst@redhat.com, hjk@linutronix.de, gregkh@suse.de,
chrisw@sous-sol.org, avi@redhat.com, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] drivers/uio/uio.c: DMA mapping, interrupt extensions, etc.
Date: Sat, 17 Apr 2010 10:04:22 -0700 [thread overview]
Message-ID: <201004171004.23155.pugs@lyon-about.com> (raw)
In-Reply-To: <20100417104309.GA23260@8bytes.org>
The current uio and uio_pci_generic allow multiple opens; I was just preserving that behavior.
On Saturday 17 April 2010 03:43:09 am Joerg Roedel wrote:
> On Thu, Apr 15, 2010 at 01:55:29PM -0700, Tom Lyon wrote:
>
> > + down(&idev->gate);
> > + if (idev->listeners == 0) { /* first open */
> > + if (idev->pmaster && !iommu_found() && !capable(CAP_SYS_RAWIO)) {
> > + up(&idev->gate);
> > + return -EPERM;
> > + }
> > + /* reset to known state if we can */
> > + if (idev->pdev)
> > + (void) pci_reset_function(idev->pdev);
> > + }
> > + idev->listeners++;
> > + up(&idev->gate);
>
> Why do you want to allow multiple opens for a device? Is it not
> sufficient to allow only one?
>
>
> > + if (idev->domain == NULL) {
> > + if (!list_empty(&listener->dm_list)) /* no mix with anywhere */
> > + return -EINVAL;
> > + if (!iommu_found())
> > + return -EINVAL;
> > + idev->domain = iommu_domain_alloc();
> > + if (idev->domain == NULL)
> > + return -ENXIO;
> > + idev->cachec = iommu_domain_has_cap(idev->domain,
> > + IOMMU_CAP_CACHE_COHERENCY);
> > + ret = iommu_attach_device(idev->domain, idev->dev->parent);
> > + if (ret) {
> > + iommu_domain_free(idev->domain);
> > + idev->domain = NULL;
> > + printk(KERN_ERR "%s: device_attach failed %d\n", __func__, ret);
> > + return ret;
> > + }
> > + }
>
> If userspace calls this path this will make all the addresses mapped
> with DMA-API paths unusable by the device. This doesn't look like a sane
> userspace interface.
>
> For better and more in-depth review I suggest that you split up this
> large patch into a series of smaler which implement specific aspects of
> your work.
>
> Joerg
>
> P.S.: I got these warning when applying your patches ...
>
> Applying: drivers/uio/uio_pci_generic.c: allow access for non-privileged processes
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:86: trailing whitespace.
> info->mem[j].name = name;
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:87: trailing whitespace.
> info->mem[j].addr = pci_resource_start(pdev, i);
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:89: trailing whitespace.
> info->mem[j].memtype = UIO_MEM_PHYS;
> warning: 3 lines add whitespace errors.
> Applying: drivers/uio/uio.c: DMA mapping, interrupt extensions, etc.
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:315: trailing whitespace.
> ret = iommu_map_range(idev->domain, iova,
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:325: trailing whitespace.
> }
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:366: trailing whitespace.
> * adjacent pages, but noone seems to really do that. So we squash
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:368: trailing whitespace.
> * This works if (a) there is an iommu, or (b) the user allocates
> /home/joro/src/linux.trees.git/.git/rebase-apply/patch:578: trailing whitespace.
> unsigned int cmd, unsigned long arg)
> warning: squelched 1 whitespace error
> warning: 6 lines add whitespace errors.
>
> And there are also some coding-style issues.
>
next prev parent reply other threads:[~2010-04-17 17:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-04-15 20:55 [PATCH V2] drivers/uio/uio.c: DMA mapping, interrupt extensions, etc Tom Lyon
2010-04-17 10:43 ` Joerg Roedel
2010-04-17 17:04 ` Tom Lyon [this message]
2010-04-17 18:34 ` Avi Kivity
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201004171004.23155.pugs@lyon-about.com \
--to=pugs@lyon-about.com \
--cc=avi@redhat.com \
--cc=chrisw@sous-sol.org \
--cc=gregkh@suse.de \
--cc=hjk@linutronix.de \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).