svn commit: r277421 - head/sys/powerpc/powerpc

[Konstantin Belousov]

On Wed, Jan 21, 2015 at 06:04:57PM -0700, Warner Losh wrote:
> 
> > On Jan 21, 2015, at 12:54 AM, Konstantin Belousov <kostikbel at gmail.com> wrote:
> > 
> > On Tue, Jan 20, 2015 at 07:59:08PM -0800, Nathan Whitehorn wrote:
> >> 
> >> On 01/20/15 11:14, Konstantin Belousov wrote:
> >>> On Tue, Jan 20, 2015 at 04:21:59PM +0000, Nathan Whitehorn wrote:
> >>>> Author: nwhitehorn
> >>>> Date: Tue Jan 20 16:21:59 2015
> >>>> New Revision: 277421
> >>>> URL: https://svnweb.freebsd.org/changeset/base/277421
> >>>> 
> >>>> Log:
> >>>>   There does not seem to be any reason to acquire GIANT here. Follow amd64
> >>>>   in removing it.
> >>>> 
> >>>>   MFC after:	1 month
> >>>> 
> >>>> Modified:
> >>>>   head/sys/powerpc/powerpc/mem.c
> >>>> 
> >>>> Modified: head/sys/powerpc/powerpc/mem.c
> >>>> ==============================================================================
> >>>> --- head/sys/powerpc/powerpc/mem.c	Tue Jan 20 15:45:09 2015	(r277420)
> >>>> +++ head/sys/powerpc/powerpc/mem.c	Tue Jan 20 16:21:59 2015	(r277421)
> >>>> @@ -100,8 +100,6 @@ memrw(struct cdev *dev, struct uio *uio,
> >>>>  	cnt = 0;
> >>>>  	error = 0;
> >>>> 
> >>>> -	GIANT_REQUIRED;
> >>>> -
> >>> This is not an acquisition, to be pedantic.  Really, it is cdevsw which
> >>> has D_NEEDGIANT flag which acquires Giant.  After architectures get
> >>> rid of GIANT_REQUIRED, flag can be removed.
> >>> 
> >> Just so I understand, you are not objecting to this commit, right?
> > Absolutely not, this is the right thing to do.
> > 
> >> Just 
> >> pointing out that (a) my commit message was wrong and that (b) once all 
> >> architectures make this change (presumably more involved) we can get rid 
> >> of the D_NEEDGIANT in /sys/dev/mem/memdev.c?
> > Exactly.
> 
> There doesn???t seem to be a reason for i386 either. Was just looking
> at the code today on the plane for unrelated reasons.
>
I agree.  For i386, there is definitely no reason to require Giant.

I looked over arm+mips+sparc64, and I do not see a reason.  Note that
I do not know/understand their pmap implementations, but I expect that
Giant is not needed due to properties of the used (pmap) functions.

Below is the trivial patch.  Would be nice to get at least a nod from
some arch maintainers.

diff --git a/sys/arm/arm/mem.c b/sys/arm/arm/mem.c
index 460a004..30d4b1d 100644
--- a/sys/arm/arm/mem.c
+++ b/sys/arm/arm/mem.c
@@ -82,8 +82,6 @@ memrw(struct cdev *dev, struct uio *uio, int flags)
 	int error = 0;
 	vm_offset_t addr, eaddr;
 
-	GIANT_REQUIRED;
-
 	while (uio->uio_resid > 0 && error == 0) {
 		iov = uio->uio_iov;
 		if (iov->iov_len == 0) {
diff --git a/sys/dev/mem/memdev.c b/sys/dev/mem/memdev.c
index 37bad15..c3d1af1 100644
--- a/sys/dev/mem/memdev.c
+++ b/sys/dev/mem/memdev.c
@@ -52,7 +52,7 @@ static struct cdev *memdev, *kmemdev;
 
 static struct cdevsw mem_cdevsw = {
 	.d_version =	D_VERSION,
-	.d_flags =	D_MEM|D_NEEDGIANT,
+	.d_flags =	D_MEM,
 	.d_open =	memopen,
 	.d_read =	memrw,
 	.d_write =	memrw,
diff --git a/sys/i386/i386/mem.c b/sys/i386/i386/mem.c
index 9c83f47..b036bd3 100644
--- a/sys/i386/i386/mem.c
+++ b/sys/i386/i386/mem.c
@@ -86,10 +86,6 @@ memrw(struct cdev *dev, struct uio *uio, int flags)
 	int error = 0;
 	vm_offset_t addr;
 
-	/* XXX UPS Why ? */
-	GIANT_REQUIRED;
-
-
 	if (dev2unit(dev) != CDEV_MINOR_MEM && dev2unit(dev) != CDEV_MINOR_KMEM)
 		return EIO;
 
diff --git a/sys/mips/mips/mem.c b/sys/mips/mips/mem.c
index d40c424..08bb6b0 100644
--- a/sys/mips/mips/mem.c
+++ b/sys/mips/mips/mem.c
@@ -85,8 +85,6 @@ memrw(struct cdev *dev, struct uio *uio, int flags)
 	cnt = 0;
 	error = 0;
 
-	GIANT_REQUIRED;
-
 	pmap_page_init(&m);
 	while (uio->uio_resid > 0 && !error) {
 		iov = uio->uio_iov;
diff --git a/sys/sparc64/sparc64/mem.c b/sys/sparc64/sparc64/mem.c
index d09f6b8..6bd5225 100644
--- a/sys/sparc64/sparc64/mem.c
+++ b/sys/sparc64/sparc64/mem.c
@@ -99,8 +99,6 @@ memrw(struct cdev *dev, struct uio *uio, int flags)
 	error = 0;
 	ova = 0;
 
-	GIANT_REQUIRED;
-
 	while (uio->uio_resid > 0 && error == 0) {
 		iov = uio->uio_iov;
 		if (iov->iov_len == 0) {