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

Groups > linux.kernel > #1322477 > unrolled thread

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

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 v3] dmi: Make dmi_walk and dmi_walk_early return real  error codes Darren Hart <dvhart@infradead.org> - 2016-01-30 19:10 +0100
    Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real  error codes Andy Lutomirski <luto@amacapital.net> - 2016-01-30 19:20 +0100
      Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real  error codes Jean Delvare <jdelvare@suse.de> - 2016-01-30 20:20 +0100
        Re: [PATCH v3] dmi: Make dmi_walk and dmi_walk_early return real  error codes Darren Hart <dvhart@infradead.org> - 2016-02-02 18:10 +0100

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

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

[toc] | [next] | [standalone]

#1322481

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

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

#1322495

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

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

#1324288

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web