Re: [PATCH RFC v3 4/9] staging: most: move interface dev to private section

2020-01-15 Thread Christian.Gromm
On Wed, 2020-01-15 at 13:17 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Jan 14, 2020 at 04:57:53PM +0100, Christian Gromm wrote:
> > This patch moves the struct device of the interface structure to
> > its
> > private section, because only the core should access it directly.
> > For
> > other entities an API is provided.
> 
> This feels "wrong".
> 
> Shouldn't the struct device in this structure be the thing that is
> handling the reference counting and sysfs integration for this
> structure?

Yes, that's right.

>   To put it as a "pointer" in a private section of the
> structure feels like it is now backwards.
> 
> What is this helping with?  Who was messing with the device structure
> here that needed to not mess with it?

Well, it's not that somebody was messing with it. It's just the
fact that somebody could. 

> 
> It's good that you are now releasing the memory for the device
> structure
> properly, but this still feels really wrong.  What is keeping the
> lifetime of this structure now that the device is removed from it?

It's the most_dev structure of the of USB module (or any other lower
adapter driver), which embeds the most_interface sturcture that 
contained the dev struct (which I moved to the private section).

The thing that might be confusing is that the place, where the parent 
structure with the device is being allocated, is not the same where
the device actually gets registered with the kernel. These are two
different kernel modules actually.

thanks,
Chris

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


Re: [PATCH RFC v3 4/9] staging: most: move interface dev to private section

2020-01-15 Thread Greg KH
On Tue, Jan 14, 2020 at 04:57:53PM +0100, Christian Gromm wrote:
> This patch moves the struct device of the interface structure to its
> private section, because only the core should access it directly. For
> other entities an API is provided.

This feels "wrong".

Shouldn't the struct device in this structure be the thing that is
handling the reference counting and sysfs integration for this
structure?  To put it as a "pointer" in a private section of the
structure feels like it is now backwards.

What is this helping with?  Who was messing with the device structure
here that needed to not mess with it?

It's good that you are now releasing the memory for the device structure
properly, but this still feels really wrong.  What is keeping the
lifetime of this structure now that the device is removed from it?

thanks,

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


[PATCH RFC v3 4/9] staging: most: move interface dev to private section

2020-01-14 Thread Christian Gromm
This patch moves the struct device of the interface structure to its
private section, because only the core should access it directly. For
other entities an API is provided.

Signed-off-by: Christian Gromm 
---
v3:
This patch has been added to the series.

 drivers/staging/most/core.c  | 68 ++--
 drivers/staging/most/dim2/dim2.c |  2 +-
 drivers/staging/most/most.h  |  6 ++--
 drivers/staging/most/usb/usb.c   |  5 +--
 4 files changed, 51 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 0a3ce29..4f60c09 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -74,11 +74,15 @@ struct most_channel {
 
 struct interface_private {
int dev_id;
+   struct device dev;
+   struct most_interface *iface;
char name[STRING_SIZE];
struct most_channel *channel[MAX_CHANNELS];
struct list_head channel_list;
 };
 
+#define to_iface_priv(d) container_of(d, struct interface_private, dev)
+
 static const struct {
int most_ch_data_type;
const char *name;
@@ -401,7 +405,7 @@ static ssize_t description_show(struct device *dev,
struct device_attribute *attr,
char *buf)
 {
-   struct most_interface *iface = to_most_interface(dev);
+   struct most_interface *iface = to_iface_priv(dev)->iface;
 
return snprintf(buf, PAGE_SIZE, "%s\n", iface->description);
 }
@@ -410,7 +414,7 @@ static ssize_t interface_show(struct device *dev,
  struct device_attribute *attr,
  char *buf)
 {
-   struct most_interface *iface = to_most_interface(dev);
+   struct most_interface *iface = to_iface_priv(dev)->iface;
 
switch (iface->interface) {
case ITYPE_LOOPBACK:
@@ -475,7 +479,7 @@ static int print_links(struct device *dev, void *data)
int offs = d->offs;
char *buf = d->buf;
struct most_channel *c;
-   struct most_interface *iface = to_most_interface(dev);
+   struct most_interface *iface = to_iface_priv(dev)->iface;
 
list_for_each_entry(c, &iface->p->channel_list, list) {
if (c->pipe0.comp) {
@@ -483,7 +487,7 @@ static int print_links(struct device *dev, void *data)
 PAGE_SIZE - offs,
 "%s:%s:%s\n",
 c->pipe0.comp->name,
-dev_name(&iface->dev),
+dev_name(&iface->p->dev),
 dev_name(&c->dev));
}
if (c->pipe1.comp) {
@@ -491,7 +495,7 @@ static int print_links(struct device *dev, void *data)
 PAGE_SIZE - offs,
 "%s:%s:%s\n",
 c->pipe1.comp->name,
-dev_name(&iface->dev),
+dev_name(&iface->p->dev),
 dev_name(&c->dev));
}
}
@@ -533,7 +537,7 @@ static struct most_channel *get_channel(char *mdev, char 
*mdev_ch)
dev = bus_find_device_by_name(&mc.bus, NULL, mdev);
if (!dev)
return NULL;
-   iface = to_most_interface(dev);
+   iface = to_iface_priv(dev)->iface;
list_for_each_entry_safe(c, tmp, &iface->p->channel_list, list) {
if (!strcmp(dev_name(&c->dev), mdev_ch))
return c;
@@ -1231,7 +1235,7 @@ static int disconnect_channels(struct device *dev, void 
*data)
struct most_channel *c, *tmp;
struct most_component *comp = data;
 
-   iface = to_most_interface(dev);
+   iface = to_iface_priv(dev)->iface;
list_for_each_entry_safe(c, tmp, &iface->p->channel_list, list) {
if (c->pipe0.comp == comp || c->pipe1.comp == comp)
comp->disconnect_channel(c->iface, c->channel_id);
@@ -1260,16 +1264,35 @@ int most_deregister_component(struct most_component 
*comp)
 }
 EXPORT_SYMBOL_GPL(most_deregister_component);
 
-static void release_interface(struct device *dev)
+static void release_interface_priv(struct device *dev)
 {
+   struct interface_private *p = to_iface_priv(dev);
+
dev_info(&mc.dev, "releasing interface dev %s...\n", dev_name(dev));
+   kfree(p);
 }
 
 static void release_channel(struct device *dev)
 {
+   struct most_channel *c = to_channel(dev);
+
dev_info(&mc.dev, "releasing channel dev %s...\n", dev_name(dev));
+   kfree(c);
 }
 
+struct device *most_get_iface_dev(struct most_interface *iface)
+{
+   get_device(&iface->p->dev);
+   return &iface->p->dev;
+}
+EXPORT_SYMBOL_GPL(most_get_iface_dev);
+
+void most_put_iface_dev(struct device *de