[v4,1/5] dell-laptop: move dell_smi_error() to dell-smbios

Commit Message

Michał Kępień Feb. 24, 2016, 7:20 a.m. UTC

The dell_smi_error() method could be used by modules other than
dell-laptop for convenient translation of SMBIOS request errors into
errno values.  Thus, move it to dell-smbios.

Signed-off-by: Micha? K?pie? <[e-mail]>
---
 drivers/platform/x86/dell-laptop.c |   14 --------------
 drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 3 files changed, 18 insertions(+), 14 deletions(-)

Comments

Pali Rohár Feb. 29, 2016, 12:52 p.m. UTC | #1

On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> The dell_smi_error() method could be used by modules other than
> dell-laptop for convenient translation of SMBIOS request errors into
> errno values.  Thus, move it to dell-smbios.
> 
> Signed-off-by: Micha? K?pie? <[e-mail]>
> ---
>  drivers/platform/x86/dell-laptop.c |   14 --------------
>  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |    2 ++
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 76064c8..cbafb95 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> -static inline int dell_smi_error(int value)
> -{
> -	switch (value) {
> -	case 0: /* Completed successfully */
> -		return 0;
> -	case -1: /* Completed with error */
> -		return -EIO;
> -	case -2: /* Function not supported */
> -		return -ENXIO;
> -	default: /* Unknown error */
> -		return -EINVAL;
> -	}
> -}
> -
>  /*
>   * Derived from information in smbios-wireless-ctl:
>   *
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 2a4992a..942572f 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -16,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/dmi.h>
> +#include <linux/err.h>
>  #include <linux/gfp.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> @@ -39,6 +40,21 @@ static int da_command_code;
>  static int da_num_tokens;
>  static struct calling_interface_token *da_tokens;
>  
> +int dell_smi_error(int value)
> +{
> +	switch (value) {
> +	case 0: /* Completed successfully */
> +		return 0;
> +	case -1: /* Completed with error */
> +		return -EIO;
> +	case -2: /* Function not supported */
> +		return -ENXIO;
> +	default: /* Unknown error */
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(dell_smi_error);
> +
>  struct calling_interface_buffer *dell_smbios_get_buffer(void)
>  {
>  	mutex_lock(&buffer_mutex);
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index 4f69b16..52febe6 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -35,6 +35,8 @@ struct calling_interface_token {
>  	};
>  };
>  
> +int dell_smi_error(int value);
> +
>  struct calling_interface_buffer *dell_smbios_get_buffer(void);
>  void dell_smbios_clear_buffer(void);
>  void dell_smbios_release_buffer(void);

And... here what about inline vs EXPORT_SYMBOL function? Just asking...

Michał Kępień Feb. 29, 2016, 8:22 p.m. UTC | #2

> On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> > The dell_smi_error() method could be used by modules other than
> > dell-laptop for convenient translation of SMBIOS request errors into
> > errno values.  Thus, move it to dell-smbios.
> > 
> > Signed-off-by: Micha? K?pie? <[e-mail]>
> > ---
> >  drivers/platform/x86/dell-laptop.c |   14 --------------
> >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> >  drivers/platform/x86/dell-smbios.h |    2 ++
> >  3 files changed, 18 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> > index 76064c8..cbafb95 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
> >  	{ }
> >  };
> >  
> > -static inline int dell_smi_error(int value)
> > -{
> > -	switch (value) {
> > -	case 0: /* Completed successfully */
> > -		return 0;
> > -	case -1: /* Completed with error */
> > -		return -EIO;
> > -	case -2: /* Function not supported */
> > -		return -ENXIO;
> > -	default: /* Unknown error */
> > -		return -EINVAL;
> > -	}
> > -}
> > -
> >  /*
> >   * Derived from information in smbios-wireless-ctl:
> >   *
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> > index 2a4992a..942572f 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/dmi.h>
> > +#include <linux/err.h>
> >  #include <linux/gfp.h>
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> > @@ -39,6 +40,21 @@ static int da_command_code;
> >  static int da_num_tokens;
> >  static struct calling_interface_token *da_tokens;
> >  
> > +int dell_smi_error(int value)
> > +{
> > +	switch (value) {
> > +	case 0: /* Completed successfully */
> > +		return 0;
> > +	case -1: /* Completed with error */
> > +		return -EIO;
> > +	case -2: /* Function not supported */
> > +		return -ENXIO;
> > +	default: /* Unknown error */
> > +		return -EINVAL;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > +
> >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> >  {
> >  	mutex_lock(&buffer_mutex);
> > diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> > index 4f69b16..52febe6 100644
> > --- a/drivers/platform/x86/dell-smbios.h
> > +++ b/drivers/platform/x86/dell-smbios.h
> > @@ -35,6 +35,8 @@ struct calling_interface_token {
> >  	};
> >  };
> >  
> > +int dell_smi_error(int value);
> > +
> >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> >  void dell_smbios_clear_buffer(void);
> >  void dell_smbios_release_buffer(void);
> 
> And... here what about inline vs EXPORT_SYMBOL function? Just asking...

Well, what about it? :)  The commit message is pretty explicit in
describing what happens here, i.e. a previously static function is moved
to another module so that it can be reused.  Thus, keeping the inline
keyword makes no sense.  What exactly is your concern?

Pali Rohár Feb. 29, 2016, 8:24 p.m. UTC | #3

On Monday 29 February 2016 21:22:54 Micha? K?pie? wrote:
> > On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> > > The dell_smi_error() method could be used by modules other than
> > > dell-laptop for convenient translation of SMBIOS request errors
> > > into errno values.  Thus, move it to dell-smbios.
> > > 
> > > Signed-off-by: Micha? K?pie? <[e-mail]>
> > > ---
> > > 
> > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > 100644
> > > --- a/drivers/platform/x86/dell-laptop.c
> > > +++ b/drivers/platform/x86/dell-laptop.c
> > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > dell_quirks[] __initconst = {
> > > 
> > >  	{ }
> > >  
> > >  };
> > > 
> > > -static inline int dell_smi_error(int value)
> > > -{
> > > -	switch (value) {
> > > -	case 0: /* Completed successfully */
> > > -		return 0;
> > > -	case -1: /* Completed with error */
> > > -		return -EIO;
> > > -	case -2: /* Function not supported */
> > > -		return -ENXIO;
> > > -	default: /* Unknown error */
> > > -		return -EINVAL;
> > > -	}
> > > -}
> > > -
> > > 
> > >  /*
> > >  
> > >   * Derived from information in smbios-wireless-ctl:
> > >   *
> > > 
> > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > 100644
> > > --- a/drivers/platform/x86/dell-smbios.c
> > > +++ b/drivers/platform/x86/dell-smbios.c
> > > @@ -16,6 +16,7 @@
> > > 
> > >  #include <linux/kernel.h>
> > >  #include <linux/module.h>
> > >  #include <linux/dmi.h>
> > > 
> > > +#include <linux/err.h>
> > > 
> > >  #include <linux/gfp.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/slab.h>
> > > 
> > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > 
> > >  static int da_num_tokens;
> > >  static struct calling_interface_token *da_tokens;
> > > 
> > > +int dell_smi_error(int value)
> > > +{
> > > +	switch (value) {
> > > +	case 0: /* Completed successfully */
> > > +		return 0;
> > > +	case -1: /* Completed with error */
> > > +		return -EIO;
> > > +	case -2: /* Function not supported */
> > > +		return -ENXIO;
> > > +	default: /* Unknown error */
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > +
> > > 
> > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > >  {
> > >  
> > >  	mutex_lock(&buffer_mutex);
> > > 
> > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > 100644
> > > --- a/drivers/platform/x86/dell-smbios.h
> > > +++ b/drivers/platform/x86/dell-smbios.h
> > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > 
> > >  	};
> > >  
> > >  };
> > > 
> > > +int dell_smi_error(int value);
> > > +
> > > 
> > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > >  void dell_smbios_clear_buffer(void);
> > >  void dell_smbios_release_buffer(void);
> > 
> > And... here what about inline vs EXPORT_SYMBOL function? Just
> > asking...
> 
> Well, what about it? :)  The commit message is pretty explicit in
> describing what happens here, i.e. a previously static function is
> moved to another module so that it can be reused.  Thus, keeping the
> inline keyword makes no sense.  What exactly is your concern?

Just asking if this function should be or not be inline (of course in 
header file, not in module .c).

Michał Kępień Feb. 29, 2016, 8:41 p.m. UTC | #4

> On Monday 29 February 2016 21:22:54 Micha? K?pie? wrote:
> > > On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> > > > The dell_smi_error() method could be used by modules other than
> > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > into errno values.  Thus, move it to dell-smbios.
> > > > 
> > > > Signed-off-by: Micha? K?pie? <[e-mail]>
> > > > ---
> > > > 
> > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > 100644
> > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > dell_quirks[] __initconst = {
> > > > 
> > > >  	{ }
> > > >  
> > > >  };
> > > > 
> > > > -static inline int dell_smi_error(int value)
> > > > -{
> > > > -	switch (value) {
> > > > -	case 0: /* Completed successfully */
> > > > -		return 0;
> > > > -	case -1: /* Completed with error */
> > > > -		return -EIO;
> > > > -	case -2: /* Function not supported */
> > > > -		return -ENXIO;
> > > > -	default: /* Unknown error */
> > > > -		return -EINVAL;
> > > > -	}
> > > > -}
> > > > -
> > > > 
> > > >  /*
> > > >  
> > > >   * Derived from information in smbios-wireless-ctl:
> > > >   *
> > > > 
> > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > 100644
> > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > @@ -16,6 +16,7 @@
> > > > 
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/dmi.h>
> > > > 
> > > > +#include <linux/err.h>
> > > > 
> > > >  #include <linux/gfp.h>
> > > >  #include <linux/mutex.h>
> > > >  #include <linux/slab.h>
> > > > 
> > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > 
> > > >  static int da_num_tokens;
> > > >  static struct calling_interface_token *da_tokens;
> > > > 
> > > > +int dell_smi_error(int value)
> > > > +{
> > > > +	switch (value) {
> > > > +	case 0: /* Completed successfully */
> > > > +		return 0;
> > > > +	case -1: /* Completed with error */
> > > > +		return -EIO;
> > > > +	case -2: /* Function not supported */
> > > > +		return -ENXIO;
> > > > +	default: /* Unknown error */
> > > > +		return -EINVAL;
> > > > +	}
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > +
> > > > 
> > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > >  {
> > > >  
> > > >  	mutex_lock(&buffer_mutex);
> > > > 
> > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > 100644
> > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > 
> > > >  	};
> > > >  
> > > >  };
> > > > 
> > > > +int dell_smi_error(int value);
> > > > +
> > > > 
> > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > >  void dell_smbios_clear_buffer(void);
> > > >  void dell_smbios_release_buffer(void);
> > > 
> > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > asking...
> > 
> > Well, what about it? :)  The commit message is pretty explicit in
> > describing what happens here, i.e. a previously static function is
> > moved to another module so that it can be reused.  Thus, keeping the
> > inline keyword makes no sense.  What exactly is your concern?
> 
> Just asking if this function should be or not be inline (of course in 
> header file, not in module .c).

If you mark a function as inline in the header file, you have to provide
its definition, otherwise you'll get a compilation error.  Given that
this is in no way performance-critical code, I see no point in
clobbering the header file with the body of this function.

Darren Hart Feb. 29, 2016, 10:50 p.m. UTC | #5

On Mon, Feb 29, 2016 at 09:41:36PM +0100, Micha? K?pie? wrote:
> > On Monday 29 February 2016 21:22:54 Micha? K?pie? wrote:
> > > > On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> > > > > The dell_smi_error() method could be used by modules other than
> > > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > > into errno values.  Thus, move it to dell-smbios.
> > > > > 
> > > > > Signed-off-by: Micha? K?pie? <[e-mail]>
> > > > > ---
> > > > > 
> > > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > > 100644
> > > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > > dell_quirks[] __initconst = {
> > > > > 
> > > > >  	{ }
> > > > >  
> > > > >  };
> > > > > 
> > > > > -static inline int dell_smi_error(int value)
> > > > > -{
> > > > > -	switch (value) {
> > > > > -	case 0: /* Completed successfully */
> > > > > -		return 0;
> > > > > -	case -1: /* Completed with error */
> > > > > -		return -EIO;
> > > > > -	case -2: /* Function not supported */
> > > > > -		return -ENXIO;
> > > > > -	default: /* Unknown error */
> > > > > -		return -EINVAL;
> > > > > -	}
> > > > > -}
> > > > > -
> > > > > 
> > > > >  /*
> > > > >  
> > > > >   * Derived from information in smbios-wireless-ctl:
> > > > >   *
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > > 100644
> > > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > > @@ -16,6 +16,7 @@
> > > > > 
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/dmi.h>
> > > > > 
> > > > > +#include <linux/err.h>
> > > > > 
> > > > >  #include <linux/gfp.h>
> > > > >  #include <linux/mutex.h>
> > > > >  #include <linux/slab.h>
> > > > > 
> > > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > > 
> > > > >  static int da_num_tokens;
> > > > >  static struct calling_interface_token *da_tokens;
> > > > > 
> > > > > +int dell_smi_error(int value)
> > > > > +{
> > > > > +	switch (value) {
> > > > > +	case 0: /* Completed successfully */
> > > > > +		return 0;
> > > > > +	case -1: /* Completed with error */
> > > > > +		return -EIO;
> > > > > +	case -2: /* Function not supported */
> > > > > +		return -ENXIO;
> > > > > +	default: /* Unknown error */
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > > +
> > > > > 
> > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > > >  {
> > > > >  
> > > > >  	mutex_lock(&buffer_mutex);
> > > > > 
> > > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > > 100644
> > > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > > 
> > > > >  	};
> > > > >  
> > > > >  };
> > > > > 
> > > > > +int dell_smi_error(int value);
> > > > > +
> > > > > 
> > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > > >  void dell_smbios_clear_buffer(void);
> > > > >  void dell_smbios_release_buffer(void);
> > > > 
> > > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > > asking...
> > > 
> > > Well, what about it? :)  The commit message is pretty explicit in
> > > describing what happens here, i.e. a previously static function is
> > > moved to another module so that it can be reused.  Thus, keeping the
> > > inline keyword makes no sense.  What exactly is your concern?
> > 
> > Just asking if this function should be or not be inline (of course in 
> > header file, not in module .c).
> 
> If you mark a function as inline in the header file, you have to provide
> its definition, otherwise you'll get a compilation error.  Given that
> this is in no way performance-critical code, I see no point in
> clobbering the header file with the body of this function.

Agreed, please leave it as is.

For a discussion on inline, please see:
CodingStyle: Chapter 15: The inline disease

Michał Kępień March 2, 2016, 11:49 a.m. UTC | #6

> On Mon, Feb 29, 2016 at 09:41:36PM +0100, Micha? K?pie? wrote:
> > > On Monday 29 February 2016 21:22:54 Micha? K?pie? wrote:
> > > > > On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> > > > > > The dell_smi_error() method could be used by modules other than
> > > > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > > > into errno values.  Thus, move it to dell-smbios.
> > > > > > 
> > > > > > Signed-off-by: Micha? K?pie? <[e-mail]>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > > > 100644
> > > > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > > > dell_quirks[] __initconst = {
> > > > > > 
> > > > > >  	{ }
> > > > > >  
> > > > > >  };
> > > > > > 
> > > > > > -static inline int dell_smi_error(int value)
> > > > > > -{
> > > > > > -	switch (value) {
> > > > > > -	case 0: /* Completed successfully */
> > > > > > -		return 0;
> > > > > > -	case -1: /* Completed with error */
> > > > > > -		return -EIO;
> > > > > > -	case -2: /* Function not supported */
> > > > > > -		return -ENXIO;
> > > > > > -	default: /* Unknown error */
> > > > > > -		return -EINVAL;
> > > > > > -	}
> > > > > > -}
> > > > > > -
> > > > > > 
> > > > > >  /*
> > > > > >  
> > > > > >   * Derived from information in smbios-wireless-ctl:
> > > > > >   *
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > > > 100644
> > > > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > > > @@ -16,6 +16,7 @@
> > > > > > 
> > > > > >  #include <linux/kernel.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/dmi.h>
> > > > > > 
> > > > > > +#include <linux/err.h>
> > > > > > 
> > > > > >  #include <linux/gfp.h>
> > > > > >  #include <linux/mutex.h>
> > > > > >  #include <linux/slab.h>
> > > > > > 
> > > > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > > > 
> > > > > >  static int da_num_tokens;
> > > > > >  static struct calling_interface_token *da_tokens;
> > > > > > 
> > > > > > +int dell_smi_error(int value)
> > > > > > +{
> > > > > > +	switch (value) {
> > > > > > +	case 0: /* Completed successfully */
> > > > > > +		return 0;
> > > > > > +	case -1: /* Completed with error */
> > > > > > +		return -EIO;
> > > > > > +	case -2: /* Function not supported */
> > > > > > +		return -ENXIO;
> > > > > > +	default: /* Unknown error */
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > > > +
> > > > > > 
> > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > > > >  {
> > > > > >  
> > > > > >  	mutex_lock(&buffer_mutex);
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > > > 100644
> > > > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > > > 
> > > > > >  	};
> > > > > >  
> > > > > >  };
> > > > > > 
> > > > > > +int dell_smi_error(int value);
> > > > > > +
> > > > > > 
> > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > > > >  void dell_smbios_clear_buffer(void);
> > > > > >  void dell_smbios_release_buffer(void);
> > > > > 
> > > > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > > > asking...
> > > > 
> > > > Well, what about it? :)  The commit message is pretty explicit in
> > > > describing what happens here, i.e. a previously static function is
> > > > moved to another module so that it can be reused.  Thus, keeping the
> > > > inline keyword makes no sense.  What exactly is your concern?
> > > 
> > > Just asking if this function should be or not be inline (of course in 
> > > header file, not in module .c).
> > 
> > If you mark a function as inline in the header file, you have to provide
> > its definition, otherwise you'll get a compilation error.  Given that
> > this is in no way performance-critical code, I see no point in
> > clobbering the header file with the body of this function.
> 
> Agreed, please leave it as is.

Pali,

Given Darren's remark, are you okay with acking this patch?

Pali Rohár March 3, 2016, 11:38 a.m. UTC | #7

On Wednesday 02 March 2016 12:49:39 Micha? K?pie? wrote:
> > On Mon, Feb 29, 2016 at 09:41:36PM +0100, Micha? K?pie? wrote:
> > > > On Monday 29 February 2016 21:22:54 Micha? K?pie? wrote:
> > > > > > On Wednesday 24 February 2016 08:20:11 Micha? K?pie? wrote:
> > > > > > > The dell_smi_error() method could be used by modules other than
> > > > > > > dell-laptop for convenient translation of SMBIOS request errors
> > > > > > > into errno values.  Thus, move it to dell-smbios.
> > > > > > > 
> > > > > > > Signed-off-by: Micha? K?pie? <[e-mail]>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/platform/x86/dell-laptop.c |   14 --------------
> > > > > > >  drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
> > > > > > >  drivers/platform/x86/dell-smbios.h |    2 ++
> > > > > > >  3 files changed, 18 insertions(+), 14 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/dell-laptop.c
> > > > > > > b/drivers/platform/x86/dell-laptop.c index 76064c8..cbafb95
> > > > > > > 100644
> > > > > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > > > > @@ -273,20 +273,6 @@ static const struct dmi_system_id
> > > > > > > dell_quirks[] __initconst = {
> > > > > > > 
> > > > > > >  	{ }
> > > > > > >  
> > > > > > >  };
> > > > > > > 
> > > > > > > -static inline int dell_smi_error(int value)
> > > > > > > -{
> > > > > > > -	switch (value) {
> > > > > > > -	case 0: /* Completed successfully */
> > > > > > > -		return 0;
> > > > > > > -	case -1: /* Completed with error */
> > > > > > > -		return -EIO;
> > > > > > > -	case -2: /* Function not supported */
> > > > > > > -		return -ENXIO;
> > > > > > > -	default: /* Unknown error */
> > > > > > > -		return -EINVAL;
> > > > > > > -	}
> > > > > > > -}
> > > > > > > -
> > > > > > > 
> > > > > > >  /*
> > > > > > >  
> > > > > > >   * Derived from information in smbios-wireless-ctl:
> > > > > > >   *
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/dell-smbios.c
> > > > > > > b/drivers/platform/x86/dell-smbios.c index 2a4992a..942572f
> > > > > > > 100644
> > > > > > > --- a/drivers/platform/x86/dell-smbios.c
> > > > > > > +++ b/drivers/platform/x86/dell-smbios.c
> > > > > > > @@ -16,6 +16,7 @@
> > > > > > > 
> > > > > > >  #include <linux/kernel.h>
> > > > > > >  #include <linux/module.h>
> > > > > > >  #include <linux/dmi.h>
> > > > > > > 
> > > > > > > +#include <linux/err.h>
> > > > > > > 
> > > > > > >  #include <linux/gfp.h>
> > > > > > >  #include <linux/mutex.h>
> > > > > > >  #include <linux/slab.h>
> > > > > > > 
> > > > > > > @@ -39,6 +40,21 @@ static int da_command_code;
> > > > > > > 
> > > > > > >  static int da_num_tokens;
> > > > > > >  static struct calling_interface_token *da_tokens;
> > > > > > > 
> > > > > > > +int dell_smi_error(int value)
> > > > > > > +{
> > > > > > > +	switch (value) {
> > > > > > > +	case 0: /* Completed successfully */
> > > > > > > +		return 0;
> > > > > > > +	case -1: /* Completed with error */
> > > > > > > +		return -EIO;
> > > > > > > +	case -2: /* Function not supported */
> > > > > > > +		return -ENXIO;
> > > > > > > +	default: /* Unknown error */
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(dell_smi_error);
> > > > > > > +
> > > > > > > 
> > > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void)
> > > > > > >  {
> > > > > > >  
> > > > > > >  	mutex_lock(&buffer_mutex);
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/dell-smbios.h
> > > > > > > b/drivers/platform/x86/dell-smbios.h index 4f69b16..52febe6
> > > > > > > 100644
> > > > > > > --- a/drivers/platform/x86/dell-smbios.h
> > > > > > > +++ b/drivers/platform/x86/dell-smbios.h
> > > > > > > @@ -35,6 +35,8 @@ struct calling_interface_token {
> > > > > > > 
> > > > > > >  	};
> > > > > > >  
> > > > > > >  };
> > > > > > > 
> > > > > > > +int dell_smi_error(int value);
> > > > > > > +
> > > > > > > 
> > > > > > >  struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > > > > > >  void dell_smbios_clear_buffer(void);
> > > > > > >  void dell_smbios_release_buffer(void);
> > > > > > 
> > > > > > And... here what about inline vs EXPORT_SYMBOL function? Just
> > > > > > asking...
> > > > > 
> > > > > Well, what about it? :)  The commit message is pretty explicit in
> > > > > describing what happens here, i.e. a previously static function is
> > > > > moved to another module so that it can be reused.  Thus, keeping the
> > > > > inline keyword makes no sense.  What exactly is your concern?
> > > > 
> > > > Just asking if this function should be or not be inline (of course in 
> > > > header file, not in module .c).
> > > 
> > > If you mark a function as inline in the header file, you have to provide
> > > its definition, otherwise you'll get a compilation error.  Given that
> > > this is in no way performance-critical code, I see no point in
> > > clobbering the header file with the body of this function.
> > 
> > Agreed, please leave it as is.
> 
> Pali,
> 
> Given Darren's remark, are you okay with acking this patch?

I'm just thinking if such function should not be macro as it translate 3
error codes from firmware to standard errno... As external function
exported by EXPORT_SYMBOL looks for me as overkill...

Patch is anyway OK, so add my Reviewed-by.

Patch

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 76064c8..cbafb95 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -273,20 +273,6 @@  static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static inline int dell_smi_error(int value)
-{
-	switch (value) {
-	case 0: /* Completed successfully */
-		return 0;
-	case -1: /* Completed with error */
-		return -EIO;
-	case -2: /* Function not supported */
-		return -ENXIO;
-	default: /* Unknown error */
-		return -EINVAL;
-	}
-}
-
 /*
  * Derived from information in smbios-wireless-ctl:
  *
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 2a4992a..942572f 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -16,6 +16,7 @@ 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -39,6 +40,21 @@  static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(dell_smi_error);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 4f69b16..52febe6 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,8 @@  struct calling_interface_token {
 	};
 };
 
+int dell_smi_error(int value);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);