[PATCH RFC v2 0/9] staging: most: move core module out of staging

[PATCH RFC v2 0/9] staging: most: move core module out of staging

[Christian Gromm]

The MOST driver was pushed to the staging area with kernel 4.3. Since then
it has encountered many refinements by the community and should be ready
for an upstream audit and to be moved out of the staging area. Since the
driver consists of multiple modules the core module is about to go first
and the other modules will follow subsequently. This patchset executes the
necessary steps to move the core module out of staging.

v2:
Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
	- use -M option to create patches

Christian Gromm (9):
  staging: most: rename core.h to most.h
  staging: most: rename struct core_component
  staging: most: rename enum mbo_status_flags
  staging: most: configfs: use strlcpy
  staging: most: configfs: reduce array size
  staging: most: use angle brackets in include path
  staging: most: move core files out of the staging area
  staging: most: Documentation: update ABI description
  staging: most: Documentation: move ABI description files out of
    staging area

 .../ABI/testing/configfs-most                      |  8 -----
 .../ABI/testing/sysfs-bus-most                     | 24 ++-------------
 drivers/Kconfig                                    |  1 +
 drivers/Makefile                                   |  1 +
 drivers/most/Kconfig                               | 15 ++++++++++
 drivers/most/Makefile                              |  4 +++
 drivers/{staging => }/most/configfs.c              | 32 +++++++++++---------
 drivers/{staging => }/most/core.c                  | 34 +++++++++++-----------
 drivers/staging/most/Kconfig                       |  6 ++--
 drivers/staging/most/Makefile                      |  5 ----
 drivers/staging/most/cdev/cdev.c                   |  4 +--
 drivers/staging/most/dim2/dim2.c                   |  2 +-
 drivers/staging/most/i2c/i2c.c                     |  2 +-
 drivers/staging/most/net/net.c                     |  6 ++--
 drivers/staging/most/sound/sound.c                 |  8 ++---
 drivers/staging/most/usb/usb.c                     |  2 +-
 drivers/staging/most/video/video.c                 |  6 ++--
 .../staging/most/core.h => include/linux/most.h    | 24 +++++++--------
 18 files changed, 89 insertions(+), 95 deletions(-)
 rename drivers/staging/most/Documentation/ABI/configfs-most.txt => Documentation/ABI/testing/configfs-most (94%)
 rename drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt => Documentation/ABI/testing/sysfs-bus-most (92%)
 create mode 100644 drivers/most/Kconfig
 create mode 100644 drivers/most/Makefile
 rename drivers/{staging => }/most/configfs.c (96%)
 rename drivers/{staging => }/most/core.c (98%)
 rename drivers/staging/most/core.h => include/linux/most.h (95%)

-- 
2.7.4

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

[PATCH RFC v2 1/9] staging: most: rename core.h to most.h

[Christian Gromm]

This patch renames the core header file core.h to most.h. The intention
behind this is to have a meaningful name once this file is moved to the
/include/linux directory.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:

 drivers/staging/most/cdev/cdev.c        | 2 +-
 drivers/staging/most/configfs.c         | 2 +-
 drivers/staging/most/core.c             | 2 +-
 drivers/staging/most/dim2/dim2.c        | 2 +-
 drivers/staging/most/i2c/i2c.c          | 2 +-
 drivers/staging/most/{core.h => most.h} | 0
 drivers/staging/most/net/net.c          | 2 +-
 drivers/staging/most/sound/sound.c      | 2 +-
 drivers/staging/most/usb/usb.c          | 2 +-
 drivers/staging/most/video/video.c      | 2 +-
 10 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/staging/most/{core.h => most.h} (100%)

diff --git a/drivers/staging/most/cdev/cdev.c b/drivers/staging/most/cdev/cdev.c
index f880147..df4cb5a 100644
--- a/drivers/staging/most/cdev/cdev.c
+++ b/drivers/staging/most/cdev/cdev.c
@@ -16,7 +16,7 @@
 #include <linux/kfifo.h>
 #include <linux/uaccess.h>
 #include <linux/idr.h>
-#include "most/core.h"
+#include "most/most.h"
 
 #define CHRDEV_REGION_SIZE 50
 
diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
index 34a9fb5..e17d846 100644
--- a/drivers/staging/most/configfs.c
+++ b/drivers/staging/most/configfs.c
@@ -10,7 +10,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/configfs.h>
-#include <most/core.h>
+#include <most/most.h>
 
 struct mdev_link {
 	struct config_item item;
diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 51a6b41..e32030c 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -21,7 +21,7 @@
 #include <linux/kthread.h>
 #include <linux/dma-mapping.h>
 #include <linux/idr.h>
-#include <most/core.h>
+#include <most/most.h>
 
 #define MAX_CHANNELS	64
 #define STRING_SIZE	80
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 64c9791..e15e847 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -21,7 +21,7 @@
 #include <linux/sched.h>
 #include <linux/kthread.h>
 
-#include "most/core.h"
+#include "most/most.h"
 #include "hal.h"
 #include "errors.h"
 #include "sysfs.h"
diff --git a/drivers/staging/most/i2c/i2c.c b/drivers/staging/most/i2c/i2c.c
index 4a4fc10..d4606ac 100644
--- a/drivers/staging/most/i2c/i2c.c
+++ b/drivers/staging/most/i2c/i2c.c
@@ -14,7 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/err.h>
 
-#include "most/core.h"
+#include "most/most.h"
 
 enum { CH_RX, CH_TX, NUM_CHANNELS };
 
diff --git a/drivers/staging/most/core.h b/drivers/staging/most/most.h
similarity index 100%
rename from drivers/staging/most/core.h
rename to drivers/staging/most/most.h
diff --git a/drivers/staging/most/net/net.c b/drivers/staging/most/net/net.c
index 6cab1bb..34d93c4 100644
--- a/drivers/staging/most/net/net.c
+++ b/drivers/staging/most/net/net.c
@@ -15,7 +15,7 @@
 #include <linux/list.h>
 #include <linux/wait.h>
 #include <linux/kobject.h>
-#include "most/core.h"
+#include "most/most.h"
 
 #define MEP_HDR_LEN 8
 #define MDP_HDR_LEN 16
diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 1ccfcb8..38642b2 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -17,7 +17,7 @@
 #include <sound/pcm_params.h>
 #include <linux/sched.h>
 #include <linux/kthread.h>
-#include <most/core.h>
+#include <most/most.h>
 
 #define DRIVER_NAME "sound"
 #define STRING_SIZE	80
diff --git a/drivers/staging/most/usb/usb.c b/drivers/staging/most/usb/usb.c
index 360cb5b..69756ca 100644
--- a/drivers/staging/most/usb/usb.c
+++ b/drivers/staging/most/usb/usb.c
@@ -23,7 +23,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/etherdevice.h>
 #include <linux/uaccess.h>
-#include "most/core.h"
+#include "most/most.h"
 
 #define USB_MTU			512
 #define NO_ISOCHRONOUS_URB	0
diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 10c1ef7..b75ccc8 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -21,7 +21,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
 
-#include "most/core.h"
+#include "most/most.h"
 
 #define V4L2_CMP_MAX_INPUT  1
 
-- 
2.7.4

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

[PATCH RFC v2 2/9] staging: most: rename struct core_component

[Christian Gromm]

The stuctures defined in the most.h header file will be exposed to the
kernel once the file is moved out of the staging area. That's why the name
is changed into something more descriptive.

Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:
	This patch has been added to the series.

 drivers/staging/most/cdev/cdev.c   |  2 +-
 drivers/staging/most/configfs.c    |  4 ++--
 drivers/staging/most/core.c        | 32 ++++++++++++++++----------------
 drivers/staging/most/most.h        | 20 ++++++++++----------
 drivers/staging/most/net/net.c     |  4 ++--
 drivers/staging/most/sound/sound.c |  6 +++---
 drivers/staging/most/video/video.c |  4 ++--
 7 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/most/cdev/cdev.c b/drivers/staging/most/cdev/cdev.c
index df4cb5a..9ce2f23 100644
--- a/drivers/staging/most/cdev/cdev.c
+++ b/drivers/staging/most/cdev/cdev.c
@@ -25,7 +25,7 @@ static struct cdev_component {
 	struct ida minor_id;
 	unsigned int major;
 	struct class *class;
-	struct core_component cc;
+	struct most_component cc;
 } comp;
 
 struct comp_channel {
diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
index e17d846..d671022 100644
--- a/drivers/staging/most/configfs.c
+++ b/drivers/staging/most/configfs.c
@@ -630,7 +630,7 @@ static struct most_sound most_sound_subsys = {
 	},
 };
 
-int most_register_configfs_subsys(struct core_component *c)
+int most_register_configfs_subsys(struct most_component *c)
 {
 	int ret;
 
@@ -674,7 +674,7 @@ void most_interface_register_notify(const char *mdev)
 		most_cfg_complete("sound");
 }
 
-void most_deregister_configfs_subsys(struct core_component *c)
+void most_deregister_configfs_subsys(struct most_component *c)
 {
 	if (!strcmp(c->name, "cdev"))
 		configfs_unregister_subsystem(&most_cdev.subsys);
diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index e32030c..af542ed 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -39,7 +39,7 @@ static struct mostcore {
 #define to_driver(d) container_of(d, struct mostcore, drv)
 
 struct pipe {
-	struct core_component *comp;
+	struct most_component *comp;
 	int refs;
 	int num_buffers;
 };
@@ -454,9 +454,9 @@ static const struct attribute_group *interface_attr_groups[] = {
 	NULL,
 };
 
-static struct core_component *match_component(char *name)
+static struct most_component *match_component(char *name)
 {
-	struct core_component *comp;
+	struct most_component *comp;
 
 	list_for_each_entry(comp, &mc.comp_list, list) {
 		if (!strcmp(comp->name, name))
@@ -510,7 +510,7 @@ static ssize_t links_show(struct device_driver *drv, char *buf)
 
 static ssize_t components_show(struct device_driver *drv, char *buf)
 {
-	struct core_component *comp;
+	struct most_component *comp;
 	int offs = 0;
 
 	list_for_each_entry(comp, &mc.comp_list, list) {
@@ -544,12 +544,12 @@ static struct most_channel *get_channel(char *mdev, char *mdev_ch)
 
 static
 inline int link_channel_to_component(struct most_channel *c,
-				     struct core_component *comp,
+				     struct most_component *comp,
 				     char *name,
 				     char *comp_param)
 {
 	int ret;
-	struct core_component **comp_ptr;
+	struct most_component **comp_ptr;
 
 	if (!c->pipe0.comp)
 		comp_ptr = &c->pipe0.comp;
@@ -660,7 +660,7 @@ int most_set_cfg_packets_xact(char *mdev, char *mdev_ch, u16 val)
 
 int most_cfg_complete(char *comp_name)
 {
-	struct core_component *comp;
+	struct most_component *comp;
 
 	comp = match_component(comp_name);
 	if (!comp)
@@ -673,7 +673,7 @@ int most_add_link(char *mdev, char *mdev_ch, char *comp_name, char *link_name,
 		  char *comp_param)
 {
 	struct most_channel *c = get_channel(mdev, mdev_ch);
-	struct core_component *comp = match_component(comp_name);
+	struct most_component *comp = match_component(comp_name);
 
 	if (!c || !comp)
 		return -ENODEV;
@@ -684,7 +684,7 @@ int most_add_link(char *mdev, char *mdev_ch, char *comp_name, char *link_name,
 int most_remove_link(char *mdev, char *mdev_ch, char *comp_name)
 {
 	struct most_channel *c;
-	struct core_component *comp;
+	struct most_component *comp;
 
 	comp = match_component(comp_name);
 	if (!comp)
@@ -950,7 +950,7 @@ static void most_write_completion(struct mbo *mbo)
 }
 
 int channel_has_mbo(struct most_interface *iface, int id,
-		    struct core_component *comp)
+		    struct most_component *comp)
 {
 	struct most_channel *c = iface->p->channel[id];
 	unsigned long flags;
@@ -981,7 +981,7 @@ EXPORT_SYMBOL_GPL(channel_has_mbo);
  * Returns a pointer to MBO on success or NULL otherwise.
  */
 struct mbo *most_get_mbo(struct most_interface *iface, int id,
-			 struct core_component *comp)
+			 struct most_component *comp)
 {
 	struct mbo *mbo;
 	struct most_channel *c;
@@ -1087,7 +1087,7 @@ static void most_read_completion(struct mbo *mbo)
  * Returns 0 on success or error code otherwise.
  */
 int most_start_channel(struct most_interface *iface, int id,
-		       struct core_component *comp)
+		       struct most_component *comp)
 {
 	int num_buffer;
 	int ret;
@@ -1157,7 +1157,7 @@ EXPORT_SYMBOL_GPL(most_start_channel);
  * @comp: driver component
  */
 int most_stop_channel(struct most_interface *iface, int id,
-		      struct core_component *comp)
+		      struct most_component *comp)
 {
 	struct most_channel *c;
 
@@ -1215,7 +1215,7 @@ EXPORT_SYMBOL_GPL(most_stop_channel);
  * most_register_component - registers a driver component with the core
  * @comp: driver component
  */
-int most_register_component(struct core_component *comp)
+int most_register_component(struct most_component *comp)
 {
 	if (!comp) {
 		pr_err("Bad component\n");
@@ -1231,7 +1231,7 @@ static int disconnect_channels(struct device *dev, void *data)
 {
 	struct most_interface *iface;
 	struct most_channel *c, *tmp;
-	struct core_component *comp = data;
+	struct most_component *comp = data;
 
 	iface = to_most_interface(dev);
 	list_for_each_entry_safe(c, tmp, &iface->p->channel_list, list) {
@@ -1249,7 +1249,7 @@ static int disconnect_channels(struct device *dev, void *data)
  * most_deregister_component - deregisters a driver component with the core
  * @comp: driver component
  */
-int most_deregister_component(struct core_component *comp)
+int most_deregister_component(struct most_component *comp)
 {
 	if (!comp) {
 		pr_err("Bad component\n");
diff --git a/drivers/staging/most/most.h b/drivers/staging/most/most.h
index 1380e75..fa6b29f 100644
--- a/drivers/staging/most/most.h
+++ b/drivers/staging/most/most.h
@@ -254,7 +254,7 @@ struct most_interface {
 #define to_most_interface(d) container_of(d, struct most_interface, dev)
 
 /**
- * struct core_component - identifies a loadable component for the mostcore
+ * struct most_component - identifies a loadable component for the mostcore
  * @list: list_head
  * @name: component name
  * @probe_channel: function for core to notify driver about channel connection
@@ -262,7 +262,7 @@ struct most_interface {
  * @rx_completion: completion handler for received packets
  * @tx_completion: completion handler for transmitted packets
  */
-struct core_component {
+struct most_component {
 	struct list_head list;
 	const char *name;
 	struct module *mod;
@@ -310,20 +310,20 @@ void most_stop_enqueue(struct most_interface *iface, int channel_idx);
  * in wait fifo.
  */
 void most_resume_enqueue(struct most_interface *iface, int channel_idx);
-int most_register_component(struct core_component *comp);
-int most_deregister_component(struct core_component *comp);
+int most_register_component(struct most_component *comp);
+int most_deregister_component(struct most_component *comp);
 struct mbo *most_get_mbo(struct most_interface *iface, int channel_idx,
-			 struct core_component *comp);
+			 struct most_component *comp);
 void most_put_mbo(struct mbo *mbo);
 int channel_has_mbo(struct most_interface *iface, int channel_idx,
-		    struct core_component *comp);
+		    struct most_component *comp);
 int most_start_channel(struct most_interface *iface, int channel_idx,
-		       struct core_component *comp);
+		       struct most_component *comp);
 int most_stop_channel(struct most_interface *iface, int channel_idx,
-		      struct core_component *comp);
+		      struct most_component *comp);
 int __init configfs_init(void);
-int most_register_configfs_subsys(struct core_component *comp);
-void most_deregister_configfs_subsys(struct core_component *comp);
+int most_register_configfs_subsys(struct most_component *comp);
+void most_deregister_configfs_subsys(struct most_component *comp);
 int most_add_link(char *mdev, char *mdev_ch, char *comp_name, char *link_name,
 		  char *comp_param);
 int most_remove_link(char *mdev, char *mdev_ch, char *comp_name);
diff --git a/drivers/staging/most/net/net.c b/drivers/staging/most/net/net.c
index 34d93c4..e3dd337 100644
--- a/drivers/staging/most/net/net.c
+++ b/drivers/staging/most/net/net.c
@@ -70,7 +70,7 @@ struct net_dev_context {
 static struct list_head net_devices = LIST_HEAD_INIT(net_devices);
 static struct mutex probe_disc_mt; /* ch->linked = true, most_nd_open */
 static DEFINE_SPINLOCK(list_lock); /* list_head, ch->linked = false, dev_hold */
-static struct core_component comp;
+static struct most_component comp;
 
 static int skb_to_mamac(const struct sk_buff *skb, struct mbo *mbo)
 {
@@ -497,7 +497,7 @@ static int comp_rx_data(struct mbo *mbo)
 	return ret;
 }
 
-static struct core_component comp = {
+static struct most_component comp = {
 	.mod = THIS_MODULE,
 	.name = "net",
 	.probe_channel = comp_probe_channel,
diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 38642b2..23baf4b 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -22,7 +22,7 @@
 #define DRIVER_NAME "sound"
 #define STRING_SIZE	80
 
-static struct core_component comp;
+static struct most_component comp;
 
 /**
  * struct channel - private structure to keep channel specific data
@@ -736,9 +736,9 @@ static int audio_tx_completion(struct most_interface *iface, int channel_id)
 }
 
 /**
- * Initialization of the struct core_component
+ * Initialization of the struct most_component
  */
-static struct core_component comp = {
+static struct most_component comp = {
 	.mod = THIS_MODULE,
 	.name = DRIVER_NAME,
 	.probe_channel = audio_probe_channel,
diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index b75ccc8..0f45b04 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -25,7 +25,7 @@
 
 #define V4L2_CMP_MAX_INPUT  1
 
-static struct core_component comp;
+static struct most_component comp;
 
 struct most_video_dev {
 	struct most_interface *iface;
@@ -527,7 +527,7 @@ static int comp_disconnect_channel(struct most_interface *iface,
 	return 0;
 }
 
-static struct core_component comp = {
+static struct most_component comp = {
 	.mod = THIS_MODULE,
 	.name = "video",
 	.probe_channel = comp_probe_channel,
-- 
2.7.4

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

[PATCH RFC v2 3/9] staging: most: rename enum mbo_status_flags

[Christian Gromm]

The data structures of the most.h header file will be exposed
to the kernel once the file is moved out of the staging area.
This is why the name is prefixed with the string 'most'.

Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:
	This patch has been added to the series.

 drivers/staging/most/most.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/most/most.h b/drivers/staging/most/most.h
index fa6b29f..d93c6ce 100644
--- a/drivers/staging/most/most.h
+++ b/drivers/staging/most/most.h
@@ -47,7 +47,7 @@ enum most_channel_data_type {
 	MOST_CH_SYNC = 1 << 5,
 };
 
-enum mbo_status_flags {
+enum most_status_flags {
 	/* MBO was processed successfully (data was send or received )*/
 	MBO_SUCCESS = 0,
 	/* The MBO contains wrong or missing information.  */
@@ -184,7 +184,7 @@ struct mbo {
 	dma_addr_t bus_address;
 	u16 buffer_length;
 	u16 processed_length;
-	enum mbo_status_flags status;
+	enum most_status_flags status;
 	void (*complete)(struct mbo *mbo);
 };
 
-- 
2.7.4

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

[PATCH RFC v2 4/9] staging: most: configfs: use strlcpy

[Christian Gromm]

This patch uses strlcpy to copy data provided by userspace in order
to not overflow the allocated space.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:
	This patch has beed added to the series.

 drivers/staging/most/configfs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
index d671022..9fbcadf 100644
--- a/drivers/staging/most/configfs.c
+++ b/drivers/staging/most/configfs.c
@@ -197,7 +197,7 @@ static ssize_t mdev_link_device_store(struct config_item *item,
 {
 	struct mdev_link *mdev_link = to_mdev_link(item);
 
-	strcpy(mdev_link->device, page);
+	strlcpy(mdev_link->device, page, sizeof(mdev_link->device));
 	strim(mdev_link->device);
 	return count;
 }
@@ -212,7 +212,7 @@ static ssize_t mdev_link_channel_store(struct config_item *item,
 {
 	struct mdev_link *mdev_link = to_mdev_link(item);
 
-	strcpy(mdev_link->channel, page);
+	strlcpy(mdev_link->channel, page, sizeof(mdev_link->channel));
 	strim(mdev_link->channel);
 	return count;
 }
@@ -227,7 +227,8 @@ static ssize_t mdev_link_comp_store(struct config_item *item,
 {
 	struct mdev_link *mdev_link = to_mdev_link(item);
 
-	strcpy(mdev_link->comp, page);
+	strlcpy(mdev_link->comp, page, sizeof(mdev_link->comp));
+	strim(mdev_link->comp);
 	return count;
 }
 
@@ -242,7 +243,8 @@ static ssize_t mdev_link_comp_params_store(struct config_item *item,
 {
 	struct mdev_link *mdev_link = to_mdev_link(item);
 
-	strcpy(mdev_link->comp_params, page);
+	strlcpy(mdev_link->comp_params, page, sizeof(mdev_link->comp_params));
+	strim(mdev_link->comp_params);
 	return count;
 }
 
-- 
2.7.4

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

[PATCH RFC v2 5/9] staging: most: configfs: reduce array size

[Christian Gromm]

This patch reduces the size of the arrays inside the mdev_link struct to
a reasonable value.

Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:
	This patch has been added to the series.

 drivers/staging/most/configfs.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
index 9fbcadf..9818f6c 100644
--- a/drivers/staging/most/configfs.c
+++ b/drivers/staging/most/configfs.c
@@ -12,6 +12,8 @@
 #include <linux/configfs.h>
 #include <most/most.h>
 
+#define MAX_STRING_SIZE 80
+
 struct mdev_link {
 	struct config_item item;
 	struct list_head list;
@@ -22,13 +24,13 @@ struct mdev_link {
 	u16 subbuffer_size;
 	u16 packets_per_xact;
 	u16 dbr_size;
-	char datatype[PAGE_SIZE];
-	char direction[PAGE_SIZE];
-	char name[PAGE_SIZE];
-	char device[PAGE_SIZE];
-	char channel[PAGE_SIZE];
-	char comp[PAGE_SIZE];
-	char comp_params[PAGE_SIZE];
+	char datatype[MAX_STRING_SIZE];
+	char direction[MAX_STRING_SIZE];
+	char name[MAX_STRING_SIZE];
+	char device[MAX_STRING_SIZE];
+	char channel[MAX_STRING_SIZE];
+	char comp[MAX_STRING_SIZE];
+	char comp_params[MAX_STRING_SIZE];
 };
 
 static struct list_head mdev_link_list;
-- 
2.7.4

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

[PATCH RFC v2 6/9] staging: most: use angle brackets in include path

[Christian Gromm]

This patch replaces the double quotes in all include paths
with angle brackets.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:

 drivers/staging/most/cdev/cdev.c   | 2 +-
 drivers/staging/most/dim2/dim2.c   | 2 +-
 drivers/staging/most/i2c/i2c.c     | 2 +-
 drivers/staging/most/net/net.c     | 2 +-
 drivers/staging/most/usb/usb.c     | 2 +-
 drivers/staging/most/video/video.c | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/most/cdev/cdev.c b/drivers/staging/most/cdev/cdev.c
index 9ce2f23..59f346d 100644
--- a/drivers/staging/most/cdev/cdev.c
+++ b/drivers/staging/most/cdev/cdev.c
@@ -16,7 +16,7 @@
 #include <linux/kfifo.h>
 #include <linux/uaccess.h>
 #include <linux/idr.h>
-#include "most/most.h"
+#include <most/most.h>
 
 #define CHRDEV_REGION_SIZE 50
 
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index e15e847..9eb10fc 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -21,7 +21,7 @@
 #include <linux/sched.h>
 #include <linux/kthread.h>
 
-#include "most/most.h"
+#include <most/most.h>
 #include "hal.h"
 #include "errors.h"
 #include "sysfs.h"
diff --git a/drivers/staging/most/i2c/i2c.c b/drivers/staging/most/i2c/i2c.c
index d4606ac..d07719c 100644
--- a/drivers/staging/most/i2c/i2c.c
+++ b/drivers/staging/most/i2c/i2c.c
@@ -14,7 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/err.h>
 
-#include "most/most.h"
+#include <most/most.h>
 
 enum { CH_RX, CH_TX, NUM_CHANNELS };
 
diff --git a/drivers/staging/most/net/net.c b/drivers/staging/most/net/net.c
index e3dd337..db42732 100644
--- a/drivers/staging/most/net/net.c
+++ b/drivers/staging/most/net/net.c
@@ -15,7 +15,7 @@
 #include <linux/list.h>
 #include <linux/wait.h>
 #include <linux/kobject.h>
-#include "most/most.h"
+#include <most/most.h>
 
 #define MEP_HDR_LEN 8
 #define MDP_HDR_LEN 16
diff --git a/drivers/staging/most/usb/usb.c b/drivers/staging/most/usb/usb.c
index 69756ca..491b38e 100644
--- a/drivers/staging/most/usb/usb.c
+++ b/drivers/staging/most/usb/usb.c
@@ -23,7 +23,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/etherdevice.h>
 #include <linux/uaccess.h>
-#include "most/most.h"
+#include <most/most.h>
 
 #define USB_MTU			512
 #define NO_ISOCHRONOUS_URB	0
diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 0f45b04..9e9e45a 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -21,7 +21,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
 
-#include "most/most.h"
+#include <most/most.h>
 
 #define V4L2_CMP_MAX_INPUT  1
 
-- 
2.7.4

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

[PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Christian Gromm]

This patch moves the core module to the /drivers/most directory
and makes all necessary changes in order to not break the build.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:

 drivers/Kconfig                                |  1 +
 drivers/Makefile                               |  1 +
 drivers/most/Kconfig                           | 15 +++++++++++++++
 drivers/most/Makefile                          |  4 ++++
 drivers/{staging => }/most/configfs.c          |  2 +-
 drivers/{staging => }/most/core.c              |  2 +-
 drivers/staging/most/Kconfig                   |  6 +++---
 drivers/staging/most/Makefile                  |  5 -----
 drivers/staging/most/cdev/cdev.c               |  2 +-
 drivers/staging/most/dim2/dim2.c               |  2 +-
 drivers/staging/most/i2c/i2c.c                 |  2 +-
 drivers/staging/most/net/net.c                 |  2 +-
 drivers/staging/most/sound/sound.c             |  2 +-
 drivers/staging/most/usb/usb.c                 |  2 +-
 drivers/staging/most/video/video.c             |  2 +-
 {drivers/staging/most => include/linux}/most.h |  0
 16 files changed, 33 insertions(+), 17 deletions(-)
 create mode 100644 drivers/most/Kconfig
 create mode 100644 drivers/most/Makefile
 rename drivers/{staging => }/most/configfs.c (99%)
 rename drivers/{staging => }/most/core.c (99%)
 rename {drivers/staging/most => include/linux}/most.h (100%)

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 8befa53..c739665 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -228,4 +228,5 @@ source "drivers/interconnect/Kconfig"
 
 source "drivers/counter/Kconfig"
 
+source "drivers/most/Kconfig"
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index aaef17c..0e1dcbc 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -186,3 +186,4 @@ obj-$(CONFIG_SIOX)		+= siox/
 obj-$(CONFIG_GNSS)		+= gnss/
 obj-$(CONFIG_INTERCONNECT)	+= interconnect/
 obj-$(CONFIG_COUNTER)		+= counter/
+obj-$(CONFIG_MOST)		+= most/
diff --git a/drivers/most/Kconfig b/drivers/most/Kconfig
new file mode 100644
index 0000000..58d7999
--- /dev/null
+++ b/drivers/most/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0
+menuconfig MOST
+	tristate "MOST support"
+	depends on HAS_DMA && CONFIGFS_FS
+	default n
+	help
+	  Say Y here if you want to enable MOST support.
+	  This driver needs at least one additional component to enable the
+	  desired access from userspace (e.g. character devices) and one that
+	  matches the network controller's hardware interface (e.g. USB).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called most_core.
+
+	  If in doubt, say N here.
diff --git a/drivers/most/Makefile b/drivers/most/Makefile
new file mode 100644
index 0000000..e810cd3
--- /dev/null
+++ b/drivers/most/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MOST) += most_core.o
+most_core-y :=	core.o \
+		configfs.o
diff --git a/drivers/staging/most/configfs.c b/drivers/most/configfs.c
similarity index 99%
rename from drivers/staging/most/configfs.c
rename to drivers/most/configfs.c
index 9818f6c..77b0855 100644
--- a/drivers/staging/most/configfs.c
+++ b/drivers/most/configfs.c
@@ -10,7 +10,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/configfs.h>
-#include <most/most.h>
+#include <linux/most.h>
 
 #define MAX_STRING_SIZE 80
 
diff --git a/drivers/staging/most/core.c b/drivers/most/core.c
similarity index 99%
rename from drivers/staging/most/core.c
rename to drivers/most/core.c
index af542ed..8c05e0d 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/most/core.c
@@ -21,7 +21,7 @@
 #include <linux/kthread.h>
 #include <linux/dma-mapping.h>
 #include <linux/idr.h>
-#include <most/most.h>
+#include <linux/most.h>
 
 #define MAX_CHANNELS	64
 #define STRING_SIZE	80
diff --git a/drivers/staging/most/Kconfig b/drivers/staging/most/Kconfig
index 6262eb2..c5a99f7 100644
--- a/drivers/staging/most/Kconfig
+++ b/drivers/staging/most/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
-menuconfig MOST
+menuconfig MOST_COMPONENTS
 	tristate "MOST support"
-	depends on HAS_DMA && CONFIGFS_FS
+	depends on HAS_DMA && CONFIGFS_FS && MOST
 	default n
 	help
 	  Say Y here if you want to enable MOST support.
@@ -16,7 +16,7 @@ menuconfig MOST
 
 
 
-if MOST
+if MOST_COMPONENTS
 
 source "drivers/staging/most/cdev/Kconfig"
 
diff --git a/drivers/staging/most/Makefile b/drivers/staging/most/Makefile
index 85ea5a4..3c446bb 100644
--- a/drivers/staging/most/Makefile
+++ b/drivers/staging/most/Makefile
@@ -1,9 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_MOST) += most_core.o
-most_core-y := core.o
-most_core-y += configfs.o
-ccflags-y += -I $(srctree)/drivers/staging/
-
 obj-$(CONFIG_MOST_CDEV)	+= cdev/
 obj-$(CONFIG_MOST_NET)	+= net/
 obj-$(CONFIG_MOST_SOUND)	+= sound/
diff --git a/drivers/staging/most/cdev/cdev.c b/drivers/staging/most/cdev/cdev.c
index 59f346d..cc1e3de 100644
--- a/drivers/staging/most/cdev/cdev.c
+++ b/drivers/staging/most/cdev/cdev.c
@@ -16,7 +16,7 @@
 #include <linux/kfifo.h>
 #include <linux/uaccess.h>
 #include <linux/idr.h>
-#include <most/most.h>
+#include <linux/most.h>
 
 #define CHRDEV_REGION_SIZE 50
 
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 9eb10fc..10ff0cc 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -21,7 +21,7 @@
 #include <linux/sched.h>
 #include <linux/kthread.h>
 
-#include <most/most.h>
+#include <linux/most.h>
 #include "hal.h"
 #include "errors.h"
 #include "sysfs.h"
diff --git a/drivers/staging/most/i2c/i2c.c b/drivers/staging/most/i2c/i2c.c
index d07719c..d4a9cd8 100644
--- a/drivers/staging/most/i2c/i2c.c
+++ b/drivers/staging/most/i2c/i2c.c
@@ -14,7 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/err.h>
 
-#include <most/most.h>
+#include <linux/most.h>
 
 enum { CH_RX, CH_TX, NUM_CHANNELS };
 
diff --git a/drivers/staging/most/net/net.c b/drivers/staging/most/net/net.c
index db42732..d6d5b2a 100644
--- a/drivers/staging/most/net/net.c
+++ b/drivers/staging/most/net/net.c
@@ -15,7 +15,7 @@
 #include <linux/list.h>
 #include <linux/wait.h>
 #include <linux/kobject.h>
-#include <most/most.h>
+#include <linux/most.h>
 
 #define MEP_HDR_LEN 8
 #define MDP_HDR_LEN 16
diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 23baf4b..1527f41 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -17,7 +17,7 @@
 #include <sound/pcm_params.h>
 #include <linux/sched.h>
 #include <linux/kthread.h>
-#include <most/most.h>
+#include <linux/most.h>
 
 #define DRIVER_NAME "sound"
 #define STRING_SIZE	80
diff --git a/drivers/staging/most/usb/usb.c b/drivers/staging/most/usb/usb.c
index 491b38e..4c11fad 100644
--- a/drivers/staging/most/usb/usb.c
+++ b/drivers/staging/most/usb/usb.c
@@ -23,7 +23,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/etherdevice.h>
 #include <linux/uaccess.h>
-#include <most/most.h>
+#include <linux/most.h>
 
 #define USB_MTU			512
 #define NO_ISOCHRONOUS_URB	0
diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 9e9e45a..ce3a44a 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -21,7 +21,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
 
-#include <most/most.h>
+#include <linux/most.h>
 
 #define V4L2_CMP_MAX_INPUT  1
 
diff --git a/drivers/staging/most/most.h b/include/linux/most.h
similarity index 100%
rename from drivers/staging/most/most.h
rename to include/linux/most.h
-- 
2.7.4

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

[PATCH RFC v2 8/9] staging: most: Documentation: update ABI description

[Christian Gromm]

This patch updates the ABI description files to be in sync
with current implementation.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:

 .../most/Documentation/ABI/configfs-most.txt       |  8 --------
 .../most/Documentation/ABI/sysfs-bus-most.txt      | 24 +++-------------------
 2 files changed, 3 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/most/Documentation/ABI/configfs-most.txt b/drivers/staging/most/Documentation/ABI/configfs-most.txt
index 2bf8114..ed67a4d 100644
--- a/drivers/staging/most/Documentation/ABI/configfs-most.txt
+++ b/drivers/staging/most/Documentation/ABI/configfs-most.txt
@@ -194,11 +194,3 @@ Description:
 
 		destroy_link	write '1' to this attribute to destroy an
 				active link
-
-What: 		/sys/kernel/config/rdma_cm/<hca>/ports/<port-num>/default_roce_tos
-Date: 		March 8, 2019
-KernelVersion:  5.2
-Description: 	RDMA-CM QPs from HCA <hca> at port <port-num>
-		will be created with this TOS as default.
-		This can be overridden by using the rdma_set_option API.
-		The possible RoCE TOS values are 0-255.
diff --git a/drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt b/drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt
index d8fa841..6b1d06e 100644
--- a/drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt
+++ b/drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt
@@ -5,7 +5,7 @@ Contact:	Christian Gromm <christian.gromm@microchip.com>
 Description:
 		Provides information about the interface type and the physical
 		location of the device. Hardware attached via USB, for instance,
-		might return <usb_device 1-1.1:1.0>
+		might return <1-1.1:1.0>
 Users:
 
 What:		/sys/bus/most/devices/.../interface
@@ -278,25 +278,7 @@ Description:
 		Indicates whether current channel ran out of buffers.
 Users:
 
-What:		/sys/bus/most/drivers/mostcore/add_link
-Date:		March 2017
-KernelVersion:	4.15
-Contact:	Christian Gromm <christian.gromm@microchip.com>
-Description:
-		This is used to link a channel to a component of the
-		mostcore. A link created by writing to this file is
-		referred to as pipe.
-Users:
-
-What:		/sys/bus/most/drivers/mostcore/remove_link
-Date:		March 2017
-KernelVersion:	4.15
-Contact:	Christian Gromm <christian.gromm@microchip.com>
-Description:
-		This is used to unlink a channel from a component.
-Users:
-
-What:		/sys/bus/most/drivers/mostcore/components
+What:		/sys/bus/most/drivers/most_core/components
 Date:		March 2017
 KernelVersion:	4.15
 Contact:	Christian Gromm <christian.gromm@microchip.com>
@@ -304,7 +286,7 @@ Description:
 		This is used to retrieve a list of registered components.
 Users:
 
-What:		/sys/bus/most/drivers/mostcore/links
+What:		/sys/bus/most/drivers/most_core/links
 Date:		March 2017
 KernelVersion:	4.15
 Contact:	Christian Gromm <christian.gromm@microchip.com>
-- 
2.7.4

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

[PATCH RFC v2 9/9] staging: most: Documentation: move ABI description files out of staging area

[Christian Gromm]

This patch moves the ABI description fils sysfs-bus-most and
configfs-most to the kernel's documentation folder.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
v2:

 .../ABI/configfs-most.txt => Documentation/ABI/testing/configfs-most      | 0
 .../ABI/sysfs-bus-most.txt => Documentation/ABI/testing/sysfs-bus-most    | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename drivers/staging/most/Documentation/ABI/configfs-most.txt => Documentation/ABI/testing/configfs-most (100%)
 rename drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt => Documentation/ABI/testing/sysfs-bus-most (100%)

diff --git a/drivers/staging/most/Documentation/ABI/configfs-most.txt b/Documentation/ABI/testing/configfs-most
similarity index 100%
rename from drivers/staging/most/Documentation/ABI/configfs-most.txt
rename to Documentation/ABI/testing/configfs-most
diff --git a/drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt b/Documentation/ABI/testing/sysfs-bus-most
similarity index 100%
rename from drivers/staging/most/Documentation/ABI/sysfs-bus-most.txt
rename to Documentation/ABI/testing/sysfs-bus-most
-- 
2.7.4

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

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Greg KH]

On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> This patch moves the core module to the /drivers/most directory
> and makes all necessary changes in order to not break the build.
> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>

I've applied the patches up to this one in the series, but I still have
questions about the file you are trying to move here.

It's not in this patch, but I'll just quote from the file
drivers/staging/most/core.c directly:

 * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co. KG

You've touched this file since 2015 :)

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

No need for this, You have drivers here, no need to use any pr_* calls,
as you always have a device structure.
Along with that, almost all of your pr_info() calls are really
errors/warnigns, so use dev_err() or dev_warn() instead please.

The one:
pr_info("registered new core component %s\n", comp->name);

Should at best be a dev_info() line, but really, you don't need to be
loud if all goes well, right?

pr_info("deregistering component %s\n", comp->name);

Should be dev_dbg().

static void release_interface(struct device *dev)
{
	pr_info("releasing interface dev %s...\n", dev_name(dev));
}

static void release_channel(struct device *dev)
{
	pr_info("releasing channel dev %s...\n", dev_name(dev));
}

How did I miss this before?

The driver core documentation used to have a line saying I was allowed
to make fun of programmers who did this, but that had to be removed :(

Anyway, this is totally wrong, first off, delete the debugging lines.
Secondly how are you really releasing anything?  You have to free the
memory here.  You can not have an "empty" release function, the driver
core requires you to actually do something here.

Same for release_most_sub() and anywhere else I missed in my review.

That's a good start, fix that up and I'll be glad to look at the code
again.

thanks,

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

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Christian.Gromm]

On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > This patch moves the core module to the /drivers/most directory
> > and makes all necessary changes in order to not break the build.
> > 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> 
> I've applied the patches up to this one in the series, but I still
> have
> questions about the file you are trying to move here.
> 
> It's not in this patch, but I'll just quote from the file
> drivers/staging/most/core.c directly:
> 
>  * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co.
> KG
> 
> You've touched this file since 2015 :)
> 
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> No need for this, You have drivers here, no need to use any pr_*
> calls,
> as you always have a device structure.
> Along with that, almost all of your pr_info() calls are really
> errors/warnigns, so use dev_err() or dev_warn() instead please.
> 
> The one:
> pr_info("registered new core component %s\n", comp->name);
> 
> Should at best be a dev_info() line, but really, you don't need to be
> loud if all goes well, right?
> 
> pr_info("deregistering component %s\n", comp->name);
> 
> Should be dev_dbg().
> 
> static void release_interface(struct device *dev)
> {
>         pr_info("releasing interface dev %s...\n", dev_name(dev));
> }
> 
> static void release_channel(struct device *dev)
> {
>         pr_info("releasing channel dev %s...\n", dev_name(dev));
> }
> 
> How did I miss this before?
> 
> The driver core documentation used to have a line saying I was
> allowed
> to make fun of programmers who did this, but that had to be removed
> :(
> 
> Anyway, this is totally wrong, first off, delete the debugging lines.
> Secondly how are you really releasing anything?

Allocated memory is being freed inside the deregister* functions,
once a device is detached from the system or the physical adapter
driver has been removed. There a loop frees all channels and interfaces
and the devices are unregistered with the kernel.

I can move this to the release functions.

> You have to free the
> memory here.  You can not have an "empty" release function, the
> driver
> core requires you to actually do something here.
> 
> Same for release_most_sub() and anywhere else I missed in my review.

Here no memory has been allocated dynamically. What am I supposed to
free up?

I've seen code in the kernel that does host empty release functions.
That's probably why I didn't recognized this as a red flag or sensed
any bad feelings.

thanks,
Chris


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

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Greg KH]

On Wed, Dec 18, 2019 at 02:02:43PM +0000, Christian.Gromm@microchip.com wrote:
> On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > > This patch moves the core module to the /drivers/most directory
> > > and makes all necessary changes in order to not break the build.
> > > 
> > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > 
> > I've applied the patches up to this one in the series, but I still
> > have
> > questions about the file you are trying to move here.
> > 
> > It's not in this patch, but I'll just quote from the file
> > drivers/staging/most/core.c directly:
> > 
> >  * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co.
> > KG
> > 
> > You've touched this file since 2015 :)
> > 
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > 
> > No need for this, You have drivers here, no need to use any pr_*
> > calls,
> > as you always have a device structure.
> > Along with that, almost all of your pr_info() calls are really
> > errors/warnigns, so use dev_err() or dev_warn() instead please.
> > 
> > The one:
> > pr_info("registered new core component %s\n", comp->name);
> > 
> > Should at best be a dev_info() line, but really, you don't need to be
> > loud if all goes well, right?
> > 
> > pr_info("deregistering component %s\n", comp->name);
> > 
> > Should be dev_dbg().
> > 
> > static void release_interface(struct device *dev)
> > {
> >         pr_info("releasing interface dev %s...\n", dev_name(dev));
> > }
> > 
> > static void release_channel(struct device *dev)
> > {
> >         pr_info("releasing channel dev %s...\n", dev_name(dev));
> > }
> > 
> > How did I miss this before?
> > 
> > The driver core documentation used to have a line saying I was
> > allowed
> > to make fun of programmers who did this, but that had to be removed
> > :(
> > 
> > Anyway, this is totally wrong, first off, delete the debugging lines.
> > Secondly how are you really releasing anything?
> 
> Allocated memory is being freed inside the deregister* functions,
> once a device is detached from the system or the physical adapter
> driver has been removed. There a loop frees all channels and interfaces
> and the devices are unregistered with the kernel.
> 
> I can move this to the release functions.

It has to go there, as you have no idea if someone else has a reference
to those structures.  You have to abide by the fact that they are
dynamic reference-counted structures, and that means you never "know"
what the reference count is :)

> > You have to free the
> > memory here.  You can not have an "empty" release function, the
> > driver
> > core requires you to actually do something here.
> > 
> > Same for release_most_sub() and anywhere else I missed in my review.
> 
> Here no memory has been allocated dynamically. What am I supposed to
> free up?

You have a structure that is reference counted, it had to be allocated
dynamically, otherwise why is there a release function?

> I've seen code in the kernel that does host empty release functions.

Where?  I'll go yell at them :)

thanks,

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

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Christian.Gromm]

On Wed, 2019-12-18 at 15:08 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Dec 18, 2019 at 02:02:43PM +0000, 
> Christian.Gromm@microchip.com wrote:
> > On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > > > This patch moves the core module to the /drivers/most directory
> > > > and makes all necessary changes in order to not break the
> > > > build.
> > > > 
> > > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > 
> > > I've applied the patches up to this one in the series, but I
> > > still
> > > have
> > > questions about the file you are trying to move here.
> > > 
> > > It's not in this patch, but I'll just quote from the file
> > > drivers/staging/most/core.c directly:
> > > 
> > >  * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH &
> > > Co.
> > > KG
> > > 
> > > You've touched this file since 2015 :)
> > > 
> > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > 
> > > No need for this, You have drivers here, no need to use any pr_*
> > > calls,
> > > as you always have a device structure.
> > > Along with that, almost all of your pr_info() calls are really
> > > errors/warnigns, so use dev_err() or dev_warn() instead please.
> > > 
> > > The one:
> > > pr_info("registered new core component %s\n", comp->name);
> > > 
> > > Should at best be a dev_info() line, but really, you don't need
> > > to be
> > > loud if all goes well, right?
> > > 
> > > pr_info("deregistering component %s\n", comp->name);
> > > 
> > > Should be dev_dbg().
> > > 
> > > static void release_interface(struct device *dev)
> > > {
> > >         pr_info("releasing interface dev %s...\n",
> > > dev_name(dev));
> > > }
> > > 
> > > static void release_channel(struct device *dev)
> > > {
> > >         pr_info("releasing channel dev %s...\n", dev_name(dev));
> > > }
> > > 
> > > How did I miss this before?
> > > 
> > > The driver core documentation used to have a line saying I was
> > > allowed
> > > to make fun of programmers who did this, but that had to be
> > > removed
> > > :(
> > > 
> > > Anyway, this is totally wrong, first off, delete the debugging
> > > lines.
> > > Secondly how are you really releasing anything?
> > 
> > Allocated memory is being freed inside the deregister* functions,
> > once a device is detached from the system or the physical adapter
> > driver has been removed. There a loop frees all channels and
> > interfaces
> > and the devices are unregistered with the kernel.
> > 
> > I can move this to the release functions.
> 
> It has to go there, as you have no idea if someone else has a
> reference
> to those structures.  You have to abide by the fact that they are
> dynamic reference-counted structures, and that means you never "know"
> what the reference count is :)
> 
> > > You have to free the
> > > memory here.  You can not have an "empty" release function, the
> > > driver
> > > core requires you to actually do something here.
> > > 
> > > Same for release_most_sub() and anywhere else I missed in my
> > > review.
> > 
> > Here no memory has been allocated dynamically. What am I supposed
> > to
> > free up?
> 
> You have a structure that is reference counted, it had to be
> allocated
> dynamically, otherwise why is there a release function?

Actually, no! The release function is there, because I have
a struct device embedded. And the kernel prints this
"scary complaint", when I try to register it with no release
function assigned. :)

> 
> > I've seen code in the kernel that does host empty release
> > functions.
> 
> Where?  I'll go yell at them :)

drivers/usb/gadget/udc/core.c
drivers/media/platform/vicodec/vicodec-core.c

maybe more...

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Greg KH]

On Wed, Dec 18, 2019 at 02:50:32PM +0000, Christian.Gromm@microchip.com wrote:
> On Wed, 2019-12-18 at 15:08 +0100, Greg KH wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Wed, Dec 18, 2019 at 02:02:43PM +0000, 
> > Christian.Gromm@microchip.com wrote:
> > > On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > know the content is safe
> > > > 
> > > > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > > > > This patch moves the core module to the /drivers/most directory
> > > > > and makes all necessary changes in order to not break the
> > > > > build.
> > > > > 
> > > > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > > 
> > > > I've applied the patches up to this one in the series, but I
> > > > still
> > > > have
> > > > questions about the file you are trying to move here.
> > > > 
> > > > It's not in this patch, but I'll just quote from the file
> > > > drivers/staging/most/core.c directly:
> > > > 
> > > >  * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH &
> > > > Co.
> > > > KG
> > > > 
> > > > You've touched this file since 2015 :)
> > > > 
> > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > 
> > > > No need for this, You have drivers here, no need to use any pr_*
> > > > calls,
> > > > as you always have a device structure.
> > > > Along with that, almost all of your pr_info() calls are really
> > > > errors/warnigns, so use dev_err() or dev_warn() instead please.
> > > > 
> > > > The one:
> > > > pr_info("registered new core component %s\n", comp->name);
> > > > 
> > > > Should at best be a dev_info() line, but really, you don't need
> > > > to be
> > > > loud if all goes well, right?
> > > > 
> > > > pr_info("deregistering component %s\n", comp->name);
> > > > 
> > > > Should be dev_dbg().
> > > > 
> > > > static void release_interface(struct device *dev)
> > > > {
> > > >         pr_info("releasing interface dev %s...\n",
> > > > dev_name(dev));
> > > > }
> > > > 
> > > > static void release_channel(struct device *dev)
> > > > {
> > > >         pr_info("releasing channel dev %s...\n", dev_name(dev));
> > > > }
> > > > 
> > > > How did I miss this before?
> > > > 
> > > > The driver core documentation used to have a line saying I was
> > > > allowed
> > > > to make fun of programmers who did this, but that had to be
> > > > removed
> > > > :(
> > > > 
> > > > Anyway, this is totally wrong, first off, delete the debugging
> > > > lines.
> > > > Secondly how are you really releasing anything?
> > > 
> > > Allocated memory is being freed inside the deregister* functions,
> > > once a device is detached from the system or the physical adapter
> > > driver has been removed. There a loop frees all channels and
> > > interfaces
> > > and the devices are unregistered with the kernel.
> > > 
> > > I can move this to the release functions.
> > 
> > It has to go there, as you have no idea if someone else has a
> > reference
> > to those structures.  You have to abide by the fact that they are
> > dynamic reference-counted structures, and that means you never "know"
> > what the reference count is :)
> > 
> > > > You have to free the
> > > > memory here.  You can not have an "empty" release function, the
> > > > driver
> > > > core requires you to actually do something here.
> > > > 
> > > > Same for release_most_sub() and anywhere else I missed in my
> > > > review.
> > > 
> > > Here no memory has been allocated dynamically. What am I supposed
> > > to
> > > free up?
> > 
> > You have a structure that is reference counted, it had to be
> > allocated
> > dynamically, otherwise why is there a release function?
> 
> Actually, no! The release function is there, because I have
> a struct device embedded. And the kernel prints this
> "scary complaint", when I try to register it with no release
> function assigned. :)

Stop and think _why_ someone (i.e. me) took the time and energy to write
code to have the kernel print out that scary complaint.  It wasn't just
because I had nothing better to do...

I wrote that code in order to tell people "hey, your code is buggy, fix
it properly!"  I didn't do that to tell people, "hey, provide an empty
release function to quiet this foolish warning that I should never have
added!"

When the kernel complains about something, don't try to work around it.
It is complaining for a good reason.

You owe me 5 recitations of Documentation/driver-api/driver-model/* and
Documentation/kobject.txt :)

thanks,

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

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Christian.Gromm]

On Wed, 2019-12-18 at 16:02 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Dec 18, 2019 at 02:50:32PM +0000, 
> Christian.Gromm@microchip.com wrote:
> > On Wed, 2019-12-18 at 15:08 +0100, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > On Wed, Dec 18, 2019 at 02:02:43PM +0000,
> > > Christian.Gromm@microchip.com wrote:
> > > > On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless
> > > > > you
> > > > > know the content is safe
> > > > > 
> > > > > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm
> > > > > wrote:
> > > > > > This patch moves the core module to the /drivers/most
> > > > > > directory
> > > > > > and makes all necessary changes in order to not break the
> > > > > > build.
> > > > > > 
> > > > > > Signed-off-by: Christian Gromm <
> > > > > > christian.gromm@microchip.com>
> > > > > 
> > > > > I've applied the patches up to this one in the series, but I
> > > > > still
> > > > > have
> > > > > questions about the file you are trying to move here.
> > > > > 
> > > > > It's not in this patch, but I'll just quote from the file
> > > > > drivers/staging/most/core.c directly:
> > > > > 
> > > > >  * Copyright (C) 2013-2015 Microchip Technology Germany II
> > > > > GmbH &
> > > > > Co.
> > > > > KG
> > > > > 
> > > > > You've touched this file since 2015 :)
> > > > > 
> > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > > 
> > > > > No need for this, You have drivers here, no need to use any
> > > > > pr_*
> > > > > calls,
> > > > > as you always have a device structure.
> > > > > Along with that, almost all of your pr_info() calls are
> > > > > really
> > > > > errors/warnigns, so use dev_err() or dev_warn() instead
> > > > > please.
> > > > > 
> > > > > The one:
> > > > > pr_info("registered new core component %s\n", comp->name);
> > > > > 
> > > > > Should at best be a dev_info() line, but really, you don't
> > > > > need
> > > > > to be
> > > > > loud if all goes well, right?
> > > > > 
> > > > > pr_info("deregistering component %s\n", comp->name);
> > > > > 
> > > > > Should be dev_dbg().
> > > > > 
> > > > > static void release_interface(struct device *dev)
> > > > > {
> > > > >         pr_info("releasing interface dev %s...\n",
> > > > > dev_name(dev));
> > > > > }
> > > > > 
> > > > > static void release_channel(struct device *dev)
> > > > > {
> > > > >         pr_info("releasing channel dev %s...\n",
> > > > > dev_name(dev));
> > > > > }
> > > > > 
> > > > > How did I miss this before?
> > > > > 
> > > > > The driver core documentation used to have a line saying I
> > > > > was
> > > > > allowed
> > > > > to make fun of programmers who did this, but that had to be
> > > > > removed
> > > > > :(
> > > > > 
> > > > > Anyway, this is totally wrong, first off, delete the
> > > > > debugging
> > > > > lines.
> > > > > Secondly how are you really releasing anything?
> > > > 
> > > > Allocated memory is being freed inside the deregister*
> > > > functions,
> > > > once a device is detached from the system or the physical
> > > > adapter
> > > > driver has been removed. There a loop frees all channels and
> > > > interfaces
> > > > and the devices are unregistered with the kernel.
> > > > 
> > > > I can move this to the release functions.
> > > 
> > > It has to go there, as you have no idea if someone else has a
> > > reference
> > > to those structures.  You have to abide by the fact that they are
> > > dynamic reference-counted structures, and that means you never
> > > "know"
> > > what the reference count is :)
> > > 
> > > > > You have to free the
> > > > > memory here.  You can not have an "empty" release function,
> > > > > the
> > > > > driver
> > > > > core requires you to actually do something here.
> > > > > 
> > > > > Same for release_most_sub() and anywhere else I missed in my
> > > > > review.
> > > > 
> > > > Here no memory has been allocated dynamically. What am I
> > > > supposed
> > > > to
> > > > free up?
> > > 
> > > You have a structure that is reference counted, it had to be
> > > allocated
> > > dynamically, otherwise why is there a release function?
> > 
> > Actually, no! The release function is there, because I have
> > a struct device embedded. And the kernel prints this
> > "scary complaint", when I try to register it with no release
> > function assigned. :)
> 
> Stop and think _why_ someone (i.e. me) took the time and energy to
> write
> code to have the kernel print out that scary complaint.  It wasn't
> just
> because I had nothing better to do...
> 
> I wrote that code in order to tell people "hey, your code is buggy,
> fix
> it properly!"  I didn't do that to tell people, "hey, provide an
> empty
> release function to quiet this foolish warning that I should never
> have
> added!"
> 
> When the kernel complains about something, don't try to work around
> it.
> It is complaining for a good reason.
> 

Got your point. I'll rethink this...

> You owe me 5 recitations of Documentation/driver-api/driver-model/*
> and
> Documentation/kobject.txt :)

Sorry! Can I buy you a beer next ELCE?

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

Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Greg KH]

On Wed, Dec 18, 2019 at 04:12:57PM +0000, Christian.Gromm@microchip.com wrote:
> > You owe me 5 recitations of Documentation/driver-api/driver-model/*
> > and
> > Documentation/kobject.txt :)
> 
> Sorry! Can I buy you a beer next ELCE?

Always!
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel