[PATCH 00/18] staging: gasket: sundry fixes and fixups

[PATCH 00/18] staging: gasket: sundry fixes and fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Various fixes mainly from the chromium review of the gasket and apex
drivers.  More to come.

Various (18):
  staging: gasket: remove X86 Kconfig restriction
  MAINTAINERS: Add maintainer for drivers/staging/gasket
  staging: gasket: typo and whitespace cleanups
  staging: gasket: device registration error and unregister fixups
  staging: gasket: sysfs mapping creation fixups
  staging: gasket: fix deadlock in pci driver unregister path
  staging: gasket: convert gasket_mmap_has_permissions to bool return
  staging: gasket: gasket_wait_with_reschedule fixups
  staging: gasket: bail out of reset sequence on device callback error
  staging: gasket: gasket_open use container_of()
  staging: gasket: always allow root open for write
  staging: gasket: annotate ioctl arg with __user
  staging: gasket: gasket_enable_dev fixups
  staging: gasket: fix class create bug handling
  staging: gasket: remove unnecessary code in coherent allocator
  staging: gasket: gasket core error handling fixups
  staging: gasket: don't release coherent mappings
  staging: gasket: various cleanups

 MAINTAINERS                                |   1 +
 drivers/staging/gasket/Kconfig             |   2 +-
 drivers/staging/gasket/apex.h              |   7 +-
 drivers/staging/gasket/apex_driver.c       |  79 +++++++--------
 drivers/staging/gasket/gasket_core.c       | 107 ++++++++++-----------
 drivers/staging/gasket/gasket_core.h       |  11 ++-
 drivers/staging/gasket/gasket_ioctl.c      | 100 ++++++++++---------
 drivers/staging/gasket/gasket_ioctl.h      |   4 +-
 drivers/staging/gasket/gasket_page_table.c |  69 +++++++------
 drivers/staging/gasket/gasket_page_table.h |   8 +-
 drivers/staging/gasket/gasket_sysfs.c      |   4 +-
 11 files changed, 198 insertions(+), 194 deletions(-)

-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 01/18] staging: gasket: remove X86 Kconfig restriction

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

The gasket and apex drivers are to be used on other architectures
besides X86.

Signed-off-by: Todd Poynor <toddpoynor@gmail.com>
---
 drivers/staging/gasket/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/Kconfig b/drivers/staging/gasket/Kconfig
index c836389c1402..ef40c4c75e0f 100644
--- a/drivers/staging/gasket/Kconfig
+++ b/drivers/staging/gasket/Kconfig
@@ -2,7 +2,7 @@ menu "Gasket devices"
 
 config STAGING_GASKET_FRAMEWORK
 	tristate "Gasket framework"
-	depends on PCI && X86_64
+	depends on PCI
 	help
 	  This framework supports Gasket-compatible devices, such as Apex.
 	  It is required for any of the following module(s).
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 02/18] MAINTAINERS: Add maintainer for drivers/staging/gasket

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Todd Poynor <toddpoynor@google.com> will be a maintainer of the Gasket
and Apex drivers.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 562ee1e1cfd6..8f44e7bed49e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5968,6 +5968,7 @@ GASKET DRIVER FRAMEWORK
 M:	Rob Springer <rspringer@google.com>
 M:	John Joseph <jnjoseph@google.com>
 M:	Ben Chan <benchan@chromium.org>
+M:	Todd Poynor <toddpoynor@mgail.com>
 S:	Maintained
 F:	drivers/staging/gasket/
 
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 02/18] MAINTAINERS: Add maintainer for drivers/staging/gasket

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:00PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Todd Poynor <toddpoynor@google.com> will be a maintainer of the Gasket
> and Apex drivers.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>

I need an ack, or signed-off-by by the current maintainers that this is
ok.  And 4 maintainers is a lot, are you sure everyone here is the
"main" contact?

thanks,

greg k-h

[PATCH 03/18] staging: gasket: typo and whitespace cleanups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Trivial typo and whitespace fixes.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c       | 2 +-
 drivers/staging/gasket/gasket_core.h       | 2 +-
 drivers/staging/gasket/gasket_page_table.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 395256704428..f2d082c06c98 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -500,7 +500,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 					APEX_BAR2_REG_USER_HIB_DMA_PAUSED, 1, 1,
 					APEX_RESET_DELAY, APEX_RESET_RETRY)) {
 		gasket_log_error(gasket_dev,
-				 "DMAs did not quiece within timeout (%d ms)",
+				 "DMAs did not quiesce within timeout (%d ms)",
 				 APEX_RESET_RETRY * APEX_RESET_DELAY);
 		return -EINVAL;
 	}
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 5d6535a0f254..77d08c1265ca 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -63,7 +63,7 @@ enum gasket_interrupt_type {
 
 /* Used to describe a Gasket interrupt. Contains an interrupt index, a register,
  * and packing data for that interrupt. The register and packing data
- * fields is relevant only for PCI_MSIX interrupt type and can be
+ * fields are relevant only for PCI_MSIX interrupt type and can be
  * set to 0 for everything else.
  */
 struct gasket_interrupt_desc {
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 5d3d33cac12f..d7600d8e385f 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -354,7 +354,7 @@ int gasket_page_table_init(
 	    pg_tbl->config.mode == GASKET_PAGE_TABLE_MODE_SIMPLE) {
 		pg_tbl->num_simple_entries = total_entries;
 		pg_tbl->num_extended_entries = 0;
-		pg_tbl->extended_flag =	1ull << page_table_config->extended_bit;
+		pg_tbl->extended_flag = 1ull << page_table_config->extended_bit;
 	} else {
 		pg_tbl->num_simple_entries = 0;
 		pg_tbl->num_extended_entries = total_entries;
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 03/18] staging: gasket: typo and whitespace cleanups

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:01PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Trivial typo and whitespace fixes.

typos are different from whitespace issues.  Please break this up.

Yeah, it's a pain, sorry, but that's a requirement for kernel patches.
It's to make it easier on the reviewer, not necessarily the creator of
the patch :)

thanks,

greg k-h

[PATCH 04/18] staging: gasket: device registration error and unregister fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove device registration on class creation fail.

Hold mutex around device removal updates for proper ordering of
updates.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 45914ebc8f44..3bdf7d36b397 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -342,7 +342,8 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	if (IS_ERR_OR_NULL(internal->class)) {
 		gasket_nodev_error("Cannot register %s class [ret=%ld]",
 				   driver_desc->name, PTR_ERR(internal->class));
-		return PTR_ERR(internal->class);
+		ret = PTR_ERR(internal->class);
+		goto fail0;
 	}
 
 	/*
@@ -376,7 +377,10 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 fail1:
 	class_destroy(internal->class);
 
+fail0:
+	mutex_lock(&g_mutex);
 	g_descs[desc_idx].driver_desc = NULL;
+	mutex_unlock(&g_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(gasket_register_device);
@@ -413,7 +417,9 @@ void gasket_unregister_device(const struct gasket_driver_desc *driver_desc)
 	class_destroy(internal_desc->class);
 
 	/* Finally, effectively "remove" the driver. */
+	mutex_lock(&g_mutex);
 	g_descs[desc_idx].driver_desc = NULL;
+	mutex_unlock(&g_mutex);
 
 	gasket_nodev_info("removed %s driver", driver_desc->name);
 }
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 04/18] staging: gasket: device registration error and unregister fixups

[Dan Carpenter]

On Fri, Jul 13, 2018 at 10:58:02PM -0700, Todd Poynor wrote:
> @@ -376,7 +377,10 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
>  fail1:
>  	class_destroy(internal->class);
>  
> +fail0:

GW-BASIC sytle label names are an anti-pattern.  It's better to name the
labels after what they do, just like function names describes what the
function does.  Here it would be "goto clear_desc;" or something.  It
doesn't have to be perfect but so long as you *try* to name it something
useful that's better than not trying.

> +	mutex_lock(&g_mutex);
>  	g_descs[desc_idx].driver_desc = NULL;
> +	mutex_unlock(&g_mutex);
>  	return ret;
>  }

regards,
dan carpenter

Re: [PATCH 04/18] staging: gasket: device registration error and unregister fixups

[Todd Poynor]

>>  fail1:
>>       class_destroy(internal->class);
>>
>> +fail0:
>
> GW-BASIC sytle label names are an anti-pattern.  It's better to name the
> labels after what they do, just like function names describes what the
> function does.  Here it would be "goto clear_desc;" or something.  It
> doesn't have to be perfect but so long as you *try* to name it something
> useful that's better than not trying.
>
>> +     mutex_lock(&g_mutex);
>>       g_descs[desc_idx].driver_desc = NULL;
>> +     mutex_unlock(&g_mutex);
>>       return ret;
>>  }
>
> regards,
> dan carpenter

Thanks, I'll fix up the newly-added one in a respin of this patch and
fix up existing labels in a future patch.

-- 
Todd

[PATCH 05/18] staging: gasket: sysfs mapping creation fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Return EBUSY for attempt to create a mapping already in use.

Remove stale pointers on error allocating attr array.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_sysfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_sysfs.c b/drivers/staging/gasket/gasket_sysfs.c
index a3705d6e088a..d1856a09b894 100644
--- a/drivers/staging/gasket/gasket_sysfs.c
+++ b/drivers/staging/gasket/gasket_sysfs.c
@@ -213,7 +213,7 @@ int gasket_sysfs_create_mapping(
 			"0x%p.", device);
 		put_mapping(mapping);
 		mutex_unlock(&function_mutex);
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	/* Find the first empty entry in the array. */
@@ -244,6 +244,8 @@ int gasket_sysfs_create_mapping(
 	mapping->attribute_count = 0;
 	if (!mapping->attributes) {
 		gasket_nodev_error("Unable to allocate sysfs attribute array.");
+		mapping->device = NULL;
+		mapping->gasket_dev = NULL;
 		mutex_unlock(&mapping->mutex);
 		mutex_unlock(&function_mutex);
 		return -ENOMEM;
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 05/18] staging: gasket: sysfs mapping creation fixups

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:03PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Return EBUSY for attempt to create a mapping already in use.

Why?

> Remove stale pointers on error allocating attr array.

This should be a different patch.

thanks,

greg k-h

Re: [PATCH 05/18] staging: gasket: sysfs mapping creation fixups

[Todd Poynor]

On Fri, Jul 13, 2018 at 11:32 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Jul 13, 2018 at 10:58:03PM -0700, Todd Poynor wrote:
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> Return EBUSY for attempt to create a mapping already in use.
>
> Why?

The existing code returns EINVAL which often means something bogus was
requested, whereas EBUSY is sometimes used for requesting something
already in use.  If that's not an error typically returned in this
situation I'm happy to drop it.

>
>> Remove stale pointers on error allocating attr array.
>
> This should be a different patch.

Will do.

>
> thanks,
>
> greg k-h



-- 
Todd

Re: [PATCH 05/18] staging: gasket: sysfs mapping creation fixups

[Greg Kroah-Hartman]

On Mon, Jul 16, 2018 at 02:14:27PM -0700, Todd Poynor wrote:
> On Fri, Jul 13, 2018 at 11:32 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jul 13, 2018 at 10:58:03PM -0700, Todd Poynor wrote:
> >> From: Todd Poynor <toddpoynor@google.com>
> >>
> >> Return EBUSY for attempt to create a mapping already in use.
> >
> > Why?
> 
> The existing code returns EINVAL which often means something bogus was
> requested, whereas EBUSY is sometimes used for requesting something
> already in use.  If that's not an error typically returned in this
> situation I'm happy to drop it.

My "why" was rhetorical, please spell this out in the changelog text :)

thanks,

greg k-h

[PATCH 06/18] staging: gasket: fix deadlock in pci driver unregister path

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

g_mutex held across pci_unregister_driver() call, also held in
gasket_pci_remove(), which deadlocks.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 3bdf7d36b397..6d240dc59557 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -668,13 +668,10 @@ static void gasket_pci_remove(struct pci_dev *pci_dev)
 	struct gasket_dev *gasket_dev = NULL;
 	const struct gasket_driver_desc *driver_desc;
 	/* Find the device desc. */
-	mutex_lock(&g_mutex);
+	__must_hold(&g_mutex);
 	internal_desc = lookup_internal_desc(pci_dev);
-	if (!internal_desc) {
-		mutex_unlock(&g_mutex);
+	if (!internal_desc)
 		return;
-	}
-	mutex_unlock(&g_mutex);
 
 	driver_desc = internal_desc->driver_desc;
 
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 06/18] staging: gasket: fix deadlock in pci driver unregister path

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:04PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> g_mutex held across pci_unregister_driver() call, also held in
> gasket_pci_remove(), which deadlocks.

Which reminds me, why in the world do you all wrap pci
register/unregister within your new subsystem?  That's very odd, and
should be reworked...  Someone should go add that to the TODO list :)

thanks,

greg k-h

Re: [PATCH 06/18] staging: gasket: fix deadlock in pci driver unregister path

[Dmitry Torokhov]

On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>
> From: Todd Poynor <toddpoynor@google.com>
>
> g_mutex held across pci_unregister_driver() call, also held in
> gasket_pci_remove(), which deadlocks.
>
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/gasket_core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index 3bdf7d36b397..6d240dc59557 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -668,13 +668,10 @@ static void gasket_pci_remove(struct pci_dev *pci_dev)
>         struct gasket_dev *gasket_dev = NULL;
>         const struct gasket_driver_desc *driver_desc;
>         /* Find the device desc. */
> -       mutex_lock(&g_mutex);
> +       __must_hold(&g_mutex);

And what exactly ensures that mutex is held here? Yes, we are holding
the mutex when we unload the driver, but PCI hot-unplug or unbinding
the device though sysfs do not go through module unload code path, so
you'll end up here without holding the mutex.

>         internal_desc = lookup_internal_desc(pci_dev);
> -       if (!internal_desc) {
> -               mutex_unlock(&g_mutex);
> +       if (!internal_desc)
>                 return;
> -       }
> -       mutex_unlock(&g_mutex);
>
>         driver_desc = internal_desc->driver_desc;

Thanks,
Dmitry

Re: [PATCH 06/18] staging: gasket: fix deadlock in pci driver unregister path

[Greg Kroah-Hartman]

On Sat, Jul 14, 2018 at 11:07:21AM +0300, Dmitry Torokhov wrote:
> On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
> >
> > From: Todd Poynor <toddpoynor@google.com>
> >
> > g_mutex held across pci_unregister_driver() call, also held in
> > gasket_pci_remove(), which deadlocks.
> >
> > Reported-by: Dmitry Torokhov <dtor@chromium.org>
> > Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> > Signed-off-by: Todd Poynor <toddpoynor@google.com>
> > ---
> >  drivers/staging/gasket/gasket_core.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> > index 3bdf7d36b397..6d240dc59557 100644
> > --- a/drivers/staging/gasket/gasket_core.c
> > +++ b/drivers/staging/gasket/gasket_core.c
> > @@ -668,13 +668,10 @@ static void gasket_pci_remove(struct pci_dev *pci_dev)
> >         struct gasket_dev *gasket_dev = NULL;
> >         const struct gasket_driver_desc *driver_desc;
> >         /* Find the device desc. */
> > -       mutex_lock(&g_mutex);
> > +       __must_hold(&g_mutex);
> 
> And what exactly ensures that mutex is held here? Yes, we are holding
> the mutex when we unload the driver, but PCI hot-unplug or unbinding
> the device though sysfs do not go through module unload code path, so
> you'll end up here without holding the mutex.

Which is a huge reason the whole "wrap the pci core calls" is not going
to work here at all.  The device ownership rules are all wonky because
of this.  Unwinding that is key to getting all of this right.

thanks,

greg k-h

Re: [PATCH 06/18] staging: gasket: fix deadlock in pci driver unregister path

[Todd Poynor]

On Sat, Jul 14, 2018 at 5:57 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Jul 14, 2018 at 11:07:21AM +0300, Dmitry Torokhov wrote:
>> On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>> >
>> > From: Todd Poynor <toddpoynor@google.com>
>> >
>> > g_mutex held across pci_unregister_driver() call, also held in
>> > gasket_pci_remove(), which deadlocks.
>> >
>> > Reported-by: Dmitry Torokhov <dtor@chromium.org>
>> > Signed-off-by: Zhongze Hu <frankhu@chromium.org>
>> > Signed-off-by: Todd Poynor <toddpoynor@google.com>
>> > ---
>> >  drivers/staging/gasket/gasket_core.c | 7 ++-----
>> >  1 file changed, 2 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
>> > index 3bdf7d36b397..6d240dc59557 100644
>> > --- a/drivers/staging/gasket/gasket_core.c
>> > +++ b/drivers/staging/gasket/gasket_core.c
>> > @@ -668,13 +668,10 @@ static void gasket_pci_remove(struct pci_dev *pci_dev)
>> >         struct gasket_dev *gasket_dev = NULL;
>> >         const struct gasket_driver_desc *driver_desc;
>> >         /* Find the device desc. */
>> > -       mutex_lock(&g_mutex);
>> > +       __must_hold(&g_mutex);
>>
>> And what exactly ensures that mutex is held here? Yes, we are holding
>> the mutex when we unload the driver, but PCI hot-unplug or unbinding
>> the device though sysfs do not go through module unload code path, so
>> you'll end up here without holding the mutex.
>
> Which is a huge reason the whole "wrap the pci core calls" is not going
> to work here at all.  The device ownership rules are all wonky because
> of this.  Unwinding that is key to getting all of this right.

OK, I'll drop this patch in favor of redoing things not to wrap PCI
core calls in the future, thanks.

>
> thanks,
>
> greg k-h



-- 
Todd

[PATCH 07/18] staging: gasket: convert gasket_mmap_has_permissions to bool return

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

gasket_mmap_has_permissions() should return a boolean value.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 6d240dc59557..155c69446f0b 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1245,19 +1245,19 @@ static int gasket_release(struct inode *inode, struct file *file)
  * that the provided descriptor/range is of adequate size to hold the range to
  * be mapped.
  */
-static int gasket_mmap_has_permissions(
+static bool gasket_mmap_has_permissions(
 	struct gasket_dev *gasket_dev, struct vm_area_struct *vma,
 	int bar_permissions)
 {
 	int requested_permissions;
 	/* Always allow sysadmin to access. */
 	if (capable(CAP_SYS_ADMIN))
-		return 1;
+		return true;
 
 	/* Never allow non-sysadmins to access to a dead device. */
 	if (gasket_dev->status != GASKET_STATUS_ALIVE) {
 		gasket_log_info(gasket_dev, "Device is dead.");
-		return 0;
+		return false;
 	}
 
 	/* Make sure that no wrong flags are set. */
@@ -1269,7 +1269,7 @@ static int gasket_mmap_has_permissions(
 			"Attempting to map a region with requested permissions "
 			"0x%x, but region has permissions 0x%x.",
 			requested_permissions, bar_permissions);
-		return 0;
+		return false;
 	}
 
 	/* Do not allow a non-owner to write. */
@@ -1279,10 +1279,10 @@ static int gasket_mmap_has_permissions(
 			gasket_dev,
 			"Attempting to mmap a region for write without owning "
 			"device.");
-		return 0;
+		return false;
 	}
 
-	return 1;
+	return true;
 }
 
 /*
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 08/18] staging: gasket: gasket_wait_with_reschedule fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Return -ETIMEDOUT on timeouts.

Don't need 64-bit sized retry count.

Use msleep().

Return immediately when condition hit.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c |  8 ++++----
 drivers/staging/gasket/gasket_core.c | 11 ++++++-----
 drivers/staging/gasket/gasket_core.h |  2 +-
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index f2d082c06c98..b1318482ba65 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -502,7 +502,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 		gasket_log_error(gasket_dev,
 				 "DMAs did not quiesce within timeout (%d ms)",
 				 APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	/*  - Enable GCB reset (0x1 to rg_rst_gcb) */
@@ -525,7 +525,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 			gasket_dev,
 			"RAM did not shut down within timeout (%d ms)",
 			APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -576,7 +576,7 @@ static int apex_quit_reset(struct gasket_dev *gasket_dev, uint type)
 			gasket_dev,
 			"RAM did not enable within timeout (%d ms)",
 			APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	/*    - Wait for Reset complete. */
@@ -588,7 +588,7 @@ static int apex_quit_reset(struct gasket_dev *gasket_dev, uint type)
 			gasket_dev,
 			"GCB did not leave reset within timeout (%d ms)",
 			APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	if (!allow_hw_clock_gating) {
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 155c69446f0b..6316e6c800ba 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -21,6 +21,7 @@
 #include "gasket_page_table.h"
 #include "gasket_sysfs.h"
 
+#include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/of.h>
@@ -2135,16 +2136,16 @@ EXPORT_SYMBOL(gasket_wait_sync);
  **/
 int gasket_wait_with_reschedule(
 	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
-	u64 max_retries, u64 delay_ms)
+	uint max_retries, u64 delay_ms)
 {
-	u64 retries = 0;
+	uint retries = 0;
 	u64 tmp;
 
 	while (retries < max_retries) {
 		tmp = gasket_dev_read_64(gasket_dev, bar, offset);
 		if ((tmp & mask) == val)
-			break;
-		schedule_timeout(msecs_to_jiffies(delay_ms));
+			return 0;
+		msleep(delay_ms);
 		retries++;
 	}
 	if (retries == max_retries) {
@@ -2152,7 +2153,7 @@ int gasket_wait_with_reschedule(
 			gasket_dev,
 			"gasket_wait_with_reschedule timeout: reg %llx timeout (%llu ms)",
 			offset, max_retries * delay_ms);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 	return 0;
 }
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 77d08c1265ca..16bee478dce6 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -714,6 +714,6 @@ int gasket_wait_sync(
 /* Helper function, Asynchronous waits on a given set of bits. */
 int gasket_wait_with_reschedule(
 	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
-	u64 max_retries, u64 delay_ms);
+	uint max_retries, u64 delay_ms);
 
 #endif /* __GASKET_CORE_H__ */
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 08/18] staging: gasket: gasket_wait_with_reschedule fixups

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:06PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Return -ETIMEDOUT on timeouts.
> 
> Don't need 64-bit sized retry count.
> 
> Use msleep().
> 
> Return immediately when condition hit.

Huge hint, when you have to list the different things you do in a patch,
that means it should be split up into smaller patches.

Please rework this patch series based on this.  I'll stop reviewing them
now and wait for a new set of patches.

thanks,

greg k-h

[PATCH 09/18] staging: gasket: bail out of reset sequence on device callback error

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

If device reset callback returns an error, error out at the gasket
level.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 6316e6c800ba..ffd6ce801313 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1879,9 +1879,11 @@ int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
 
 	/* Perform a device reset of the requested type. */
 	ret = driver_desc->device_reset_cb(gasket_dev, reset_type);
-	if (ret)
+	if (ret) {
 		gasket_log_error(
 			gasket_dev, "Device reset cb returned %d.", ret);
+		return ret;
+	}
 
 	/* Reinitialize the page tables and interrupt framework. */
 	for (i = 0; i < driver_desc->num_page_tables; ++i)
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 10/18] staging: gasket: gasket_open use container_of()

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Use container_of(), drop unnecessary NULL check.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index ffd6ce801313..0c45c54254fb 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1099,12 +1099,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
 	const struct gasket_driver_desc *driver_desc;
 	struct gasket_ownership *ownership;
 	char task_name[TASK_COMM_LEN];
-	struct gasket_cdev_info *dev_info = gasket_cdev_get_info(inode->i_cdev);
+	struct gasket_cdev_info *dev_info =
+	    container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
 
-	if (!dev_info) {
-		gasket_nodev_error("Unable to retrieve device data");
-		return -EINVAL;
-	}
 	gasket_dev = dev_info->gasket_dev_ptr;
 	driver_desc = gasket_dev->internal_desc->driver_desc;
 	ownership = &dev_info->ownership;
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 10/18] staging: gasket: gasket_open use container_of()

[Dmitry Torokhov]

On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>
> From: Todd Poynor <toddpoynor@google.com>
>
> Use container_of(), drop unnecessary NULL check.
>
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/gasket_core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index ffd6ce801313..0c45c54254fb 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -1099,12 +1099,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
>         const struct gasket_driver_desc *driver_desc;
>         struct gasket_ownership *ownership;
>         char task_name[TASK_COMM_LEN];
> -       struct gasket_cdev_info *dev_info = gasket_cdev_get_info(inode->i_cdev);

Are there other users of gasket_cdev_get_info()? if this was the only
one we should remove the macro,otherwise we should convert them as
well.

> +       struct gasket_cdev_info *dev_info =
> +           container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
>
> -       if (!dev_info) {
> -               gasket_nodev_error("Unable to retrieve device data");
> -               return -EINVAL;
> -       }
>         gasket_dev = dev_info->gasket_dev_ptr;
>         driver_desc = gasket_dev->internal_desc->driver_desc;
>         ownership = &dev_info->ownership;
> --
> 2.18.0.203.gfac676dfb9-goog
>

Re: [PATCH 10/18] staging: gasket: gasket_open use container_of()

[Todd Poynor]

On Sat, Jul 14, 2018 at 1:15 AM, Dmitry Torokhov <dtor@chromium.org> wrote:
> On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>>
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> Use container_of(), drop unnecessary NULL check.
>>
>> Reported-by: Dmitry Torokhov <dtor@chromium.org>
>> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
>> Signed-off-by: Todd Poynor <toddpoynor@google.com>
>> ---
>>  drivers/staging/gasket/gasket_core.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
>> index ffd6ce801313..0c45c54254fb 100644
>> --- a/drivers/staging/gasket/gasket_core.c
>> +++ b/drivers/staging/gasket/gasket_core.c
>> @@ -1099,12 +1099,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
>>         const struct gasket_driver_desc *driver_desc;
>>         struct gasket_ownership *ownership;
>>         char task_name[TASK_COMM_LEN];
>> -       struct gasket_cdev_info *dev_info = gasket_cdev_get_info(inode->i_cdev);
>
> Are there other users of gasket_cdev_get_info()? if this was the only
> one we should remove the macro,otherwise we should convert them as
> well.

No other users, I'll add a patch to remove it, thanks.

>
>> +       struct gasket_cdev_info *dev_info =
>> +           container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
>>
>> -       if (!dev_info) {
>> -               gasket_nodev_error("Unable to retrieve device data");
>> -               return -EINVAL;
>> -       }
>>         gasket_dev = dev_info->gasket_dev_ptr;
>>         driver_desc = gasket_dev->internal_desc->driver_desc;
>>         ownership = &dev_info->ownership;
>> --
>> 2.18.0.203.gfac676dfb9-goog
>>



-- 
Todd

[PATCH 11/18] staging: gasket: always allow root open for write

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Always allow root to open device for writing.

Drop special-casing of ioctl permissions for root vs. owner.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c  |  9 +++------
 drivers/staging/gasket/gasket_core.c  |  8 +++++---
 drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index b1318482ba65..ffe11d8168ea 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
 static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 {
 	struct gasket_dev *gasket_dev = filp->private_data;
-	int root = capable(CAP_SYS_ADMIN);
-	int is_owner = gasket_dev->dev_info.ownership.is_owned &&
-		       current->tgid == gasket_dev->dev_info.ownership.owner;
+	fmode_t write;
 
-	if (root || is_owner)
-		return 1;
-	return 0;
+	write = filp->f_mode & FMODE_WRITE;
+	return write;
 }
 
 /*
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 0c45c54254fb..5908964f0039 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1101,6 +1101,7 @@ static int gasket_open(struct inode *inode, struct file *filp)
 	char task_name[TASK_COMM_LEN];
 	struct gasket_cdev_info *dev_info =
 	    container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
+	int is_root = capable(CAP_SYS_ADMIN);
 
 	gasket_dev = dev_info->gasket_dev_ptr;
 	driver_desc = gasket_dev->internal_desc->driver_desc;
@@ -1114,7 +1115,7 @@ static int gasket_open(struct inode *inode, struct file *filp)
 		"Attempting to open with tgid %u (%s) (f_mode: 0%03o, "
 		"fmode_write: %d is_root: %u)",
 		current->tgid, task_name, filp->f_mode,
-		(filp->f_mode & FMODE_WRITE), capable(CAP_SYS_ADMIN));
+		(filp->f_mode & FMODE_WRITE), is_root);
 
 	/* Always allow non-writing accesses. */
 	if (!(filp->f_mode & FMODE_WRITE)) {
@@ -1128,8 +1129,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
 		gasket_dev, "Current owner open count (owning tgid %u): %d.",
 		ownership->owner, ownership->write_open_count);
 
-	/* Opening a node owned by another TGID is an error (even root.) */
-	if (ownership->is_owned && ownership->owner != current->tgid) {
+	/* Opening a node owned by another TGID is an error (unless root) */
+	if (ownership->is_owned && ownership->owner != current->tgid &&
+	    !is_root) {
 		gasket_log_error(
 			gasket_dev,
 			"Process %u is opening a node held by %u.",
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index 4758083fb19b..23875fd465f6 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -180,7 +180,7 @@ long gasket_is_supported_ioctl(uint cmd)
  */
 static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
 {
-	uint alive, root, device_owner;
+	uint alive;
 	fmode_t read, write;
 	struct gasket_dev *gasket_dev = (struct gasket_dev *)filp->private_data;
 
@@ -191,33 +191,30 @@ static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
 			alive, gasket_dev->status);
 	}
 
-	root = capable(CAP_SYS_ADMIN);
 	read = filp->f_mode & FMODE_READ;
 	write = filp->f_mode & FMODE_WRITE;
-	device_owner = (gasket_dev->dev_info.ownership.is_owned &&
-			current->tgid == gasket_dev->dev_info.ownership.owner);
 
 	switch (cmd) {
 	case GASKET_IOCTL_RESET:
 	case GASKET_IOCTL_CLEAR_INTERRUPT_COUNTS:
-		return root || (write && device_owner);
+		return write;
 
 	case GASKET_IOCTL_PAGE_TABLE_SIZE:
 	case GASKET_IOCTL_SIMPLE_PAGE_TABLE_SIZE:
 	case GASKET_IOCTL_NUMBER_PAGE_TABLES:
-		return root || read;
+		return read;
 
 	case GASKET_IOCTL_PARTITION_PAGE_TABLE:
 	case GASKET_IOCTL_CONFIG_COHERENT_ALLOCATOR:
-		return alive && (root || (write && device_owner));
+		return alive && write;
 
 	case GASKET_IOCTL_MAP_BUFFER:
 	case GASKET_IOCTL_UNMAP_BUFFER:
-		return alive && (root || (write && device_owner));
+		return alive && write;
 
 	case GASKET_IOCTL_CLEAR_EVENTFD:
 	case GASKET_IOCTL_SET_EVENTFD:
-		return alive && (root || (write && device_owner));
+		return alive && write;
 	}
 
 	return 0; /* unknown permissions */
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Dmitry Torokhov]

On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>
> From: Todd Poynor <toddpoynor@google.com>
>
> Always allow root to open device for writing.
>
> Drop special-casing of ioctl permissions for root vs. owner.
>
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>

I think this patch is good as is, but as a followup you should create
a patch that supports user namespaces, i.e. replaces
capable(CAP_SYS_ADMIN) with ns_capable(...) in gasket_open() so you
can allow containers to control the device, if necessary.

Reviewed-by: Dmitry Torokhov <dtor@chromium.org>

Thanks,
Dmitry

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Todd Poynor]

On Sat, Jul 14, 2018 at 1:25 AM, Dmitry Torokhov <dtor@chromium.org> wrote:
> On Sat, Jul 14, 2018 at 8:58 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>>
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> Always allow root to open device for writing.
>>
>> Drop special-casing of ioctl permissions for root vs. owner.
>>
>> Reported-by: Dmitry Torokhov <dtor@chromium.org>
>> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
>> Signed-off-by: Todd Poynor <toddpoynor@google.com>
>
> I think this patch is good as is, but as a followup you should create
> a patch that supports user namespaces, i.e. replaces
> capable(CAP_SYS_ADMIN) with ns_capable(...) in gasket_open() so you
> can allow containers to control the device, if necessary.

Thanks, I'll add that to the list.

>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
>
> Thanks,
> Dmitry



-- 
Todd

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:09PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Always allow root to open device for writing.
> 
> Drop special-casing of ioctl permissions for root vs. owner.
> 
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/apex_driver.c  |  9 +++------
>  drivers/staging/gasket/gasket_core.c  |  8 +++++---
>  drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
>  3 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> index b1318482ba65..ffe11d8168ea 100644
> --- a/drivers/staging/gasket/apex_driver.c
> +++ b/drivers/staging/gasket/apex_driver.c
> @@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
>  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>  {
>  	struct gasket_dev *gasket_dev = filp->private_data;
> -	int root = capable(CAP_SYS_ADMIN);
> -	int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> -		       current->tgid == gasket_dev->dev_info.ownership.owner;
> +	fmode_t write;
>  
> -	if (root || is_owner)
> -		return 1;
> -	return 0;
> +	write = filp->f_mode & FMODE_WRITE;

Ok, this is insane.  You don't change, or check, the permissions on a
file handle while it is already open, as you only check the permissions
on OPEN, not on WRITE.  See the recent rant from Linus on the linux-api
list for yet-another-long-threaad in which he explains this.

So this whole ioctl can just be removed, it is totally crazy and wrong
and should just be removed.

Want me to go remove it right now?

thanks,

greg k-h

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Dmitry Torokhov]

On Sun, Jul 15, 2018 at 12:05 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jul 13, 2018 at 10:58:09PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <toddpoynor@google.com>
> >
> > Always allow root to open device for writing.
> >
> > Drop special-casing of ioctl permissions for root vs. owner.
> >
> > Reported-by: Dmitry Torokhov <dtor@chromium.org>
> > Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> > Signed-off-by: Todd Poynor <toddpoynor@google.com>
> > ---
> >  drivers/staging/gasket/apex_driver.c  |  9 +++------
> >  drivers/staging/gasket/gasket_core.c  |  8 +++++---
> >  drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
> >  3 files changed, 14 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> > index b1318482ba65..ffe11d8168ea 100644
> > --- a/drivers/staging/gasket/apex_driver.c
> > +++ b/drivers/staging/gasket/apex_driver.c
> > @@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
> >  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
> >  {
> >       struct gasket_dev *gasket_dev = filp->private_data;
> > -     int root = capable(CAP_SYS_ADMIN);
> > -     int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> > -                    current->tgid == gasket_dev->dev_info.ownership.owner;
> > +     fmode_t write;
> >
> > -     if (root || is_owner)
> > -             return 1;
> > -     return 0;
> > +     write = filp->f_mode & FMODE_WRITE;
>
> Ok, this is insane.  You don't change, or check, the permissions on a
> file handle while it is already open, as you only check the permissions
> on OPEN, not on WRITE.  See the recent rant from Linus on the linux-api
> list for yet-another-long-threaad in which he explains this.
>
> So this whole ioctl can just be removed, it is totally crazy and wrong
> and should just be removed.

No, the code checks whether the requested ioctl command is compatible
with the mode the file handle was open with. There are some ioctls
that are allowed on file handle opened for read and others that
require file handle to be opened for write. That is all. It does not
change permissions past open.

Thanks,
Dmitry

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Greg Kroah-Hartman]

On Sun, Jul 15, 2018 at 12:11:47PM +0300, Dmitry Torokhov wrote:
> On Sun, Jul 15, 2018 at 12:05 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jul 13, 2018 at 10:58:09PM -0700, Todd Poynor wrote:
> > > From: Todd Poynor <toddpoynor@google.com>
> > >
> > > Always allow root to open device for writing.
> > >
> > > Drop special-casing of ioctl permissions for root vs. owner.
> > >
> > > Reported-by: Dmitry Torokhov <dtor@chromium.org>
> > > Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> > > Signed-off-by: Todd Poynor <toddpoynor@google.com>
> > > ---
> > >  drivers/staging/gasket/apex_driver.c  |  9 +++------
> > >  drivers/staging/gasket/gasket_core.c  |  8 +++++---
> > >  drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
> > >  3 files changed, 14 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> > > index b1318482ba65..ffe11d8168ea 100644
> > > --- a/drivers/staging/gasket/apex_driver.c
> > > +++ b/drivers/staging/gasket/apex_driver.c
> > > @@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
> > >  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
> > >  {
> > >       struct gasket_dev *gasket_dev = filp->private_data;
> > > -     int root = capable(CAP_SYS_ADMIN);
> > > -     int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> > > -                    current->tgid == gasket_dev->dev_info.ownership.owner;
> > > +     fmode_t write;
> > >
> > > -     if (root || is_owner)
> > > -             return 1;
> > > -     return 0;
> > > +     write = filp->f_mode & FMODE_WRITE;
> >
> > Ok, this is insane.  You don't change, or check, the permissions on a
> > file handle while it is already open, as you only check the permissions
> > on OPEN, not on WRITE.  See the recent rant from Linus on the linux-api
> > list for yet-another-long-threaad in which he explains this.
> >
> > So this whole ioctl can just be removed, it is totally crazy and wrong
> > and should just be removed.
> 
> No, the code checks whether the requested ioctl command is compatible
> with the mode the file handle was open with. There are some ioctls
> that are allowed on file handle opened for read and others that
> require file handle to be opened for write. That is all. It does not
> change permissions past open.

That's really not obvious here :)

And is odd on a whole other set of "crazy design", but ok, let's let it
live for now.

I can't wait for people to just realize this whole "new" subsystem can
be replaced with UIO, but that's a topic for a different thread...

thanks,

greg k-h

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Dmitry Torokhov]

On Sun, Jul 15, 2018 at 12:32 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jul 15, 2018 at 12:11:47PM +0300, Dmitry Torokhov wrote:
> > On Sun, Jul 15, 2018 at 12:05 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jul 13, 2018 at 10:58:09PM -0700, Todd Poynor wrote:
> > > > From: Todd Poynor <toddpoynor@google.com>
> > > >
> > > > Always allow root to open device for writing.
> > > >
> > > > Drop special-casing of ioctl permissions for root vs. owner.
> > > >
> > > > Reported-by: Dmitry Torokhov <dtor@chromium.org>
> > > > Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> > > > Signed-off-by: Todd Poynor <toddpoynor@google.com>
> > > > ---
> > > >  drivers/staging/gasket/apex_driver.c  |  9 +++------
> > > >  drivers/staging/gasket/gasket_core.c  |  8 +++++---
> > > >  drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
> > > >  3 files changed, 14 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> > > > index b1318482ba65..ffe11d8168ea 100644
> > > > --- a/drivers/staging/gasket/apex_driver.c
> > > > +++ b/drivers/staging/gasket/apex_driver.c
> > > > @@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
> > > >  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
> > > >  {
> > > >       struct gasket_dev *gasket_dev = filp->private_data;
> > > > -     int root = capable(CAP_SYS_ADMIN);
> > > > -     int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> > > > -                    current->tgid == gasket_dev->dev_info.ownership.owner;
> > > > +     fmode_t write;
> > > >
> > > > -     if (root || is_owner)
> > > > -             return 1;
> > > > -     return 0;
> > > > +     write = filp->f_mode & FMODE_WRITE;
> > >
> > > Ok, this is insane.  You don't change, or check, the permissions on a
> > > file handle while it is already open, as you only check the permissions
> > > on OPEN, not on WRITE.  See the recent rant from Linus on the linux-api
> > > list for yet-another-long-threaad in which he explains this.
> > >
> > > So this whole ioctl can just be removed, it is totally crazy and wrong
> > > and should just be removed.
> >
> > No, the code checks whether the requested ioctl command is compatible
> > with the mode the file handle was open with. There are some ioctls
> > that are allowed on file handle opened for read and others that
> > require file handle to be opened for write. That is all. It does not
> > change permissions past open.
>
> That's really not obvious here :)
>
> And is odd on a whole other set of "crazy design", but ok, let's let it
> live for now.

Are you talking about ioctl still or something else? Because such
ioctl handling is quite common, take a look at blkdev for example,
where disacrd ioctl is only allowed when device is open for writing.

>
> I can't wait for people to just realize this whole "new" subsystem can
> be replaced with UIO, but that's a topic for a different thread...

Yes, that is true and that is why I am not sure why we are going
through all this staging exercise.

As far as I understand we'd still need to have quite a bit of kernel
code so that we can safely program DMA controller (it does not look
like uio_dmem_genirq.c is sufficient as is for gasket needs), but that
should be solvable.

Thanks,
Dmitry

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Greg Kroah-Hartman]

On Sun, Jul 15, 2018 at 12:53:09PM +0300, Dmitry Torokhov wrote:
> > I can't wait for people to just realize this whole "new" subsystem can
> > be replaced with UIO, but that's a topic for a different thread...
> 
> Yes, that is true and that is why I am not sure why we are going
> through all this staging exercise.
> 
> As far as I understand we'd still need to have quite a bit of kernel
> code so that we can safely program DMA controller (it does not look
> like uio_dmem_genirq.c is sufficient as is for gasket needs), but that
> should be solvable.

I agree, it should be solvable, and much smaller and simpler than this
whole large chunk of "subsystem+driver" code.  But I'm not the one
having to do this work, and it provides a bunch of easy cleanups for
people looking to get into kernel development, so I don't mind :)

But the "maintainers" should keep this in mind, as it is, this code is
_not_ acceptable for the main kernel tree because of the UIO framework
already present.

thanks,

greg k-h

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Todd Poynor]

On Sun, Jul 15, 2018 at 3:03 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Jul 15, 2018 at 12:53:09PM +0300, Dmitry Torokhov wrote:
>> > I can't wait for people to just realize this whole "new" subsystem can
>> > be replaced with UIO, but that's a topic for a different thread...
>>
>> Yes, that is true and that is why I am not sure why we are going
>> through all this staging exercise.
>>
>> As far as I understand we'd still need to have quite a bit of kernel
>> code so that we can safely program DMA controller (it does not look
>> like uio_dmem_genirq.c is sufficient as is for gasket needs), but that
>> should be solvable.
>
> I agree, it should be solvable, and much smaller and simpler than this
> whole large chunk of "subsystem+driver" code.  But I'm not the one
> having to do this work, and it provides a bunch of easy cleanups for
> people looking to get into kernel development, so I don't mind :)
>
> But the "maintainers" should keep this in mind, as it is, this code is
> _not_ acceptable for the main kernel tree because of the UIO framework
> already present.

My own preference is to rewrite the apex driver entirely in-kernel and
pull in its userspace parts here.  If I don't receive significant
pushback on that I'll start doing that real soon.

>
> thanks,
>
> greg k-h



-- 
Todd

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Greg Kroah-Hartman]

On Sun, Jul 15, 2018 at 11:15:26AM -0700, Todd Poynor wrote:
> On Sun, Jul 15, 2018 at 3:03 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sun, Jul 15, 2018 at 12:53:09PM +0300, Dmitry Torokhov wrote:
> >> > I can't wait for people to just realize this whole "new" subsystem can
> >> > be replaced with UIO, but that's a topic for a different thread...
> >>
> >> Yes, that is true and that is why I am not sure why we are going
> >> through all this staging exercise.
> >>
> >> As far as I understand we'd still need to have quite a bit of kernel
> >> code so that we can safely program DMA controller (it does not look
> >> like uio_dmem_genirq.c is sufficient as is for gasket needs), but that
> >> should be solvable.
> >
> > I agree, it should be solvable, and much smaller and simpler than this
> > whole large chunk of "subsystem+driver" code.  But I'm not the one
> > having to do this work, and it provides a bunch of easy cleanups for
> > people looking to get into kernel development, so I don't mind :)
> >
> > But the "maintainers" should keep this in mind, as it is, this code is
> > _not_ acceptable for the main kernel tree because of the UIO framework
> > already present.
> 
> My own preference is to rewrite the apex driver entirely in-kernel and
> pull in its userspace parts here.  If I don't receive significant
> pushback on that I'll start doing that real soon.

Why would we object to that?

Re: [PATCH 11/18] staging: gasket: always allow root open for write

[Todd Poynor]

On Sun, Jul 15, 2018 at 12:41 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Jul 15, 2018 at 11:15:26AM -0700, Todd Poynor wrote:
>> On Sun, Jul 15, 2018 at 3:03 AM, Greg Kroah-Hartman
>...
>> My own preference is to rewrite the apex driver entirely in-kernel and
>> pull in its userspace parts here.  If I don't receive significant
>> pushback on that I'll start doing that real soon.
>
> Why would we object to that?

I've seen folks that have a preference for UIO whenever possible, but
I'm of the "just do kernel-like stuff in the kernel" camp.  There are
folks cc'ed who may have strong opinions about the subject for various
reasons; absent any serious objections I'll put that on a longer-term
todo list.  Thanks,

-- 
Todd

[PATCH 12/18] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

For sparse checking.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c  | 12 ++---
 drivers/staging/gasket/gasket_core.c  |  6 ++-
 drivers/staging/gasket/gasket_core.h  |  4 +-
 drivers/staging/gasket/gasket_ioctl.c | 72 ++++++++++++++-------------
 drivers/staging/gasket/gasket_ioctl.h |  4 +-
 5 files changed, 52 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index ffe11d8168ea..40af91952283 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -12,6 +12,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -149,9 +150,9 @@ static int apex_get_status(struct gasket_dev *gasket_dev);
 
 static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
 
-static long apex_ioctl(struct file *file, uint cmd, ulong arg);
+static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
 
-static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg);
+static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg);
 
 static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type);
 
@@ -643,7 +644,6 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
  */
 static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 {
-	struct gasket_dev *gasket_dev = filp->private_data;
 	fmode_t write;
 
 	write = filp->f_mode & FMODE_WRITE;
@@ -653,7 +653,7 @@ static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 /*
  * Apex-specific ioctl handler.
  */
-static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
+static long apex_ioctl(struct file *filp, uint cmd, void __user *arg)
 {
 	struct gasket_dev *gasket_dev = filp->private_data;
 
@@ -673,7 +673,7 @@ static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
  * @gasket_dev: Gasket device pointer.
  * @arg: User ioctl arg, in this case to a apex_gate_clock_ioctl struct.
  */
-static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
+static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct apex_gate_clock_ioctl ibuf;
 
@@ -681,7 +681,7 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
 		return 0;
 
 	if (allow_sw_clock_gating) {
-		if (copy_from_user(&ibuf, (void __user *)arg, sizeof(ibuf)))
+		if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
 			return -EFAULT;
 
 		gasket_log_error(
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 5908964f0039..65aa7cf454fb 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -21,6 +21,7 @@
 #include "gasket_page_table.h"
 #include "gasket_sysfs.h"
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -1842,14 +1843,15 @@ static long gasket_ioctl(struct file *filp, uint cmd, ulong arg)
 		 * check_and_invoke_callback.
 		 */
 		if (driver_desc->ioctl_handler_cb)
-			return driver_desc->ioctl_handler_cb(filp, cmd, arg);
+			return driver_desc->ioctl_handler_cb(
+			    filp, cmd, (void __user *)arg);
 
 		gasket_log_error(
 			gasket_dev, "Received unknown ioctl 0x%x", cmd);
 		return -EINVAL;
 	}
 
-	return gasket_handle_ioctl(filp, cmd, arg);
+	return gasket_handle_ioctl(filp, cmd, (void __user *)arg);
 }
 
 int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 16bee478dce6..177b239e2c50 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -322,7 +322,7 @@ struct gasket_dev {
 
 /* Type of the ioctl permissions check callback. See below. */
 typedef int (*gasket_ioctl_permissions_cb_t)(
-	struct file *filp, uint cmd, ulong arg);
+	struct file *filp, uint cmd, void __user *arg);
 
 /*
  * Device type descriptor.
@@ -556,7 +556,7 @@ struct gasket_driver_desc {
 	 * return -EINVAL. Should return an error status (either -EINVAL or
 	 * the error result of the ioctl being handled).
 	 */
-	long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
+	long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
 
 	/*
 	 * device_status_cb: Callback to determine device health.
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index 23875fd465f6..0c1e0723bef1 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -16,6 +16,7 @@
 #include "gasket_interrupt.h"
 #include "gasket_logging.h"
 #include "gasket_page_table.h"
+#include <linux/compiler.h>
 #include <linux/fs.h>
 #include <linux/uaccess.h>
 
@@ -32,17 +33,18 @@
 #endif
 
 static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd);
-static int gasket_set_event_fd(struct gasket_dev *dev, ulong arg);
+static int gasket_set_event_fd(struct gasket_dev *dev, void __user *arg);
 static int gasket_read_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 static int gasket_read_simple_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 static int gasket_partition_page_table(
-	struct gasket_dev *gasket_dev, ulong arg);
-static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg);
-static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
+static int gasket_map_buffers(struct gasket_dev *gasket_dev, void __user *arg);
+static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
+				void __user *arg);
 static int gasket_config_coherent_allocator(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 
 /*
  * standard ioctl dispatch function.
@@ -52,9 +54,10 @@ static int gasket_config_coherent_allocator(
  *
  * Standard ioctl dispatcher; forwards operations to individual handlers.
  */
-long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
+long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg)
 {
 	struct gasket_dev *gasket_dev;
+	ulong intarg = (ulong)arg;
 	int retval;
 
 	gasket_dev = (struct gasket_dev *)filp->private_data;
@@ -83,16 +86,16 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
 	 */
 	switch (cmd) {
 	case GASKET_IOCTL_RESET:
-		trace_gasket_ioctl_integer_data(arg);
-		retval = gasket_reset(gasket_dev, arg);
+		trace_gasket_ioctl_integer_data(intarg);
+		retval = gasket_reset(gasket_dev, intarg);
 		break;
 	case GASKET_IOCTL_SET_EVENTFD:
 		retval = gasket_set_event_fd(gasket_dev, arg);
 		break;
 	case GASKET_IOCTL_CLEAR_EVENTFD:
-		trace_gasket_ioctl_integer_data(arg);
+		trace_gasket_ioctl_integer_data(intarg);
 		retval = gasket_interrupt_clear_eventfd(
-			gasket_dev->interrupt_data, (int)arg);
+			gasket_dev->interrupt_data, (int)intarg);
 		break;
 	case GASKET_IOCTL_PARTITION_PAGE_TABLE:
 		trace_gasket_ioctl_integer_data(arg);
@@ -100,7 +103,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
 		break;
 	case GASKET_IOCTL_NUMBER_PAGE_TABLES:
 		trace_gasket_ioctl_integer_data(gasket_dev->num_page_tables);
-		if (copy_to_user((void __user *)arg,
+		if (copy_to_user(arg,
 				 &gasket_dev->num_page_tables,
 				 sizeof(uint64_t)))
 			retval = -EFAULT;
@@ -225,11 +228,11 @@ static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_interrupt_eventfd struct in userspace.
  */
-static int gasket_set_event_fd(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_set_event_fd(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct gasket_interrupt_eventfd die;
 
-	if (copy_from_user(&die, (void __user *)arg,
+	if (copy_from_user(&die, arg,
 			   sizeof(struct gasket_interrupt_eventfd))) {
 		return -EFAULT;
 	}
@@ -245,13 +248,13 @@ static int gasket_set_event_fd(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_read_page_table_size(struct gasket_dev *gasket_dev,
+				       void __user *arg)
 {
 	int ret = 0;
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
@@ -264,7 +267,7 @@ static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
@@ -276,13 +279,12 @@ static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
 static int gasket_read_simple_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg)
+	struct gasket_dev *gasket_dev, void __user *arg)
 {
 	int ret = 0;
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
@@ -295,7 +297,7 @@ static int gasket_read_simple_page_table_size(
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
@@ -306,14 +308,14 @@ static int gasket_read_simple_page_table_size(
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_partition_page_table(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_partition_page_table(struct gasket_dev *gasket_dev,
+				       void __user *arg)
 {
 	int ret;
 	struct gasket_page_table_ioctl ibuf;
 	uint max_page_table_size;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -347,12 +349,12 @@ static int gasket_partition_page_table(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to a gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_map_buffers(struct gasket_dev *gasket_dev,
+			      void __user *arg)
 {
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -377,12 +379,12 @@ static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to a gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
+				void __user *arg)
 {
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -410,12 +412,12 @@ static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg)
  * @arg: Pointer to a gasket_coherent_alloc_config_ioctl struct in userspace.
  */
 static int gasket_config_coherent_allocator(
-	struct gasket_dev *gasket_dev, ulong arg)
+	struct gasket_dev *gasket_dev, void __user *arg)
 {
 	int ret;
 	struct gasket_coherent_alloc_config_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
+	if (copy_from_user(&ibuf, arg,
 			   sizeof(struct gasket_coherent_alloc_config_ioctl)))
 		return -EFAULT;
 
@@ -439,7 +441,7 @@ static int gasket_config_coherent_allocator(
 			gasket_dev, ibuf.size, &ibuf.dma_address,
 			ibuf.page_table_index);
 	}
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
diff --git a/drivers/staging/gasket/gasket_ioctl.h b/drivers/staging/gasket/gasket_ioctl.h
index df868000c803..a9e10e4bc84a 100644
--- a/drivers/staging/gasket/gasket_ioctl.h
+++ b/drivers/staging/gasket/gasket_ioctl.h
@@ -14,6 +14,8 @@
 
 #include "gasket_core.h"
 
+#include <linux/compiler.h>
+
 /*
  * Handle Gasket common ioctls.
  * @filp: Pointer to the ioctl's file.
@@ -22,7 +24,7 @@
  *
  * Returns 0 on success and nonzero on failure.
  */
-long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg);
+long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg);
 
 /*
  * Determines if an ioctl is part of the standard Gasket framework.
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 12/18] staging: gasket: annotate ioctl arg with __user

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:10PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> For sparse checking.
> 
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---

Are you sure this patch even applies?  What tree did you make it
against?

I ask because this fragment:

> @@ -12,6 +12,7 @@
>   * GNU General Public License for more details.
>   */

is no longer in the tree.

thanks,

greg k-h

Re: [PATCH 12/18] staging: gasket: annotate ioctl arg with __user

[Greg Kroah-Hartman]

On Sat, Jul 14, 2018 at 12:12:38AM -0700, Todd Poynor wrote:

HTML email gets rejected by the mailing lists :(

Please fix your email client to not do that.

> On Fri, Jul 13, 2018 at 11:35 PM, Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
> 
>     On Fri, Jul 13, 2018 at 10:58:10PM -0700, Todd Poynor wrote:
>     > From: Todd Poynor <toddpoynor@google.com>
>     >
>     > For sparse checking.
>     >
>     > Reported-by: Dmitry Torokhov <dtor@chromium.org>
>     > Signed-off-by: Zhongze Hu <frankhu@chromium.org>
>     > Signed-off-by: Todd Poynor <toddpoynor@google.com>
>     > ---
> 
>     Are you sure this patch even applies?  What tree did you make it
>     against?
> 
> 
> I'm working in linux-next at tag next-20180713, which is also tagged master at
> this writing. 
> 
> I think I'm following http://www.kroah.com/log/linux/linux-staging-update.html,
> but if I'm off in the weeds do clue me in, thanks. 

That's something I wrote 9 years ago :)

Anyway, yes, linux-next is usually the right place to work off of, but
it lags my staging.git tree usually by at least a day, maybe two,
depending on when I push and when -next picks my tree up.  Also there is
the lag of the weekend when -next is not being generated.

Normally that is fine, except for when working on code that is "fast
moving".  This driver has had lots of submissions against it lately as
there are loads of "low hanging fruit" that people easily see to fix up.
Look at the mailing list archives for this week alone for all of those
patches.

When working on something that lots of other people are working on, pay
attention to the patches on the mailing list and work off of my
staging.git tree, usually the staging-next branch, but sometimes
staging-testing is needed if lots of patches are being sent rapidly.

>     I ask because this fragment:
>    
>     > @@ -12,6 +12,7 @@
>     >   * GNU General Public License for more details.
>     >   */
> 
>     is no longer in the tree.
> 
> 
> Sorry, which tree?  I've made sure my linux-next is up-to-date and I still see
> this code.

Look at my staging-next branch in staging.git.  It's long-gone :)

thanks,

greg k-h

Re: [PATCH 12/18] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

>> I think I'm following http://www.kroah.com/log/linux/linux-staging-update.html,
>> but if I'm off in the weeds do clue me in, thanks.
>
> That's something I wrote 9 years ago :)

Still the top search result for how to work in staging, suggest an update. :)

Re: [PATCH 12/18] staging: gasket: annotate ioctl arg with __user

[Greg Kroah-Hartman]

On Sat, Jul 14, 2018 at 12:20:51PM -0700, Todd Poynor wrote:
> >> I think I'm following http://www.kroah.com/log/linux/linux-staging-update.html,
> >> but if I'm off in the weeds do clue me in, thanks.
> >
> > That's something I wrote 9 years ago :)
> 
> Still the top search result for how to work in staging, suggest an update. :)

Do all subsystems have such descriptions?  No, working in staging is no
different from working in any other kernel subsystem.  If you want to
find the location of the latest code, look in the MAINTAINERS file for
the git tree.

There's nothing "special" about drivers/staging, except the quality of
the code...

greg k-h

Re: [PATCH 12/18] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

Yes you're right, I see the entry now.

Thanks -- Todd

On Sun, Jul 15, 2018 at 2:00 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Jul 14, 2018 at 12:20:51PM -0700, Todd Poynor wrote:
>> >> I think I'm following http://www.kroah.com/log/linux/linux-staging-update.html,
>> >> but if I'm off in the weeds do clue me in, thanks.
>> >
>> > That's something I wrote 9 years ago :)
>>
>> Still the top search result for how to work in staging, suggest an update. :)
>
> Do all subsystems have such descriptions?  No, working in staging is no
> different from working in any other kernel subsystem.  If you want to
> find the location of the latest code, look in the MAINTAINERS file for
> the git tree.
>
> There's nothing "special" about drivers/staging, except the quality of
> the code...
>
> greg k-h



-- 
Todd

[PATCH 13/18] staging: gasket: gasket_enable_dev fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove unnecessary variable.

Bail out if no physical device.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 65aa7cf454fb..f7d8f66e8746 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -915,8 +915,6 @@ static int gasket_enable_dev(
 {
 	int tbl_idx;
 	int ret;
-	bool has_dma_ops;
-	struct device *ddev;
 	const struct gasket_driver_desc *driver_desc =
 		internal_desc->driver_desc;
 
@@ -934,26 +932,21 @@ static int gasket_enable_dev(
 		return ret;
 	}
 
-	has_dma_ops = true;
+	if (!gasket_dev->pci_dev) {
+		gasket_log_error(gasket_dev,
+				 "%s: no physical device", __func__);
+		return -ENODEV;
+	}
 
 	for (tbl_idx = 0; tbl_idx < driver_desc->num_page_tables; tbl_idx++) {
 		gasket_log_debug(
 			gasket_dev, "Initializing page table %d.", tbl_idx);
-		if (gasket_dev->pci_dev) {
-			ddev = &gasket_dev->pci_dev->dev;
-		} else {
-			gasket_log_error(
-				gasket_dev,
-				"gasket_enable_dev with no physical device!!");
-			WARN_ON(1);
-			ddev = NULL;
-		}
 		ret = gasket_page_table_init(
 			&gasket_dev->page_table[tbl_idx],
 			&gasket_dev->bar_data[
 				driver_desc->page_table_bar_index],
 			&driver_desc->page_table_configs[tbl_idx],
-			ddev, gasket_dev->pci_dev, has_dma_ops);
+			&gasket_dev->pci_dev->dev, gasket_dev->pci_dev, true);
 		if (ret) {
 			gasket_log_error(
 				gasket_dev,
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 13/18] staging: gasket: gasket_enable_dev fixups

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:11PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Remove unnecessary variable.
> 
> Bail out if no physical device.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/gasket_core.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index 65aa7cf454fb..f7d8f66e8746 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -915,8 +915,6 @@ static int gasket_enable_dev(
>  {
>  	int tbl_idx;
>  	int ret;
> -	bool has_dma_ops;
> -	struct device *ddev;
>  	const struct gasket_driver_desc *driver_desc =
>  		internal_desc->driver_desc;
>  
> @@ -934,26 +932,21 @@ static int gasket_enable_dev(
>  		return ret;
>  	}
>  
> -	has_dma_ops = true;
> +	if (!gasket_dev->pci_dev) {
> +		gasket_log_error(gasket_dev,
> +				 "%s: no physical device", __func__);
> +		return -ENODEV;
> +	}

How can this ever be true?  I don't see where this can be removed to
enable this error check to be hit.  What am I missing?

thanks,

greg k-h

Re: [PATCH 13/18] staging: gasket: gasket_enable_dev fixups

[Todd Poynor]

On Fri, Jul 13, 2018 at 11:39 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Jul 13, 2018 at 10:58:11PM -0700, Todd Poynor wrote:
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> Remove unnecessary variable.
>>
>> Bail out if no physical device.
>>
>> Signed-off-by: Todd Poynor <toddpoynor@google.com>
>> ---
>>  drivers/staging/gasket/gasket_core.c | 19 ++++++-------------
>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
>> index 65aa7cf454fb..f7d8f66e8746 100644
>> --- a/drivers/staging/gasket/gasket_core.c
>> +++ b/drivers/staging/gasket/gasket_core.c
>> @@ -915,8 +915,6 @@ static int gasket_enable_dev(
>>  {
>>       int tbl_idx;
>>       int ret;
>> -     bool has_dma_ops;
>> -     struct device *ddev;
>>       const struct gasket_driver_desc *driver_desc =
>>               internal_desc->driver_desc;
>>
>> @@ -934,26 +932,21 @@ static int gasket_enable_dev(
>>               return ret;
>>       }
>>
>> -     has_dma_ops = true;
>> +     if (!gasket_dev->pci_dev) {
>> +             gasket_log_error(gasket_dev,
>> +                              "%s: no physical device", __func__);
>> +             return -ENODEV;
>> +     }
>
> How can this ever be true?  I don't see where this can be removed to
> enable this error check to be hit.  What am I missing?

Agree, I'll split this into two patches for the two things it does,
and just remove the unnecessary check.  Thanks,

>
> thanks,
>
> greg k-h



-- 
Todd

[PATCH 14/18] staging: gasket: fix class create bug handling

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

class_create() never returns NULL, and this driver should never return
PTR_ERR(NULL) anyway.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index f7d8f66e8746..0ef37667e0f1 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -341,7 +341,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	internal->class =
 		class_create(driver_desc->module, driver_desc->name);
 
-	if (IS_ERR_OR_NULL(internal->class)) {
+	if (IS_ERR(internal->class)) {
 		gasket_nodev_error("Cannot register %s class [ret=%ld]",
 				   driver_desc->name, PTR_ERR(internal->class));
 		ret = PTR_ERR(internal->class);
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 14/18] staging: gasket: fix class create bug handling

[Dmitry Torokhov]

On Sat, Jul 14, 2018 at 8:59 AM Todd Poynor <toddpoynor@gmail.com> wrote:
>
> From: Todd Poynor <toddpoynor@google.com>
>
> class_create() never returns NULL, and this driver should never return
> PTR_ERR(NULL) anyway.
>
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>

Reviewed-by: Dmitry Torokhov <dtor@chromium.org>

> ---
>  drivers/staging/gasket/gasket_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index f7d8f66e8746..0ef37667e0f1 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -341,7 +341,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
>         internal->class =
>                 class_create(driver_desc->module, driver_desc->name);
>
> -       if (IS_ERR_OR_NULL(internal->class)) {
> +       if (IS_ERR(internal->class)) {
>                 gasket_nodev_error("Cannot register %s class [ret=%ld]",
>                                    driver_desc->name, PTR_ERR(internal->class));
>                 ret = PTR_ERR(internal->class);
> --
> 2.18.0.203.gfac676dfb9-goog
>

[PATCH 15/18] staging: gasket: remove unnecessary code in coherent allocator

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove extraneous statement in gasket_config_coherent_allocator()

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_ioctl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index 0c1e0723bef1..fcf58f44efed 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -427,10 +427,8 @@ static int gasket_config_coherent_allocator(
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
 		return -EFAULT;
 
-	if (ibuf.size > PAGE_SIZE * MAX_NUM_COHERENT_PAGES) {
-		ibuf.size = PAGE_SIZE * MAX_NUM_COHERENT_PAGES;
+	if (ibuf.size > PAGE_SIZE * MAX_NUM_COHERENT_PAGES)
 		return -ENOMEM;
-	}
 
 	if (ibuf.enable == 0) {
 		ret = gasket_free_coherent_memory(
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 16/18] staging: gasket: gasket core error handling fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Minor fixups to error codes and error handling in gasket core.

No device reset callback registered is not an error condition.

Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 0ef37667e0f1..65780d4dffbf 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -814,7 +814,7 @@ static int gasket_map_pci_bar(struct gasket_dev *gasket_dev, int bar_num)
 			gasket_dev,
 			"Cannot get BAR %d memory region %p",
 			bar_num, &gasket_dev->pci_dev->resource[bar_num]);
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	gasket_dev->bar_data[bar_num].virt_base =
@@ -1655,7 +1655,7 @@ static int gasket_mmap(struct file *filp, struct vm_area_struct *vma)
 			"0x%lx",
 			raw_offset);
 		trace_gasket_mmap_exit(bar_index);
-		return bar_index;
+		return -EINVAL;
 	}
 
 	vma->vm_private_data = gasket_dev;
@@ -1865,11 +1865,8 @@ int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
 	const struct gasket_driver_desc *driver_desc;
 
 	driver_desc = gasket_dev->internal_desc->driver_desc;
-	if (!driver_desc->device_reset_cb) {
-		gasket_log_error(
-			gasket_dev, "No device reset callback was registered.");
-		return -EINVAL;
-	}
+	if (!driver_desc->device_reset_cb)
+		return 0;
 
 	/* Perform a device reset of the requested type. */
 	ret = driver_desc->device_reset_cb(gasket_dev, reset_type);
@@ -1894,7 +1891,7 @@ int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
 	gasket_dev->status = gasket_get_hw_status(gasket_dev);
 	if (gasket_dev->status == GASKET_STATUS_DEAD) {
 		gasket_log_error(gasket_dev, "Device reported as dead.");
-		return -EINVAL;
+		return -EIO;
 	}
 
 	return 0;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 17/18] staging: gasket: don't release coherent mappings

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

coherent address mappings aren't backed by a struct page, don't need to
be released, and don't count as an active page in the page table
bookkeeping.

Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_page_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index d7600d8e385f..9a4a81c010f9 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -1248,7 +1248,7 @@ static void gasket_perform_unmapping(
 				dma_unmap_page(pg_tbl->device, ptes[i].dma_addr,
 					       PAGE_SIZE, DMA_FROM_DEVICE);
 			}
-			if (gasket_release_page(ptes[i].page))
+			if (ptes[i].page && gasket_release_page(ptes[i].page))
 				--pg_tbl->num_active_pages;
 		}
 		ptes[i].status = PTE_FREE;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 18/18] staging: gasket: various cleanups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Cleanups to error codes, code style, error conditions, etc.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex.h              |  7 ++-
 drivers/staging/gasket/apex_driver.c       | 50 ++++++++---------
 drivers/staging/gasket/gasket_core.c       | 10 ++--
 drivers/staging/gasket/gasket_core.h       |  3 +-
 drivers/staging/gasket/gasket_ioctl.c      |  9 +++
 drivers/staging/gasket/gasket_page_table.c | 65 +++++++++++-----------
 drivers/staging/gasket/gasket_page_table.h |  8 +--
 7 files changed, 80 insertions(+), 72 deletions(-)

diff --git a/drivers/staging/gasket/apex.h b/drivers/staging/gasket/apex.h
index f2600aa05191..8c2f004d4427 100644
--- a/drivers/staging/gasket/apex.h
+++ b/drivers/staging/gasket/apex.h
@@ -30,9 +30,10 @@
 
 #define APEX_EXTENDED_SHIFT 63 /* Extended address bit position. */
 
-/* Addresses are 2^3=8 bytes each. */
-/* page in second level page table */
-/* holds APEX_PAGE_SIZE/8 addresses  */
+/*
+ * Addresses are 2^3=8 bytes each. Page in second level page table holds
+ * APEX_PAGE_SIZE/8 addresses.
+ */
 #define APEX_ADDR_SHIFT 3
 #define APEX_LEVEL_SHIFT (APEX_PAGE_SHIFT - APEX_ADDR_SHIFT)
 #define APEX_LEVEL_SIZE BIT(APEX_LEVEL_SHIFT)
diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 40af91952283..647ad6ce144c 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -148,7 +148,7 @@ static int apex_reset(struct gasket_dev *gasket_dev, uint type);
 
 static int apex_get_status(struct gasket_dev *gasket_dev);
 
-static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
+static bool apex_ioctl_check_permissions(struct file *file, uint cmd);
 
 static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
 
@@ -640,9 +640,9 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
  * @file: File pointer from ioctl.
  * @cmd: ioctl command.
  *
- * Returns 1 if the current user may execute this ioctl, and 0 otherwise.
+ * Returns true if the current user may execute this ioctl, and false otherwise.
  */
-static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
+static bool apex_ioctl_check_permissions(struct file *filp, uint cmd)
 {
 	fmode_t write;
 
@@ -677,33 +677,31 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct apex_gate_clock_ioctl ibuf;
 
-	if (bypass_top_level)
+	if (bypass_top_level || !allow_sw_clock_gating)
 		return 0;
 
-	if (allow_sw_clock_gating) {
-		if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
-			return -EFAULT;
+	if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
+		return -EFAULT;
 
-		gasket_log_error(
-			gasket_dev, "apex_clock_gating %llu", ibuf.enable);
+	gasket_log_info(
+	    gasket_dev, "apex_clock_gating %llu", ibuf.enable);
 
-		if (ibuf.enable) {
-			/* Quiesce AXI, gate GCB clock. */
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_AXI_QUIESCE, 0x1, 1, 16);
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_GCB_CLOCK_GATE, 0x1, 2, 18);
-		} else {
-			/* Un-gate GCB clock, un-quiesce AXI. */
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_GCB_CLOCK_GATE, 0x0, 2, 18);
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_AXI_QUIESCE, 0x0, 1, 16);
-		}
+	if (ibuf.enable) {
+		/* Quiesce AXI, gate GCB clock. */
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_AXI_QUIESCE, 0x1, 1, 16);
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_GCB_CLOCK_GATE, 0x1, 2, 18);
+	} else {
+		/* Un-gate GCB clock, un-quiesce AXI. */
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_GCB_CLOCK_GATE, 0x0, 2, 18);
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_AXI_QUIESCE, 0x0, 1, 16);
 	}
 	return 0;
 }
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 65780d4dffbf..fc89654e562d 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1619,10 +1619,10 @@ static int gasket_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	driver_desc = gasket_dev->internal_desc->driver_desc;
 
-	if (vma->vm_start & (PAGE_SIZE - 1)) {
+	if (vma->vm_start & ~PAGE_MASK) {
 		gasket_log_error(
-			gasket_dev, "Base address not page-aligned: 0x%p\n",
-			(void *)vma->vm_start);
+			gasket_dev, "Base address not page-aligned: %lx\n",
+			vma->vm_start);
 		trace_gasket_mmap_exit(-EINVAL);
 		return -EINVAL;
 	}
@@ -1917,7 +1917,7 @@ static ssize_t gasket_write_mappable_regions(
 	if (bar_desc.permissions == GASKET_NOMAP)
 		return 0;
 	for (i = 0;
-	     (i < bar_desc.num_mappable_regions) && (total_written < PAGE_SIZE);
+	     i < bar_desc.num_mappable_regions && total_written < PAGE_SIZE;
 	     i++) {
 		min_addr = bar_desc.mappable_regions[i].start -
 			   driver_desc->legacy_mmap_address_offset;
@@ -2081,7 +2081,7 @@ struct device *gasket_get_device(struct gasket_dev *dev)
  *
  * Description: Busy waits for a specific combination of bits to be set
  * on a Gasket register.
- **/
+ */
 int gasket_wait_sync(
 	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
 	u64 timeout_ns)
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 177b239e2c50..ab3d69ee6faa 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -61,7 +61,8 @@ enum gasket_interrupt_type {
 	PLATFORM_WIRE = 2,
 };
 
-/* Used to describe a Gasket interrupt. Contains an interrupt index, a register,
+/*
+ * Used to describe a Gasket interrupt. Contains an interrupt index, a register,
  * and packing data for that interrupt. The register and packing data
  * fields are relevant only for PCI_MSIX interrupt type and can be
  * set to 0 for everything else.
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index fcf58f44efed..e6b27f5c067c 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -361,6 +361,9 @@ static int gasket_map_buffers(struct gasket_dev *gasket_dev,
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
+	if (ibuf.size < PAGE_SIZE)
+		return -EINVAL;
+
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
 		return -EFAULT;
 
@@ -424,12 +427,18 @@ static int gasket_config_coherent_allocator(
 	trace_gasket_ioctl_config_coherent_allocator(
 		ibuf.enable, ibuf.size, ibuf.dma_address);
 
+	if (ibuf.size < PAGE_SIZE)
+		return -EINVAL;
+
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
 		return -EFAULT;
 
 	if (ibuf.size > PAGE_SIZE * MAX_NUM_COHERENT_PAGES)
 		return -ENOMEM;
 
+	if (gasket_dev->page_table[ibuf.page_table_index] == 0)
+		return -EINVAL;
+
 	if (ibuf.enable == 0) {
 		ret = gasket_free_coherent_memory(
 			gasket_dev, ibuf.size, ibuf.dma_address,
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 9a4a81c010f9..299af8a6c54f 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -269,16 +269,16 @@ static void gasket_perform_unmapping(
 static void gasket_free_extended_subtable(
 	struct gasket_page_table *pg_tbl, struct gasket_page_table_entry *pte,
 	u64 __iomem *att_reg);
-static int gasket_release_page(struct page *page);
+static bool gasket_release_page(struct page *page);
 
 /* Other/utility declarations */
-static inline int gasket_addr_is_simple(
+static inline bool gasket_addr_is_simple(
 	struct gasket_page_table *pg_tbl, ulong addr);
-static int gasket_is_simple_dev_addr_bad(
+static bool gasket_is_simple_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages);
-static int gasket_is_extended_dev_addr_bad(
+static bool gasket_is_extended_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages);
-static int gasket_is_pte_range_free(
+static bool gasket_is_pte_range_free(
 	struct gasket_page_table_entry *pte, uint num_entries);
 static void gasket_page_table_garbage_collect_nolock(
 	struct gasket_page_table *pg_tbl);
@@ -564,7 +564,7 @@ int gasket_page_table_lookup_page(
 }
 
 /* See gasket_page_table.h for description. */
-int gasket_page_table_are_addrs_bad(
+bool gasket_page_table_are_addrs_bad(
 	struct gasket_page_table *pg_tbl, ulong host_addr, ulong dev_addr,
 	ulong bytes)
 {
@@ -573,7 +573,7 @@ int gasket_page_table_are_addrs_bad(
 			pg_tbl,
 			"host mapping address 0x%lx must be page aligned",
 			host_addr);
-		return 1;
+		return true;
 	}
 
 	return gasket_page_table_is_dev_addr_bad(pg_tbl, dev_addr, bytes);
@@ -581,7 +581,7 @@ int gasket_page_table_are_addrs_bad(
 EXPORT_SYMBOL(gasket_page_table_are_addrs_bad);
 
 /* See gasket_page_table.h for description. */
-int gasket_page_table_is_dev_addr_bad(
+bool gasket_page_table_is_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, ulong bytes)
 {
 	uint num_pages = bytes / PAGE_SIZE;
@@ -590,7 +590,7 @@ int gasket_page_table_is_dev_addr_bad(
 		gasket_pg_tbl_error(
 			pg_tbl,
 			"mapping size 0x%lX must be page aligned", bytes);
-		return 1;
+		return true;
 	}
 
 	if (num_pages == 0) {
@@ -598,15 +598,14 @@ int gasket_page_table_is_dev_addr_bad(
 			pg_tbl,
 			"requested mapping is less than one page: %lu / %lu",
 			bytes, PAGE_SIZE);
-		return 1;
+		return true;
 	}
 
 	if (gasket_addr_is_simple(pg_tbl, dev_addr))
 		return gasket_is_simple_dev_addr_bad(
 			pg_tbl, dev_addr, num_pages);
-	else
-		return gasket_is_extended_dev_addr_bad(
-			pg_tbl, dev_addr, num_pages);
+	return gasket_is_extended_dev_addr_bad(
+	    pg_tbl, dev_addr, num_pages);
 }
 EXPORT_SYMBOL(gasket_page_table_is_dev_addr_bad);
 
@@ -1300,23 +1299,23 @@ static void gasket_free_extended_subtable(
 /*
  * Safely return a page to the OS.
  * @page: The page to return to the OS.
- * Returns 1 if the page was released, 0 if it was
+ * Returns true if the page was released, false if it was
  * ignored.
  */
-static int gasket_release_page(struct page *page)
+static bool gasket_release_page(struct page *page)
 {
 	if (!page)
-		return 0;
+		return false;
 
 	if (!PageReserved(page))
 		SetPageDirty(page);
 	put_page(page);
 
-	return 1;
+	return true;
 }
 
 /* Evaluates to nonzero if the specified virtual address is simple. */
-static inline int gasket_addr_is_simple(
+static inline bool gasket_addr_is_simple(
 	struct gasket_page_table *pg_tbl, ulong addr)
 {
 	return !((addr) & (pg_tbl)->extended_flag);
@@ -1332,7 +1331,7 @@ static inline int gasket_addr_is_simple(
  * address to/from page + offset) and that the requested page range starts and
  * ends within the set of currently-partitioned simple pages.
  */
-static int gasket_is_simple_dev_addr_bad(
+static bool gasket_is_simple_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages)
 {
 	ulong page_offset = dev_addr & (PAGE_SIZE - 1);
@@ -1343,7 +1342,7 @@ static int gasket_is_simple_dev_addr_bad(
 		pg_tbl, 1, page_index, page_offset) != dev_addr) {
 		gasket_pg_tbl_error(
 			pg_tbl, "address is invalid, 0x%lX", dev_addr);
-		return 1;
+		return true;
 	}
 
 	if (page_index >= pg_tbl->num_simple_entries) {
@@ -1351,7 +1350,7 @@ static int gasket_is_simple_dev_addr_bad(
 			pg_tbl,
 			"starting slot at %lu is too large, max is < %u",
 			page_index, pg_tbl->num_simple_entries);
-		return 1;
+		return true;
 	}
 
 	if (page_index + num_pages > pg_tbl->num_simple_entries) {
@@ -1359,10 +1358,10 @@ static int gasket_is_simple_dev_addr_bad(
 			pg_tbl,
 			"ending slot at %lu is too large, max is <= %u",
 			page_index + num_pages, pg_tbl->num_simple_entries);
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 
 /*
@@ -1374,7 +1373,7 @@ static int gasket_is_simple_dev_addr_bad(
  * @dev_addr: The device address to which the pages will be mapped.
  * @num_pages: The number of second-level/sub pages in the range to consider.
  */
-static int gasket_is_extended_dev_addr_bad(
+static bool gasket_is_extended_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages)
 {
 	/* Starting byte index of dev_addr into the first mapped page */
@@ -1388,7 +1387,7 @@ static int gasket_is_extended_dev_addr_bad(
 	if (addr >> (GASKET_EXTENDED_LVL0_WIDTH + GASKET_EXTENDED_LVL0_SHIFT)) {
 		gasket_pg_tbl_error(pg_tbl, "device address out of bound, 0x%p",
 				    (void *)dev_addr);
-		return 1;
+		return true;
 	}
 
 	/* Find the starting sub-page index in the space of all sub-pages. */
@@ -1406,7 +1405,7 @@ static int gasket_is_extended_dev_addr_bad(
 		pg_tbl, 0, page_global_idx, page_offset) != dev_addr) {
 		gasket_pg_tbl_error(
 			pg_tbl, "address is invalid, 0x%p", (void *)dev_addr);
-		return 1;
+		return true;
 	}
 
 	if (page_lvl0_idx >= pg_tbl->num_extended_entries) {
@@ -1414,7 +1413,7 @@ static int gasket_is_extended_dev_addr_bad(
 			pg_tbl,
 			"starting level 0 slot at %lu is too large, max is < "
 			"%u", page_lvl0_idx, pg_tbl->num_extended_entries);
-		return 1;
+		return true;
 	}
 
 	if (page_lvl0_idx + num_lvl0_pages > pg_tbl->num_extended_entries) {
@@ -1423,10 +1422,10 @@ static int gasket_is_extended_dev_addr_bad(
 			"ending level 0 slot at %lu is too large, max is <= %u",
 			page_lvl0_idx + num_lvl0_pages,
 			pg_tbl->num_extended_entries);
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 
 /*
@@ -1440,17 +1439,17 @@ static int gasket_is_extended_dev_addr_bad(
  *
  * The page table mutex must be held before this call.
  */
-static int gasket_is_pte_range_free(
+static bool gasket_is_pte_range_free(
 	struct gasket_page_table_entry *ptes, uint num_entries)
 {
 	int i;
 
 	for (i = 0; i < num_entries; i++) {
 		if (ptes[i].status != PTE_FREE)
-			return 0;
+			return false;
 	}
 
-	return 1;
+	return true;
 }
 
 /*
@@ -1656,7 +1655,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev *gasket_dev, u64 size,
 	dma_addr_t handle;
 	void *mem;
 	int j;
-	unsigned int num_pages = (size + PAGE_SIZE - 1) / (PAGE_SIZE);
+	unsigned int num_pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
 	const struct gasket_driver_desc *driver_desc =
 		gasket_get_driver_desc(gasket_dev);
 
diff --git a/drivers/staging/gasket/gasket_page_table.h b/drivers/staging/gasket/gasket_page_table.h
index f2f519a3022d..9180b56984ce 100644
--- a/drivers/staging/gasket/gasket_page_table.h
+++ b/drivers/staging/gasket/gasket_page_table.h
@@ -169,9 +169,9 @@ int gasket_page_table_lookup_page(
  * specified by both addresses and the size are valid for mapping pages into
  * device memory.
  *
- * Returns 1 if true - if the mapping is bad, 0 otherwise.
+ * Returns true if the mapping is bad, false otherwise.
  */
-int gasket_page_table_are_addrs_bad(
+bool gasket_page_table_are_addrs_bad(
 	struct gasket_page_table *page_table, ulong host_addr, ulong dev_addr,
 	ulong bytes);
 
@@ -185,9 +185,9 @@ int gasket_page_table_are_addrs_bad(
  * specified by the device address and the size is valid for mapping pages into
  * device memory.
  *
- * Returns 1 if true - if the address is bad, 0 otherwise.
+ * Returns true if the address is bad, false otherwise.
  */
-int gasket_page_table_is_dev_addr_bad(
+bool gasket_page_table_is_dev_addr_bad(
 	struct gasket_page_table *page_table, ulong dev_addr, ulong bytes);
 
 /*
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 18/18] staging: gasket: various cleanups

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 10:58:16PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Cleanups to error codes, code style, error conditions, etc.

You you need to break this up into "one type of logical change per
patch" here for this change, sorry.

thanks,

greg k-h