Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-12-01 Thread Greg KH 
On Thu, Dec 01, 2016 at 10:20:47AM +0100, Christian Gromm wrote:
> On Thu, 1 Dec 2016 09:56:11 +0100
> Greg KH  wrote:
> 
> > On Thu, Dec 01, 2016 at 09:50:12AM +0100, Christian Gromm wrote:
> > > On Thu, 1 Dec 2016 08:00:57 +0100
> > > Greg KH  wrote:
> > > 
> > > > On Wed, Nov 30, 2016 at 10:48:32PM -0700, Jason Litzinger wrote:
> > > > > > This is fine, but the fact that the most subsystem feels like it 
> > > > > > has to
> > > > > > have its own attribute type is the big problem with this file, that
> > > > > > should not be needed at all, and hopefully will be fixed up someday 
> > > > > > soon
> > > > > > (i.e. it's a requirement before it can get out of staging...)
> > > 
> > > That's interesting. We still need to set up a strategy to get this driver
> > > out of staging one day, as we decided on the ELCE 2015 in Dublin. A good
> > > starting point would be a list of requirements that need to be met to
> > > achieve this goal.
> > > 
> > > Does this make sense?
> > 
> > Yes, and starting with cleaning up the kobject mess would be a good
> > first item for that list :)
> 
> And what "kobject mess" exactly is it that you're talking about?
> The kobjects to hook things up in sysfs?

Yes, you should never be using "raw" kobjects in a driver subsystem like
this, you should tie into the "real" driver model by being a bus and
driver subsystem.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-12-01 Thread Christian Gromm 
On Thu, 1 Dec 2016 09:56:11 +0100
Greg KH  wrote:

> On Thu, Dec 01, 2016 at 09:50:12AM +0100, Christian Gromm wrote:
> > On Thu, 1 Dec 2016 08:00:57 +0100
> > Greg KH  wrote:
> > 
> > > On Wed, Nov 30, 2016 at 10:48:32PM -0700, Jason Litzinger wrote:
> > > > > This is fine, but the fact that the most subsystem feels like it has 
> > > > > to
> > > > > have its own attribute type is the big problem with this file, that
> > > > > should not be needed at all, and hopefully will be fixed up someday 
> > > > > soon
> > > > > (i.e. it's a requirement before it can get out of staging...)
> > 
> > That's interesting. We still need to set up a strategy to get this driver
> > out of staging one day, as we decided on the ELCE 2015 in Dublin. A good
> > starting point would be a list of requirements that need to be met to
> > achieve this goal.
> > 
> > Does this make sense?
> 
> Yes, and starting with cleaning up the kobject mess would be a good
> first item for that list :)

And what "kobject mess" exactly is it that you're talking about?
The kobjects to hook things up in sysfs?

regards,
Chris

> 
> thanks,
> 
> greg k-h

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-12-01 Thread Greg KH 
On Thu, Dec 01, 2016 at 09:50:12AM +0100, Christian Gromm wrote:
> On Thu, 1 Dec 2016 08:00:57 +0100
> Greg KH  wrote:
> 
> > On Wed, Nov 30, 2016 at 10:48:32PM -0700, Jason Litzinger wrote:
> > > > This is fine, but the fact that the most subsystem feels like it has to
> > > > have its own attribute type is the big problem with this file, that
> > > > should not be needed at all, and hopefully will be fixed up someday soon
> > > > (i.e. it's a requirement before it can get out of staging...)
> 
> That's interesting. We still need to set up a strategy to get this driver
> out of staging one day, as we decided on the ELCE 2015 in Dublin. A good
> starting point would be a list of requirements that need to be met to
> achieve this goal.
> 
> Does this make sense?

Yes, and starting with cleaning up the kobject mess would be a good
first item for that list :)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-12-01 Thread Christian Gromm 
On Thu, 1 Dec 2016 08:00:57 +0100
Greg KH  wrote:

> On Wed, Nov 30, 2016 at 10:48:32PM -0700, Jason Litzinger wrote:
> > > This is fine, but the fact that the most subsystem feels like it has to
> > > have its own attribute type is the big problem with this file, that
> > > should not be needed at all, and hopefully will be fixed up someday soon
> > > (i.e. it's a requirement before it can get out of staging...)

That's interesting. We still need to set up a strategy to get this driver
out of staging one day, as we decided on the ELCE 2015 in Dublin. A good
starting point would be a list of requirements that need to be met to
achieve this goal.

Does this make sense?

  
> > Ok, couple follow up questions.
> > 
> > Something like struct device/DEVICE_ATTR_* (or something that exists
> > already) to expose the same functionality?
> 
> Yes, that is correct.
> 
> > I'm happy to iterate patches to address this, but, Christian, do you already
> > have a plan/patchset in the works?  I haven't come across a prior
> > discussion of this in the mailing list, but I may have missed it in my
> > search.

Yes, we do have plans to fix this up. I'm just not sure _when_ we are
going to send in patches for this.

> 
> It's a non-trivial change, and it requires you to understand the driver
> model code a bunch to make a new bus type and register devices to it.
> If you have questions about it, let me know.
> 
> good luck!
> 
> greg k-h

regards,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-11-30 Thread Greg KH 
On Wed, Nov 30, 2016 at 10:48:32PM -0700, Jason Litzinger wrote:
> > This is fine, but the fact that the most subsystem feels like it has to
> > have its own attribute type is the big problem with this file, that
> > should not be needed at all, and hopefully will be fixed up someday soon
> > (i.e. it's a requirement before it can get out of staging...)
> Ok, couple follow up questions.
> 
> Something like struct device/DEVICE_ATTR_* (or something that exists
> already) to expose the same functionality?

Yes, that is correct.

> I'm happy to iterate patches to address this, but, Christian, do you already
> have a plan/patchset in the works?  I haven't come across a prior
> discussion of this in the mailing list, but I may have missed it in my
> search.

It's a non-trivial change, and it requires you to understand the driver
model code a bunch to make a new bus type and register devices to it.
If you have questions about it, let me know.

good luck!

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-11-30 Thread Jason Litzinger 
> This is fine, but the fact that the most subsystem feels like it has to
> have its own attribute type is the big problem with this file, that
> should not be needed at all, and hopefully will be fixed up someday soon
> (i.e. it's a requirement before it can get out of staging...)
Ok, couple follow up questions.

Something like struct device/DEVICE_ATTR_* (or something that exists
already) to expose the same functionality?

I'm happy to iterate patches to address this, but, Christian, do you already
have a plan/patchset in the works?  I haven't come across a prior
discussion of this in the mailing list, but I may have missed it in my
search.

Thanks,
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-11-29 Thread Greg KH 
On Fri, Nov 25, 2016 at 03:31:09PM -0700, Jason Litzinger wrote:
> Fix checkpatch warnings regarding the use of symbolic permissions.
> 
> Where the MOST_CHANNEL_ATTR macro is used, convert to octal
> permissions over symbolic.
> 
> Where _ATTR is used directly, replace with _ATTR_RW/_ATTR_WO and
> update the show/store function names appropriately.
> 
> Signed-off-by: Jason Litzinger 
> ---
> changelog:
> v2)  Fix missing semicolon on hunk with _ATTR_WO.
> 
> Tested/built/loaded linux-next(next-20161125)
> Test-built staging-next from staging tree

This is fine, but the fact that the most subsystem feels like it has to
have its own attribute type is the big problem with this file, that
should not be needed at all, and hopefully will be fixed up someday soon
(i.e. it's a requirement before it can get out of staging...)

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: most: Eliminate usage of symbolic permissions

2016-11-25 Thread Jason Litzinger 
Fix checkpatch warnings regarding the use of symbolic permissions.

Where the MOST_CHANNEL_ATTR macro is used, convert to octal
permissions over symbolic.

Where _ATTR is used directly, replace with _ATTR_RW/_ATTR_WO and
update the show/store function names appropriately.

Signed-off-by: Jason Litzinger 
---
changelog:
v2)  Fix missing semicolon on hunk with _ATTR_WO.

Tested/built/loaded linux-next(next-20161125)
Test-built staging-next from staging tree

 drivers/staging/most/mostcore/core.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/most/mostcore/core.c 
b/drivers/staging/most/mostcore/core.c
index 4c580d1a6a46..191404bc5906 100644
--- a/drivers/staging/most/mostcore/core.c
+++ b/drivers/staging/most/mostcore/core.c
@@ -342,7 +342,7 @@ static ssize_t show_channel_starving(struct most_c_obj *c,
 }
 
 #define create_show_channel_attribute(val) \
-   static MOST_CHNL_ATTR(val, S_IRUGO, show_##val, NULL)
+   static MOST_CHNL_ATTR(val, 0444, show_##val, NULL)
 
 create_show_channel_attribute(available_directions);
 create_show_channel_attribute(available_datatypes);
@@ -494,9 +494,7 @@ static ssize_t store_set_packets_per_xact(struct most_c_obj 
*c,
 }
 
 #define create_channel_attribute(value) \
-   static MOST_CHNL_ATTR(value, S_IRUGO | S_IWUSR, \
- show_##value, \
- store_##value)
+   static MOST_CHNL_ATTR(value, 0644, show_##value, store_##value)
 
 create_channel_attribute(set_buffer_size);
 create_channel_attribute(set_number_of_buffers);
@@ -690,7 +688,7 @@ static ssize_t show_interface(struct most_inst_obj 
*instance_obj,
 }
 
 #define create_inst_attribute(value) \
-   static MOST_INST_ATTR(value, S_IRUGO, show_##value, NULL)
+   static MOST_INST_ATTR(value, 0444, show_##value, NULL)
 
 create_inst_attribute(description);
 create_inst_attribute(interface);
@@ -849,7 +847,7 @@ static void most_aim_release(struct kobject *kobj)
kfree(aim_obj);
 }
 
-static ssize_t show_add_link(struct most_aim_obj *aim_obj,
+static ssize_t add_link_show(struct most_aim_obj *aim_obj,
 struct most_aim_attribute *attr,
 char *buf)
 {
@@ -966,7 +964,7 @@ most_c_obj *get_channel_by_name(char *mdev, char *mdev_ch)
  * (1) would create the device node /dev/my_rxchannel
  * (2) would create the device node /dev/mdev1-ep81
  */
-static ssize_t store_add_link(struct most_aim_obj *aim_obj,
+static ssize_t add_link_store(struct most_aim_obj *aim_obj,
  struct most_aim_attribute *attr,
  const char *buf,
  size_t len)
@@ -1016,7 +1014,7 @@ static ssize_t store_add_link(struct most_aim_obj 
*aim_obj,
 }
 
 static struct most_aim_attribute most_aim_attr_add_link =
-   __ATTR(add_link, S_IRUGO | S_IWUSR, show_add_link, store_add_link);
+   __ATTR_RW(add_link);
 
 /**
  * store_remove_link - store function for remove_link attribute
@@ -1028,7 +1026,7 @@ static struct most_aim_attribute most_aim_attr_add_link =
  * Example:
  * echo "mdev0:ep81" >remove_link
  */
-static ssize_t store_remove_link(struct most_aim_obj *aim_obj,
+static ssize_t remove_link_store(struct most_aim_obj *aim_obj,
 struct most_aim_attribute *attr,
 const char *buf,
 size_t len)
@@ -1059,7 +1057,7 @@ static ssize_t store_remove_link(struct most_aim_obj 
*aim_obj,
 }
 
 static struct most_aim_attribute most_aim_attr_remove_link =
-   __ATTR(remove_link, S_IWUSR, NULL, store_remove_link);
+   __ATTR_WO(remove_link);
 
 static struct attribute *most_aim_def_attrs[] = {
_aim_attr_add_link.attr,
-- 
2.10.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel