diff mbox series

[1/2] module: add a function to add module references

Message ID a078eb2e46d00ec59c8a91ea0afa5190730c9e58.1651212016.git.mchehab@kernel.org (mailing list archive)
State New
Headers show
Series Let userspace know when snd-hda-intel needs i915 | expand

Commit Message

Mauro Carvalho Chehab April 29, 2022, 6:31 a.m. UTC
Sometimes, device drivers are bound using indirect references,
which is not visible when looking at /proc/modules or lsmod.

Add a function to allow setting up module references for such
cases.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---

See [PATCH 0/2] at: https://lore.kernel.org/all/cover.1651212016.git.mchehab@kernel.org/

 include/linux/module.h |  7 +++++++
 kernel/module/main.c   | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Daniel Vetter April 29, 2022, 7:54 a.m. UTC | #1
On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:
> Sometimes, device drivers are bound using indirect references,
> which is not visible when looking at /proc/modules or lsmod.
> 
> Add a function to allow setting up module references for such
> cases.
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

This sounds like duct tape at the wrong level. We should have a
device_link connecting these devices, and maybe device_link internally
needs to make sure the respective driver modules stay around for long
enough too. But open-coding this all over the place into every driver that
has some kind of cross-driver dependency sounds terrible.

Or maybe the bug is that the snd driver keeps accessing the hw/component
side when that is just plain gone. Iirc there's still fundamental issues
there on the sound side of things, which have been attempted to paper over
by timeouts and stuff like that in the past instead of enforcing a hard
link between the snd and i915 side.

Adding Greg for device model questions like this.
-Daniel

> ---
> 
> See [PATCH 0/2] at: https://lore.kernel.org/all/cover.1651212016.git.mchehab@kernel.org/
> 
>  include/linux/module.h |  7 +++++++
>  kernel/module/main.c   | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 46d4d5f2516e..be74f807e41d 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -596,6 +596,8 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
>  /* Search for module by name: must be in a RCU-sched critical section. */
>  struct module *find_module(const char *name);
>  
> +int module_add_named_dependency(const char *name, struct module *this);
> +
>  /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
>     symnum out of range. */
>  int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> @@ -772,6 +774,11 @@ static inline int lookup_module_symbol_attrs(unsigned long addr, unsigned long *
>  	return -ERANGE;
>  }
>  
> +static inline int module_add_named_dependency(const char *name, struct module *this)
> +{
> +	return 0;
> +}
> +
>  static inline int module_get_kallsym(unsigned int symnum, unsigned long *value,
>  					char *type, char *name,
>  					char *module_name, int *exported)
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 05a42d8fcd7a..dbd577ccc38c 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -631,6 +631,37 @@ static int ref_module(struct module *a, struct module *b)
>  	return 0;
>  }
>  
> +int module_add_named_dependency(const char *name, struct module *this)
> +{
> +	struct module *mod;
> +	int ret;
> +
> +	if (!name || !this || !this->name) {
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&module_mutex);
> +	mod = find_module(name);
> +	if (!mod) {
> +		ret = -EINVAL;
> +		goto ret;
> +	}
> +
> +	ret = ref_module(this, mod);
> +	if (ret)
> +		goto ret;
> +
> +#ifdef CONFIG_MODULE_UNLOAD
> +	ret = sysfs_create_link(mod->holders_dir,
> +				&this->mkobj.kobj, this->name);
> +#endif
> +
> +ret:
> +	mutex_unlock(&module_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(module_add_named_dependency);
> +
>  /* Clear the unload stuff of the module. */
>  static void module_unload_free(struct module *mod)
>  {
> -- 
> 2.35.1
>
Mauro Carvalho Chehab April 29, 2022, 8:07 a.m. UTC | #2
Hi Daniel,

Em Fri, 29 Apr 2022 09:54:10 +0200
Daniel Vetter <daniel@ffwll.ch> escreveu:

> On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:
> > Sometimes, device drivers are bound using indirect references,
> > which is not visible when looking at /proc/modules or lsmod.
> > 
> > Add a function to allow setting up module references for such
> > cases.
> > 
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>  
> 
> This sounds like duct tape at the wrong level. We should have a
> device_link connecting these devices, and maybe device_link internally
> needs to make sure the respective driver modules stay around for long
> enough too. But open-coding this all over the place into every driver that
> has some kind of cross-driver dependency sounds terrible.
> 
> Or maybe the bug is that the snd driver keeps accessing the hw/component
> side when that is just plain gone. Iirc there's still fundamental issues
> there on the sound side of things, which have been attempted to paper over
> by timeouts and stuff like that in the past instead of enforcing a hard
> link between the snd and i915 side.

I agree with you that the device link between snd-hda and the DRM driver
should properly handle unbinding on both directions. This is something
that require further discussions with ALSA and DRM people, and we should
keep working on it.

Yet, the binding between those drivers do exist, but, despite other
similar inter-driver bindings being properly reported by lsmod, this one
is invisible for userspace.

What this series does is to make such binding visible. As simple as that.


> 
> Adding Greg for device model questions like this.
> -Daniel
> 
> > ---
> > 
> > See [PATCH 0/2] at: https://lore.kernel.org/all/cover.1651212016.git.mchehab@kernel.org/
> > 
> >  include/linux/module.h |  7 +++++++
> >  kernel/module/main.c   | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 46d4d5f2516e..be74f807e41d 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -596,6 +596,8 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
> >  /* Search for module by name: must be in a RCU-sched critical section. */
> >  struct module *find_module(const char *name);
> >  
> > +int module_add_named_dependency(const char *name, struct module *this);
> > +
> >  /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
> >     symnum out of range. */
> >  int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> > @@ -772,6 +774,11 @@ static inline int lookup_module_symbol_attrs(unsigned long addr, unsigned long *
> >  	return -ERANGE;
> >  }
> >  
> > +static inline int module_add_named_dependency(const char *name, struct module *this)
> > +{
> > +	return 0;
> > +}
> > +
> >  static inline int module_get_kallsym(unsigned int symnum, unsigned long *value,
> >  					char *type, char *name,
> >  					char *module_name, int *exported)
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 05a42d8fcd7a..dbd577ccc38c 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -631,6 +631,37 @@ static int ref_module(struct module *a, struct module *b)
> >  	return 0;
> >  }
> >  
> > +int module_add_named_dependency(const char *name, struct module *this)
> > +{
> > +	struct module *mod;
> > +	int ret;
> > +
> > +	if (!name || !this || !this->name) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	mutex_lock(&module_mutex);
> > +	mod = find_module(name);
> > +	if (!mod) {
> > +		ret = -EINVAL;
> > +		goto ret;
> > +	}
> > +
> > +	ret = ref_module(this, mod);
> > +	if (ret)
> > +		goto ret;
> > +
> > +#ifdef CONFIG_MODULE_UNLOAD
> > +	ret = sysfs_create_link(mod->holders_dir,
> > +				&this->mkobj.kobj, this->name);
> > +#endif
> > +
> > +ret:
> > +	mutex_unlock(&module_mutex);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(module_add_named_dependency);
> > +
> >  /* Clear the unload stuff of the module. */
> >  static void module_unload_free(struct module *mod)
> >  {
> > -- 
> > 2.35.1
> >   
>
Greg Kroah-Hartman April 29, 2022, 8:30 a.m. UTC | #3
On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:
> Hi Daniel,
> 
> Em Fri, 29 Apr 2022 09:54:10 +0200
> Daniel Vetter <daniel@ffwll.ch> escreveu:
> 
> > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:
> > > Sometimes, device drivers are bound using indirect references,
> > > which is not visible when looking at /proc/modules or lsmod.
> > > 
> > > Add a function to allow setting up module references for such
> > > cases.
> > > 
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>  
> > 
> > This sounds like duct tape at the wrong level. We should have a
> > device_link connecting these devices, and maybe device_link internally
> > needs to make sure the respective driver modules stay around for long
> > enough too. But open-coding this all over the place into every driver that
> > has some kind of cross-driver dependency sounds terrible.
> > 
> > Or maybe the bug is that the snd driver keeps accessing the hw/component
> > side when that is just plain gone. Iirc there's still fundamental issues
> > there on the sound side of things, which have been attempted to paper over
> > by timeouts and stuff like that in the past instead of enforcing a hard
> > link between the snd and i915 side.
> 
> I agree with you that the device link between snd-hda and the DRM driver
> should properly handle unbinding on both directions. This is something
> that require further discussions with ALSA and DRM people, and we should
> keep working on it.
> 
> Yet, the binding between those drivers do exist, but, despite other
> similar inter-driver bindings being properly reported by lsmod, this one
> is invisible for userspace.
> 
> What this series does is to make such binding visible. As simple as that.

It also increases the reference count, and creates a user/kernel api
with the symlinks, right?  Will the reference count increase prevent the
modules from now being unloadable?

This feels like a very "weak" link between modules that should not be
needed if reference counting is implemented properly (so that things are
cleaned up in the correct order.)

Please don't paper over the real issue with this hack.

thanks,

greg k-h
Mauro Carvalho Chehab April 29, 2022, 9:15 a.m. UTC | #4
HI Greg,

Em Fri, 29 Apr 2022 10:30:33 +0200
Greg KH <gregkh@linuxfoundation.org> escreveu:

> On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:
> > Hi Daniel,
> > 
> > Em Fri, 29 Apr 2022 09:54:10 +0200
> > Daniel Vetter <daniel@ffwll.ch> escreveu:
> >   
> > > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:  
> > > > Sometimes, device drivers are bound using indirect references,
> > > > which is not visible when looking at /proc/modules or lsmod.
> > > > 
> > > > Add a function to allow setting up module references for such
> > > > cases.
> > > > 
> > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>    
> > > 
> > > This sounds like duct tape at the wrong level. We should have a
> > > device_link connecting these devices, and maybe device_link internally
> > > needs to make sure the respective driver modules stay around for long
> > > enough too. But open-coding this all over the place into every driver that
> > > has some kind of cross-driver dependency sounds terrible.
> > > 
> > > Or maybe the bug is that the snd driver keeps accessing the hw/component
> > > side when that is just plain gone. Iirc there's still fundamental issues
> > > there on the sound side of things, which have been attempted to paper over
> > > by timeouts and stuff like that in the past instead of enforcing a hard
> > > link between the snd and i915 side.  
> > 
> > I agree with you that the device link between snd-hda and the DRM driver
> > should properly handle unbinding on both directions. This is something
> > that require further discussions with ALSA and DRM people, and we should
> > keep working on it.
> > 
> > Yet, the binding between those drivers do exist, but, despite other
> > similar inter-driver bindings being properly reported by lsmod, this one
> > is invisible for userspace.
> > 
> > What this series does is to make such binding visible. As simple as that.  
> 
> It also increases the reference count, and creates a user/kernel api
> with the symlinks, right?  Will the reference count increase prevent the
> modules from now being unloadable?
>
> This feels like a very "weak" link between modules that should not be
> needed if reference counting is implemented properly (so that things are
> cleaned up in the correct order.)

The refcount increment exists even without this patch, as
hda_component_master_bind() at sound/hda/hdac_component.c uses 
try_module_get() when it creates the device link.

This series won't change anything with that regards. The only difference
is that it will now properly report userspace that snd-hda will be
using something inside the DRM driver (basically, it uses the DRM driver
to power-control the HDA hardware on modern CPU/GPUs).

-

Btw, this is not the only case where userspace invisible bindings between
two driver happen within the Kernel. On media, DVB drivers attach
the frontend/tuner drivers using I2C bus on a way where lsmod doesn't
current report any dependencies. See, for instance, PCTV 290e driver
(partial) dependency chain:

	$ lsmod
	Module                  Size  Used by
	rc_pinnacle_pctv_hd    16384  0
	em28xx_rc              20480  0
	tda18271               53248  1
	cxd2820r               45056  1
	em28xx_dvb             36864  0
	dvb_core              155648  2 cxd2820r,em28xx_dvb
	em28xx                106496  2 em28xx_rc,em28xx_dvb
	tveeprom               28672  1 em28xx
	videobuf2_vmalloc      20480  1 uvcvideo
	videobuf2_memops       20480  1 videobuf2_vmalloc
	videobuf2_common       69632  4 videobuf2_vmalloc,videobuf2_v4l2,uvcvideo,videobuf2_memops
	videodev              266240  4 videobuf2_v4l2,uvcvideo,videobuf2_common,em28xx
	mc                     65536  6 videodev,videobuf2_v4l2,uvcvideo,dvb_core,videobuf2_common,em28xx

In the above example, tda18271 is an I2C tuner driver which talks
to the hardware via the I2C bus registered by the em28xx driver.
It is loaded during em28xx probing time.

Again, lsmod doesn't show such dependencies. One can't remove the
tuner driver without first removing the em28xx driver, which is
the one that increments its refcount.

-

Back to the snd-hda issue, the problem is not with refcount. It is, instead,
to provide a way for userspace to know what's the correct order to 
remove/unbind modules.

Regards,
Mauro
Greg Kroah-Hartman April 29, 2022, 10:10 a.m. UTC | #5
On Fri, Apr 29, 2022 at 10:15:03AM +0100, Mauro Carvalho Chehab wrote:
> HI Greg,
> 
> Em Fri, 29 Apr 2022 10:30:33 +0200
> Greg KH <gregkh@linuxfoundation.org> escreveu:
> 
> > On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:
> > > Hi Daniel,
> > > 
> > > Em Fri, 29 Apr 2022 09:54:10 +0200
> > > Daniel Vetter <daniel@ffwll.ch> escreveu:
> > >   
> > > > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:  
> > > > > Sometimes, device drivers are bound using indirect references,
> > > > > which is not visible when looking at /proc/modules or lsmod.
> > > > > 
> > > > > Add a function to allow setting up module references for such
> > > > > cases.
> > > > > 
> > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>    
> > > > 
> > > > This sounds like duct tape at the wrong level. We should have a
> > > > device_link connecting these devices, and maybe device_link internally
> > > > needs to make sure the respective driver modules stay around for long
> > > > enough too. But open-coding this all over the place into every driver that
> > > > has some kind of cross-driver dependency sounds terrible.
> > > > 
> > > > Or maybe the bug is that the snd driver keeps accessing the hw/component
> > > > side when that is just plain gone. Iirc there's still fundamental issues
> > > > there on the sound side of things, which have been attempted to paper over
> > > > by timeouts and stuff like that in the past instead of enforcing a hard
> > > > link between the snd and i915 side.  
> > > 
> > > I agree with you that the device link between snd-hda and the DRM driver
> > > should properly handle unbinding on both directions. This is something
> > > that require further discussions with ALSA and DRM people, and we should
> > > keep working on it.
> > > 
> > > Yet, the binding between those drivers do exist, but, despite other
> > > similar inter-driver bindings being properly reported by lsmod, this one
> > > is invisible for userspace.
> > > 
> > > What this series does is to make such binding visible. As simple as that.  
> > 
> > It also increases the reference count, and creates a user/kernel api
> > with the symlinks, right?  Will the reference count increase prevent the
> > modules from now being unloadable?
> >
> > This feels like a very "weak" link between modules that should not be
> > needed if reference counting is implemented properly (so that things are
> > cleaned up in the correct order.)
> 
> The refcount increment exists even without this patch, as
> hda_component_master_bind() at sound/hda/hdac_component.c uses 
> try_module_get() when it creates the device link.

Ok, then why shouldn't try_module_get() be creating this link instead of
having to manually do it this way again?  You don't want to have to go
around and add this call to all users of that function, right?

thanks,

greg k-h
Mauro Carvalho Chehab April 29, 2022, 10:23 a.m. UTC | #6
Em Fri, 29 Apr 2022 12:10:07 +0200
Greg KH <gregkh@linuxfoundation.org> escreveu:

> On Fri, Apr 29, 2022 at 10:15:03AM +0100, Mauro Carvalho Chehab wrote:
> > HI Greg,
> > 
> > Em Fri, 29 Apr 2022 10:30:33 +0200
> > Greg KH <gregkh@linuxfoundation.org> escreveu:
> >   
> > > On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:  
> > > > Hi Daniel,
> > > > 
> > > > Em Fri, 29 Apr 2022 09:54:10 +0200
> > > > Daniel Vetter <daniel@ffwll.ch> escreveu:
> > > >     
> > > > > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:    
> > > > > > Sometimes, device drivers are bound using indirect references,
> > > > > > which is not visible when looking at /proc/modules or lsmod.
> > > > > > 
> > > > > > Add a function to allow setting up module references for such
> > > > > > cases.
> > > > > > 
> > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>      
> > > > > 
> > > > > This sounds like duct tape at the wrong level. We should have a
> > > > > device_link connecting these devices, and maybe device_link internally
> > > > > needs to make sure the respective driver modules stay around for long
> > > > > enough too. But open-coding this all over the place into every driver that
> > > > > has some kind of cross-driver dependency sounds terrible.
> > > > > 
> > > > > Or maybe the bug is that the snd driver keeps accessing the hw/component
> > > > > side when that is just plain gone. Iirc there's still fundamental issues
> > > > > there on the sound side of things, which have been attempted to paper over
> > > > > by timeouts and stuff like that in the past instead of enforcing a hard
> > > > > link between the snd and i915 side.    
> > > > 
> > > > I agree with you that the device link between snd-hda and the DRM driver
> > > > should properly handle unbinding on both directions. This is something
> > > > that require further discussions with ALSA and DRM people, and we should
> > > > keep working on it.
> > > > 
> > > > Yet, the binding between those drivers do exist, but, despite other
> > > > similar inter-driver bindings being properly reported by lsmod, this one
> > > > is invisible for userspace.
> > > > 
> > > > What this series does is to make such binding visible. As simple as that.    
> > > 
> > > It also increases the reference count, and creates a user/kernel api
> > > with the symlinks, right?  Will the reference count increase prevent the
> > > modules from now being unloadable?
> > >
> > > This feels like a very "weak" link between modules that should not be
> > > needed if reference counting is implemented properly (so that things are
> > > cleaned up in the correct order.)  
> > 
> > The refcount increment exists even without this patch, as
> > hda_component_master_bind() at sound/hda/hdac_component.c uses 
> > try_module_get() when it creates the device link.  
> 
> Ok, then why shouldn't try_module_get() be creating this link instead of
> having to manually do it this way again?  You don't want to have to go
> around and add this call to all users of that function, right?

Works for me, but this is not a too trivial change, as the new 
try_module_get() function will require two parameters, instead of one:

	- the module to be referenced;
	- the module which will reference it.

On trivial cases, one will be THIS_MODULE, but, in the specific case
of snd_hda, the binding is done via an ancillary routine under 
snd_hda_core, but the actual binding happens at snd_hda_intel.

Ok, we could add a __try_module_get() (or whatever other name that
would properly express what it does) with two parameters, and then
define try_module_get() as:

	#define try_module_get(mod) __try_module_get(mod, THIS_MODULE)

Would that work for you?

Regards,
Mauro
Greg Kroah-Hartman April 29, 2022, 10:36 a.m. UTC | #7
On Fri, Apr 29, 2022 at 11:23:51AM +0100, Mauro Carvalho Chehab wrote:
> Em Fri, 29 Apr 2022 12:10:07 +0200
> Greg KH <gregkh@linuxfoundation.org> escreveu:
> 
> > On Fri, Apr 29, 2022 at 10:15:03AM +0100, Mauro Carvalho Chehab wrote:
> > > HI Greg,
> > > 
> > > Em Fri, 29 Apr 2022 10:30:33 +0200
> > > Greg KH <gregkh@linuxfoundation.org> escreveu:
> > >   
> > > > On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:  
> > > > > Hi Daniel,
> > > > > 
> > > > > Em Fri, 29 Apr 2022 09:54:10 +0200
> > > > > Daniel Vetter <daniel@ffwll.ch> escreveu:
> > > > >     
> > > > > > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:    
> > > > > > > Sometimes, device drivers are bound using indirect references,
> > > > > > > which is not visible when looking at /proc/modules or lsmod.
> > > > > > > 
> > > > > > > Add a function to allow setting up module references for such
> > > > > > > cases.
> > > > > > > 
> > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>      
> > > > > > 
> > > > > > This sounds like duct tape at the wrong level. We should have a
> > > > > > device_link connecting these devices, and maybe device_link internally
> > > > > > needs to make sure the respective driver modules stay around for long
> > > > > > enough too. But open-coding this all over the place into every driver that
> > > > > > has some kind of cross-driver dependency sounds terrible.
> > > > > > 
> > > > > > Or maybe the bug is that the snd driver keeps accessing the hw/component
> > > > > > side when that is just plain gone. Iirc there's still fundamental issues
> > > > > > there on the sound side of things, which have been attempted to paper over
> > > > > > by timeouts and stuff like that in the past instead of enforcing a hard
> > > > > > link between the snd and i915 side.    
> > > > > 
> > > > > I agree with you that the device link between snd-hda and the DRM driver
> > > > > should properly handle unbinding on both directions. This is something
> > > > > that require further discussions with ALSA and DRM people, and we should
> > > > > keep working on it.
> > > > > 
> > > > > Yet, the binding between those drivers do exist, but, despite other
> > > > > similar inter-driver bindings being properly reported by lsmod, this one
> > > > > is invisible for userspace.
> > > > > 
> > > > > What this series does is to make such binding visible. As simple as that.    
> > > > 
> > > > It also increases the reference count, and creates a user/kernel api
> > > > with the symlinks, right?  Will the reference count increase prevent the
> > > > modules from now being unloadable?
> > > >
> > > > This feels like a very "weak" link between modules that should not be
> > > > needed if reference counting is implemented properly (so that things are
> > > > cleaned up in the correct order.)  
> > > 
> > > The refcount increment exists even without this patch, as
> > > hda_component_master_bind() at sound/hda/hdac_component.c uses 
> > > try_module_get() when it creates the device link.  
> > 
> > Ok, then why shouldn't try_module_get() be creating this link instead of
> > having to manually do it this way again?  You don't want to have to go
> > around and add this call to all users of that function, right?
> 
> Works for me, but this is not a too trivial change, as the new 
> try_module_get() function will require two parameters, instead of one:
> 
> 	- the module to be referenced;
> 	- the module which will reference it.
> 
> On trivial cases, one will be THIS_MODULE, but, in the specific case
> of snd_hda, the binding is done via an ancillary routine under 
> snd_hda_core, but the actual binding happens at snd_hda_intel.

For calls that want to increment a module reference on behalf of a
different code segment than is calling it, create a new function so we
can audit-the-heck out of those code paths as odds are, they are unsafe.

For the normal code path, just turn try_module_get() into a macro that
includes THIS_MODULE as part of it like we do for the driver register
functions (see usb_register_driver() in include/linux/usb.h as an
example of how to do that.)

> Ok, we could add a __try_module_get() (or whatever other name that
> would properly express what it does) with two parameters, and then
> define try_module_get() as:
> 
> 	#define try_module_get(mod) __try_module_get(mod, THIS_MODULE)

Yes, that's the way forward.

thanks,

greg k-h
Lucas De Marchi April 29, 2022, 3:52 p.m. UTC | #8
On Fri, Apr 29, 2022 at 11:23:51AM +0100, Mauro Carvalho Chehab wrote:
>Em Fri, 29 Apr 2022 12:10:07 +0200
>Greg KH <gregkh@linuxfoundation.org> escreveu:
>
>> On Fri, Apr 29, 2022 at 10:15:03AM +0100, Mauro Carvalho Chehab wrote:
>> > HI Greg,
>> >
>> > Em Fri, 29 Apr 2022 10:30:33 +0200
>> > Greg KH <gregkh@linuxfoundation.org> escreveu:
>> >
>> > > On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:
>> > > > Hi Daniel,
>> > > >
>> > > > Em Fri, 29 Apr 2022 09:54:10 +0200
>> > > > Daniel Vetter <daniel@ffwll.ch> escreveu:
>> > > >
>> > > > > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:
>> > > > > > Sometimes, device drivers are bound using indirect references,
>> > > > > > which is not visible when looking at /proc/modules or lsmod.
>> > > > > >
>> > > > > > Add a function to allow setting up module references for such
>> > > > > > cases.
>> > > > > >
>> > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>> > > > >
>> > > > > This sounds like duct tape at the wrong level. We should have a
>> > > > > device_link connecting these devices, and maybe device_link internally
>> > > > > needs to make sure the respective driver modules stay around for long
>> > > > > enough too. But open-coding this all over the place into every driver that
>> > > > > has some kind of cross-driver dependency sounds terrible.
>> > > > >
>> > > > > Or maybe the bug is that the snd driver keeps accessing the hw/component
>> > > > > side when that is just plain gone. Iirc there's still fundamental issues
>> > > > > there on the sound side of things, which have been attempted to paper over
>> > > > > by timeouts and stuff like that in the past instead of enforcing a hard
>> > > > > link between the snd and i915 side.
>> > > >
>> > > > I agree with you that the device link between snd-hda and the DRM driver
>> > > > should properly handle unbinding on both directions. This is something
>> > > > that require further discussions with ALSA and DRM people, and we should
>> > > > keep working on it.
>> > > >
>> > > > Yet, the binding between those drivers do exist, but, despite other
>> > > > similar inter-driver bindings being properly reported by lsmod, this one
>> > > > is invisible for userspace.
>> > > >
>> > > > What this series does is to make such binding visible. As simple as that.
>> > >
>> > > It also increases the reference count, and creates a user/kernel api
>> > > with the symlinks, right?  Will the reference count increase prevent the
>> > > modules from now being unloadable?
>> > >
>> > > This feels like a very "weak" link between modules that should not be
>> > > needed if reference counting is implemented properly (so that things are
>> > > cleaned up in the correct order.)
>> >
>> > The refcount increment exists even without this patch, as
>> > hda_component_master_bind() at sound/hda/hdac_component.c uses
>> > try_module_get() when it creates the device link.
>>
>> Ok, then why shouldn't try_module_get() be creating this link instead of
>> having to manually do it this way again?  You don't want to have to go
>> around and add this call to all users of that function, right?
>
>Works for me, but this is not a too trivial change, as the new
>try_module_get() function will require two parameters, instead of one:
>
>	- the module to be referenced;
>	- the module which will reference it.
>
>On trivial cases, one will be THIS_MODULE, but, in the specific case
>of snd_hda, the binding is done via an ancillary routine under
>snd_hda_core, but the actual binding happens at snd_hda_intel.
>
>Ok, we could add a __try_module_get() (or whatever other name that
>would properly express what it does) with two parameters, and then
>define try_module_get() as:
>
>	#define try_module_get(mod) __try_module_get(mod, THIS_MODULE)

agree that this should be done at this level rather than open coding it
at every driver. Main improvement being fixed here regardless of the
snd-hda-intel issue is to properly annotate what is holding a module.

Right now we have 1) symbol module dependencies; 2) kernel references;
3) userspace references. With (2) and (3) being unknown to the user from
lsmod pov. Handling this any time try_module_get() is called would make
(2) visible to lsmod.

Paired with fixes to the (unreleased) kmod 30[1], this allows `modprobe
-r --remove-holders <module>` to also try removing the holders before
removing the module itself.

thanks
Lucas De Marchi

[1] https://lore.kernel.org/linux-modules/20220329090912.geymr6xk7taq4rtq@ldmartin-desk2.jf.intel.com/T/#t


>
>Would that work for you?
>
>Regards,
>Mauro
Daniel Vetter May 4, 2022, 8:49 a.m. UTC | #9
On Fri, Apr 29, 2022 at 12:36:01PM +0200, Greg KH wrote:
> On Fri, Apr 29, 2022 at 11:23:51AM +0100, Mauro Carvalho Chehab wrote:
> > Em Fri, 29 Apr 2022 12:10:07 +0200
> > Greg KH <gregkh@linuxfoundation.org> escreveu:
> > 
> > > On Fri, Apr 29, 2022 at 10:15:03AM +0100, Mauro Carvalho Chehab wrote:
> > > > HI Greg,
> > > > 
> > > > Em Fri, 29 Apr 2022 10:30:33 +0200
> > > > Greg KH <gregkh@linuxfoundation.org> escreveu:
> > > >   
> > > > > On Fri, Apr 29, 2022 at 09:07:57AM +0100, Mauro Carvalho Chehab wrote:  
> > > > > > Hi Daniel,
> > > > > > 
> > > > > > Em Fri, 29 Apr 2022 09:54:10 +0200
> > > > > > Daniel Vetter <daniel@ffwll.ch> escreveu:
> > > > > >     
> > > > > > > On Fri, Apr 29, 2022 at 07:31:15AM +0100, Mauro Carvalho Chehab wrote:    
> > > > > > > > Sometimes, device drivers are bound using indirect references,
> > > > > > > > which is not visible when looking at /proc/modules or lsmod.
> > > > > > > > 
> > > > > > > > Add a function to allow setting up module references for such
> > > > > > > > cases.
> > > > > > > > 
> > > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>      
> > > > > > > 
> > > > > > > This sounds like duct tape at the wrong level. We should have a
> > > > > > > device_link connecting these devices, and maybe device_link internally
> > > > > > > needs to make sure the respective driver modules stay around for long
> > > > > > > enough too. But open-coding this all over the place into every driver that
> > > > > > > has some kind of cross-driver dependency sounds terrible.
> > > > > > > 
> > > > > > > Or maybe the bug is that the snd driver keeps accessing the hw/component
> > > > > > > side when that is just plain gone. Iirc there's still fundamental issues
> > > > > > > there on the sound side of things, which have been attempted to paper over
> > > > > > > by timeouts and stuff like that in the past instead of enforcing a hard
> > > > > > > link between the snd and i915 side.    
> > > > > > 
> > > > > > I agree with you that the device link between snd-hda and the DRM driver
> > > > > > should properly handle unbinding on both directions. This is something
> > > > > > that require further discussions with ALSA and DRM people, and we should
> > > > > > keep working on it.
> > > > > > 
> > > > > > Yet, the binding between those drivers do exist, but, despite other
> > > > > > similar inter-driver bindings being properly reported by lsmod, this one
> > > > > > is invisible for userspace.
> > > > > > 
> > > > > > What this series does is to make such binding visible. As simple as that.    
> > > > > 
> > > > > It also increases the reference count, and creates a user/kernel api
> > > > > with the symlinks, right?  Will the reference count increase prevent the
> > > > > modules from now being unloadable?
> > > > >
> > > > > This feels like a very "weak" link between modules that should not be
> > > > > needed if reference counting is implemented properly (so that things are
> > > > > cleaned up in the correct order.)  
> > > > 
> > > > The refcount increment exists even without this patch, as
> > > > hda_component_master_bind() at sound/hda/hdac_component.c uses 
> > > > try_module_get() when it creates the device link.  
> > > 
> > > Ok, then why shouldn't try_module_get() be creating this link instead of
> > > having to manually do it this way again?  You don't want to have to go
> > > around and add this call to all users of that function, right?
> > 
> > Works for me, but this is not a too trivial change, as the new 
> > try_module_get() function will require two parameters, instead of one:
> > 
> > 	- the module to be referenced;
> > 	- the module which will reference it.
> > 
> > On trivial cases, one will be THIS_MODULE, but, in the specific case
> > of snd_hda, the binding is done via an ancillary routine under 
> > snd_hda_core, but the actual binding happens at snd_hda_intel.
> 
> For calls that want to increment a module reference on behalf of a
> different code segment than is calling it, create a new function so we
> can audit-the-heck out of those code paths as odds are, they are unsafe.
> 
> For the normal code path, just turn try_module_get() into a macro that
> includes THIS_MODULE as part of it like we do for the driver register
> functions (see usb_register_driver() in include/linux/usb.h as an
> example of how to do that.)
> 
> > Ok, we could add a __try_module_get() (or whatever other name that
> > would properly express what it does) with two parameters, and then
> > define try_module_get() as:
> > 
> > 	#define try_module_get(mod) __try_module_get(mod, THIS_MODULE)
> 
> Yes, that's the way forward.

This might result in some complaints because it can create module refcount
loops. Well, seemingly module refcount loops, you can break them by first
unloading the driver, and then unloading the module. Currently a lot of
folks rely on just unload the module to tear down the drivers (and all in
right order through device links and component and all that). So maybe we
want to make this some kind of weak reference, i.e. it shows up in lsmod,
but the magic teardown still works and the module isn't actually pinnned.

Either way I agree that something like this sounds like the right
approach.
-Daniel
> 
> thanks,
> 
> greg k-h
diff mbox series

Patch

diff --git a/include/linux/module.h b/include/linux/module.h
index 46d4d5f2516e..be74f807e41d 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -596,6 +596,8 @@  static inline bool within_module(unsigned long addr, const struct module *mod)
 /* Search for module by name: must be in a RCU-sched critical section. */
 struct module *find_module(const char *name);
 
+int module_add_named_dependency(const char *name, struct module *this);
+
 /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
    symnum out of range. */
 int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
@@ -772,6 +774,11 @@  static inline int lookup_module_symbol_attrs(unsigned long addr, unsigned long *
 	return -ERANGE;
 }
 
+static inline int module_add_named_dependency(const char *name, struct module *this)
+{
+	return 0;
+}
+
 static inline int module_get_kallsym(unsigned int symnum, unsigned long *value,
 					char *type, char *name,
 					char *module_name, int *exported)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 05a42d8fcd7a..dbd577ccc38c 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -631,6 +631,37 @@  static int ref_module(struct module *a, struct module *b)
 	return 0;
 }
 
+int module_add_named_dependency(const char *name, struct module *this)
+{
+	struct module *mod;
+	int ret;
+
+	if (!name || !this || !this->name) {
+		return -EINVAL;
+	}
+
+	mutex_lock(&module_mutex);
+	mod = find_module(name);
+	if (!mod) {
+		ret = -EINVAL;
+		goto ret;
+	}
+
+	ret = ref_module(this, mod);
+	if (ret)
+		goto ret;
+
+#ifdef CONFIG_MODULE_UNLOAD
+	ret = sysfs_create_link(mod->holders_dir,
+				&this->mkobj.kobj, this->name);
+#endif
+
+ret:
+	mutex_unlock(&module_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(module_add_named_dependency);
+
 /* Clear the unload stuff of the module. */
 static void module_unload_free(struct module *mod)
 {