* [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err @ 2019-09-17 23:10 Matthias Maennich 2019-09-18 14:39 ` Alan Stern ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Matthias Maennich @ 2019-09-17 23:10 UTC (permalink / raw) To: linux-kernel Cc: kernel-team, maennich, Alan Stern, Greg Kroah-Hartman, usb-storage Follow common practice and retire printk(KERN_ERR ...) in favor of pr_fmt and pr_err. Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: usb-storage@lists.one-eyed-alien.net Signed-off-by: Matthias Maennich <maennich@google.com> --- drivers/usb/storage/scsiglue.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index 6737fab94959..0b6fa225b352 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -28,6 +28,8 @@ * status of a command. */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/blkdev.h> #include <linux/dma-mapping.h> #include <linux/module.h> @@ -379,8 +381,7 @@ static int queuecommand_lck(struct scsi_cmnd *srb, /* check for state-transition errors */ if (us->srb != NULL) { - printk(KERN_ERR "usb-storage: Error in %s: us->srb = %p\n", - __func__, us->srb); + pr_err("Error in %s: us->srb = %p\n", __func__, us->srb); return SCSI_MLQUEUE_HOST_BUSY; } -- 2.23.0.237.gc6a4ce50a0-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err 2019-09-17 23:10 [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err Matthias Maennich @ 2019-09-18 14:39 ` Alan Stern 2019-09-18 17:53 ` [PATCH v2] " Matthias Maennich ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Alan Stern @ 2019-09-18 14:39 UTC (permalink / raw) To: Matthias Maennich Cc: linux-kernel, kernel-team, Greg Kroah-Hartman, usb-storage On Wed, 18 Sep 2019, Matthias Maennich wrote: > Follow common practice and retire printk(KERN_ERR ...) in favor of > pr_fmt and pr_err. As long as you're changing this, why not change it to dev_err() instead? That would be a lot more useful. > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: usb-storage@lists.one-eyed-alien.net > Signed-off-by: Matthias Maennich <maennich@google.com> > --- > drivers/usb/storage/scsiglue.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index 6737fab94959..0b6fa225b352 100644 > --- a/drivers/usb/storage/scsiglue.c > +++ b/drivers/usb/storage/scsiglue.c > @@ -28,6 +28,8 @@ > * status of a command. > */ > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > #include <linux/blkdev.h> > #include <linux/dma-mapping.h> > #include <linux/module.h> > @@ -379,8 +381,7 @@ static int queuecommand_lck(struct scsi_cmnd *srb, > > /* check for state-transition errors */ > if (us->srb != NULL) { > - printk(KERN_ERR "usb-storage: Error in %s: us->srb = %p\n", > - __func__, us->srb); > + pr_err("Error in %s: us->srb = %p\n", __func__, us->srb); > return SCSI_MLQUEUE_HOST_BUSY; > } dev_err(&us->pusb_intf->dev, "Error in ... Alan Stern ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] usb-storage: SCSI glue: use pr_fmt and pr_err 2019-09-17 23:10 [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err Matthias Maennich 2019-09-18 14:39 ` Alan Stern @ 2019-09-18 17:53 ` Matthias Maennich 2019-09-18 18:39 ` Alan Stern 2019-10-02 11:03 ` [PATCH v3] usb-storage: SCSI glue: use dev_err instead of printk Matthias Maennich 2019-10-04 9:57 ` [PATCH] depmod: create and use System.map.no_namespaces Matthias Maennich 3 siblings, 1 reply; 10+ messages in thread From: Matthias Maennich @ 2019-09-18 17:53 UTC (permalink / raw) To: linux-kernel Cc: kernel-team, maennich, Alan Stern, Greg Kroah-Hartman, usb-storage Follow common practice and retire printk(KERN_ERR ...) in favor of pr_fmt and dev_err(). Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: usb-storage@lists.one-eyed-alien.net Signed-off-by: Matthias Maennich <maennich@google.com> --- drivers/usb/storage/scsiglue.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index 6737fab94959..afc4e3221369 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -28,6 +28,8 @@ * status of a command. */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/blkdev.h> #include <linux/dma-mapping.h> #include <linux/module.h> @@ -379,8 +381,8 @@ static int queuecommand_lck(struct scsi_cmnd *srb, /* check for state-transition errors */ if (us->srb != NULL) { - printk(KERN_ERR "usb-storage: Error in %s: us->srb = %p\n", - __func__, us->srb); + dev_err(&us->pusb_intf->dev, + "Error in %s: us->srb = %p\n", __func__, us->srb); return SCSI_MLQUEUE_HOST_BUSY; } -- 2.23.0.237.gc6a4ce50a0-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] usb-storage: SCSI glue: use pr_fmt and pr_err 2019-09-18 17:53 ` [PATCH v2] " Matthias Maennich @ 2019-09-18 18:39 ` Alan Stern 0 siblings, 0 replies; 10+ messages in thread From: Alan Stern @ 2019-09-18 18:39 UTC (permalink / raw) To: Matthias Maennich Cc: linux-kernel, kernel-team, Greg Kroah-Hartman, usb-storage On Wed, 18 Sep 2019, Matthias Maennich wrote: > Follow common practice and retire printk(KERN_ERR ...) in favor of > pr_fmt and dev_err(). > > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: usb-storage@lists.one-eyed-alien.net > Signed-off-by: Matthias Maennich <maennich@google.com> > --- > drivers/usb/storage/scsiglue.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index 6737fab94959..afc4e3221369 100644 > --- a/drivers/usb/storage/scsiglue.c > +++ b/drivers/usb/storage/scsiglue.c > @@ -28,6 +28,8 @@ > * status of a command. > */ > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + What's this here for? Alan Stern > #include <linux/blkdev.h> > #include <linux/dma-mapping.h> > #include <linux/module.h> > @@ -379,8 +381,8 @@ static int queuecommand_lck(struct scsi_cmnd *srb, > > /* check for state-transition errors */ > if (us->srb != NULL) { > - printk(KERN_ERR "usb-storage: Error in %s: us->srb = %p\n", > - __func__, us->srb); > + dev_err(&us->pusb_intf->dev, > + "Error in %s: us->srb = %p\n", __func__, us->srb); > return SCSI_MLQUEUE_HOST_BUSY; > } > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] usb-storage: SCSI glue: use dev_err instead of printk 2019-09-17 23:10 [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err Matthias Maennich 2019-09-18 14:39 ` Alan Stern 2019-09-18 17:53 ` [PATCH v2] " Matthias Maennich @ 2019-10-02 11:03 ` Matthias Maennich 2019-10-04 9:57 ` [PATCH] depmod: create and use System.map.no_namespaces Matthias Maennich 3 siblings, 0 replies; 10+ messages in thread From: Matthias Maennich @ 2019-10-02 11:03 UTC (permalink / raw) To: linux-kernel Cc: kernel-team, maennich, Alan Stern, Greg Kroah-Hartman, usb-storage Follow common practice and retire printk(KERN_ERR ...) in favor of dev_err(). Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: usb-storage@lists.one-eyed-alien.net Signed-off-by: Matthias Maennich <maennich@google.com> --- drivers/usb/storage/scsiglue.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index 6737fab94959..4c0c247e4101 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -379,8 +379,8 @@ static int queuecommand_lck(struct scsi_cmnd *srb, /* check for state-transition errors */ if (us->srb != NULL) { - printk(KERN_ERR "usb-storage: Error in %s: us->srb = %p\n", - __func__, us->srb); + dev_err(&us->pusb_intf->dev, + "Error in %s: us->srb = %p\n", __func__, us->srb); return SCSI_MLQUEUE_HOST_BUSY; } -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] depmod: create and use System.map.no_namespaces 2019-09-17 23:10 [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err Matthias Maennich ` (2 preceding siblings ...) 2019-10-02 11:03 ` [PATCH v3] usb-storage: SCSI glue: use dev_err instead of printk Matthias Maennich @ 2019-10-04 9:57 ` Matthias Maennich 2019-10-04 18:25 ` Lucas De Marchi 3 siblings, 1 reply; 10+ messages in thread From: Matthias Maennich @ 2019-10-04 9:57 UTC (permalink / raw) To: linux-kernel Cc: kernel-team, maennich, Stefan Wahren, Masahiro Yamada, Lucas De Marchi, Jessica Yu, Martijn Coenen, Greg Kroah-Hartman, linux-modules depmod in its current version is not aware of symbol namespace in ksymtab entries introduced with 8651ec01daed ("module: add support for symbol namespaces."). They have the form __ksymtab_NAMESPACE.symbol_name A fix for kmod's depmod has been proposed [1]. In order to support older versions of depmod as well, create a System.map.no_namespaces during scripts/depmod.sh that has the pre-namespaces format. That way users do not immediately upgrade the userspace tool. [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ Reported-by: Stefan Wahren <stefan.wahren@i2se.com> Fixes: 8651ec01daed ("module: add support for symbol namespaces.") Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> Cc: Jessica Yu <jeyu@kernel.org> Cc: Martijn Coenen <maco@android.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-modules@vger.kernel.org Signed-off-by: Matthias Maennich <maennich@google.com> --- Please note this depends on the new ksymtab entry format proposed in https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence this patch depends on it. Cheers, Matthias .gitignore | 1 + scripts/depmod.sh | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 70580bdd352c..5ed58a7cb433 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,7 @@ modules.order /vmlinux-gdb.py /vmlinuz /System.map +/System.map.no_namespaces /Module.markers /modules.builtin.modinfo diff --git a/scripts/depmod.sh b/scripts/depmod.sh index e083bcae343f..602e1af072c7 100755 --- a/scripts/depmod.sh +++ b/scripts/depmod.sh @@ -39,7 +39,13 @@ if $depmod_hack_needed; then KERNELRELEASE=99.98.$KERNELRELEASE fi -set -- -ae -F System.map +# Older versions of depmod do not support symbol namespaces in ksymtab entries, +# hence create an alternative System.map with namespace patched out to use for +# depmod. I.e. transform entries as follows: +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces + +set -- -ae -F System.map.no_namespaces if test -n "$INSTALL_MOD_PATH"; then set -- "$@" -b "$INSTALL_MOD_PATH" fi -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] depmod: create and use System.map.no_namespaces 2019-10-04 9:57 ` [PATCH] depmod: create and use System.map.no_namespaces Matthias Maennich @ 2019-10-04 18:25 ` Lucas De Marchi 2019-10-05 7:53 ` Masahiro Yamada 0 siblings, 1 reply; 10+ messages in thread From: Lucas De Marchi @ 2019-10-04 18:25 UTC (permalink / raw) To: Matthias Maennich Cc: lkml, kernel-team, Stefan Wahren, Masahiro Yamada, Jessica Yu, Martijn Coenen, Greg Kroah-Hartman, linux-modules On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: > > depmod in its current version is not aware of symbol namespace in > ksymtab entries introduced with 8651ec01daed ("module: add support for > symbol namespaces."). They have the form > > __ksymtab_NAMESPACE.symbol_name > > A fix for kmod's depmod has been proposed [1]. In order to support older > versions of depmod as well, create a System.map.no_namespaces during > scripts/depmod.sh that has the pre-namespaces format. That way users do > not immediately upgrade the userspace tool. > > [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Fixes: 8651ec01daed ("module: add support for symbol namespaces.") > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> > Cc: Jessica Yu <jeyu@kernel.org> > Cc: Martijn Coenen <maco@android.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-modules@vger.kernel.org > Signed-off-by: Matthias Maennich <maennich@google.com> > --- > > Please note this depends on the new ksymtab entry format proposed in > https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ I don't really agree with that thought, more below. > > That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence > this patch depends on it. > > Cheers, > Matthias > > .gitignore | 1 + > scripts/depmod.sh | 8 +++++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/.gitignore b/.gitignore > index 70580bdd352c..5ed58a7cb433 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -59,6 +59,7 @@ modules.order > /vmlinux-gdb.py > /vmlinuz > /System.map > +/System.map.no_namespaces > /Module.markers > /modules.builtin.modinfo > > diff --git a/scripts/depmod.sh b/scripts/depmod.sh > index e083bcae343f..602e1af072c7 100755 > --- a/scripts/depmod.sh > +++ b/scripts/depmod.sh > @@ -39,7 +39,13 @@ if $depmod_hack_needed; then > KERNELRELEASE=99.98.$KERNELRELEASE > fi > > -set -- -ae -F System.map > +# Older versions of depmod do not support symbol namespaces in ksymtab entries, > +# hence create an alternative System.map with namespace patched out to use for > +# depmod. I.e. transform entries as follows: > +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name > +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces So people with old kmod will have to know they need to pass System.map.no_namespaces rather than the usual System.map. Also, distros will need to be update to also copy the new file to the kernel package (or upgrade/patch kmod). I'd rather maintain the current format and fix the bug that patch is fixing. The namespace in the end IMO is just a small annoyance with a reason to exist. Lucas De Marchi > + > +set -- -ae -F System.map.no_namespaces > if test -n "$INSTALL_MOD_PATH"; then > set -- "$@" -b "$INSTALL_MOD_PATH" > fi > -- > 2.23.0.581.g78d2f28ef7-goog > -- Lucas De Marchi ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] depmod: create and use System.map.no_namespaces 2019-10-04 18:25 ` Lucas De Marchi @ 2019-10-05 7:53 ` Masahiro Yamada 2019-10-07 6:46 ` Matthias Maennich 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2019-10-05 7:53 UTC (permalink / raw) To: Lucas De Marchi Cc: Matthias Maennich, lkml, Cc: Android Kernel, Stefan Wahren, Jessica Yu, Martijn Coenen, Greg Kroah-Hartman, linux-modules On Sat, Oct 5, 2019 at 3:25 AM Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > > On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: > > > > depmod in its current version is not aware of symbol namespace in > > ksymtab entries introduced with 8651ec01daed ("module: add support for > > symbol namespaces."). They have the form > > > > __ksymtab_NAMESPACE.symbol_name > > > > A fix for kmod's depmod has been proposed [1]. In order to support older > > versions of depmod as well, create a System.map.no_namespaces during > > scripts/depmod.sh that has the pre-namespaces format. That way users do > > not immediately upgrade the userspace tool. > > > > [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ > > > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > Fixes: 8651ec01daed ("module: add support for symbol namespaces.") > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> > > Cc: Jessica Yu <jeyu@kernel.org> > > Cc: Martijn Coenen <maco@android.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: linux-modules@vger.kernel.org > > Signed-off-by: Matthias Maennich <maennich@google.com> > > --- > > > > Please note this depends on the new ksymtab entry format proposed in > > https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ > > I don't really agree with that thought, more below. > > > > > That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence > > this patch depends on it. > > > > Cheers, > > Matthias > > > > .gitignore | 1 + > > scripts/depmod.sh | 8 +++++++- > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/.gitignore b/.gitignore > > index 70580bdd352c..5ed58a7cb433 100644 > > --- a/.gitignore > > +++ b/.gitignore > > @@ -59,6 +59,7 @@ modules.order > > /vmlinux-gdb.py > > /vmlinuz > > /System.map > > +/System.map.no_namespaces > > /Module.markers > > /modules.builtin.modinfo > > > > diff --git a/scripts/depmod.sh b/scripts/depmod.sh > > index e083bcae343f..602e1af072c7 100755 > > --- a/scripts/depmod.sh > > +++ b/scripts/depmod.sh > > @@ -39,7 +39,13 @@ if $depmod_hack_needed; then > > KERNELRELEASE=99.98.$KERNELRELEASE > > fi > > > > -set -- -ae -F System.map > > +# Older versions of depmod do not support symbol namespaces in ksymtab entries, > > +# hence create an alternative System.map with namespace patched out to use for > > +# depmod. I.e. transform entries as follows: > > +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name > > +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces > > So people with old kmod will have to know they need to pass > System.map.no_namespaces rather than the usual > System.map. Also, distros will need to be update to also copy the new > file to the kernel package (or upgrade/patch kmod). > > I'd rather maintain the current format and fix the bug that patch is > fixing. The namespace > in the end IMO is just a small annoyance with a reason to exist. I agree, this fix is bad. We should not bother kmod or any tools. And System.map.no_namespaces is a cheesy workaround. BTW, I expressed my negative opinion in the review process for the patch set. I am still not convinced with the namespace feature, but anyway it was merged (with poor review and test). Get back on track, probably the right fix would be to stop using __ksymtab_<namespace>.<symbol>. It is not used for any purposes but passing <namespace> / <symbol> pairs to modpost. For example, __kstrtabns_##sym points to the namespace string, so it would be possible to parse it from modpost? Then, asm("__ksymtab_" #ns NS_SEPARATOR #sym) will go away. Masahiro > Lucas De Marchi > > > + > > +set -- -ae -F System.map.no_namespaces > > if test -n "$INSTALL_MOD_PATH"; then > > set -- "$@" -b "$INSTALL_MOD_PATH" > > fi > > -- > > 2.23.0.581.g78d2f28ef7-goog > > > > > -- > Lucas De Marchi -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] depmod: create and use System.map.no_namespaces 2019-10-05 7:53 ` Masahiro Yamada @ 2019-10-07 6:46 ` Matthias Maennich 2019-10-07 11:25 ` Jessica Yu 0 siblings, 1 reply; 10+ messages in thread From: Matthias Maennich @ 2019-10-07 6:46 UTC (permalink / raw) To: Masahiro Yamada Cc: Lucas De Marchi, lkml, Cc: Android Kernel, Stefan Wahren, Jessica Yu, Martijn Coenen, Greg Kroah-Hartman, linux-modules Hi! On Sat, Oct 05, 2019 at 04:53:56PM +0900, Masahiro Yamada wrote: >On Sat, Oct 5, 2019 at 3:25 AM Lucas De Marchi ><lucas.de.marchi@gmail.com> wrote: >> >> On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: >> > >> > depmod in its current version is not aware of symbol namespace in >> > ksymtab entries introduced with 8651ec01daed ("module: add support for >> > symbol namespaces."). They have the form >> > >> > __ksymtab_NAMESPACE.symbol_name >> > >> > A fix for kmod's depmod has been proposed [1]. In order to support older >> > versions of depmod as well, create a System.map.no_namespaces during >> > scripts/depmod.sh that has the pre-namespaces format. That way users do >> > not immediately upgrade the userspace tool. >> > >> > [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ >> > >> > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> >> > Fixes: 8651ec01daed ("module: add support for symbol namespaces.") >> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> >> > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> >> > Cc: Jessica Yu <jeyu@kernel.org> >> > Cc: Martijn Coenen <maco@android.com> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > Cc: linux-modules@vger.kernel.org >> > Signed-off-by: Matthias Maennich <maennich@google.com> >> > --- >> > >> > Please note this depends on the new ksymtab entry format proposed in >> > https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ >> >> I don't really agree with that thought, more below. >> >> > >> > That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence >> > this patch depends on it. >> > >> > Cheers, >> > Matthias >> > >> > .gitignore | 1 + >> > scripts/depmod.sh | 8 +++++++- >> > 2 files changed, 8 insertions(+), 1 deletion(-) >> > >> > diff --git a/.gitignore b/.gitignore >> > index 70580bdd352c..5ed58a7cb433 100644 >> > --- a/.gitignore >> > +++ b/.gitignore >> > @@ -59,6 +59,7 @@ modules.order >> > /vmlinux-gdb.py >> > /vmlinuz >> > /System.map >> > +/System.map.no_namespaces >> > /Module.markers >> > /modules.builtin.modinfo >> > >> > diff --git a/scripts/depmod.sh b/scripts/depmod.sh >> > index e083bcae343f..602e1af072c7 100755 >> > --- a/scripts/depmod.sh >> > +++ b/scripts/depmod.sh >> > @@ -39,7 +39,13 @@ if $depmod_hack_needed; then >> > KERNELRELEASE=99.98.$KERNELRELEASE >> > fi >> > >> > -set -- -ae -F System.map >> > +# Older versions of depmod do not support symbol namespaces in ksymtab entries, >> > +# hence create an alternative System.map with namespace patched out to use for >> > +# depmod. I.e. transform entries as follows: >> > +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name >> > +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces >> >> So people with old kmod will have to know they need to pass >> System.map.no_namespaces rather than the usual >> System.map. Also, distros will need to be update to also copy the new >> file to the kernel package (or upgrade/patch kmod). >> >> I'd rather maintain the current format and fix the bug that patch is >> fixing. The namespace >> in the end IMO is just a small annoyance with a reason to exist. > >I agree, this fix is bad. >We should not bother kmod or any tools. >And System.map.no_namespaces is a cheesy workaround. Thanks for the feedback! Based on the discussion from last year about changing kmod/depmod for this change to the ksymtab entries, (https://lore.kernel.org/lkml/CAKi4VA+wvVxPxVSP6ruSR++TYvavKPNAQ5XhREF_9ZxeOKQirg@mail.gmail.com/), I assumed this approach would be acceptable. The workaround (yeah, it is a hack) is a way to mitigate the issue for people that can't update depmod so easily. System.map.no_namespaces was not intended to be part of any distribution, but rather used locally in depmod.sh. I could have made this more clear. > >BTW, I expressed my negative opinion in the review process >for the patch set. I am still not convinced with the >namespace feature, but anyway it was merged >(with poor review and test). > > > >Get back on track, probably the right fix would be to >stop using __ksymtab_<namespace>.<symbol>. > >It is not used for any purposes but passing ><namespace> / <symbol> pairs to modpost. > > >For example, __kstrtabns_##sym points to >the namespace string, so it would be possible >to parse it from modpost? > >Then, asm("__ksymtab_" #ns NS_SEPARATOR #sym) >will go away. I will give this a try. Cheers, Matthias > > >Masahiro > > > > >> Lucas De Marchi >> >> > + >> > +set -- -ae -F System.map.no_namespaces >> > if test -n "$INSTALL_MOD_PATH"; then >> > set -- "$@" -b "$INSTALL_MOD_PATH" >> > fi >> > -- >> > 2.23.0.581.g78d2f28ef7-goog >> > >> >> >> -- >> Lucas De Marchi > > > >-- >Best Regards >Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] depmod: create and use System.map.no_namespaces 2019-10-07 6:46 ` Matthias Maennich @ 2019-10-07 11:25 ` Jessica Yu 0 siblings, 0 replies; 10+ messages in thread From: Jessica Yu @ 2019-10-07 11:25 UTC (permalink / raw) To: Matthias Maennich Cc: Masahiro Yamada, Lucas De Marchi, lkml, Cc: Android Kernel, Stefan Wahren, Martijn Coenen, Greg Kroah-Hartman, linux-modules +++ Matthias Maennich [07/10/19 07:46 +0100]: >Hi! > >On Sat, Oct 05, 2019 at 04:53:56PM +0900, Masahiro Yamada wrote: >>On Sat, Oct 5, 2019 at 3:25 AM Lucas De Marchi >><lucas.de.marchi@gmail.com> wrote: >>> >>>On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: >>>> >>>> depmod in its current version is not aware of symbol namespace in >>>> ksymtab entries introduced with 8651ec01daed ("module: add support for >>>> symbol namespaces."). They have the form >>>> >>>> __ksymtab_NAMESPACE.symbol_name >>>> >>>> A fix for kmod's depmod has been proposed [1]. In order to support older >>>> versions of depmod as well, create a System.map.no_namespaces during >>>> scripts/depmod.sh that has the pre-namespaces format. That way users do >>>> not immediately upgrade the userspace tool. >>>> >>>> [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ >>>> >>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com> >>>> Fixes: 8651ec01daed ("module: add support for symbol namespaces.") >>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> >>>> Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> >>>> Cc: Jessica Yu <jeyu@kernel.org> >>>> Cc: Martijn Coenen <maco@android.com> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> Cc: linux-modules@vger.kernel.org >>>> Signed-off-by: Matthias Maennich <maennich@google.com> >>>> --- >>>> >>>> Please note this depends on the new ksymtab entry format proposed in >>>> https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ >>> >>>I don't really agree with that thought, more below. >>> >>>> >>>> That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence >>>> this patch depends on it. >>>> >>>> Cheers, >>>> Matthias >>>> >>>> .gitignore | 1 + >>>> scripts/depmod.sh | 8 +++++++- >>>> 2 files changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/.gitignore b/.gitignore >>>> index 70580bdd352c..5ed58a7cb433 100644 >>>> --- a/.gitignore >>>> +++ b/.gitignore >>>> @@ -59,6 +59,7 @@ modules.order >>>> /vmlinux-gdb.py >>>> /vmlinuz >>>> /System.map >>>> +/System.map.no_namespaces >>>> /Module.markers >>>> /modules.builtin.modinfo >>>> >>>> diff --git a/scripts/depmod.sh b/scripts/depmod.sh >>>> index e083bcae343f..602e1af072c7 100755 >>>> --- a/scripts/depmod.sh >>>> +++ b/scripts/depmod.sh >>>> @@ -39,7 +39,13 @@ if $depmod_hack_needed; then >>>> KERNELRELEASE=99.98.$KERNELRELEASE >>>> fi >>>> >>>> -set -- -ae -F System.map >>>> +# Older versions of depmod do not support symbol namespaces in ksymtab entries, >>>> +# hence create an alternative System.map with namespace patched out to use for >>>> +# depmod. I.e. transform entries as follows: >>>> +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name >>>> +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces >>> >>>So people with old kmod will have to know they need to pass >>>System.map.no_namespaces rather than the usual >>>System.map. Also, distros will need to be update to also copy the new >>>file to the kernel package (or upgrade/patch kmod). >>> >>>I'd rather maintain the current format and fix the bug that patch is >>>fixing. The namespace >>>in the end IMO is just a small annoyance with a reason to exist. >> >>I agree, this fix is bad. >>We should not bother kmod or any tools. >>And System.map.no_namespaces is a cheesy workaround. > >Thanks for the feedback! > >Based on the discussion from last year about changing kmod/depmod for >this change to the ksymtab entries, >(https://lore.kernel.org/lkml/CAKi4VA+wvVxPxVSP6ruSR++TYvavKPNAQ5XhREF_9ZxeOKQirg@mail.gmail.com/), >I assumed this approach would be acceptable. The workaround (yeah, it is >a hack) is a way to mitigate the issue for people that can't update >depmod so easily. System.map.no_namespaces was not intended to be part >of any distribution, but rather used locally in depmod.sh. I could have >made this more clear. > >> >>BTW, I expressed my negative opinion in the review process >>for the patch set. I am still not convinced with the >>namespace feature, but anyway it was merged >>(with poor review and test). >> >> >> >>Get back on track, probably the right fix would be to >>stop using __ksymtab_<namespace>.<symbol>. >> >>It is not used for any purposes but passing >><namespace> / <symbol> pairs to modpost. >> >> >>For example, __kstrtabns_##sym points to >>the namespace string, so it would be possible >>to parse it from modpost? >> >>Then, asm("__ksymtab_" #ns NS_SEPARATOR #sym) >>will go away. > >I will give this a try. That sounds like a good idea. Maybe one way of going about it is to take the symbol value of __kstrtabns_##sym, subtract the section address (the section referred to by the st_shndx) from it to obtain the offset into __ksymtab_strings, and read in the namespace string from that offset. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-07 11:26 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-17 23:10 [PATCH] usb-storage: SCSI glue: use pr_fmt and pr_err Matthias Maennich 2019-09-18 14:39 ` Alan Stern 2019-09-18 17:53 ` [PATCH v2] " Matthias Maennich 2019-09-18 18:39 ` Alan Stern 2019-10-02 11:03 ` [PATCH v3] usb-storage: SCSI glue: use dev_err instead of printk Matthias Maennich 2019-10-04 9:57 ` [PATCH] depmod: create and use System.map.no_namespaces Matthias Maennich 2019-10-04 18:25 ` Lucas De Marchi 2019-10-05 7:53 ` Masahiro Yamada 2019-10-07 6:46 ` Matthias Maennich 2019-10-07 11:25 ` Jessica Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).