From: Patrick Rudolph <[email protected]>
This patch series fixes 3 independent bugs in the google firmware
drivers.
Patch 1-2 do proper cleanup at kernel module unloading.
Patch 3 adds a check if the optional GSMI SMM handler is actually
present in the firmware and responses to the driver.
Arthur Heymans (2):
firmware: google: Unregister driver_info on failure and exit in gsmi
firmware: google: Probe for a GSMI handler in firmware
Patrick Rudolph (1):
firmware: google: Release devices before unregistering the bus
drivers/firmware/google/coreboot_table.c | 6 ++++++
drivers/firmware/google/gsmi.c | 24 ++++++++++++++++++++++++
2 files changed, 30 insertions(+)
--
2.21.0
From: Patrick Rudolph <[email protected]>
Fix a bug where the kernel module can't be loaded after it has been
unloaded as the devices are still present and conflicting with the
to be created coreboot devices.
Signed-off-by: Patrick Rudolph <[email protected]>
---
drivers/firmware/google/coreboot_table.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 8d132e4f008a..88c6545bebf4 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -163,8 +163,14 @@ static int coreboot_table_probe(struct platform_device *pdev)
return ret;
}
+static int __cb_dev_unregister(struct device *dev, void *dummy)
+{
+ device_unregister(dev);
+}
+
static int coreboot_table_remove(struct platform_device *pdev)
{
+ bus_for_each_dev(&coreboot_bus_type, NULL, NULL, __cb_dev_unregister);
bus_unregister(&coreboot_bus_type);
return 0;
}
--
2.21.0
From: Arthur Heymans <[email protected]>
Fix a bug where the kernel module couldn't be loaded after unloading,
as the platform driver wasn't released on exit.
Signed-off-by: Arthur Heymans <[email protected]>
---
drivers/firmware/google/gsmi.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index edaa4e5d84ad..974c769b75cf 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -1016,6 +1016,9 @@ static __init int gsmi_init(void)
dma_pool_destroy(gsmi_dev.dma_pool);
platform_device_unregister(gsmi_dev.pdev);
pr_info("gsmi: failed to load: %d\n", ret);
+#ifdef CONFIG_PM
+ platform_driver_unregister(&gsmi_driver_info);
+#endif
return ret;
}
@@ -1037,6 +1040,9 @@ static void __exit gsmi_exit(void)
gsmi_buf_free(gsmi_dev.name_buf);
dma_pool_destroy(gsmi_dev.dma_pool);
platform_device_unregister(gsmi_dev.pdev);
+#ifdef CONFIG_PM
+ platform_driver_unregister(&gsmi_driver_info);
+#endif
}
module_init(gsmi_init);
--
2.21.0
From: Arthur Heymans <[email protected]>
Currently this driver is loaded if the DMI string matches coreboot
and has a proper smi_command in the ACPI FADT table, but a GSMI handler in
SMM is an optional feature in coreboot.
So probe for a SMM GSMI handler before initializing the driver.
If the smihandler leaves the calling argument in %eax in the SMM save state
untouched that generally means the is no handler for GSMI.
Signed-off-by: Arthur Heymans <[email protected]>
---
drivers/firmware/google/gsmi.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index 974c769b75cf..a3eaa9f41125 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -746,6 +746,7 @@ MODULE_DEVICE_TABLE(dmi, gsmi_dmi_table);
static __init int gsmi_system_valid(void)
{
u32 hash;
+ u16 cmd, result;
if (!dmi_check_system(gsmi_dmi_table))
return -ENODEV;
@@ -780,6 +781,23 @@ static __init int gsmi_system_valid(void)
return -ENODEV;
}
+ /* Test the smihandler with a bogus command. If it leaves the
+ * calling argument in %ax untouched, there is no handler for
+ * GSMI commands.
+ */
+ cmd = GSMI_CALLBACK | 0xff << 8;
+ asm volatile (
+ "outb %%al, %%dx\n\t"
+ : "=a" (result)
+ : "0" (cmd),
+ "d" (acpi_gbl_FADT.smi_command)
+ : "memory", "cc"
+ );
+ if (cmd == result) {
+ pr_info("gsmi: no gsmi handler in firmware\n");
+ return -ENODEV;
+ }
+
/* Found */
return 0;
}
--
2.21.0
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.4-rc7 next-20191115]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/patrick-rudolph-9elements-com/firmware-google-Fix-minor-bugs/20191116-024230
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 96b95eff4a591dbac582c2590d067e356a18aacb
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/firmware/google/coreboot_table.c: In function '__cb_dev_unregister':
>> drivers/firmware/google/coreboot_table.c:169:1: warning: no return statement in function returning non-void [-Wreturn-type]
}
^
vim +169 drivers/firmware/google/coreboot_table.c
165
166 static int __cb_dev_unregister(struct device *dev, void *dummy)
167 {
168 device_unregister(dev);
> 169 }
170
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
On Fri, Nov 15, 2019 at 02:48:37PM +0100, [email protected] wrote:
> From: Patrick Rudolph <[email protected]>
>
> Fix a bug where the kernel module can't be loaded after it has been
> unloaded as the devices are still present and conflicting with the
> to be created coreboot devices.
>
> Signed-off-by: Patrick Rudolph <[email protected]>
> ---
> drivers/firmware/google/coreboot_table.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index 8d132e4f008a..88c6545bebf4 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -163,8 +163,14 @@ static int coreboot_table_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static int __cb_dev_unregister(struct device *dev, void *dummy)
> +{
> + device_unregister(dev);
Did you build this patch???
Did it work when you tested it? How?
Please fix up,
greg k-h
On Fri, Nov 15, 2019 at 02:48:38PM +0100, [email protected] wrote:
> From: Arthur Heymans <[email protected]>
>
> Fix a bug where the kernel module couldn't be loaded after unloading,
> as the platform driver wasn't released on exit.
>
> Signed-off-by: Arthur Heymans <[email protected]>
> ---
When sending a patch from someone else, you too have to add your s-o-b
so that we can accept it as that shows the progression of the patch (and
you are accepting responsibility for it being correct.)
Please fix up when you resend.
thanks,
greg k-h
On Fri, Nov 15, 2019 at 02:48:38PM +0100, [email protected] wrote:
> From: Arthur Heymans <[email protected]>
>
> Fix a bug where the kernel module couldn't be loaded after unloading,
> as the platform driver wasn't released on exit.
>
> Signed-off-by: Arthur Heymans <[email protected]>
> ---
> drivers/firmware/google/gsmi.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
> index edaa4e5d84ad..974c769b75cf 100644
> --- a/drivers/firmware/google/gsmi.c
> +++ b/drivers/firmware/google/gsmi.c
> @@ -1016,6 +1016,9 @@ static __init int gsmi_init(void)
> dma_pool_destroy(gsmi_dev.dma_pool);
> platform_device_unregister(gsmi_dev.pdev);
> pr_info("gsmi: failed to load: %d\n", ret);
> +#ifdef CONFIG_PM
> + platform_driver_unregister(&gsmi_driver_info);
> +#endif
> return ret;
> }
>
> @@ -1037,6 +1040,9 @@ static void __exit gsmi_exit(void)
> gsmi_buf_free(gsmi_dev.name_buf);
> dma_pool_destroy(gsmi_dev.dma_pool);
> platform_device_unregister(gsmi_dev.pdev);
> +#ifdef CONFIG_PM
> + platform_driver_unregister(&gsmi_driver_info);
Why the #ifdef here? Why does PM change things?
#ifdefs in .c code is really frowned on.
thanks,
greg k-h
On Sat, 2019-11-16 at 14:40 +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 15, 2019 at 02:48:38PM +0100,
> [email protected] wrote:
> > From: Arthur Heymans <[email protected]>
> >
> > Fix a bug where the kernel module couldn't be loaded after
> > unloading,
> > as the platform driver wasn't released on exit.
> >
> > Signed-off-by: Arthur Heymans <[email protected]>
> > ---
> > drivers/firmware/google/gsmi.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/firmware/google/gsmi.c
> > b/drivers/firmware/google/gsmi.c
> > index edaa4e5d84ad..974c769b75cf 100644
> > --- a/drivers/firmware/google/gsmi.c
> > +++ b/drivers/firmware/google/gsmi.c
> > @@ -1016,6 +1016,9 @@ static __init int gsmi_init(void)
> > dma_pool_destroy(gsmi_dev.dma_pool);
> > platform_device_unregister(gsmi_dev.pdev);
> > pr_info("gsmi: failed to load: %d\n", ret);
> > +#ifdef CONFIG_PM
> > + platform_driver_unregister(&gsmi_driver_info);
> > +#endif
> > return ret;
> > }
> >
> > @@ -1037,6 +1040,9 @@ static void __exit gsmi_exit(void)
> > gsmi_buf_free(gsmi_dev.name_buf);
> > dma_pool_destroy(gsmi_dev.dma_pool);
> > platform_device_unregister(gsmi_dev.pdev);
> > +#ifdef CONFIG_PM
> > + platform_driver_unregister(&gsmi_driver_info);
>
> Why the #ifdef here? Why does PM change things?
>
The driver is only registered if CONFIG_PM is selected, thus it only
needs to be unregistered if CONFIG_PM is selected.
See 8942b2d5094b0 for reference.
Regards,
Patrick
> #ifdefs in .c code is really frowned on.
>
> thanks,
>
> greg k-h
On Mon, Nov 18, 2019 at 08:59:32AM +0100, [email protected] wrote:
> On Sat, 2019-11-16 at 14:40 +0100, Greg Kroah-Hartman wrote:
> > On Fri, Nov 15, 2019 at 02:48:38PM +0100,
> > [email protected] wrote:
> > > From: Arthur Heymans <[email protected]>
> > >
> > > Fix a bug where the kernel module couldn't be loaded after
> > > unloading,
> > > as the platform driver wasn't released on exit.
> > >
> > > Signed-off-by: Arthur Heymans <[email protected]>
> > > ---
> > > drivers/firmware/google/gsmi.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/firmware/google/gsmi.c
> > > b/drivers/firmware/google/gsmi.c
> > > index edaa4e5d84ad..974c769b75cf 100644
> > > --- a/drivers/firmware/google/gsmi.c
> > > +++ b/drivers/firmware/google/gsmi.c
> > > @@ -1016,6 +1016,9 @@ static __init int gsmi_init(void)
> > > dma_pool_destroy(gsmi_dev.dma_pool);
> > > platform_device_unregister(gsmi_dev.pdev);
> > > pr_info("gsmi: failed to load: %d\n", ret);
> > > +#ifdef CONFIG_PM
> > > + platform_driver_unregister(&gsmi_driver_info);
> > > +#endif
> > > return ret;
> > > }
> > >
> > > @@ -1037,6 +1040,9 @@ static void __exit gsmi_exit(void)
> > > gsmi_buf_free(gsmi_dev.name_buf);
> > > dma_pool_destroy(gsmi_dev.dma_pool);
> > > platform_device_unregister(gsmi_dev.pdev);
> > > +#ifdef CONFIG_PM
> > > + platform_driver_unregister(&gsmi_driver_info);
> >
> > Why the #ifdef here? Why does PM change things?
> >
> The driver is only registered if CONFIG_PM is selected, thus it only
> needs to be unregistered if CONFIG_PM is selected.
>
> See 8942b2d5094b0 for reference.
That is a "fun" abuse of the platform driver interface :(
Why not just have this registration of your device for the "normal"
device your driver binds to? Why create a special platform device
instead? That means you have double the number of "devices" for your
single real device.
thanks,
greg k-h
On Sat, 2019-11-16 at 14:38 +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 15, 2019 at 02:48:37PM +0100,
> [email protected] wrote:
> > From: Patrick Rudolph <[email protected]>
> >
> > Fix a bug where the kernel module can't be loaded after it has been
> > unloaded as the devices are still present and conflicting with the
> > to be created coreboot devices.
> >
> > Signed-off-by: Patrick Rudolph <[email protected]>
> > ---
> > drivers/firmware/google/coreboot_table.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/firmware/google/coreboot_table.c
> > b/drivers/firmware/google/coreboot_table.c
> > index 8d132e4f008a..88c6545bebf4 100644
> > --- a/drivers/firmware/google/coreboot_table.c
> > +++ b/drivers/firmware/google/coreboot_table.c
> > @@ -163,8 +163,14 @@ static int coreboot_table_probe(struct
> > platform_device *pdev)
> > return ret;
> > }
> >
> > +static int __cb_dev_unregister(struct device *dev, void *dummy)
> > +{
> > + device_unregister(dev);
>
> Did you build this patch???
>
> Did it work when you tested it? How?
>
It builds without a warning and is working.
It looks like there's no -Werror=return-type in the kernel's Makefile,
which is kind of odd as this is kind of undefined behaviour in C...
Will fix.
> Please fix up,
>
> greg k-h