[PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Andy Lutomirski]

Currently they return -1 on error, which will confuse callers if
they try to interpret it as a normal negative error code.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v3:
 - Split out from the series it was in.
 - Use -ENXIO for "there's no DMI".
 - Also fix docs and !DMI case.

Changes from v2:
 - Total rewrite.

drivers/firmware/dmi_scan.c | 9 +++++----
 include/linux/dmi.h         | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 0e08e665f715..0418fed261bb 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 
 	buf = dmi_early_remap(dmi_base, orig_dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, NULL);
 
@@ -970,7 +970,8 @@ EXPORT_SYMBOL(dmi_get_date);
  *	@decode: Callback function
  *	@private_data: Private data to be passed to the callback function
  *
- *	Returns -1 when the DMI table can't be reached, 0 on success.
+ *	Returns 0 on success, -ENXIO if DMI is not selected or not present,
+ *	or a different negative error code if DMI walking fails.
  */
 int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	     void *private_data)
@@ -978,11 +979,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
 
 	buf = dmi_remap(dmi_base, dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, private_data);
 
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5055ac34142d..770d548e9a9d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -135,7 +135,7 @@ static inline int dmi_name_in_vendors(const char *s) { return 0; }
 static inline int dmi_name_in_serial(const char *s) { return 0; }
 #define dmi_available 0
 static inline int dmi_walk(void (*decode)(const struct dmi_header *, void *),
-	void *private_data) { return -1; }
+	void *private_data) { return -ENXIO; }
 static inline bool dmi_match(enum dmi_field f, const char *str)
 	{ return false; }
 static inline void dmi_memdev_name(u16 handle, const char **bank,
-- 
2.5.0

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Jean Delvare]

Hi Andy,

Sorry for the delay.

On Tue, 19 Jan 2016 15:54:46 -0800, Andy Lutomirski wrote:
> Currently they return -1 on error, which will confuse callers if
> they try to interpret it as a normal negative error code.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Changes from v3:

You mean from v2...

>  - Split out from the series it was in.
>  - Use -ENXIO for "there's no DMI".
>  - Also fix docs and !DMI case.
> 
> Changes from v2:

... and from v1.

>  - Total rewrite.
> 
> drivers/firmware/dmi_scan.c | 9 +++++----
>  include/linux/dmi.h         | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 0e08e665f715..0418fed261bb 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  
>  	buf = dmi_early_remap(dmi_base, orig_dmi_len);
>  	if (buf == NULL)
> -		return -1;
> +		return -ENOMEM;
>  
>  	dmi_decode_table(buf, decode, NULL);
>  
> @@ -970,7 +970,8 @@ EXPORT_SYMBOL(dmi_get_date);
>   *	@decode: Callback function
>   *	@private_data: Private data to be passed to the callback function
>   *
> - *	Returns -1 when the DMI table can't be reached, 0 on success.
> + *	Returns 0 on success, -ENXIO if DMI is not selected or not present,
> + *	or a different negative error code if DMI walking fails.

Returning an error from DMI walking isn't yet implemented so this is
confusing. If it ever is, most likely it will be implemented as a
separate function. Or were you only referring to the -ENOMEM case below?

>   */
>  int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>  	     void *private_data)
> @@ -978,11 +979,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>  	u8 *buf;
>  
>  	if (!dmi_available)
> -		return -1;
> +		return -ENOENT;

Should be -ENXIO as documented above? Not that I really understand how
"No such device or address" is going to be a helpful error message for
the user. What's wrong with -ENOTSUP I suggested earlier?

>  
>  	buf = dmi_remap(dmi_base, dmi_len);
>  	if (buf == NULL)
> -		return -1;
> +		return -ENOMEM;
>  
>  	dmi_decode_table(buf, decode, private_data);
>  
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 5055ac34142d..770d548e9a9d 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -135,7 +135,7 @@ static inline int dmi_name_in_vendors(const char *s) { return 0; }
>  static inline int dmi_name_in_serial(const char *s) { return 0; }
>  #define dmi_available 0
>  static inline int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> -	void *private_data) { return -1; }
> +	void *private_data) { return -ENXIO; }
>  static inline bool dmi_match(enum dmi_field f, const char *str)
>  	{ return false; }
>  static inline void dmi_memdev_name(u16 handle, const char **bank,


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Darren Hart]

On Fri, Jan 22, 2016 at 10:12:22AM +0100, Jean Delvare wrote:
> Hi Andy,
> 
> Sorry for the delay.
> 
> On Tue, 19 Jan 2016 15:54:46 -0800, Andy Lutomirski wrote:
> > Currently they return -1 on error, which will confuse callers if
> > they try to interpret it as a normal negative error code.
> > 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> > 
> > Changes from v3:
> 
> You mean from v2...
> 
> >  - Split out from the series it was in.
> >  - Use -ENXIO for "there's no DMI".
> >  - Also fix docs and !DMI case.
> > 
> > Changes from v2:
> 
> ... and from v1.
> 
> >  - Total rewrite.
> > 
> > drivers/firmware/dmi_scan.c | 9 +++++----
> >  include/linux/dmi.h         | 2 +-
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > index 0e08e665f715..0418fed261bb 100644
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
> >  
> >  	buf = dmi_early_remap(dmi_base, orig_dmi_len);
> >  	if (buf == NULL)
> > -		return -1;
> > +		return -ENOMEM;
> >  
> >  	dmi_decode_table(buf, decode, NULL);
> >  
> > @@ -970,7 +970,8 @@ EXPORT_SYMBOL(dmi_get_date);
> >   *	@decode: Callback function
> >   *	@private_data: Private data to be passed to the callback function
> >   *
> > - *	Returns -1 when the DMI table can't be reached, 0 on success.
> > + *	Returns 0 on success, -ENXIO if DMI is not selected or not present,
> > + *	or a different negative error code if DMI walking fails.
> 
> Returning an error from DMI walking isn't yet implemented so this is
> confusing. If it ever is, most likely it will be implemented as a
> separate function. Or were you only referring to the -ENOMEM case below?
> 
> >   */
> >  int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> >  	     void *private_data)
> > @@ -978,11 +979,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> >  	u8 *buf;
> >  
> >  	if (!dmi_available)
> > -		return -1;
> > +		return -ENOENT;
> 
> Should be -ENXIO as documented above? Not that I really understand how
> "No such device or address" is going to be a helpful error message for
> the user. What's wrong with -ENOTSUP I suggested earlier?
> 

Andy,

If I understand this correctly, this is the first of 5 patches, and this one has
some unanswered questions from Jean here. If this patch gets respun, the
following are also impacted:

dell-wmi: Stop storing pointers to DMI tables
dell-wmi, dell-laptop: select DMI
dell-wmi: Clean up hotkey table size check
dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)

Is that correct?

If so, please close on this patch with Jean, and resend the series of 5 together
and be sure to include me on Cc.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Andy Lutomirski]

On Sat, Jan 30, 2016 at 10:05 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Fri, Jan 22, 2016 at 10:12:22AM +0100, Jean Delvare wrote:
>> Hi Andy,
>>
>> Sorry for the delay.
>>
>> On Tue, 19 Jan 2016 15:54:46 -0800, Andy Lutomirski wrote:
>> > Currently they return -1 on error, which will confuse callers if
>> > they try to interpret it as a normal negative error code.
>> >
>> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> > ---
>> >
>> > Changes from v3:
>>
>> You mean from v2...
>>
>> >  - Split out from the series it was in.
>> >  - Use -ENXIO for "there's no DMI".
>> >  - Also fix docs and !DMI case.
>> >
>> > Changes from v2:
>>
>> ... and from v1.
>>
>> >  - Total rewrite.
>> >
>> > drivers/firmware/dmi_scan.c | 9 +++++----
>> >  include/linux/dmi.h         | 2 +-
>> >  2 files changed, 6 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> > index 0e08e665f715..0418fed261bb 100644
>> > --- a/drivers/firmware/dmi_scan.c
>> > +++ b/drivers/firmware/dmi_scan.c
>> > @@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>> >
>> >     buf = dmi_early_remap(dmi_base, orig_dmi_len);
>> >     if (buf == NULL)
>> > -           return -1;
>> > +           return -ENOMEM;
>> >
>> >     dmi_decode_table(buf, decode, NULL);
>> >
>> > @@ -970,7 +970,8 @@ EXPORT_SYMBOL(dmi_get_date);
>> >   * @decode: Callback function
>> >   * @private_data: Private data to be passed to the callback function
>> >   *
>> > - * Returns -1 when the DMI table can't be reached, 0 on success.
>> > + * Returns 0 on success, -ENXIO if DMI is not selected or not present,
>> > + * or a different negative error code if DMI walking fails.
>>
>> Returning an error from DMI walking isn't yet implemented so this is
>> confusing. If it ever is, most likely it will be implemented as a
>> separate function. Or were you only referring to the -ENOMEM case below?
>>
>> >   */
>> >  int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>> >          void *private_data)
>> > @@ -978,11 +979,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>> >     u8 *buf;
>> >
>> >     if (!dmi_available)
>> > -           return -1;
>> > +           return -ENOENT;
>>
>> Should be -ENXIO as documented above? Not that I really understand how
>> "No such device or address" is going to be a helpful error message for
>> the user. What's wrong with -ENOTSUP I suggested earlier?
>>
>
> Andy,
>
> If I understand this correctly, this is the first of 5 patches, and this one has
> some unanswered questions from Jean here. If this patch gets respun, the
> following are also impacted:
>
> dell-wmi: Stop storing pointers to DMI tables
> dell-wmi, dell-laptop: select DMI
> dell-wmi: Clean up hotkey table size check
> dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)
>
> Is that correct?

Not really.  It's just the three patches here:

http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8503

This patch (the dmi_walk error code one) is no longer really related.
Due to Jean's earlier comment about what happens if DMI isn't enabled
at all, I no longer propagate the error code from dmi_walk in
dell-wmi, so the error code won't have any effect.  (Instead I just
warn and let the driver load in legacy mode, which matches the current
behavior.)

I think the way to go is for the v3 "dell-wmi: DMI misuse fixes"
series to go in through your tree, and I'll hash out the error code
thing separately with Jean.

Does that seem sensible?

--Andy

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Jean Delvare]

On Sat, 30 Jan 2016 10:13:09 -0800, Andy Lutomirski wrote:
> On Sat, Jan 30, 2016 at 10:05 AM, Darren Hart <dvhart@infradead.org> wrote:
> > If I understand this correctly, this is the first of 5 patches, and this one has
> > some unanswered questions from Jean here. If this patch gets respun, the
> > following are also impacted:
> >
> > dell-wmi: Stop storing pointers to DMI tables
> > dell-wmi, dell-laptop: select DMI
> > dell-wmi: Clean up hotkey table size check
> > dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)
> >
> > Is that correct?
> 
> Not really.  It's just the three patches here:
> 
> http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8503
> 
> This patch (the dmi_walk error code one) is no longer really related.
> Due to Jean's earlier comment about what happens if DMI isn't enabled
> at all, I no longer propagate the error code from dmi_walk in
> dell-wmi, so the error code won't have any effect.  (Instead I just
> warn and let the driver load in legacy mode, which matches the current
> behavior.)
> 
> I think the way to go is for the v3 "dell-wmi: DMI misuse fixes"
> series to go in through your tree, and I'll hash out the error code
> thing separately with Jean.
> 
> Does that seem sensible?

Yes, I agree that this patch is independent from the dell-wmi patch
series now.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Darren Hart]

On Sat, Jan 30, 2016 at 08:18:50PM +0100, Jean Delvare wrote:
> On Sat, 30 Jan 2016 10:13:09 -0800, Andy Lutomirski wrote:
> > On Sat, Jan 30, 2016 at 10:05 AM, Darren Hart <dvhart@infradead.org> wrote:
> > > If I understand this correctly, this is the first of 5 patches, and this one has
> > > some unanswered questions from Jean here. If this patch gets respun, the
> > > following are also impacted:
> > >
> > > dell-wmi: Stop storing pointers to DMI tables
> > > dell-wmi, dell-laptop: select DMI
> > > dell-wmi: Clean up hotkey table size check
> > > dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)
> > >
> > > Is that correct?
> > 
> > Not really.  It's just the three patches here:
> > 
> > http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8503
> > 
> > This patch (the dmi_walk error code one) is no longer really related.
> > Due to Jean's earlier comment about what happens if DMI isn't enabled
> > at all, I no longer propagate the error code from dmi_walk in
> > dell-wmi, so the error code won't have any effect.  (Instead I just
> > warn and let the driver load in legacy mode, which matches the current
> > behavior.)
> > 
> > I think the way to go is for the v3 "dell-wmi: DMI misuse fixes"
> > series to go in through your tree, and I'll hash out the error code
> > thing separately with Jean.
> > 
> > Does that seem sensible?
> 
> Yes, I agree that this patch is independent from the dell-wmi patch
> series now.

Excellent, works for me.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Andy Lutomirski]

On Tue, Feb 2, 2016 at 9:00 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Sat, Jan 30, 2016 at 08:18:50PM +0100, Jean Delvare wrote:
>> On Sat, 30 Jan 2016 10:13:09 -0800, Andy Lutomirski wrote:
>> > On Sat, Jan 30, 2016 at 10:05 AM, Darren Hart <dvhart@infradead.org> wrote:
>> > > If I understand this correctly, this is the first of 5 patches, and this one has
>> > > some unanswered questions from Jean here. If this patch gets respun, the
>> > > following are also impacted:
>> > >
>> > > dell-wmi: Stop storing pointers to DMI tables
>> > > dell-wmi, dell-laptop: select DMI
>> > > dell-wmi: Clean up hotkey table size check
>> > > dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)
>> > >
>> > > Is that correct?
>> >
>> > Not really.  It's just the three patches here:
>> >
>> > http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8503
>> >
>> > This patch (the dmi_walk error code one) is no longer really related.
>> > Due to Jean's earlier comment about what happens if DMI isn't enabled
>> > at all, I no longer propagate the error code from dmi_walk in
>> > dell-wmi, so the error code won't have any effect.  (Instead I just
>> > warn and let the driver load in legacy mode, which matches the current
>> > behavior.)
>> >
>> > I think the way to go is for the v3 "dell-wmi: DMI misuse fixes"
>> > series to go in through your tree, and I'll hash out the error code
>> > thing separately with Jean.
>> >
>> > Does that seem sensible?
>>
>> Yes, I agree that this patch is independent from the dell-wmi patch
>> series now.
>
> Excellent, works for me.
>

Is any further action from me needed here?

--Andy

Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real error codes

[Darren Hart]

On Fri, Feb 12, 2016 at 10:59:10AM -0800, Andy Lutomirski wrote:
> On Tue, Feb 2, 2016 at 9:00 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Sat, Jan 30, 2016 at 08:18:50PM +0100, Jean Delvare wrote:
> >> On Sat, 30 Jan 2016 10:13:09 -0800, Andy Lutomirski wrote:
> >> > On Sat, Jan 30, 2016 at 10:05 AM, Darren Hart <dvhart@infradead.org> wrote:
> >> > > If I understand this correctly, this is the first of 5 patches, and this one has
> >> > > some unanswered questions from Jean here. If this patch gets respun, the
> >> > > following are also impacted:
> >> > >
> >> > > dell-wmi: Stop storing pointers to DMI tables
> >> > > dell-wmi, dell-laptop: select DMI
> >> > > dell-wmi: Clean up hotkey table size check
> >> > > dell-wmi: Support new hotkeys on the XPS 13 9350 (Skylake)
> >> > >
> >> > > Is that correct?
> >> >
> >> > Not really.  It's just the three patches here:
> >> >
> >> > http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8503
> >> >
> >> > This patch (the dmi_walk error code one) is no longer really related.
> >> > Due to Jean's earlier comment about what happens if DMI isn't enabled
> >> > at all, I no longer propagate the error code from dmi_walk in
> >> > dell-wmi, so the error code won't have any effect.  (Instead I just
> >> > warn and let the driver load in legacy mode, which matches the current
> >> > behavior.)
> >> >
> >> > I think the way to go is for the v3 "dell-wmi: DMI misuse fixes"
> >> > series to go in through your tree, and I'll hash out the error code
> >> > thing separately with Jean.
> >> >
> >> > Does that seem sensible?
> >>
> >> Yes, I agree that this patch is independent from the dell-wmi patch
> >> series now.
> >
> > Excellent, works for me.
> >
> 
> Is any further action from me needed here?

Now that the Dell SMBIOS update is in (yesterday), I am looking for a
consolidated patch series from you, ending with the skylake support.

-- 
Darren Hart
Intel Open Source Technology Center