| Message ID | 20170126153013.12753-8-jprvita@endlessm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > --- > drivers/platform/x86/asus-wireless.c | 15 ++++++--------- > drivers/platform/x86/asus-wireless.h | 10 ++++++++++ > 2 files changed, 16 insertions(+), 9 deletions(-) > create mode 100644 drivers/platform/x86/asus-wireless.h > > diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c > index 5a238fad35fb..fa28613688b4 100644 > --- a/drivers/platform/x86/asus-wireless.c > +++ b/drivers/platform/x86/asus-wireless.c > @@ -17,6 +17,8 @@ > #include <linux/pci_ids.h> > #include <linux/leds.h> > > +#include "asus-wireless.h" > + > #define ASUS_WIRELESS_LED_STATUS 0x2 > > struct hswc_params { > @@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = { > { 0x5, 0x4 }, > }; > > -static const struct acpi_device_id device_ids[] = { > - {"ATK4001", 0}, > - {"ATK4002", 0}, > - {"", 0}, > -}; > -MODULE_DEVICE_TABLE(acpi, device_ids); > +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids); > No, Don't do this. > static u64 asus_wireless_method(acpi_handle handle, const char *method, > int param) > @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev) > adev->driver_data = data; > > hid = acpi_device_hid(adev); > - for (i = 0; strcmp(device_ids[i].id, ""); i++) { This is wrong. > - if (!strcmp(device_ids[i].id, hid)) { > + for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) { This is too. Potential infinite loop. On top of that seems you just introduced this by previous patches and changing here. Often it means you need to reconsider how you actually split the series on logical pieces. > + if (!strcmp(asus_wireless_ids[i].id, hid)) { > data->hswc_params = &id_params[i]; > break; > } > @@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev) > static struct acpi_driver asus_wireless_driver = { > .name = "Asus Wireless Radio Control Driver", > .class = "hotkey", > - .ids = device_ids, > + .ids = asus_wireless_ids, > .ops = { > .add = asus_wireless_add, > .remove = asus_wireless_remove, > diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h > new file mode 100644 > index 000000000000..10a345863da6 > --- /dev/null > +++ b/drivers/platform/x86/asus-wireless.h > @@ -0,0 +1,10 @@ > +#ifndef _ASUS_WIRELESS_H_ > +#define _ASUS_WIRELESS_H_ > + > +const struct acpi_device_id asus_wireless_ids[] = { > + {"ATK4001", 0}, > + {"ATK4002", 0}, > + {"", 0}, > +}; > + > +#endif /* !_ASUS_WIRELESS_H_ */ > -- > 2.11.0 >
On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita > <jprvita@gmail.com> wrote: >> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> >> --- >> drivers/platform/x86/asus-wireless.c | 15 ++++++--------- >> drivers/platform/x86/asus-wireless.h | 10 ++++++++++ >> 2 files changed, 16 insertions(+), 9 deletions(-) >> create mode 100644 drivers/platform/x86/asus-wireless.h >> >> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c >> index 5a238fad35fb..fa28613688b4 100644 >> --- a/drivers/platform/x86/asus-wireless.c >> +++ b/drivers/platform/x86/asus-wireless.c >> @@ -17,6 +17,8 @@ >> #include <linux/pci_ids.h> >> #include <linux/leds.h> >> >> +#include "asus-wireless.h" >> + >> #define ASUS_WIRELESS_LED_STATUS 0x2 >> >> struct hswc_params { >> @@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = { >> { 0x5, 0x4 }, >> }; >> >> -static const struct acpi_device_id device_ids[] = { >> - {"ATK4001", 0}, >> - {"ATK4002", 0}, >> - {"", 0}, >> -}; >> -MODULE_DEVICE_TABLE(acpi, device_ids); >> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids); >> > > No, Don't do this. > Why? >> static u64 asus_wireless_method(acpi_handle handle, const char *method, >> int param) >> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev) >> adev->driver_data = data; >> >> hid = acpi_device_hid(adev); >> - for (i = 0; strcmp(device_ids[i].id, ""); i++) { > > This is wrong. > >> - if (!strcmp(device_ids[i].id, hid)) { >> + for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) { > > This is too. > > Potential infinite loop. > > On top of that seems you just introduced this by previous patches and > changing here. > Often it means you need to reconsider how you actually split the > series on logical pieces. > Can you please elaborate a bit more? All this change does is to change the name of the variable being iterated in the loop. As you said, the loop was introduced in a previous series, and you didn't spot any problems there. I don't think it makes sense to also rename the variable in that other series, since I'm only renaming it because I'm moving it to a header so it can be used by asus-wmi.c as well. >> + if (!strcmp(asus_wireless_ids[i].id, hid)) { >> data->hswc_params = &id_params[i]; >> break; >> } >> @@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev) >> static struct acpi_driver asus_wireless_driver = { >> .name = "Asus Wireless Radio Control Driver", >> .class = "hotkey", >> - .ids = device_ids, >> + .ids = asus_wireless_ids, >> .ops = { >> .add = asus_wireless_add, >> .remove = asus_wireless_remove, >> diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h >> new file mode 100644 >> index 000000000000..10a345863da6 >> --- /dev/null >> +++ b/drivers/platform/x86/asus-wireless.h >> @@ -0,0 +1,10 @@ >> +#ifndef _ASUS_WIRELESS_H_ >> +#define _ASUS_WIRELESS_H_ >> + >> +const struct acpi_device_id asus_wireless_ids[] = { >> + {"ATK4001", 0}, >> + {"ATK4002", 0}, >> + {"", 0}, >> +}; >> + >> +#endif /* !_ASUS_WIRELESS_H_ */ >> -- >> 2.11.0 >> > > > > -- > With Best Regards, > Andy Shevchenko -- João Paulo Rechi Vita http://about.me/jprvita
On Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: > On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita >> <jprvita@gmail.com> wrote: Fill commit message, btw. >>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> >>> -static const struct acpi_device_id device_ids[] = { >>> - {"ATK4001", 0}, >>> - {"ATK4002", 0}, >>> - {"", 0}, >>> -}; >>> -MODULE_DEVICE_TABLE(acpi, device_ids); >>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids); >>> >> >> No, Don't do this. >> > > Why? Table is a property of certain driver. You make it visible to parts that non need it. Moreover, you may here the list itself non-explicit, which reduces readability and understandability worse. If you would like to maintain a list of devices in two (semi-)independent modules, it would be not good looking in any case, either you make a hard dependency (if they already are it's okay to just export a function which helps you to find an ID in the list), or copy it in both modules. I need to check this as well. >>> static u64 asus_wireless_method(acpi_handle handle, const char *method, >>> int param) >>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev) >>> adev->driver_data = data; >>> >>> hid = acpi_device_hid(adev); >>> - for (i = 0; strcmp(device_ids[i].id, ""); i++) { >> >> This is wrong. >> >>> - if (!strcmp(device_ids[i].id, hid)) { >>> + for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) { >> >> This is too. >> >> Potential infinite loop. >> >> On top of that seems you just introduced this by previous patches and >> changing here. >> Often it means you need to reconsider how you actually split the >> series on logical pieces. >> > > Can you please elaborate a bit more? The original code relies on "" in the first parameter which basically can be NULL. This fragile. But this is part subject to change in a sequential patch. > All this change does is to change > the name of the variable being iterated in the loop. As you said, the > loop was introduced in a previous series, and you didn't spot any > problems there. If I didn't spot it doesn't mean there is no issues, right? ;) > I don't think it makes sense to also rename the > variable in that other series, since I'm only renaming it because I'm > moving it to a header so it can be used by asus-wmi.c as well. The main problem with the above is introduction something that you are changing soon later.
On 4 February 2017 at 10:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: >> On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita >>> <jprvita@gmail.com> wrote: > > Fill commit message, btw. > >>>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > >>>> -static const struct acpi_device_id device_ids[] = { >>>> - {"ATK4001", 0}, >>>> - {"ATK4002", 0}, >>>> - {"", 0}, >>>> -}; >>>> -MODULE_DEVICE_TABLE(acpi, device_ids); >>>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids); >>>> >>> >>> No, Don't do this. >>> >> >> Why? > > Table is a property of certain driver. You make it visible to parts > that non need it. > Moreover, you may here the list itself non-explicit, which reduces > readability and understandability worse. > > If you would like to maintain a list of devices in two > (semi-)independent modules, it would be not good looking in any case, > either you make a hard dependency (if they already are it's okay to > just export a function which helps you to find an ID in the list), or > copy it in both modules. > > I need to check this as well. > Currently these modules do not depend on each other. There are machines which only need asus-wmi, and not asus-wireless, and there may be machines in the future which will only need asus-wireless and not asus-wmi (not really seen any in that situation, but it is a possibility), so I don't think adding a dependency here is the right thing. Duplicating code is not good, so I was trying to avoid it, but maybe there isn't really any better way in this case. >>>> static u64 asus_wireless_method(acpi_handle handle, const char *method, >>>> int param) >>>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev) >>>> adev->driver_data = data; >>>> >>>> hid = acpi_device_hid(adev); >>>> - for (i = 0; strcmp(device_ids[i].id, ""); i++) { >>> >>> This is wrong. >>> >>>> - if (!strcmp(device_ids[i].id, hid)) { >>>> + for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) { >>> >>> This is too. >>> >>> Potential infinite loop. >>> >>> On top of that seems you just introduced this by previous patches and >>> changing here. >>> Often it means you need to reconsider how you actually split the >>> series on logical pieces. >>> >> >> Can you please elaborate a bit more? > > The original code relies on "" in the first parameter which basically > can be NULL. This fragile. > But this is part subject to change in a sequential patch. > >> All this change does is to change >> the name of the variable being iterated in the loop. As you said, the >> loop was introduced in a previous series, and you didn't spot any >> problems there. > > If I didn't spot it doesn't mean there is no issues, right? ;) > >> I don't think it makes sense to also rename the >> variable in that other series, since I'm only renaming it because I'm >> moving it to a header so it can be used by asus-wmi.c as well. > > The main problem with the above is introduction something that you are > changing soon later. > Ok, I should probably have sent this as RFC, as it was actually the case. But since I'm not going to export the ids list anymore, this patch will be dropped from the next revision. -- João Paulo Rechi Vita http://about.me/jprvita
diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c index 5a238fad35fb..fa28613688b4 100644 --- a/drivers/platform/x86/asus-wireless.c +++ b/drivers/platform/x86/asus-wireless.c @@ -17,6 +17,8 @@ #include <linux/pci_ids.h> #include <linux/leds.h> +#include "asus-wireless.h" + #define ASUS_WIRELESS_LED_STATUS 0x2 struct hswc_params { @@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = { { 0x5, 0x4 }, }; -static const struct acpi_device_id device_ids[] = { - {"ATK4001", 0}, - {"ATK4002", 0}, - {"", 0}, -}; -MODULE_DEVICE_TABLE(acpi, device_ids); +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids); static u64 asus_wireless_method(acpi_handle handle, const char *method, int param) @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev) adev->driver_data = data; hid = acpi_device_hid(adev); - for (i = 0; strcmp(device_ids[i].id, ""); i++) { - if (!strcmp(device_ids[i].id, hid)) { + for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) { + if (!strcmp(asus_wireless_ids[i].id, hid)) { data->hswc_params = &id_params[i]; break; } @@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev) static struct acpi_driver asus_wireless_driver = { .name = "Asus Wireless Radio Control Driver", .class = "hotkey", - .ids = device_ids, + .ids = asus_wireless_ids, .ops = { .add = asus_wireless_add, .remove = asus_wireless_remove, diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h new file mode 100644 index 000000000000..10a345863da6 --- /dev/null +++ b/drivers/platform/x86/asus-wireless.h @@ -0,0 +1,10 @@ +#ifndef _ASUS_WIRELESS_H_ +#define _ASUS_WIRELESS_H_ + +const struct acpi_device_id asus_wireless_ids[] = { + {"ATK4001", 0}, + {"ATK4002", 0}, + {"", 0}, +}; + +#endif /* !_ASUS_WIRELESS_H_ */
Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> --- drivers/platform/x86/asus-wireless.c | 15 ++++++--------- drivers/platform/x86/asus-wireless.h | 10 ++++++++++ 2 files changed, 16 insertions(+), 9 deletions(-) create mode 100644 drivers/platform/x86/asus-wireless.h