Re: [PATCH v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
On 2019-02-05, Andy Shevchenko wrote:
> On Sun, Feb 3, 2019 at 9:04 PM Mattias Jacobsson <2...@mok.nu> wrote:
> > On 2019-01-30, Andy Shevchenko wrote:
> > > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2...@mok.nu> wrote:
>
> > > > + if (len < 0 || len >= 500) {
> > >
> > > Would it even possible to get a negative number here?
> > > Same for any other number than slightly bigger than 36.
> >
> > snprintf returns a negative number on error. BTW AFAIU the code from
> > file2alias.c gets dynamically linked against a libc.
>
> OK.
>
> > > So, what about simple
> > >
> > > {
> > > DEF_FIELD_ADDR(...);
> > > size_t len;
> > >
> > > len = strlen(*guid_string);
> > > if (len != ...) {
> > > ...
> > > }
> > > sprintf(...);
> > > return 1;
> > > }
> > >
> > > ?
> >
> > Then we are missing the check that we are within the bounds of alias
>
> I don't see how. By checking a length of string we be sure, that the
> result would have a non-arbitrary length.
If you do s/500/ALIAS_SIZE/ on the patch? My code is written with that
in mind, I guess that wasn't totally clear.
BTW I've posted [1] to introduce the ALIAS_SIZE macro.
[1]: https://lore.kernel.org/lkml/20190207123022.7961-1-...@mok.nu/
>
> > as well as the negative code from s*printf(). snprintf() does this nicely
> > for us.
>
> This one I agree with, means in the above example we may do
>
> return sprintf(...);
>
> if callers recognize just a sign, or
>
> len = sprintf(...);
> if (len < 0)
> return len; // -1? 0?
>
> return 1;
>
> otherwise.
Great
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
Mattias
Re: [PATCH v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
On Sun, Feb 3, 2019 at 9:04 PM Mattias Jacobsson <2...@mok.nu> wrote:
> On 2019-01-30, Andy Shevchenko wrote:
> > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2...@mok.nu> wrote:
> > > + if (len < 0 || len >= 500) {
> >
> > Would it even possible to get a negative number here?
> > Same for any other number than slightly bigger than 36.
>
> snprintf returns a negative number on error. BTW AFAIU the code from
> file2alias.c gets dynamically linked against a libc.
OK.
> > So, what about simple
> >
> > {
> > DEF_FIELD_ADDR(...);
> > size_t len;
> >
> > len = strlen(*guid_string);
> > if (len != ...) {
> > ...
> > }
> > sprintf(...);
> > return 1;
> > }
> >
> > ?
>
> Then we are missing the check that we are within the bounds of alias
I don't see how. By checking a length of string we be sure, that the
result would have a non-arbitrary length.
> as well as the negative code from s*printf(). snprintf() does this nicely
> for us.
This one I agree with, means in the above example we may do
return sprintf(...);
if callers recognize just a sign, or
len = sprintf(...);
if (len < 0)
return len; // -1? 0?
return 1;
otherwise.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
On 2019-01-30, Andy Shevchenko wrote:
> On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2...@mok.nu> wrote:
> >
> > The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
> > can specify their device type and their array of device_ids and thereby
> > trigger the generation of the appropriate MODULE_ALIAS() output. This is
> > opposed to having to specify one MODULE_ALIAS() for each device. The WMI
> > device type is currently not supported.
> >
> > While using MODULE_DEVICE_TABLE() does increase the complexity as well
> > as spreading out the implementation across the kernel, it does come with
> > some benefits too;
> > * It makes different drivers look more similar; if you can specify the
> > array of device_ids any device type specific input to MODULE_ALIAS()
> > will automatically be generated for you.
> > * It helps each driver avoid keeping multiple versions of the same
> > information in sync. That is, both the array of device_ids and the
> > potential multitude of MODULE_ALIAS()'s.
> >
> > Add WMI support to MODULE_DEVICE_TABLE() by adding info about struct
> > wmi_device_id in devicetable-offsets.c and add a WMI entry point in
> > file2alias.c.
> >
> > The type argument for MODULE_DEVICE_TABLE(type, name) is wmi.
> >
> > Suggested-by: Pali Rohár
> > Signed-off-by: Mattias Jacobsson <2...@mok.nu>
> > ---
> >
> > What do you think about this usage of snprintf()? Now we check if there
> > is an error or if the printed string tried to exceeded the buffer.
> > Ideally 500 should be a macro or a parameter, but there isn't one
> > available. The number 500 comes from a few lines below in the function
> > do_table().
>
> This looks better, though minor comments.
>
> Indeed, 500 would be nicer to be defined as a constant (via preprocessor
> macro).
>
> > +/* Looks like: wmi:guid */
> > +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> > +{
> > + DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> > + if (strlen(*guid_string) != UUID_STRING_LEN) {
> > + warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> > + *guid_string, filename);
> > + return 0;
> > + }
> > +
>
> > + int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s",
> > *guid_string);
>
> Please, split declaration and assignment...
Done.
>
> > +
>
> ...and drop this line.
Done.
>
> > + if (len < 0 || len >= 500) {
>
> Would it even possible to get a negative number here?
> Same for any other number than slightly bigger than 36.
snprintf returns a negative number on error. BTW AFAIU the code from
file2alias.c gets dynamically linked against a libc.
>
> You have above a check and here is the matter of either sudden
> replacement of the string during the operation or how snprintf is
> broken itself.
> Do you have a case in mind which can bring to the above conditions?
>
> > + warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> > + filename);
> > + return 0;
> > + }
>
> On top of that you have an ordinary case here and in similar ones we
> don't care about buffer size at all (perhaps BUILD_BUG_ON() what is
> needed here).
I used warn() as it is what is being used in similar conditions
elsewhere in the file.
>
> So, what about simple
>
> {
> DEF_FIELD_ADDR(...);
> size_t len;
>
> len = strlen(*guid_string);
> if (len != ...) {
> ...
> }
> sprintf(...);
> return 1;
> }
>
> ?
Then we are missing the check that we are within the bounds of alias as
well as the negative code from s*printf(). snprintf() does this nicely
for us.
>
> > + return 1;
> > +}
> > +
> > /* Does namelen bytes of name exactly match the symbol? */
> > static bool sym_is(const char *name, unsigned namelen, const char *symbol)
> > {
> > @@ -1357,6 +1378,7 @@ static const struct devtable devtable[] = {
> > {"fslmc", SIZE_fsl_mc_device_id, do_fsl_mc_entry},
> > {"tbsvc", SIZE_tb_service_id, do_tbsvc_entry},
> > {"typec", SIZE_typec_device_id, do_typec_entry},
> > + {"wmi", SIZE_wmi_device_id, do_wmi_entry},
> > };
> >
> > /* Create MODULE_ALIAS() statements.
> > --
> > 2.20.1
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
Mattias
Re: [PATCH v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2...@mok.nu> wrote:
>
> The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
> can specify their device type and their array of device_ids and thereby
> trigger the generation of the appropriate MODULE_ALIAS() output. This is
> opposed to having to specify one MODULE_ALIAS() for each device. The WMI
> device type is currently not supported.
>
> While using MODULE_DEVICE_TABLE() does increase the complexity as well
> as spreading out the implementation across the kernel, it does come with
> some benefits too;
> * It makes different drivers look more similar; if you can specify the
> array of device_ids any device type specific input to MODULE_ALIAS()
> will automatically be generated for you.
> * It helps each driver avoid keeping multiple versions of the same
> information in sync. That is, both the array of device_ids and the
> potential multitude of MODULE_ALIAS()'s.
>
> Add WMI support to MODULE_DEVICE_TABLE() by adding info about struct
> wmi_device_id in devicetable-offsets.c and add a WMI entry point in
> file2alias.c.
>
> The type argument for MODULE_DEVICE_TABLE(type, name) is wmi.
>
> Suggested-by: Pali Rohár
> Signed-off-by: Mattias Jacobsson <2...@mok.nu>
> ---
>
> What do you think about this usage of snprintf()? Now we check if there
> is an error or if the printed string tried to exceeded the buffer.
> Ideally 500 should be a macro or a parameter, but there isn't one
> available. The number 500 comes from a few lines below in the function
> do_table().
This looks better, though minor comments.
Indeed, 500 would be nicer to be defined as a constant (via preprocessor macro).
> +/* Looks like: wmi:guid */
> +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> +{
> + DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> + if (strlen(*guid_string) != UUID_STRING_LEN) {
> + warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> + *guid_string, filename);
> + return 0;
> + }
> +
> + int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);
Please, split declaration and assignment...
> +
...and drop this line.
> + if (len < 0 || len >= 500) {
Would it even possible to get a negative number here?
Same for any other number than slightly bigger than 36.
You have above a check and here is the matter of either sudden
replacement of the string during the operation or how snprintf is
broken itself.
Do you have a case in mind which can bring to the above conditions?
> + warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> + filename);
> + return 0;
> + }
On top of that you have an ordinary case here and in similar ones we
don't care about buffer size at all (perhaps BUILD_BUG_ON() what is
needed here).
So, what about simple
{
DEF_FIELD_ADDR(...);
size_t len;
len = strlen(*guid_string);
if (len != ...) {
...
}
sprintf(...);
return 1;
}
?
> + return 1;
> +}
> +
> /* Does namelen bytes of name exactly match the symbol? */
> static bool sym_is(const char *name, unsigned namelen, const char *symbol)
> {
> @@ -1357,6 +1378,7 @@ static const struct devtable devtable[] = {
> {"fslmc", SIZE_fsl_mc_device_id, do_fsl_mc_entry},
> {"tbsvc", SIZE_tb_service_id, do_tbsvc_entry},
> {"typec", SIZE_typec_device_id, do_typec_entry},
> + {"wmi", SIZE_wmi_device_id, do_wmi_entry},
> };
>
> /* Create MODULE_ALIAS() statements.
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
[PATCH v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
can specify their device type and their array of device_ids and thereby
trigger the generation of the appropriate MODULE_ALIAS() output. This is
opposed to having to specify one MODULE_ALIAS() for each device. The WMI
device type is currently not supported.
While using MODULE_DEVICE_TABLE() does increase the complexity as well
as spreading out the implementation across the kernel, it does come with
some benefits too;
* It makes different drivers look more similar; if you can specify the
array of device_ids any device type specific input to MODULE_ALIAS()
will automatically be generated for you.
* It helps each driver avoid keeping multiple versions of the same
information in sync. That is, both the array of device_ids and the
potential multitude of MODULE_ALIAS()'s.
Add WMI support to MODULE_DEVICE_TABLE() by adding info about struct
wmi_device_id in devicetable-offsets.c and add a WMI entry point in
file2alias.c.
The type argument for MODULE_DEVICE_TABLE(type, name) is wmi.
Suggested-by: Pali Rohár
Signed-off-by: Mattias Jacobsson <2...@mok.nu>
---
What do you think about this usage of snprintf()? Now we check if there
is an error or if the printed string tried to exceeded the buffer.
Ideally 500 should be a macro or a parameter, but there isn't one
available. The number 500 comes from a few lines below in the function
do_table().
scripts/mod/devicetable-offsets.c | 3 +++
scripts/mod/file2alias.c | 22 ++
2 files changed, 25 insertions(+)
diff --git a/scripts/mod/devicetable-offsets.c
b/scripts/mod/devicetable-offsets.c
index 293004499b4d..99276a422e77 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -225,5 +225,8 @@ int main(void)
DEVID_FIELD(typec_device_id, svid);
DEVID_FIELD(typec_device_id, mode);
+ DEVID(wmi_device_id);
+ DEVID_FIELD(wmi_device_id, guid_string);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index a37af7d71973..abaa6c090564 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -37,6 +37,7 @@ typedef unsigned char __u8;
typedef struct {
__u8 b[16];
} uuid_le;
+#defineUUID_STRING_LEN 36
/* Big exception to the "don't include kernel headers into userspace, which
* even potentially has different endianness and word sizes, since
@@ -1287,6 +1288,26 @@ static int do_typec_entry(const char *filename, void
*symval, char *alias)
return 1;
}
+/* Looks like: wmi:guid */
+static int do_wmi_entry(const char *filename, void *symval, char *alias)
+{
+ DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
+ if (strlen(*guid_string) != UUID_STRING_LEN) {
+ warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
+ *guid_string, filename);
+ return 0;
+ }
+
+ int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);
+
+ if (len < 0 || len >= 500) {
+ warn("Could not generate all MODULE_ALIAS's in '%s'\n",
+ filename);
+ return 0;
+ }
+ return 1;
+}
+
/* Does namelen bytes of name exactly match the symbol? */
static bool sym_is(const char *name, unsigned namelen, const char *symbol)
{
@@ -1357,6 +1378,7 @@ static const struct devtable devtable[] = {
{"fslmc", SIZE_fsl_mc_device_id, do_fsl_mc_entry},
{"tbsvc", SIZE_tb_service_id, do_tbsvc_entry},
{"typec", SIZE_typec_device_id, do_typec_entry},
+ {"wmi", SIZE_wmi_device_id, do_wmi_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.20.1
