RE: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper

[<Mario_Limonciello@Dell.com>]

> -----Original Message-----
> From: Ming Lei [mailto:ming.lei@canonical.com]
> Sent: Wednesday, June 1, 2016 9:24 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper
> 
> On Wed, Jun 1, 2016 at 11:35 PM,  <Mario_Limonciello@dell.com> wrote:
> >> -----Original Message-----
> >> From: Limonciello, Mario
> >> Sent: Monday, May 16, 2016 1:28 PM
> >> To: ming.lei@canonical.com
> >> Cc: LKML <linux-kernel@vger.kernel.org>; Limonciello, Mario
> >> <Mario_Limonciello@Dell.com>
> >> Subject: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper
> >>
> >> when loading firmware dell_rbu previously would allow a userspace
> >> application to craft the payload after dell_rbu was loaded and abuse
> >> the udev userspace API.
> >>
> >> Instead require the payload to be crafted and placed in
> >> /lib/firmware/dell_rbu ahead of time.
> >>
> >> This adjusts dell_rbu to immediately load the firmware from
> >> /lib/firmware/dell_rbu when "init" is passed into image_type using
> >> the kernel helper.
> >>
> >> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> >> ---
> >>  drivers/firmware/Kconfig    | 1 -
> >>  drivers/firmware/dell_rbu.c | 2 +-
> >>  2 files changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> >> index
> >> 6664f11..85afe59 100644
> >> --- a/drivers/firmware/Kconfig
> >> +++ b/drivers/firmware/Kconfig
> >> @@ -86,7 +86,6 @@ config DELL_RBU
> >>       tristate "BIOS update support for DELL systems via sysfs"
> >>       depends on X86
> >>       select FW_LOADER
> >> -     select FW_LOADER_USER_HELPER
> >>       help
> >>        Say m if you want to have the option of updating the BIOS for your
> >>        DELL system. Note you need a Dell OpenManage or Dell Update
> >> package (DUP) diff --git a/drivers/firmware/dell_rbu.c
> >> b/drivers/firmware/dell_rbu.c index 2f452f1..77b2a77 100644
> >> --- a/drivers/firmware/dell_rbu.c
> >> +++ b/drivers/firmware/dell_rbu.c
> >> @@ -620,7 +620,7 @@ static ssize_t write_rbu_image_type(struct file
> >> *filp, struct kobject *kobj,
> >>               if (!rbu_data.entry_created) {
> >>                       spin_unlock(&rbu_data.lock);
> >>                       req_firm_rc =
> >> request_firmware_nowait(THIS_MODULE,
> >> -                             FW_ACTION_NOHOTPLUG, "dell_rbu",
> >> +                             FW_ACTION_HOTPLUG, "dell_rbu",
> >>                               &rbu_device->dev, GFP_KERNEL, &context,
> >>                               callbackfn_rbu);
> >>                       if (req_firm_rc) {
> >> --
> >> 2.7.4
> >
> > Hi Ming,
> >
> > Could you comment on these patches?  There isn't currently a subsystem
> maintainer for dell_rbu, so I think you are the most qualified to pull these in.
> They have the best implications for you since you can drop all that fallback code
> now too.
> 
> Hi Mario,
> 
> I am fine for the 1st two patches.
> 
> But IMO it isn't good to drop support for FW_LOADER_USER_HELPER_FALLBACK
> inside kernel now because udev isn't used everywhere, and not every distribution
> always put the firmware files under the default builtin path.
> 
> Thanks,

Ming,

OK that's fine to me.  
I just thought it would simplify your code, didn't realize anything there were distros still putting firmware in odd places or not using udev.
If you could include the first two patches in your tree for next kernel I would appreciate.  

Thanks,