| Message ID | 0e75e48f9944d14ed914342a4780c199095ad747.1453247603.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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,
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,
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 -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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 -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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,
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(-)