Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562634 > unrolled thread

Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK()  annotation Greg KH <gregkh@linuxfoundation.org> - 2017-01-19 12:40 +0100
    Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK()  annotation Greg KH <gregkh@linuxfoundation.org> - 2017-01-19 17:20 +0100
      Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK()  annotation "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-19 22:40 +0100
    Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK()  annotation "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-01-19 17:50 +0100

#1562634 — Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

On Thu, Jan 12, 2017 at 06:42:50AM -0800, Luis R. Rodriguez wrote:
> We need to ensure that when driver developers use the custom firmware
> fallback mechanism it was not a copy and paste bug. These use cases on
> upstream drivers are rare, we only have 2 upstream users and its for
> really old drivers. Since valid uses are rare but possible enable a
> white-list for its use, and use this same white-list annotation to refer
> to the documentation covering the custom use case.
> 
> New faulty users can be reported via 0-day now.
> 
> v2: change dependencies on rules make more sense, and fixed
>     context mode
> 
> Cc: Fengguang Wu <fengguang.wu@intel.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> Cc: linux-leds@vger.kernel.org
> Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
>  drivers/firmware/dell_rbu.c                                   | 1 +
>  drivers/leds/leds-lp55xx-common.c                             | 1 +
>  include/linux/firmware.h                                      | 7 +++++++
>  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
>  5 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> index b87a292153c6..73f509a8d2de 100644
> --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> @@ -184,8 +184,11 @@ load firmware for you through a custom path.
>  
>  The custom fallback mechanism can often be enabled by mistake. We currently
>  have only 2 users of it, and little justification to enable it for other users.
> -Since it is a common driver developer mistake to enable it, help police for
> -new users of the custom fallback mechanism with::
> +Since it is a common driver developer mistake to enable it, driver developers
> +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> +use and also refer to the documentation for the custom loading solution.
> +
> +Invalid users of the custom fallback mechanism can be policed using::

Ick, no, why?  Why not just add a checkpatch rule instead?

>  
>          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
>          $ make coccicheck MODE=report
> diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> index 2f452f1f7c8a..3f2aa35bc54d 100644
> --- a/drivers/firmware/dell_rbu.c
> +++ b/drivers/firmware/dell_rbu.c
> @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
>  	return size;
>  }
>  
> +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");

That's a pain.

>  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
>  				    struct bin_attribute *bin_attr,
>  				    char *buffer, loff_t pos, size_t count)
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 5377f22ff994..04161428ee3b 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
>  	release_firmware(chip->fw);
>  }
>  
> +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");

Same here.

>  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
>  {
>  	const char *name = chip->cl->name;
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0ccb8ac..e6ca19c03dcc 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -8,6 +8,13 @@
>  #define FW_ACTION_NOHOTPLUG 0
>  #define FW_ACTION_HOTPLUG 1
>  
> +/*
> + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> + * and so users can also easily search for the documentation for the
> + * respectively needed custom fallback mechanism.
> + */
> +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)

So you really don't need to put anything "valid" in the define argument?
This feels like such a horrid hack, I really don't like it, especially
as we don't do it anywhere else in the kernel, right?  Why start now?

thanks,

greg k-h

[toc] | [next] | [standalone]

#1562872

On Thu, Jan 19, 2017 at 05:08:25PM +0100, Luis R. Rodriguez wrote:
> On Thu, Jan 19, 2017 at 12:31:11PM +0100, Greg KH wrote:
> > On Thu, Jan 12, 2017 at 06:42:50AM -0800, Luis R. Rodriguez wrote:
> > > +Invalid users of the custom fallback mechanism can be policed using::
> > 
> > Ick, no, why?  Why not just add a checkpatch rule instead?
> 
> If its easy to do, how would we do that?

You just patch checkpatch.pl to test for the apis you don't want used
anymore.

> > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > >          $ make coccicheck MODE=report
> > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > --- a/drivers/firmware/dell_rbu.c
> > > +++ b/drivers/firmware/dell_rbu.c
> > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  	return size;
> > >  }
> > >  
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > 
> > That's a pain.
> 
> It is easier with checkpatch?

Yes.  You are modifying the .c code just to "whitelist" an api you want
to check for.  What's the odds that someone who wants to use that api
just cargo-cult-copies this line as well, to prevent the strange warning
they are getting from the build system?  :)

In short, don't worry about this, it's not a big deal nor really worth
the effort.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1563112

On Thu, Jan 19, 2017 at 05:14:36PM +0100, Greg KH wrote:
> In short, don't worry about this, it's not a big deal nor really worth
> the effort.

Fine by me, I'll drop these two patches.

  Luis

[toc] | [prev] | [next] | [standalone]

#1562890

On Thu, Jan 19, 2017 at 12:31:11PM +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 06:42:50AM -0800, Luis R. Rodriguez wrote:
> > +Invalid users of the custom fallback mechanism can be policed using::
> 
> Ick, no, why?  Why not just add a checkpatch rule instead?

If its easy to do, how would we do that?

> >  
> >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> >          $ make coccicheck MODE=report
> > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > --- a/drivers/firmware/dell_rbu.c
> > +++ b/drivers/firmware/dell_rbu.c
> > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  	return size;
> >  }
> >  
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> 
> That's a pain.

It is easier with checkpatch?

> > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > --- a/include/linux/firmware.h
> > +++ b/include/linux/firmware.h
> > @@ -8,6 +8,13 @@
> >  #define FW_ACTION_NOHOTPLUG 0
> >  #define FW_ACTION_HOTPLUG 1
> >  
> > +/*
> > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > + * and so users can also easily search for the documentation for the
> > + * respectively needed custom fallback mechanism.
> > + */
> > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> 
> So you really don't need to put anything "valid" in the define argument?
> This feels like such a horrid hack, I really don't like it, especially
> as we don't do it anywhere else in the kernel, right?  Why start now?

Correct me if I'm wrong but AFAICT we may not have had previous grammatical
policing done before so I think this is a question of how we would want to
handle such type of strategies. Indeed this is just one approach. Using
checkpatch is certainly possible as well, I however think using checkpatch
is a bit more hacky.

I could also just drop this completely but figured its worth discussion.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web