Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file
On Wed, Feb 28, 2018 at 08:11:31AM +0100, Greg KH wrote: > On Tue, Feb 27, 2018 at 09:33:28PM -0800, Kees Cook wrote: > > On Tue, Feb 27, 2018 at 5:28 PM, Luis R. Rodriguez > > wrote: > > > On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: > > >> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez > > >> wrote: > > >> > The firmware fallback code is optional. Split that code out to help > > >> > distinguish the fallback functionlity from othere core firmware loader > > >> > features. This should make it easier to maintain and review code > > >> > changes. > > >> > > > >> > The reason for keeping the configuration onto a table which is built-in > > >> > if you enable firmware loading is so that we can later enable the > > >> > kernel > > >> > after subsequent patches to tweak this configuration, even if the > > >> > firmware loader is modular. > > >> > > > >> > This introduces no functional changes. > > >> > > > >> > Signed-off-by: Luis R. Rodriguez > > >> > --- > > >> > drivers/base/Makefile | 4 +- > > >> > drivers/base/firmware_fallback.c | 661 > > >> > +++ > > >> > drivers/base/firmware_fallback.h | 61 +++ > > >> > drivers/base/firmware_fallback_table.c | 29 ++ > > >> > drivers/base/firmware_loader.c | 803 > > >> > + > > >> > drivers/base/firmware_loader.h | 115 + > > >> > 6 files changed, 874 insertions(+), 799 deletions(-) > > >> > create mode 100644 drivers/base/firmware_fallback.c > > >> > create mode 100644 drivers/base/firmware_fallback.h > > >> > create mode 100644 drivers/base/firmware_fallback_table.c > > >> > create mode 100644 drivers/base/firmware_loader.h > > >> > > >> Does it make sense to have a separate subdirectory for firmware > > >> instead? I did this _ stuff with lkdtm and have regretted it. (I'm > > >> likely going to make a subdirectory for it this cycle...) > > > > > > Sure, the only eyesore is that drivers/base/firmware.c what is that for? > > > > > > drivers/base/firmware_loader/ ok? > > > > Yeah? Seems fine to me. Greg, do you have thoughts on this? > > I don't care :) Alright, this was much easier done as a separate patch after. Luis
Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file
On Tue, Feb 27, 2018 at 09:33:28PM -0800, Kees Cook wrote: > On Tue, Feb 27, 2018 at 5:28 PM, Luis R. Rodriguez wrote: > > On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: > >> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez > >> wrote: > >> > The firmware fallback code is optional. Split that code out to help > >> > distinguish the fallback functionlity from othere core firmware loader > >> > features. This should make it easier to maintain and review code > >> > changes. > >> > > >> > The reason for keeping the configuration onto a table which is built-in > >> > if you enable firmware loading is so that we can later enable the kernel > >> > after subsequent patches to tweak this configuration, even if the > >> > firmware loader is modular. > >> > > >> > This introduces no functional changes. > >> > > >> > Signed-off-by: Luis R. Rodriguez > >> > --- > >> > drivers/base/Makefile | 4 +- > >> > drivers/base/firmware_fallback.c | 661 +++ > >> > drivers/base/firmware_fallback.h | 61 +++ > >> > drivers/base/firmware_fallback_table.c | 29 ++ > >> > drivers/base/firmware_loader.c | 803 > >> > + > >> > drivers/base/firmware_loader.h | 115 + > >> > 6 files changed, 874 insertions(+), 799 deletions(-) > >> > create mode 100644 drivers/base/firmware_fallback.c > >> > create mode 100644 drivers/base/firmware_fallback.h > >> > create mode 100644 drivers/base/firmware_fallback_table.c > >> > create mode 100644 drivers/base/firmware_loader.h > >> > >> Does it make sense to have a separate subdirectory for firmware > >> instead? I did this _ stuff with lkdtm and have regretted it. (I'm > >> likely going to make a subdirectory for it this cycle...) > > > > Sure, the only eyesore is that drivers/base/firmware.c what is that for? > > > > drivers/base/firmware_loader/ ok? > > Yeah? Seems fine to me. Greg, do you have thoughts on this? I don't care :)
Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file
On Tue, Feb 27, 2018 at 5:28 PM, Luis R. Rodriguez wrote: > On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: >> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez wrote: >> > The firmware fallback code is optional. Split that code out to help >> > distinguish the fallback functionlity from othere core firmware loader >> > features. This should make it easier to maintain and review code >> > changes. >> > >> > The reason for keeping the configuration onto a table which is built-in >> > if you enable firmware loading is so that we can later enable the kernel >> > after subsequent patches to tweak this configuration, even if the >> > firmware loader is modular. >> > >> > This introduces no functional changes. >> > >> > Signed-off-by: Luis R. Rodriguez >> > --- >> > drivers/base/Makefile | 4 +- >> > drivers/base/firmware_fallback.c | 661 +++ >> > drivers/base/firmware_fallback.h | 61 +++ >> > drivers/base/firmware_fallback_table.c | 29 ++ >> > drivers/base/firmware_loader.c | 803 >> > + >> > drivers/base/firmware_loader.h | 115 + >> > 6 files changed, 874 insertions(+), 799 deletions(-) >> > create mode 100644 drivers/base/firmware_fallback.c >> > create mode 100644 drivers/base/firmware_fallback.h >> > create mode 100644 drivers/base/firmware_fallback_table.c >> > create mode 100644 drivers/base/firmware_loader.h >> >> Does it make sense to have a separate subdirectory for firmware >> instead? I did this _ stuff with lkdtm and have regretted it. (I'm >> likely going to make a subdirectory for it this cycle...) > > Sure, the only eyesore is that drivers/base/firmware.c what is that for? > > drivers/base/firmware_loader/ ok? Yeah? Seems fine to me. Greg, do you have thoughts on this? -Kees -- Kees Cook Pixel Security
Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file
On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez wrote: > > The firmware fallback code is optional. Split that code out to help > > distinguish the fallback functionlity from othere core firmware loader > > features. This should make it easier to maintain and review code > > changes. > > > > The reason for keeping the configuration onto a table which is built-in > > if you enable firmware loading is so that we can later enable the kernel > > after subsequent patches to tweak this configuration, even if the > > firmware loader is modular. > > > > This introduces no functional changes. > > > > Signed-off-by: Luis R. Rodriguez > > --- > > drivers/base/Makefile | 4 +- > > drivers/base/firmware_fallback.c | 661 +++ > > drivers/base/firmware_fallback.h | 61 +++ > > drivers/base/firmware_fallback_table.c | 29 ++ > > drivers/base/firmware_loader.c | 803 > > + > > drivers/base/firmware_loader.h | 115 + > > 6 files changed, 874 insertions(+), 799 deletions(-) > > create mode 100644 drivers/base/firmware_fallback.c > > create mode 100644 drivers/base/firmware_fallback.h > > create mode 100644 drivers/base/firmware_fallback_table.c > > create mode 100644 drivers/base/firmware_loader.h > > Does it make sense to have a separate subdirectory for firmware > instead? I did this _ stuff with lkdtm and have regretted it. (I'm > likely going to make a subdirectory for it this cycle...) Sure, the only eyesore is that drivers/base/firmware.c what is that for? drivers/base/firmware_loader/ ok? Luis
Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file
On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez wrote: > The firmware fallback code is optional. Split that code out to help > distinguish the fallback functionlity from othere core firmware loader > features. This should make it easier to maintain and review code > changes. > > The reason for keeping the configuration onto a table which is built-in > if you enable firmware loading is so that we can later enable the kernel > after subsequent patches to tweak this configuration, even if the > firmware loader is modular. > > This introduces no functional changes. > > Signed-off-by: Luis R. Rodriguez > --- > drivers/base/Makefile | 4 +- > drivers/base/firmware_fallback.c | 661 +++ > drivers/base/firmware_fallback.h | 61 +++ > drivers/base/firmware_fallback_table.c | 29 ++ > drivers/base/firmware_loader.c | 803 > + > drivers/base/firmware_loader.h | 115 + > 6 files changed, 874 insertions(+), 799 deletions(-) > create mode 100644 drivers/base/firmware_fallback.c > create mode 100644 drivers/base/firmware_fallback.h > create mode 100644 drivers/base/firmware_fallback_table.c > create mode 100644 drivers/base/firmware_loader.h Does it make sense to have a separate subdirectory for firmware instead? I did this _ stuff with lkdtm and have regretted it. (I'm likely going to make a subdirectory for it this cycle...) -Kees -- Kees Cook Pixel Security
